The review of #768 (celeris#713) had one blocking finding, fixed on the branch in round 2 (head 6267e56). The stale-clock close reproduced with no park:
- on a running idle worker with
ReadHeaderTimeout off;
- on a paused worker that still holds a connection;
- on an adoption onto such a worker.
The worker now reads its clock on every CQE batch, as epoll does, and three new arms with their controls cover those cases. The review also left the minor findings and nits below. Items 1 and 2 are still open; each says what to do.
1. The epoll twins can skip silently in CI (nit)
engine/epoll/park_stale_clock_test.go:54 calls t.Skipf("epoll engine unavailable: %v", err), and :79 calls t.Skip("epoll engine did not bind"). CI runs engine/epoll only in the root step, without -v, and in no named interlock. #766 put its epoll pair into one. grep -n 'celeris713\|park_stale' .github/workflows/*.yml finds nothing. So a twin that skipped, was renamed or was removed would still go green. The same holds for the io_uring arms: only the engine/iouring step's tally of unlisted SKIP lines covers them.
To do: add the #713 tests to named interlocks in ci.yml:
- the epoll twins
TestAcceptAfterALongParkIsNotTimedOutEpoll and TestAdoptAfterALongParkIsNotTimedOutEpoll;
- the ten io_uring tests in
engine/iouring/park_stale_clock_test.go.
Each interlock reports want N, ran N, passed N, SKIP lines 0. Or turn the twins' skips into failures under a require variable. Doing this in #768 would touch the lines of ci.yml that #766 changes.
2. Why the async standby's drain time changed between 9f4d89b and 698bed6 (nit)
The issue's async 31 s closes on 9f4d89b are explained by rerunning the lane's recount (713/ab_recount.py) on the issue's raw files (evidence/probatorium-416/revert-cell/logs/job1):
- On 9f4d89b the async io_uring standby emptied about 1 s after the demote (
stby0_s 1.01 and 1.03). So the park at the 30.98 s gap was about 30 s, right at ReadTimeout: 81 closes and 70 failures, and 156 closes and 145 failures.
- On main 698bed6, the same probe source (
diff of the two main.go files is empty) empties it only after about 11 s (stby0_s 11.00-11.04, logs/ab-all/RECOUNT.txt).
The open question is which change moved it. The candidates are #674 (49d2726), #698 and #696.
To do: find the commit that took the async standby's drain from ~1 s to ~11 s, and say whether that is intended. One way is to bisect the revert-cell probe's stby0_s between 9f4d89b and 698bed6. It matters for any claim about how long an adaptive standby parks.
3. Items the round-2 push dealt with (for the record)
- The adoption arms measured the clock's lag, not the park (minor). A paused worker's clock lagged by about 32 iterations, which depends on the client's request cadence. At a 250 ms gap the round-1 head failed 5/5.
- With the per-batch read the lag no longer depends on the cadence. At a 250 ms gap the arms now pass 5/5, and at 1500 ms (against
ReadTimeout 2 s) 3/3 (rv2/mutants/GAP250, GAP1500).
- The comment that called the adoption arm "the adaptive promote's shape" is corrected. An adaptive promote resumes the new active engine before it adopts (
adaptive/engine.go:850, :891), so it is the accept arm's shape. The adoption arm is the reclaim onto a draining source.
- The fresh adoption stamp was shown only by a unit test (minor). The draining-worker adoption is now a committed arm,
TestAdoptOntoADrainingWorkerIsNotTimedOut. The per-batch read is what fixes it. MNOSTAMP (the adoption stamped from cachedNow again) passes every engine arm and fails only the unit test, and the comment and the body now say exactly that.
- No negative control showed the epoll twins could fail (minor).
rv2/mutants/MEPOLL63 gates epoll's per-return clock read with &0x3F, and it fails both twins 5/5. It is in the round-2 body. The CI interlock is item 1.
- No committed script built the A/B probe binaries (nit).
713/probe/build.sh builds them with -trimpath from a named tree, and it built round 2's probe-fix2. The round-1 binaries (713/probe/bin/) were built without it, and they embed the source path they were built at, so they reproduce only at that path. The objdump comparison the review made stands.
To do: nothing further.
The review of #768 (celeris#713) had one blocking finding, fixed on the branch in round 2 (head
6267e56). The stale-clock close reproduced with no park:ReadHeaderTimeoutoff;The worker now reads its clock on every CQE batch, as epoll does, and three new arms with their controls cover those cases. The review also left the minor findings and nits below. Items 1 and 2 are still open; each says what to do.
1. The epoll twins can skip silently in CI (nit)
engine/epoll/park_stale_clock_test.go:54callst.Skipf("epoll engine unavailable: %v", err), and:79callst.Skip("epoll engine did not bind"). CI runsengine/epollonly in the root step, without-v, and in no named interlock. #766 put its epoll pair into one.grep -n 'celeris713\|park_stale' .github/workflows/*.ymlfinds nothing. So a twin that skipped, was renamed or was removed would still go green. The same holds for the io_uring arms: only theengine/iouringstep's tally of unlisted SKIP lines covers them.To do: add the #713 tests to named interlocks in
ci.yml:TestAcceptAfterALongParkIsNotTimedOutEpollandTestAdoptAfterALongParkIsNotTimedOutEpoll;engine/iouring/park_stale_clock_test.go.Each interlock reports
want N, ran N, passed N, SKIP lines 0. Or turn the twins' skips into failures under a require variable. Doing this in #768 would touch the lines ofci.ymlthat #766 changes.2. Why the async standby's drain time changed between 9f4d89b and 698bed6 (nit)
The issue's async 31 s closes on 9f4d89b are explained by rerunning the lane's recount (
713/ab_recount.py) on the issue's raw files (evidence/probatorium-416/revert-cell/logs/job1):stby0_s1.01 and 1.03). So the park at the 30.98 s gap was about 30 s, right atReadTimeout: 81 closes and 70 failures, and 156 closes and 145 failures.diffof the twomain.gofiles is empty) empties it only after about 11 s (stby0_s11.00-11.04,logs/ab-all/RECOUNT.txt).The open question is which change moved it. The candidates are #674 (
49d2726), #698 and #696.To do: find the commit that took the async standby's drain from ~1 s to ~11 s, and say whether that is intended. One way is to bisect the revert-cell probe's
stby0_sbetween 9f4d89b and 698bed6. It matters for any claim about how long an adaptive standby parks.3. Items the round-2 push dealt with (for the record)
ReadTimeout2 s) 3/3 (rv2/mutants/GAP250,GAP1500).adaptive/engine.go:850,:891), so it is the accept arm's shape. The adoption arm is the reclaim onto a draining source.TestAdoptOntoADrainingWorkerIsNotTimedOut. The per-batch read is what fixes it.MNOSTAMP(the adoption stamped fromcachedNowagain) passes every engine arm and fails only the unit test, and the comment and the body now say exactly that.rv2/mutants/MEPOLL63gates epoll's per-return clock read with&0x3F, and it fails both twins 5/5. It is in the round-2 body. The CI interlock is item 1.713/probe/build.shbuilds them with-trimpathfrom a named tree, and it built round 2'sprobe-fix2. The round-1 binaries (713/probe/bin/) were built without it, and they embed the source path they were built at, so they reproduce only at that path. The objdump comparison the review made stands.To do: nothing further.