fix: bound a running actor row's claim to be in progress (dead children shown as progressing/stalled) - #1965
Open
wqymi wants to merge 1 commit into
Open
fix: bound a running actor row's claim to be in progress (dead children shown as progressing/stalled)#1965wqymi wants to merge 1 commit into
wqymi wants to merge 1 commit into
Conversation
`deriveLiveness` placed unbounded trust in a `running`/`pending` registry row. Two consequences, both of which make the orchestrator's child roster lie: - a child that died BEFORE its first turn returned `progressing` forever, because `turnCount === 0` returned early and skipped the staleness window entirely; - a child that died AFTER some turns kept reading `stalled`, and `stalled` is presented as "in progress" — routable, and implying work is already in flight. ActorRegistry's orphan sweep is the only repair for such a row, and it runs once at process init and only for rows carrying a different instance_id. Until it runs (and for any row it cannot reach) the derivation must not assert progress. Adds DEFAULT_LIVENESS_ABANDON_MS (30m), measured from the last turn for a started child and from spawn time for one that never started. The turnCount-0 leniency is kept — a queued or cold-starting first turn is still `progressing` — but bounded instead of unbounded. Past the bound the row reads `idle`: the honest reading and the only non-routable bucket.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this matters more than a display nit
The orchestrator's
<active-sessions>roster exists for exactly one purpose: make theorchestrator route work to an existing child instead of creating a new one — that is
#1741's whole point. So a dead child displayed as
progressingis worse than no rosterentry at all: the orchestrator routes work into a session that can never answer, and
progressingadditionally suppresses re-dispatch because something appears to already bein flight. Every consumer of
deriveLivenessinherits this: the roster,session list(which groups
progressing/stalledunder the heading "In progress"),session status,and the fleet table. #1741's route-to-existing mechanism is only as good as the liveness
signal it reads, so an over-optimistic signal weakens it directly.
stalledis not a safe fallback either. It also lives under "In progress" and it alsomarks the child as routable; it only says "no recent turn", not "nobody is home".
The defect
deriveLiveness(src/actor/schema.ts) placed unbounded trust in arunning/pendingrow. Two shapes:
progressingforever.if (actor.turnCount === 0) return "progressing"returned early and bypassed the staleness window entirely. The comment'sintent is right — a queued or cold-starting first turn must not be misread as a stall —
but it implemented that leniency as unbounded.
stalledforever, i.e. still "in progress".The only repair for a row whose owner died is
ActorRegistry's orphan sweep(
registry.ts:UPDATE actor_registry SET status='idle', last_outcome='failure', last_error='orphaned: process restarted' WHERE status IN ('pending','running') AND instance_id != $instanceID). That sweep runs once, at process init, and only for rowscarrying a different
instance_id. Until it runs — and for any row it cannot reach — thederivation is the only thing standing between a corpse and the router, and it asserted
progress. Same defect class as #1960's orphaned
runningtool parts: a persisted transientwhose repair lives on exactly one path.
The fix
One new constant and one guard, both in
deriveLiveness:Reference clock per row shape: a started child's evidence of life is its last turn; a
never-started one has none, so its spawn time (
time.created) is the only honestreference — which is what B needed: a longer bound measured from spawn, not removal of the
leniency.
Why 30 minutes, picked the way
DEFAULT_LIVENESS_STALL_MS's comment picks 90s:updateTurnis a per-step heartbeat, so a child that has genuinely begun cannot go 30mwithout bumping
last_turn_time— that is an order of magnitude past the 5-minutestuck-detection cutoff (
STUCK_THRESHOLD_MS) and unreachable by a live turn. A child thathas not begun is waiting on the concurrency gate, which admits work continuously, so 30m
of zero admissions means its process is gone.
Past the bound the row reads
idle— already documented as "finished with no recordedoutcome (or an unknown state)" — and
idleis the one bucket no consumer treats asroutable or in-flight. Deliberate direction of error: past the bound we would rather
report a still-queued child as
idle(worst case: the orchestrator creates a duplicate,which is wasteful) than as
progressing(worst case: work routed into a corpse andre-dispatch suppressed, which is unrecoverable).
Nothing is written to the database and no new repair mechanism is introduced; this is
purely the read side becoming honest.
abandonMsis a 4th parameter with the sameoverride shape
stallMsalready has.What I could NOT confirm, and the named follow-up
The report I worked from assumed the registry "never reconciles rows whose process is gone
— they never even became
idle". Measured on the two real fixture rows, that is nottrue, and I am reporting it rather than fixing a defect that is not there. Both rows in
the isolated dev home read:
ses_05c0af252ffeigpmiJbqaa8vUV("Fix calc.py add() bug")idlefailureorphaned: process restartedses_05c08a1e9ffeFg2DEKBuhhBxuS("Modernize docs/README.md install steps")idlefailureorphaned: process restartedAll six orphaned rows share
time_completed = 1785163573647— one sweep, at process init.So the sweep works and reached exactly these rows; the roster that showed
stalled/progressingwas rendered while they were stillrunningunder the then-liveinstance, and reconciliation only came at the next process start. That reframes the
registry-side defect from "no reconciliation exists" to "reconciliation is bound to
process init, so it cannot see a row whose owner is the CURRENT instance".
That residual half is genuinely larger than this PR:
Also worth flagging for reviewers:
<active-sessions>andlistPeerChildrenare not onmain(#1741 is still open, head8847d324e) — zero grep hits. The roster itselftherefore cannot be tested here; the on-
mainconsumers of the same signal aretool/fleet.tsandtool/session.ts, and both are covered below.Tests — one per defect, each revert-probed
test/actor/liveness.test.ts(+4) andtest/tool/fleet.test.ts(+1). The fleet test usesthe two measured fixture rows' real field values (turnCount 26 / 20) replayed a day after
their last turn, and asserts through
assembleFleet— the consumer — that neither lands ina routable bucket.
Every pre-existing expectation is unchanged; the existing literals only gained the
timefield the widened
Picknow requires, and the existing "10-minute-old cold start is stillprogressing" case still passes (10m < 30m), which is the point of keeping the leniency.
Probe 1 — restore the unbounded
turnCount === 0early return (defect B):Probe 2 — keep B's spawn bound, drop the bound for rows that HAVE run a turn (defect A):
Suite numbers
bun typecheckexit 0.Scoped run only (
test/actor+ everything that references the signal, found withrg -l 'deriveLiveness|active-sessions|listPeerChildren' test), because concurrentheavyweight suites on this host starve each other. Same command both sides
(
bun test test/actor test/tool/fleet.test.ts test/tool/session-tool.test.ts --timeout 120000),same worktree and
node_modules, the three changed files reverted toorigin/mainfor thebaseline:
origin/main(60af8f1)+5 testsis exactly the five new cases. My failure set is a strict subset of thebaseline's: both runs fail
spawn no-deadlock (F56) > checkpoint-writer settles (no hang) when session permission is '*':'ask' …, and the baseline additionally fails foursession tool joincases on their 30s timeouts. The runs were sequential rather thanside-by-side, so the four extra baseline failures are load-dependent flakes, not a
regression — none of them is touched by this change, and none appears on this branch.