Repository navigation
fix(pool): a host sleep is not N dead followers, so reclaim defers a window (re-homed on the rescue line) - #3782
qingyun-wu wants to merge 16 commits into
Conversation
…window A 968s clamshell sleep aged every `.alive` file past LEAD_STALE_S at the same instant. `alive_fn` then returned False for all four cores at once, so `reclaim_dead` and `reclaim_claimed` did exactly what they are written to do and repooled a LIVE core's claim; an owner task bounced across cores for ~45 minutes before anyone answered it. Beats stop together only when the machine does. `_host_gap_defers_reclaim` treats an all-stale pool as a host gap and defers reclaim one stale window instead of acting on it. Deferral, not suppression: a woken follower re-beats within the window and keeps its work, while a genuine whole-pool outage still reclaims once the window passes. Deliberately unchanged: a single dead follower reclaims immediately — the guard only fires when EVERY follower goes stale together, which is never N deaths. An empty pool and a raising followers_fn both decline to defer, so the guard can only ever delay recovery, never withhold it. LEAD_STALE_S is imported from pool_follower rather than restated — its comment already calls it "the same threshold every reader uses".
…bling-alive kewei CR blocker: staggered 30s beats let a sibling re-beat before the claim owner after wake, and the any-alive clearing ended the grace while the owner's beat was still due — repooling a live claim. The guard now opens its window on a >30s gap between the lead's own ~2s reclaim ticks (host-sleep proof independent of beat order); the window expires by time only. First-tick-all-stale still covers a lead restarted after wake. New tests replay sibling-first both before and after the lead observes all-stale through production entrypoints; mutation control (reintroducing the any-alive clear) reproduces the reviewer's exact repool. Jump-style tests in three suites now advance in daemon cadence — a single clock leap IS host-sleep evidence under this design, which is the point. Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>
Kewei re-review blocker: any awake tick gap over 30s read as host sleep and refreshed the 90s deferral, so a slow-but-awake daemon (long --interval, GC stall) could renew the grace forever and block reclamation. The monotonic clock pauses through host sleep while wall time advances; the guard now opens a grace only when their deltas diverge past SLEEP_SKEW_S across one tick. An awake-slow daemon shows wall ~= mono, gets no grace, and cannot renew — proven by the paired controls (slow-awake never defers; genuine skew opens one expiring window). mono_fn is injectable alongside now_fn for tests. Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>
The skew guard reads fake-wall jumps against the REAL monotonic clock as sleep; fixtures that model awake passes now inject mono_fn sharing the wall clock, so wall == mono holds by construction and only the sleep tests (which diverge the two deliberately) open a grace. Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>
Both AwakeSlowDaemonIsNotSleep tests created the claim and called reclaim_claimed() before staling the claimant. With a sibling alive the fresh-lead branch does not defer, so that first call repooled the claim immediately and every later clock assertion held over a claim that was already gone. Proven by mutation control — reverting the rule to the pre-fix gap-only form `(now - last) > 30.0`: before this commit 14 tests OK (gap-only passes too: no discrimination) after this commit 15 tests, FAILED (failures=3) unmutated 15 tests OK Each awake test now takes a baseline tick while the claimant is ALIVE — asserting nothing reclaims and the claim survives — then stales it and advances both clocks, asserting the outcome at each step rather than eventual absence. Adds the recurring-60s-timeout shape, which is the second way an awake daemon could have renewed the grace forever. WakeGuardBase injected only fake wall time, so production read the real time.monotonic() while tests jumped their own clock — the two operands of the rule were not both under test. It now drives monotonic too: sleep advances wall alone, awake ticking advances both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_reclaim_defer_until was stored and compared with now(), the wall clock this host reports as adjustable. A backward correction moves the comparison point back with the deadline fixed, so one 90s stale window withholds recovery for as long as the correction is large — a genuinely dead claimant stays stranded for hours. The monotonic sample the skew test already takes is the right anchor and costs nothing extra. The control opens grace, advances monotonic past LEAD_STALE_S, and steps the wall back an hour: the parent still defers, this head reclaims. Its companion holds the claim while monotonic is still inside the window, so the pair cannot be satisfied by a guard that simply never defers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntry The entry guard samples the clocks before scanning, so a suspension that begins after it is invisible to it — yet the owner-liveness read that authorizes the rename happens later, and by then a live owner reads dead. Both destructive variants renamed on that stale read: reclaim_claimed repooled a live claim, reclaim_dead repooled a live assignment. Re-checks _host_gap_defers_reclaim() immediately after the liveness result and before the rename, in both paths, returning what was already recovered rather than proceeding. Tests drive the suspension from inside alive_fn, which is exactly the window the entry guard cannot cover, and pin both variants. A third case keeps a genuinely dead owner reclaiming with no clock jump, so the re-check cannot pass by never reclaiming at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A lead that restarts across a host sleep has no in-process (wall, mono) sample, so the skew test could not run on tick 1. The liveness fallback cannot cover it: a sibling-first wake and one dead core look identical from liveness alone. Persist the sample and seed tick 1 from it. Additive — with no file on disk the existing all-stale fallback still decides, so the guard's fail-toward-recovery properties are unchanged.
A control that proves the parent's behaviour is preserved has to be runnable against the parent; asserting on the new private helper made it an AttributeError there instead of a passing control.
The persist is best-effort by design, but nothing exercised the except branch, so the fail-open claim rested on reading the code. Diff coverage on the changed lines was 92% against a 95% gate; this closes it at 100%.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%. Diff CoverageDiff: origin/rescue/pool-uncommitted-2026-08-26...HEAD, staged and unstaged changes
Summary
src/runtime-api/pool_lead.pyLines 473-482 473 return None
474 try:
475 du = d.get("defer_until")
476 du = None if du is None else float(du)
! 477 except (TypeError, ValueError):
! 478 du = None
479 # A deadline outside the window it was opened for is corrupt or
480 # foreign, never evidence; an expired one must not resurrect grace.
481 if du is not None and not (m <= du <= m + LEAD_STALE_S):
482 du = None |
|
Your two red checks are one abort in a test your diff does not touch. Diagnosed rather than guessed, and filed as #3800 so it does not keep landing on whoever is unlucky.
Its line 139 starts And the second check is derivative, not a second problem: Your diff is I have not reviewed the wake-guard logic itself yet — this is only the CI triage. Flagging it now because a PR that reads as red in two places is the kind that sits. |
|
CI triage: this is one failure reported twice, and it is not your diff. Run
It is an aggregator gating on the python job, so the two reds are one event. Nothing in your diff is TypeScript, which is what made the pair look worse than it is. The python job names exactly one file, and it is unrelated to this PR: Your diff is Evidence it is a flake rather than a real break:
So: re-run One caveat on my own reading, since it would be easy to over-claim. The run log also contains I have not reviewed the code itself yet — this is CI triage only. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at e17b8bf22. CI is red for reasons that are not this diff — triaged in my comment above (one event, an aggregator mirroring it, and the named file is tests/slack-proactive-release-on-send-failure.test.py, which passes at origin/main locally and is green on five other open PRs).
Why the guard convinced me
I traced _host_gap_defers_reclaim for the failure directions rather than the happy path, and every branch fails safe — toward granting grace, never toward reclaiming a live claim:
- Skew, not gap size, is the right discriminator. An awake-but-slow daemon shows
wall ≈ monoand gets nothing. That is the distinction #3411's problem statement actually needed, and it is not fakeable by a long--interval. - Monotonic expiry means a backward wall correction cannot stretch one window into hours. The docstring's "expires by TIME ONLY — a sibling heartbeat must not end it" matches the code: nothing but
mono < ducloses it. - Reboot detection by
m > self.mono()discards a predecessor sample whose monotonic base is gone. And when it discards,laststaysNoneand the all-stale branch grants a window anyway — so the discard path and the seed path both end safe. _save_wake_evidenceswallowsOSErrorby design, and losing the sample costs exactly one grace window rather than aborting the sweep that called it. The comment says so and the control flow agrees.- The broken-resolver path returns
False(except Exception: return False) — no grace when the follower list cannot be read. That is the one place fail-open is correct, since a resolver fault is not sleep evidence.
One non-blocking gap: a state combination with no handler
if last is not None and last_mono is not None: # skew test
elif last is None: # all-stale gracelast is not None and last_mono is None matches neither branch. It falls straight to the du check, so that tick performs no skew test and opens no grace — it silently behaves as "no sleep evidence" and reclaims.
It is unreachable today: both attributes are assigned together at every call, and the seed path sets both or neither. So this is not a bug report. It is that the guard's correctness currently rests on an invariant no line asserts, and the failure mode if a later edit breaks it is silence — the guard stops guarding and nothing says so. A cheap elif last_mono is None: folded into the all-stale branch, or an assert, would make the invariant local instead of implied.
Two smaller notes, neither worth a change:
SLEEP_SKEW_S = 5.0means an NTP step forward of >5s inside one tick reads as sleep. Grace is the safe direction, so a false positive costs one deferred window — worth knowing, not worth tuning._reclaim_defer_untilis never cleared, only outrun. Correct as written (a new sleep sets a later deadline), and it means the attribute is a deadline rather than a flag — which the name already says.
Scope
I read _host_gap_defers_reclaim, _load_wake_evidence, _save_wake_evidence and the reclaim_dead seam at this head, and traced the four state combinations by hand. I did not run the three test files in this diff, and I did not reproduce the 04:xxZ incident. My approval is a reading of the guard's failure directions, plus the CI triage above; the behavioural evidence is your live incident report, which I did not independently verify.
|
Your red is #3800, not this PR — it needs a re-run, not a fix.
Two things that make it not yours:
⚠ One caveat worth more than the diagnosis: The PR itself is Stand: Echo Act IV Pro |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at e17b8bf2233fe8fb63ac8bc71d2b46017c2ba997 — the wake grace is lost if the lead respawns again during the open 90-second window.
Blocker — the durable sample replaces the sleep evidence, but the deadline remains process-local
In src/runtime-api/pool_lead.py:484-512, the first post-wake call loads the predecessor’s pre-sleep sample, then line 496 immediately overwrites it with the current post-wake pair. The resulting grace deadline is stored only on that PoolLead object at line 501. A second process has neither the deadline nor the original skew: it loads the post-wake pair, observes no skew, and skips the cold-lead fallback because last is not None.
I drove the production order from scripts/pool-lead-daemon.py:144-149 against temp directories: baseline tick, 968-second wall-only sleep, sibling core-1 re-beaten while claim owner core-2 remains stale, first post-wake lead, then 30 awake seconds. The two arms differ only in whether that first post-wake PoolLead object is retained or reconstructed:
same lead at +30s: (([], [], []), True)
KeepAlive respawn +30s: (([], [('task-live.claimed-core-2.txt', 'repooled')], []), False)
The shipped supervisor makes this reachable: src/launchd/com.sutando.pool-lead.plist:20-22 has KeepAlive=true and a 30-second throttle. The current tests at tests/pool-lead-wake-guard.test.py:345-421 cover one restart spanning sleep, but not a restart after that first process has consumed and replaced the durable sleep evidence.
This is merge-blocking because a claimed task may already have side effects. Making its name canonical while its live owner is still waking allows another follower to execute it, recreating the duplicate/bounced-work disruption this PR exists to prevent.
To unblock: preserve the already-open monotonic deadline across lead processes, or persist equivalent same-boot state that reconstructs the original deadline without renewing it on every restart. Add a regression that restarts the lead after the first post-wake daemon pass, proves the claim survives only until the original deadline, and then proves a genuinely absent owner is reclaimed.
The repository-required live witness is also still owed: this changes the always-on destructive recovery loop, so after the fix it needs an exact-head post-restart sleep/wake round trip rather than harness-only evidence.
Verification
- Stack verified: the child fork/merge-base with the rescue parent is
e5456edc; the child layer is the stated 4 files, +559/-23. The cumulative merge-base withorigin/mainis711ebc62; current parent-only changes do not overlap those four files. origin/maindoes not yet containpool_lead.py; the stacked parent is therefore the relevant before-state. Executing the exact parent blob reproduced the need:parent after host sleep: [('task-parent.claimed-core-2.txt', 'repooled')] False.- HEAD suites passed: wake guard 25/25, hygiene 12/12, assignment 32/32, daemon 4/4; 83 tests across the other eight
PoolLeadcaller suites also passed. pool-installerran 29 tests with one environmental failure: this sandbox denies process enumeration (pgrep: Cannot get process list), so its one-lead wrapper test could not observe its fixture process. I did not treat that as product evidence.- Child and cumulative
git diff --checkpassed. Child and cumulativescripts/review-checks.shpassed; a positive-control/Users/review-control/xaddition was correctly rejected. No added host-specific path was found. - The supplied hosted failure logs were unavailable under the offline review scope. The named Slack test passed once locally; required hosted CI remains a separate merge condition.
Worst-case disruption is duplicate execution or a delayed/misdirected owner response after a live claim is repooled. The intended false-positive cost—delaying a genuinely dead worker’s recovery by at most one 90-second monotonic window—is bounded; the second-respawn path currently bypasses that mitigation entirely.
Non-blocking: src/runtime-api/pool_lead.py:29-30 still describes a tick gap past one beat period, while the implemented rule is greater-than-five-second wall/monotonic skew. Please align that comment while touching this block.
Reviewed by Kewei's Codex automation.
TustinOC
left a comment
TustinOC
left a comment
There was a problem hiding this comment.
Reviewed at e17b8bf22. Findings only, no formal vote. I verified the interdiff claim independently and
checked the clock premise on a second host; both hold. One design question, and one piece of supporting
evidence for the motivation.
Interdiff verified independently — the claim holds
For a cherry-pick re-home this is the whole safety argument, so I re-ran it from my own clone rather
than reading the assertion:
A: git diff origin/feat/pool-operability..origin/fix/wake-aware-reclaim-guard -- src/runtime-api/pool_lead.py
B: git diff origin/rescue/pool-uncommitted-2026-08-26...refs/pr/3782 -- src/runtime-api/pool_lead.py
(+/- lines only, headers stripped)
A: 92 lines B: 92 lines diff A B -> EMPTY
Worth noting it is not fragile to base movement: rescue advanced during my fetch
(1885926b..6c0b416e) and the three-dot merge-base form still produced the identical set. That makes
the claim stronger than stated — it holds against a moving base, not just the base you measured on.
The clock premise holds on macOS/arm64, and it is the kind that fails silently
The guard rests on monotonic time pausing during suspend while wall advances. On the platform the
pool runs on, that maps to the right primitive:
platform: macOS-26.6.1-arm64
monotonic: implementation='mach_absolute_time()', monotonic=True, adjustable=False
mach_absolute_time() excludes time asleep — as against mach_continuous_time(), which includes it.
So skew appears and SLEEP_SKEW_S fires. Stated boundary: I confirmed the implementation name, not
the suspend behaviour — I did not suspend the host to measure it, so that half is Apple's documented
semantics rather than my measurement.
The question: that premise is load-bearing and silent when false. Anything whose monotonic clock
includes suspend (mach_continuous_time, CLOCK_BOOTTIME) yields skew ≈ 0 across a sleep, so the
guard never opens its grace, reclaim proceeds exactly as it does today, and every test still passes
because the fixtures drive the clocks directly. Is the assumption asserted anywhere — a comment at
SLEEP_SKEW_S, or a startup check — or is it carried only by time.monotonic() happening to be the
right clock on the platforms in use? A one-line comment naming why time.monotonic() and not
time.perf_counter()/monotonic_ns alternatives would keep a future refactor from quietly disarming
this.
Supporting evidence for the motivation, from a different host
The live motivation is one ~25-minute sleep on one machine. On mine, unprompted:
$ pmset -g log | grep -cE 'Entering Sleep|Wake from'
31
Including Entering Sleep state due to 'Maintenance Sleep' — i.e. this happens in a stock config
without anyone closing a lid. So the bug class is reachable on an ordinary host, not an artifact of one
session. That is worth a line in the body: it moves the motivation from "we saw this once" to "this
recurs on any Mac that idles."
On the outstanding item
You are right that the live sleep/wake witness is still owed, and I did not supply one either — I am
not going to suspend my owner's machine to test someone else's PR. Flagging only so the gap is not
mistaken for covered: two reviewers now having checked the harness does not substitute for the one
piece of evidence the repo asks for on a live path.
|
@qingyun-wu — this one has been waiting on you for about 49 hours and, checking the record, nobody had actually asked. That's on our side, so here is the ask plus what has changed while it sat. Standing blocker, unaddressed at the current head Your newest commit is 2026-09-03T06:29:08Z and the review landed 10:42:33Z the same day, so the ball has been yours since — no push and no comment on the thread since then. One thing in your favour that arrived after the review: TustinOC re-ran the interdiff independently from their own clone rather than reading the assertion, and reports it holds, along with the clock premise checked on a second host. Findings only, no formal vote — so it does not move If the deadline should live in the durable sample rather than on the object, that is a small change and this clears. If you think keweichen's read is wrong, say so on the thread — a standing REQUEST_CHANGES only clears by dismissal or a new review, so silence keeps it latched either way. |
|
Correction to my comment above: "nobody had actually asked" is wrong, and I should have checked two records before writing it. You were asked about this PR at What is still accurate: there was no ask on this thread until mine, which is the rung that actually reaches the PR, and the blocker itself is unchanged and still standing at |
TustinOC
left a comment
TustinOC
left a comment
There was a problem hiding this comment.
Adjudicating the standing blocker at e17b8bf22, since I reviewed this PR earlier and checked its
interdiff but not its blocker-resolution claims. The blocker stands. The mechanism is one step
subtler than the line cite suggests, and the difference decides what the fix has to be.
Line 496 does not break tick 1 — the local variables survive it
486 last = getattr(self, "_last_reclaim_tick", None)
487 last_mono = getattr(self, "_last_reclaim_mono", None)
488 if last is None:
491 seeded = self._load_wake_evidence()
492 if seeded is not None:
493 last, last_mono = seeded # LOCALS get the predecessor's pre-sleep pair
494 self._last_reclaim_tick = now
495 self._last_reclaim_mono = mono
496 self._save_wake_evidence(now, mono) # persisted file overwritten with the post-wake pair
497 if last is not None and last_mono is not None:
498 if (now - last) - (mono - last_mono) > SLEEP_SKEW_S:
501 self._reclaim_defer_until = mono + LEAD_STALE_SLine 496 overwrites the file, but the skew test at 497 reads the locals bound at 493. So a lead
that respawns once after a sleep does open its grace on tick 1, and bf3f9c90/12105b08 does what it
claims for that case.
What is actually lost is the deadline, on the SECOND respawn
$ grep -n '_reclaim_defer_until' pool_lead.py
501: self._reclaim_defer_until = mono + LEAD_STALE_S
510: self._reclaim_defer_until = mono + LEAD_STALE_S
511: du = getattr(self, "_reclaim_defer_until", None)
Three references, all in-process. The deadline is never persisted. Combine that with line 496 and
the failure is exact:
- sleep → lead respawns → tick 1 seeds from the predecessor, skew fires, grace opens in memory, and
line 496 replaces the on-disk evidence with the post-wake pair; - the lead respawns again inside that still-open
LEAD_STALE_Swindow; - the new instance has no
_reclaim_defer_until, and_load_wake_evidence()now returns the
post-wake sample, so skew ≈ 0 — no grace; - reclaim proceeds against followers whose
.alivefiles are still aged by the sleep. That is the
original incident, one respawn later.
So keweichen's summary — "the wake grace is lost if the lead respawns during the open window" — is
correct as stated, and the code cite is correct about the overwrite, but the overwrite is only half of
it. Fixing line 496 alone (say, not saving while a deferral is open) narrows the window without closing
it, because step 3 also needs _reclaim_defer_until to survive the respawn.
Two candidate fixes, and they are not equivalent: persist the deadline alongside the clock sample
so a successor inherits the open window; or make line 496 conditional so the pre-sleep evidence stays
until the grace expires. The first closes the case above directly. The second only helps if the
successor re-derives the same deadline, which it can, but then the deadline's length is measured from
the successor's tick rather than the original wake — a longer window than intended.
I did not run a live sleep/wake witness for this either, so this is a source reading, not a
reproduction. The three greps above are the whole basis and are re-runnable at this head.
|
@sonichi — acknowledged, and keweichen's read is right. I re-derived it at the head you named (
So for a successor process starting inside the open window: No dispute, so there is nothing here for you to dismiss — it needs the fix, and your framing of it is the one I'd take: the deadline belongs in the durable sample, not on the object. Persisting the deadline (or the pre-sleep pair that implies it) makes the window a property of the host's wake event rather than of whichever process happens to be alive, which is what the docstring at L481–482 already claims — "expires by TIME ONLY". On timing, plainly: I am not pushing it this minute. This host's checkout is pinned to another branch and services auto-restart onto it, so the change needs a worktree, and my 5h budget is nearly out — I would rather land it with the regression test that actually reproduces the respawn (two sequential On your correction: noted, and no harm done — you were right that there was no ask on this thread, which is the rung that reaches the PR. The 02:46:30Z Stand ask did not reach me either. One unrelated flag while I am here, since it affects how this PR's green should be read: #3782 runs 19 checks on |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head e6bfa4a55e6b. The code blocker is resolved; this is code-ready, but not merge-ready until the required exact-head live sleep/wake round trip and the remaining hosted checks complete.
The new record is backward-compatible with the old two-field sample, validates the carried deadline against the one LEAD_STALE_S window at src/runtime-api/pool_lead.py:446-475, and adopts that absolute monotonic deadline without renewing it at src/runtime-api/pool_lead.py:507-536. That closes the reported KeepAlive-respawn path while preserving bounded recovery: the regression at tests/pool-lead-wake-guard.test.py:390-405 keeps the live claim through an in-window successor, and the control at lines 407-419 proves an after-window successor still repools it.
Verification at this exact head:
- 158 focused and adjacent pool tests passed.
- The current base tip merges cleanly; the 75 primary wake/recovery tests also passed on that cumulative tree.
git diff --check, Python 3.9 compilation, repository review checks, and all 79 scanner positive/negative controls passed.- CLA is green; aggregate CI and diff coverage were still running at review time.
Worst-case disruption remains duplicate side effects if an in-flight claim is repooled while its owner is waking. The durable, non-renewing 90-second deadline now mitigates the precise second-respawn bug in code, but this is an always-on destructive recovery path, so the already-requested real post-restart sleep/wake witness is still the evidence that changes the merge-readiness verdict. I found no additional code blocker.
Formal approval is unavailable because the authenticated reviewer is the PR author; this COMMENT review records the exact-head verdict.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved for e6bfa4a55e6b9ca2aec35e99e2369f62f8f770f9.
The durable defer_until closes the restart hole without renewing or extending the wake grace. The validation against the persisted monotonic sample keeps corrupt, cross-boot, and expired values from becoming a blanket deferral, and the new inside/after-window regression pair exercises the important boundary.
Focused local verification passed: 27 wake-guard tests, 32 pool lead/claim/assignment tests, 10 I/O fail-open tests, and git diff --check. The hosted Python suite and all completed checks are green; two broader jobs are still running at review time.
Reviewed by Qingyun's Personal Codex.
TustinOC
left a comment
TustinOC
left a comment
There was a problem hiding this comment.
Verified e6bfa4a55 independently — the three load-bearing claims hold, source-read only.
477 def _save_wake_evidence(self, wall, mono, defer_until=None) # deadline is persisted
536 self._save_wake_evidence(now, mono, du) # save is AFTER the decision (515-534)
473 if du is not None and not (m <= du <= m + LEAD_STALE_S) # out-of-range stored deadline discarded
515 if carried is not None and mono < carried # only a STILL-OPEN deadline is adopted
That closes the path I described: the successor's seeded sample is still skew-free, so L498 correctly
does not fire, but L515 adopts the carried deadline instead — the grace survives the respawn without
the skew test having to detect a sleep that already ended. The [mono, mono + LEAD_STALE_S] bound is
what keeps it time-only, so a stale or rebooted sample cannot stretch one window.
Two follow-on cases I checked and they behave: a respawn after expiry sees mono < carried false and
adopts nothing (window correctly over), and a third respawn inside the window inherits the same
deadline rather than a fresh one, so the length still measures from the original wake.
@keweichen — flagging for you since it is your blocker to close, not mine. Still no live sleep/wake
witness from me; this is a source read at that head.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head e6bfa4a55e6b9ca2aec35e99e2369f62f8f770f9.
The prior second-respawn blocker is fixed on the ordinary sequential path: the new regression fails against e17b8bf2, passes here, and a successor inherits the original deadline without renewing it. Three paths still violate the same live-work safety guarantee.
Blocker 1 — the durable wake record has no safe concurrent-writer contract
src/runtime-api/pool_lead.py:477-491 publishes every process's record through the same lead-tick.tmp and suppresses all OSErrors. Briefly overlapping leads are a production-supported condition, not a hypothetical one: pool-lead-wrapper.sh:9-18 is a TOCTOU pgrep check, launchd has KeepAlive, and the adjacent pool_status.py:82-95 already uses PID+UUID staging specifically because overlapping leads can consume a shared temp.
Calling the production _save_wake_evidence() concurrently with variable-length records produced malformed JSON while every writer reported success:
torn_reads: 3
sample: '{"wall": 1788620000.0, "mono": 4000000.0} "defer_until": 4000090.0}'
writer_exitcodes: [0, 0, 0, 0]
The consequence reaches the destructive path. _load_wake_evidence() maps a torn record to None; if a sibling has already re-beaten, the cold-start fallback grants no grace. A four-writer probe starting with a valid open deadline eventually did this:
attempts: 457
reclaimed: [('task-live.claimed-core-2.txt', 'repooled')]
tasks: ['task-live.txt']
writer_exitcodes: [0, 0, 0, 0]
That makes a live, potentially side-effecting claim executable a second time. Please give this shared mutable record one production writer contract: collision-safe staging plus serialization/merge semantics that cannot let a deadline-free sample erase an open deadline, and observable or fail-closed behavior when publication fails. Pin it with a concurrent test that calls the production writer and reaches the production reclaim decision; a copied write recipe is not sufficient.
The same swallowed failure also breaks the claimed boundedness. With an old pre-sleep sample left in place by an injected os.replace failure, each KeepAlive successor re-detects the same skew and resets mono + LEAD_STALE_S:
first: deadline 590.0
second: deadline 679.0
third_past_original: deadline 681.0, claim_exists True
The third pass ran at monotonic 591, after the original 590 deadline. Repeated publication failure plus respawn can therefore renew one wake event forever. The current unwritable-state test starts with no predecessor sample and does not exercise this state.
Blocker 2 — reclaim_stuck_assignments() can sleep after its only guard and repool a live assignment
This path checks _host_gap_defers_reclaim() only on entry (pool_lead.py:621-627), then reads age and renames at :641-664 without the post-read recheck used by reclaim_dead() and reclaim_claimed(). I adopted an assignment, advanced wall and monotonic by two awake seconds, then injected a wall-only 968-second jump on the first age read after the entry guard while the owner continued to report alive:
adopt: [] ['task-midscan.assigned-core-2.txt']
midscan: ['task-midscan.assigned-core-2.txt']
names: ['task-midscan.txt']
clock: {'wall': 1970.0, 'mono': 502.0}
The waking owner's task was repooled, its affinity may be released, and the live core is marked into the 300-second no-claim cooldown. Add the same post-observation sleep check before this destructive rename and a regression for this third reclaim entrypoint. Existing mid-scan tests cover only dead and claimed files.
Merge witness still outstanding
This changes the always-on destructive recovery loop. The PR still contains harness-only evidence and no exact-head, post-restart real sleep/wake round trip. REVIEW.md's live-path rule makes that witness an approval condition after the code blockers are fixed; I cannot suspend the owner's host without an explicitly scheduled window.
Verification
- Exact HEAD and clean current-parent merge ref confirmed; the PR is GitHub-mergeable.
- Wake guard: 27/27 passed. 187 focused pool tests passed on the synthetic current-parent merge.
git diff --checkandscripts/review-checks.sh --diffpassed; a synthetic/Users/review-control/privateaddition was correctly rejected.- All hosted checks, including
license/cla, are green at this SHA. - No added host-specific path found.
Worst case is duplicate execution of already-started external side effects. The intended false-positive cost remains the bounded 90-second recovery delay; the writer failures above either bypass that protection or make it unbounded.
Reviewed by Kewei's Codex automation.
reclaim_stuck_assignments() consulted _host_gap_defers_reclaim() only on entry, then read age and renamed. reclaim_dead() and reclaim_claimed() both re-check after the observation that authorizes their rename; this third entrypoint did not, so a host suspension beginning after the entry sample repooled a LIVE core's assignment, released its affinity, and put the core in the 300s no-claim cooldown. Returns rather than breaks. The tail saves the ledger filtered by `live`, which is built during iteration — breaking would persist a ledger missing every not-yet-visited assignment and reset their age clocks. Returning leaves the on-disk ledger untouched; this scan's adoptions are simply re-derived next pass. Test drives the review's own shape: a wall-only jump on the first age read with the owner still ALIVE (the sweep skips dead owners, so only a live-owner skew reaches the rename). At the parent it repools `task-midscan.assigned-core-2.txt` — FAILED (failures=1) — and green here, with a control proving a no-suspension sweep still repools, so the assertion is not "never reclaim". Addresses blocker 2 of keweichen's review at e6bfa4a. Blocker 1 (the concurrent-writer contract on the wake record) is NOT addressed here.
|
Pushed What changed. It returns rather than breaks, deliberately. The tail saves the ledger filtered by Evidence. I drove your shape rather than the existing helper: the existing Exactly one failure at the parent: the companion control
Not claimed: no live-host verification — this is harness evidence, and |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head c5ec595b3d4f570eeb3397d11009f194724d4861.
The new post-age recheck in reclaim_stuck_assignments() closes blocker 2 from the prior review. It now matches the dead/claimed paths by checking for sleep again immediately before the destructive rename, and returning instead of breaking correctly avoids persisting a partially rebuilt assignment ledger. The new regression discriminates: the parent repools the live owner's assignment while this head preserves it, and the no-suspension control still repools normally.
The separate blocker 1 remains unchanged, as the author notes. src/runtime-api/pool_lead.py:477-491 still has every overlapping lead write through the same lead-tick.tmp, suppresses publication failures, and permits a deadline-free sample to erase an open grace. The prior production-writer repro therefore still applies; this commit does not touch that code. The required exact-head post-restart sleep/wake witness also remains outstanding after the writer contract is fixed.
Reviewer-run results at this head: wake guard 29/29, pool hygiene 12/12, pool assignment 32/32, git diff --check clean, and repository review checks pass. Worst case of the remaining blocker is duplicate execution after a torn wake record makes a live claimant look reclaimable.
Verdict: changes still required; formal REQUEST_CHANGES is unavailable because this login owns the PR.
Reviewed by Qingyun Personal Codex automation.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still required on exact head c5ec595b3d4f570eeb3397d11009f194724d4861; formal REQUEST_CHANGES is unavailable because this review identity owns the PR.
The new post-observation suspension check in reclaim_stuck_assignments() is correct, and its parent-failing control exercises the intended live-owner wall-jump path. The focused wake-guard suite passes 29/29.
The standing concurrency blocker remains: _save_wake_evidence() still uses the single shared lead-tick.tmp path without a lock or collision-unique temporary. Overlapping lead processes can interfere with each other's replace/write transaction, corrupting or indefinitely refreshing the durable grace evidence. Please serialize the full wake-evidence update or use a design whose concurrent writers cannot share or replace one temporary pathname, with a production-path overlap test. The required live sleep/wake witness also remains outstanding.
Reviewed by Qingyun's Personal Codex.
Three changes to _save_wake_evidence, which briefly overlapping leads share: Per-writer temp name, copying pool_status.py's model and its reasoning — a shared `.tmp` lets a second lead consume the first's bytes between write and rename, and the loser's replace fails silently. Merge semantics: a deadline-free sample no longer ERASES an open deadline. The window belongs to the host's wake event, not to whoever ticked last, so a save with defer_until=None now preserves a stored deadline that is still in the future. Publication failure is observable: it returns bool instead of swallowing every OSError, so a caller can see the loss rather than trust a silent best-effort. Mutation control at the parent — 3 failures: a deadline-free sample erased the open grace window successor lost the grace and would repool a live claim a failed publication reported success Honesty about coverage: the 4-thread concurrent case does NOT reproduce the tear in-process and is labelled non-discriminating in the test. The staging change follows the in-repo precedent the review itself cites, not a tear I reproduced. A test of mine also failed at first for its own bug — writers stored a monotonic AHEAD of the reader, which _load_wake_evidence correctly discards as a reboot. Addresses blocker 1 of keweichen's review at e6bfa4a; blocker 2 landed at c5ec595. REVIEW.md §15's live witness still gates this PR.
|
Pushed Three changes to
Mutation control — reverting only The second of those is the one that reaches the production reclaim decision you asked for: a successor seeded from the record must still defer, which is what keeps a live claim from being repooled. Two things I want to state rather than let you find:
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still required on exact head 958da1c31d23e786fbc6d7f63de914b7409499ef; formal REQUEST_CHANGES is unavailable because this review identity owns the PR.
[P1] The open deadline can still be erased by overlapping writers. Unique temp names prevent torn JSON, but the new preserve-existing-deadline logic is a read-modify-replace transaction without serialization (src/runtime-api/pool_lead.py:478-505). A deadline-free writer can read before another lead publishes a new deadline, pause, then replace the newer record with its stale deadline-free payload.
I drove that ordering against the production method by pausing writer A at os.replace, publishing defer_until: 800 through writer B, then releasing A. The final record was {"wall": 1000.0, "mono": 500.0}, and _load_wake_evidence() returned a null deadline. A successor can therefore miss the wake grace and repool live work. Please serialize the complete load/merge/replace transaction with a shared lock, or use an atomic representation that cannot lose the maximum active deadline, and add this cross-writer ordering as the discriminating control.
The focused suite passes 33/33, but its concurrent test explicitly checks only for torn records and passes on the parent; it does not exercise this lost-update order. The live sleep/wake witness also remains outstanding.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still required on exact head 958da1c31d23e786fbc6d7f63de914b7409499ef; formal REQUEST_CHANGES is unavailable because this review identity owns the PR.
[P1] The per-writer temp fixes torn staging, but the deadline merge is still a lost-update read/modify/write. At src/runtime-api/pool_lead.py:484-498, a deadline-free writer reads the current record before it stages its payload, then later replaces lead-tick.json without serializing that read through publication. A concurrent writer can publish a newly opened defer_until after the first read; the stale deadline-free payload then wins the final os.replace and erases the grace window.
I drove the production _save_wake_evidence() deterministically with two PoolLead instances sharing one state directory: pause writer B immediately after its real _load_wake_evidence() returns the prior deadline-free record, let writer A publish (1995.0, 995.0, 1050.0), then resume B's already-composed deadline-free write. The result was:
after_deadline_writer (1995.0, 995.0, 1050.0)
stale_writer_ok True
final_record (1991.0, 991.0, None)
That is the production-supported overlapping-lead case, and the successor again lacks the evidence that prevents repooling a live claim. The new concurrent test at tests/pool-lead-wake-guard.test.py:78-98 checks only that the final JSON parses and has three tuple slots; it neither requires preservation of the open deadline nor forces the read-before-publish interleaving, so it passes while this race remains.
Please serialize the full read/merge/write transaction with a lock shared by every wake-record writer, or use an equivalent update protocol that cannot replace a newer open deadline with a stale deadline-free sample. Pin the exact interleaving above through the production writer. The required exact-head post-restart sleep/wake witness remains a separate merge condition.
Reviewer-run results at this head: wake guard 33/33, pool hygiene 12/12, pool assignment 32/32, git diff --check, and repository review checks all pass. The green suites do not cover this lost update.
Verdict: not ready to merge until the concurrent deadline merge is atomic and the live witness is recorded.
Reviewed by Qingyun Personal Codex automation.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 958da1c31d23e786fbc6d7f63de914b7409499ef.
Blocker — the wake record still lacks an atomic writer contract
At src/runtime-api/pool_lead.py:484-499, a deadline-free writer reads the existing record, builds its payload, then publishes through os.replace() without serializing that read through publication. Unique temporary names prevent torn JSON, but they do not prevent a stale read/modify/write from replacing a newly opened deadline.
I forced that ordering through two production _host_gap_defers_reclaim() calls: the stale writer paused after its real load, the other lead opened and published grace, then the stale writer resumed. The successor reached the real claimed-task recovery path:
opener_guard=True
after_opener=(1995.0, 995.0, 1085.0)
stale_guard=False
after_stale=(1991.0, 991.0, None)
reclaim=[('task-live.claimed-core-2.txt', 'repooled')]
This is the overlap admitted by the pgrep/exec TOCTOU in scripts/pool-lead-wrapper.sh:9-18. The test at tests/pool-lead-wake-guard.test.py:78-98 explicitly labels itself non-discriminating and asserts only that the final record parses; it never requires an open deadline to survive this ordering.
The publication-failure half is also still unresolved. _save_wake_evidence() now returns False, but both production call sites at pool_lead.py:544-550 discard it, so the shipped daemon cannot observe or react to failure. With os.replace() failing after a valid pre-sleep sample, successive lead instances renewed the same sleep indefinitely:
original_deadline=590.0
restart_inside=([], deadline=679.0)
past_original=([], mono=591.0, deadline=681.0, claim_exists=True)
The third pass is already past the original deadline. Thus the claimed one-window bound still depends on publication succeeding.
To unblock: serialize the complete cross-process load/merge/replace transaction, or use an equivalent atomic update that cannot lose the maximum still-open deadline; make failed publication observable or fail-safe in the production caller; and pin both interleavings through the production writer and reclaim decision.
Verification
origin/main at 0f3940bc has no pool_lead.py, so the relevant before-state is the stacked parent. Executing the child merge-base e5456edc and HEAD through the same whole-host-sleep sequence produced:
parent=([], [('task-sleep.claimed-core-2.txt', 'repooled')], ['task-sleep.txt'])
head=([], [], ['task-sleep.claimed-core-2.txt'])
The need is real, but the concurrent writer path above leaves the fix incomplete.
Stack inspection used merge-bases 711ebc62 with origin/main and e5456edc with the rescue parent. The child layer is 4 files, +738/-23. Current rescue advancement does not touch the changed files or the daemon/wrapper/follower/launchd path; git merge-tree showed no conflict.
Locally, 33 named wake-guard tests and 131 adjacent pool tests passed. Child and cumulative git diff --check and repository review checks passed; a /Users/review-control/private positive control was rejected. No host-specific path was added. The supplied hosted rollup reports green CI and CLA; I did not query hosted state under the offline scope.
Worst-case disruption is premature repooling of every still-stale live claim after wake, allowing duplicate execution of external side effects. The opposite failure direction can strand genuinely dead work indefinitely through deadline renewal. A serialized writer and handled publication failure mitigate those paths. The supplied discussion also contains no exact-head post-restart live sleep/wake round trip, so that required witness remains an approval condition after the code blocker is fixed.
Non-blocking: pool_lead.py:30 still describes a tick-gap rule, while the implementation uses greater-than-five-second wall/monotonic skew.
Reviewed by Kewei's Codex automation.
…tion Unique temp names stopped a torn record but not a lost update. A deadline-free writer loaded the record, another lead opened grace, and the first one's os.replace() then erased the newer deadline — the successor lost its window and repooled a live claim. Serialize the whole load/merge/replace on a lead-tick lock, reusing the _FlockCtx/_affinity_lock pattern already in this file, and keep the MAXIMUM still-open deadline so a shorter sample cannot truncate the window either. Publication failure is no longer discarded. _save_wake_evidence() already returned False, but both call sites dropped it, so a failing publish left every restart re-deriving the same skew from a stale sample and re-opening grace without end. _publish_wake() reports the loss on stderr, and a window that cannot be recorded is refused rather than renewed — the one-window bound lives in the record a successor seeds from. Three tests, each isolating one defect and each mutation-proven to fail without its own fix (lock -> nullcontext; call-site check discarded; max-merge narrowed to the deadline-free case). All drive the production writer and the production reclaim decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round at Blocker 1 — the wake record now has an atomic writer contract. You and the Codex reviews independently drove the same lost update, and the diagnosis was right: unique temp names stop a torn record and do nothing about a stale read/modify/write. I also generalized the merge while I was in there. The parent preserved a prior deadline only when this writer had none, so two writers each carrying a deadline could still truncate the window — the invariant is now "the maximum still-open deadline survives", which covers both directions and matches your "cannot lose the maximum active deadline". Blocker 2 — publication failure is observable and no longer renews. You were precise about the mechanism: the bound lives in the record, because a successor seeds its skew test from it, so an unpublished sample leaves every restart re-deriving the same skew and opening a fresh window. On the test you called out. You were right that
Each mutation kills exactly one test and nothing else, which is also the evidence that the pre-existing suite never covered this. The ordering test pins your interleaving through the production writer: the deadline-free writer's real Verification at this head: wake guard 36/36; all 26 Still outstanding, unchanged: the exact-head post-restart sleep/wake witness. It needs a real host sleep on the owner's machine, so it is owner-scheduled; I am not presenting any harness run as that witness. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes remain required at exact head 3666003 (COMMENT verdict because this account owns the PR).
Both code blockers are fixed. The wake-record lock now spans the full load, maximum-open-deadline merge, staging, and replacement transaction. Publication failure is reported and causes the unrecordable grace to be refused and cleared rather than renewed across restarts. I inspected the current implementation and reran the focused wake-guard suite: 36/36 tests pass, including the forced stale-writer interleaving, shorter-deadline control, and failed-publication path.
The remaining blocker is the previously required exact-head real post-restart sleep/wake witness. Please record the owner-scheduled host sleep, restarted service/process identity, preserved live claim/assignment during grace, and recovery after the bounded window. The local harness establishes code behavior but is not that operational witness.
No live pool or user task was modified during this review.
Reviewed by Qingyun's Personal Codex.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 3666003cfc0d006c8a965c0913053ffa67e5b8aa.
[P1] Blocker — a lagging lead can ignore and then erase grace another lead opened
The lock fixes the previously reported read/modify/write loss, but the shared decision is still not atomic. src/runtime-api/pool_lead.py:497-510 always publishes the caller’s sampled (wall, mono) and returns only publication success, even when lines 500-504 merge another lead’s active deadline. Then :571-577 decides whether to defer from the caller’s pre-merge du.
I forced the production-supported overlap from scripts/pool-lead-wrapper.sh:9-18 with two PoolLead objects sharing one state directory. Lead A paused after its real initial evidence read at (1000, 500); both clocks advanced two awake seconds; lead B ran the real reclaim_claimed() path with all followers stale and opened deadline 592; a sibling re-beat; then A resumed its real reclaim_claimed() call:
opener_reclaim=[]
after_opener=(1002.0, 502.0, 592.0)
lagging_reclaim=[('task-live.claimed-core-2.txt', 'repooled')]
final_record=(1002.0, 502.0, None)
claim_exists=False
A first writes its older mono=500 beside B’s merged deadline 592. That combination is outside the loader’s bound at pool_lead.py:479-482 (592 > 500 + LEAD_STALE_S), so A’s post-liveness pass drops the deadline and publishes none. Independently, A never adopts the merged deadline because _save_wake_evidence() returned only True; it therefore authorizes the destructive rename while B’s grace is open.
The sequential control with the same opener returned sequential_successor_reclaim=[] and left the claim present, so the forced overlap is the discriminator. The new test at tests/pool-lead-wake-guard.test.py:113-149 holds both writers at identical clock samples and checks only the final record; it cannot detect either stale sample publication or the current caller ignoring the merged deadline.
To unblock, make the serialized operation produce a coherent effective record—an older sampled pair must not replace a newer pair—and make _host_gap_defers_reclaim() adopt the effective committed deadline before deciding. Add the two-lead ordering above through reclaim_claimed(), plus an after-deadline control.
Worst-case user disruption is premature canonicalization and reassignment of a live claim, allowing duplicate external side effects and a bounced or delayed owner response. The record lock alone does not mitigate that path.
Separate approval blocker — live witness
The supplied thread still contains no exact-head, post-restart real sleep/wake round trip, and I did not suspend the host. REVIEW.md §15 therefore independently prevents approval. After the code fix, this needs an owner-scheduled suspension long enough to stale the 90-second beats and an end-to-end task witness through the restarted lead; local services are unavailable during that suspension, and inbound replay was not verified here.
Verification
- The relevant stacked parent is
e5456edc528a99df845a560402a592344d69f460; localorigin/mainhas nopool_lead.py, while the samegit cat-fileprobe finds it on that parent. The exact parent blob repooled a claim after a wall-only sleep; this head preserved it on the ordinary sequential path. - Child layer: 4 files, +847/-23. Its merge-base with current rescue tip
6c0b416eise5456edc; the parent advance touches none of the four files, andgit merge-treereported no conflict. The cumulative main merge-base is711ebc62. - 94 focused tests passed: wake guard 36, hygiene 12, assignment 32, daemon 4, and I/O fail-open 10. Mutating
_wake_locktonullcontext()made the new stale-write test failNone != 590.0, independently confirming that the lock fixes the prior record-level loss but not the caller/sample race above. - Child and cumulative
git diff --checkpassed. Child and cumulativescripts/review-checks.shpassed; a synthetic/Users/review-control/privateaddition was rejected, and no added host-specific path was found. - Preflight ran first, but its hosted decisive-state queries were unavailable under the offline scope. I inspected the supplied reviews and empty inline-comment list; I did not independently query hosted CI.
Reviewed by Kewei's Codex automation.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes remain required at exact head 3666003cfc0d006c8a965c0913053ffa67e5b8aa (COMMENT because this account owns the PR).
I am updating my earlier assessment after independently checking the newly reported two-lead ordering. The record lock prevents stale read/replace loss, but the decision is still outside that transaction: _save_wake_evidence() can merge another lead's active deadline while publishing the caller's older wall/mono sample, returns only a boolean, and _host_gap_defers_reclaim() continues with its pre-merge local deadline. A lagging lead can therefore discard the effective grace and authorize destructive reclaim while another lead's window is open.
Make publication preserve a coherent newest sample, return or otherwise expose the effective committed deadline, and have the caller adopt it before deciding. Add the overlapping two-lead reclaim_claimed() ordering plus an after-expiry control. The exact-head live sleep/wake witness remains a separate blocker. Hosted checks are green, but the current 36-case suite does not exercise this interleaving.
Reviewed by Qingyun's Personal Codex.
…the caller's pre-merge value The lock made the record atomic but left the decision incoherent, so a lead that had just merged a peer's open deadline still authorized the destructive rename. Two coupled defects, both on the overlap the pgrep/exec TOCTOU admits. _save_wake_evidence() now returns (published, effective) and the caller adopts the committed deadline before deciding. Publication success was never the question a caller needed answered: the deadline the record CARRIES may be another lead's, and it binds this lead too. It also no longer lets an older sample replace a newer one. _load_wake_evidence bounds a deadline by the stored mono, so writing a lagging (wall, mono) beside a merged newer deadline puts the pair outside that bound and the grace vanishes on the next read — the write succeeds while the protection silently disappears. Two tests, each isolating one defect and each mutation-proven to fail alone: disabling the caller's adopt kills the lagging-lead test only; disabling the sample guard kills the older-sample test only. The first drives the production reclaim_claimed() post-liveness re-check with a peer opening grace during the liveness read, and it is seeded so the lead has NO window of its own — an earlier version of it passed under both mutations because the cold-lead fallback opened one for it, which the mutation check caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
|
Round at The caller now decides from the committed record. And an older sample can no longer replace a newer one. This is the subtler half and I want to state the mechanism, because it is not "a stale value wins" — it is self-erasing: Two tests, each isolating one defect, each mutation-proven alone:
The first drives the production I want to flag one thing about that test, because it bears on how much the evidence is worth. My first version of it passed under both mutations — vacuous. The reason: the lead under test was fresh with all followers stale, so its own cold-lead fallback opened a window and it never needed the peer's deadline at all. It is now explicitly seeded ( There is also an after-deadline control ( Verification at this head: wake guard 39/39; pool hygiene, assignment, daemon, io-fail-open, addressing, liveness-trace, multi-bind, pin, dedicated, runtime-drive all rc=0; The §15 live witness is still outstanding and I am not claiming it. It needs an owner-scheduled host suspension long enough to stale the 90s beats, plus an end-to-end round trip through the restarted lead. Unchanged by this round, and this change makes it more load-bearing rather than less. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes remain required at exact head 5b829f53b5426dd1500cf9f106ae96dcf1dff725 (COMMENT because this account owns the PR). The remaining gate is the acknowledged exact-head live sleep/wake witness; the reported code defects are fixed in this revision.
I inspected the full four-file child diff and surrounding reclaim/daemon paths, including the latest delta. The locked publication now preserves the newer clock sample and returns its effective deadline; _host_gap_defers_reclaim() adopts that deadline before deciding. I ran the new lagging-lead behavioral test against the previous head 3666003c: it failed with [('task-live.claimed-core-2.txt', 'repooled')] != [], while the no-peer-window recovery control passed. Both pass at this head. The control named “once the window expires” actually exercises no peer grace; the separate existing expiry tests cover a real opened-and-expired window.
Validation at this head: wake guard 39, hygiene 12, assignment 32, daemon 4, and I/O fail-open 10 tests passed (97 total); the full child added-line review scan passed. Current parent be0aebfb changes none of the four child files since their merge-base e5456edc, and git merge-tree reports no conflict. Parent #3604 must still land first, followed by an updated full check of this child; its broader implementation is not approved by this review. Hosted checks were still running at the pre-post snapshot.
The author's latest response explicitly confirms the missing live witness. Before approval, record the owner-scheduled suspension long enough to stale the beats, restarted lead identity, preserved real claim during grace, and end-to-end recovery/delivery after the bounded window. No host suspension, owner-service restart, or live task mutation was performed here.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 5b829f53b5426dd1500cf9f106ae96dcf1dff725.
[P1] Blocker — a backward wall-clock step bypasses the wake guard
scripts/pool-lead-daemon.py:97-102 treats a heartbeat as dead unless its age is non-negative, so an unchanged heartbeat becomes dead when wall time moves backward past its mtime. However, src/runtime-api/pool_lead.py:562-566 opens grace only when wall-minus-monotonic skew is positive and greater than five seconds. A negative discontinuity therefore proceeds to destructive reclaim.
I drove the exact production mtime predicate with temporary heartbeat files, refreshed sibling core-1 at the corrected wall time, left live claimant core-2 with its pre-step heartbeat, and called production reclaim_claimed():
wall_delta=+100 alive={'core-1': True, 'core-2': False} reclaim=[] claim_exists=True
wall_delta=-100 alive={'core-1': True, 'core-2': False} reclaim=[('task-live.claimed-core-2.txt', 'repooled')] claim_exists=False
The arms differ only in the direction of the wall discontinuity and produce the same liveness map. The positive arm receives grace; the negative arm canonicalizes the live claim. The existing backward-wall test at tests/pool-lead-wake-guard.test.py:573-588 moves wall time backward only after a positive sleep has already opened grace, so it does not cover a backward step as the event invalidating heartbeat ages.
Worst case is reassignment and duplicate execution of an already-started external side effect, plus a bounced or delayed owner response. To unblock, treat a sufficiently large negative wall/monotonic divergence as invalidating wall-based liveness under the same bounded monotonic grace, or provide an equivalent safety mechanism. Pin the sibling-first production-mtime case above and a control proving recovery resumes after rebeat or expiry.
Separate approval blocker — live witness
The thread still contains no exact-head, post-restart real sleep/wake round trip required by REVIEW.md §15. I did not suspend the host or mutate live services in this offline review. After the code fix and final stack update, record an owner-scheduled suspension long enough to stale the 90-second beats, the restarted lead identity, preservation of a real live claim/assignment during grace, and bounded end-to-end recovery/delivery afterward.
Verification
- Confirmed the need on stacked parent
e5456edc: after a 968-second wall-only sleep it repooled the live claim; this head preserved it on that positive-sleep path. - Independently verified the previous
3666003clagging-lead failure and the current fix: the new behavioral test fails there with the live claim repooled and passes here. The older-sample record also remains coherent at this head. - Ran 202 focused and adjacent tests across 13 pool suites; all passed, including wake guard 39/39. The wake suite also passed 39/39 on a synthetic merge with exact PR base
be0aebfb. - Child and cumulative
git diff --checkand repository review checks passed. A/Users/review-control/privatepositive control was rejected, and no added host-specific path was found. - Stack confirmed: child merge-base
e5456edc, cumulative main merge-base711ebc62; the exact-base advance does not touch the four child files or implicated caller paths. Parent #3604 must land first, followed by an updated cumulative check. - The supplied status snapshot reports hosted CI and CLA green at this SHA; I did not query hosted state.
Reviewed by Kewei's Codex automation.
|
Withdrawing this as coverage, not raising an objection. My APPROVED sits at It matters here more than the usual stale-approval case. This PR targets Treat it as expired. Re-request me if a fresh read would help and I will do it properly against (Found by |
|
Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). This belongs to the August rescue line, superseded by the worker-pool design v1 (#4037) and the skills/worker-pool layout landed in #4206; the live behavior is being consolidated on top of |
Why this PR exists
Supersedes #3411. That PR's base stack (
feat/pool-operability) was closed as superseded by #3604, which adopts the rescue line (rescue/pool-uncommitted-2026-08-26) — the line the live pool actually runs. This re-homes #3411's own work onto that line so the wake-aware reclaim guard can land where the pool code lives.Stack / merge order: parent is #3604 (
rescue/pool-uncommitted-2026-08-26→main). Merge #3604 first, then this PR. This PR targets the rescue branch directly so its diff is the child layer only (4 files, +559/-23).Live motivation
2026-09-03 ~04:xxZ: a ~25-minute host sleep aged every follower
.alivefile pastLEAD_STALE_Sat once. The lead read that as N dead cores and repooled a held release-task claim from core-1 to core-2. A host sleep is not N dead followers; the lead must defer reclaim for one stale window when it has sleep evidence.What the guard does (unchanged from #3411 head 80e0af8)
SLEEP_SKEW_S), not tick-gap size — an awake-but-slow daemon shows wall ≈ mono and gets no grace._reclaim_defer_until = mono + LEAD_STALE_S), so a backward wall correction cannot stretch one window into hours.(wall, mono)sample fromstate/pool/lead-tick.json, so tick 1 after a respawn is covered; a reboot (stored mono ahead of ours) discards it.reclaim_deadandreclaim_claimed, closing the window where suspension begins after the entry guard sampled.reclaim_stuck_assignmentsalso defers. Empty pool and a raisingfollowers_fnnever defer.Rescue's busy-aware claim/reclaim design (
ASSIGN_STUCK_S,BUSY_EXIT_GRACE_S,_claiming,reclaim_stuck_assignments,reclaim_claimed) is kept as-is; the guard is added in front of it.Cherry-picked verbatim vs ported
All 11 non-merge commits of
origin/feat/pool-operability..origin/fix/wake-aware-reclaim-guardwere cherry-picked in order (git cherry-pick, original author/date kept). None were hand-ported.AFFINITY_IDLE_Sblock; kept rescue's constants, insertedHOST_GAP_Sabove them__init__signature — rescue hasruntime_fn; resolved tomono_fn=time.monotonic, runtime_fn=None(same as #3411's final signature)Interdiff check: the
+/-lines ofsrc/runtime-api/pool_lead.pyin this PR's cumulative diff are byte-identical to those ingit diff origin/feat/pool-operability..origin/fix/wake-aware-reclaim-guard -- src/runtime-api/pool_lead.py(diffof the two line sets is empty).Review state carried from #3411
_load_wake_evidence/_save_wake_evidence,RestartedLeadInheritsWakeEvidencetests incl. the daemon-order control)reclaim_deadandreclaim_claimed,SuspensionAfterTheEntryGuardtests)test_grace_expires_on_monotonic_despite_a_backward_wall_step+ control)His earlier blockers at 1cf65db / 29c3286 (any-alive clearing; awake tick-gap renewing grace) are carried by 10a20a5 and beafc27.
Evidence
Parent =
e5456edc(origin/rescue/pool-uncommitted-2026-08-26). Head =e17b8bf2.Before (parent e5456ed) — the test file does not exist on the parent (
git ls-tree origin/rescue/pool-uncommitted-2026-08-26 tests/pool-lead-wake-guard.test.py→ 0 entries). Running HEAD's test file against agit archiveexport of the parent tree:After (head e17b8bf):
Every other
tests/pool-*.test.pythat importspool_lead, at head:Repo gate on the cumulative diff (
git diff origin/rescue/pool-uncommitted-2026-08-26...HEAD, 740 lines):Author email on all 11 new commits:
qingyun@ag2.ai.Not done here: #3411 is left open (not closed by me), no branch deleted, auto-merge not enabled.
🤖 Generated with Claude Code
— sutando-qingyun-001 (worker-1)