fix(orchestrator): bound the completion release retry, and publish the sweep bounds that already exist - #379
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
@coderabbitai review Requested for exact head |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…e cadence that cannot
THIS COMMIT DOES NOT ADD A BOUND. It publishes the ones that already exist,
because their absence from the health stanza has now been read twice as their
absence from the code — including in the brief that asked for this fix.
WHAT THE STANZA SAID. A wedged 0.1.76 published:
"readinessReconcile": {
"state": "healthy", "consecutiveFailures": 0,
"failureThreshold": 3, "inFlightMs": 268232, "intervalMs": 60000
}
`intervalMs` is a scheduler tick and cannot preempt anything. Next to an
`inFlightMs` climbing 1:1 with wall clock it is indistinguishable from an
unbounded hang, and there was no field that could tell the two apart. The
reading taken from it — "there is no timeoutMs, so nothing bounds this" — is
false, and it is the reading this stanza invites.
WHAT IS ACTUALLY BOUNDING THAT PASS. Three deadlines, all live on this path in
0.1.76: `relayfileOperationTimeoutMs` per call (#351/#368),
`readinessReconcileTimeoutMs` on the caller's wait (#296), and the aggregate
`sweepBudgetMs` from #374 — `#reconcileReadyIssues` -> `#runOnceWithReadinessDeadline`
-> `runOnce()` -> `#runOnceWithDiscoveryFence` -> `startDiscoverySweepBudget`.
`readinessReconcile` IS the discovery sweep's health stanza; sweep-budget.ts
names it as such. The pass was bounded. It was bounded at 90 minutes, because
`sweepBudgetMs` derives from `reconcileTimeoutMs`, so at 268 s it had 89
minutes left to run and no field said so.
Two numbers now ship: `timeoutMs` (ends the wait) and `sweepBudgetMs` (unwinds
the sweep and hands the lease back). The second is the one that answers "when
does this recover", which is the question every reader of this stanza has
actually been asking.
`missedPasses` also moves onto the heartbeat record. It already existed on the
public projection (#295/#300) and was absent from the heartbeat stanza — which
is the surface an operator opens first, and the one every report so far has
quoted.
NOT A REPORTING BUG, AND DELIBERATELY NOT CHANGED. `state: "healthy"` at
268 s is correct. `derivedReadinessReconcileState` re-derives `stalled` from
`inFlightMs > intervalMs * READINESS_RECONCILE_STALL_INTERVALS`, and that
constant is 10 — so the flip was due at 600 s and the observation window
(14:37Z-14:41Z) closed 5.5 minutes early. Lowering it is the trivially wrong
fix: public-health.ts documents #36's 61-minute post-boot hydration as the
reason a small multiple cries wolf on every cold container.
TESTS, both against the real production numbers:
- must-fire: a heartbeat carrying the bounds publishes both, and reports
missedPasses 4 for the exact 268232/60000 pass above. Fail-first verified by
ablation — with only public-health.ts and types.ts reverted it fails
`expected undefined to be 5400000`.
- must-not-fire: a recorded `0` or negative bound is dropped rather than
republished as an instant deadline, and an instance predating the fields
still projects `healthy` with both absent. This one passes before and after
by construction: it is the guard on the trivially wrong version, not a
demonstration of the fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
…was actually spinning
The `no pid available to terminate ... during completion` lines repeating 15+
times in one evidence payload are a CO-SYMPTOM, not the cause. Fixing the PID
classification would have changed nothing, and this commit explains why before
it changes anything.
WHY THE MISSING PID IS NOT THE LOOP. `#releaseAndTerminateAgents` logs that
line when `#terminationRoots` returns `{ pids: [], status: 'unresolved' }`,
then falls through. Nothing on that branch reaches `failed[]` — only a throw
from `#fleet.release()` that is not `isAgentAlreadyGoneOnRelease` does. So the
three agents were re-attempted because their RELEASE kept failing, and the
no-PID line was printed once per agent per attempt on the way past.
WHY THE LOOP NEVER ENDED. `#finishDurableRelease` does not throw on a failed
release: it returns `false` and calls `#scheduleReleaseRetry`, which re-arms at
`DISPATCH_LIFECYCLE_RETRY_MS` — 1 000 ms, unbounded. Every re-arm therefore
arrives on the RESOLVED path, which is why the `.catch()` in both schedulers
never bounded it and why a bound written there would have been a fix that never
fired. The budget is charged at the scheduling point instead.
WHAT A PASS COSTS, WHICH IS WHY 1 Hz FOREVER IS NOT FREE. Each pass calls
`#terminationRoots` once per agent inside the release AND once per agent again
inside `#writeInFlightRegistry` — a process-table scan each — plus a durable
lifecycle read and write. For the three agents in the report that is order ten
scans and several state operations per second, indefinitely. #303 already
measured this exact shape once, at 1477 state GETs in 111 s, and bounded the
RATE of the capacity-wait re-arm in response. It deliberately left the COUNT
unbounded there, because waiting for capacity is legitimate.
DESIGN CHOICE: (a) BOUNDED RETRIES, NOT (b) RECLASSIFY NO-PID. Not chosen under
uncertainty — the code already tells the two cases apart, and it says (b) is
wrong. `#terminationRoots` returns `'missing'` for confirmed-gone (a remote
placement, or a process scan that came back missing AND a resolver that agreed)
and `'unresolved'` for could-not-determine (no resolver and no recorded pids,
an AMBIGUOUS scan, a resolver that returned nothing, or one that threw). The
error only fires on `'unresolved'`. Treating that as already-terminated would
mean skipping termination of a process that may well be alive — an ambiguous
scan is literally "more than one candidate matched" — leaving orphans holding
worktrees and slots. And it would not have stopped the spin regardless, per the
first section.
Release is also the opposite shape from #303's capacity wait, which is what
makes bounding the count right here and wrong there: it is the last step of a
work unit that is already finished — issue closed, writeback acknowledged,
batch slot returned — so a release that has failed ten times is not waiting for
anything. Ten attempts at the 1 s floor is ~10 s of genuine retry, which covers
a control-plane blip or a lease handover and does not cover a permanent
failure.
SCOPED SO IT CANNOT ABANDON WORK THAT WAS NEVER FAILING:
- Only release re-arms spend the budget. `#scheduleDispatchLifecycleRetry`
takes an explicit `releaseAttempt` flag, so a `DispatchLifecycleCapacityError`
or `DispatchLifecycleOwnedElsewhereError` — both legitimate waits on someone
else — still retries forever, exactly as #303 intended.
- Progress refunds the budget, so ten bounds CONSECUTIVE no-progress passes
rather than capping a slow multi-agent release. This terminates: an agent
released once is checkpointed and skipped next pass, so the remaining set
strictly shrinks and a refund can only be earned finitely often.
- The durable lifecycle is RETAINED on exhaustion. A takeover or a restart
re-drives it from the persisted phase. This bounds one process's spin; it
does not declare the work unit clean.
- Keyed by `dispatchLifecycleKey`, so the budget follows the work unit rather
than an agent, a surface or a dispatcher — the AR-448 identity rule.
Exhaustion is logged at `error`, not `warn`, and increments
`dispatchLifecycleReleaseAbandoned`. Every layer of this failure so far has
been invisible until somebody read stderr by hand, and a work unit whose
cleanup this process has permanently given up on must not be inferable only
from the absence of further log lines.
TESTS (3), against a fleet that reproduces the production shape exactly —
`release()` throws for `issue-done` and `resolveAgentPid` returns
`'unresolved'`, so the same no-PID line is emitted on every pass:
- must-fire: the dead-letter counter reaches 1, the exhaustion error is logged,
and three further seconds of wall clock buy no additional release attempts.
Fail-first verified by ablation: with factory.ts reverted it fails after
40 543 ms with `expected undefined to be 1` — the wait can only end in its
own deadline, because the loop re-arms for as long as the process lives.
That is a property of the loop, not of any number chosen in the test.
- must-not-fire: a release that succeeds still completes the work unit and
releases each agent exactly once, with the counter unset. The trivially wrong
way to stop a retry loop is to stop retrying.
- must-not-fire: a release that fails several times and then succeeds still
completes, with the counter unset — the transient case the retry exists for.
Both must-not-fires passed under the ablation too, which is what makes them
guards rather than restatements of the fix.
RETRY CADENCE IS NOW AN INJECTABLE PORT, and that is a test-stability fix in
its own right rather than a convenience. Exhausting a ten-attempt budget at the
real 1 s floor costs ten real seconds per case; the first version of this suite
did exactly that and added 41 s to `factory.test.ts`. Run beside two other
files it pushed an already-300 s combination over an edge and four UNRELATED
tests began failing on timing — the reopen-fence and Slack-reply-route cases —
while the same three files passed on `origin/main` and `factory.test.ts` alone
passed 631/631 on the branch. Buying a fourth flake in this suite (it already
carries #342 and #373) to test a fix for a spin is the wrong trade.
`dispatchLifecycleRetryMs` follows the existing convention for exactly this —
`babysitterWakeUnreachableRetryMs`, `babysitterWakeUnreachableEscalateMs`,
`startupAgentExitDrainTimeoutMs` are all test-only port overrides of a built-in
timing. Only the delay between attempts moves; the BUDGET under test is the
real one. Overhead is now +4 s, the four unrelated failures are gone (709/709
on the same three files), and the ablation still fails with `expected undefined
to be 1` — unambiguously, because `dispatchLifecycleReleaseAbandoned` does not
exist on `origin/main` at any cadence.
The transient case sets a failure count on the fake rather than flipping a flag
from the test body, so it cannot race the cadence it runs under.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
…e path Answers three P1 findings on #379. The first is the important one: the bound as first written DID NOT FIRE in production, and the review caught it. 1. THE BUDGET RESET ON THE PATH THAT MATTERS, SO THE BOUND NEVER FIRED. `#driveDispatchLifecycle` discards `#finishDurableRelease`'s boolean in its `phase === 'releasing'` branch, and that method returns `false` rather than throwing on a failed release. So a FAILED release makes the drive RESOLVE, and the scheduler's success handler ran on every re-arm — where it called `#clearReleaseAttempts`. The counter was zeroed once per pass and could never reach the cap. This is the same never-fires shape the first version of this commit correctly rejected in the `.catch()`, moved one layer over into the `.then()`. Diagnosing the resolved path as the live one and then putting the refund on it was the error. The refund is removed from the scheduler entirely. It now happens only where success is actually known: `#finishDurableRelease` clears the budget on real per-agent progress and again when the work unit completes. WHY THE ORIGINAL TESTS MISSED IT. `#usesDurableDispatchLifecycle()` is `durableOwnership ?? placementLocality === 'remote'`, and `FakeFleetClient` places locally, so all three original cases exercised `#scheduleReleaseRetry`'s own timer — which has no success handler and therefore no reset. The deployed Factory places remotely. The suite proved a property of the path production does not take. New must-fire on the DURABLE path (`RemoteLifecycleFleetClient` + `InMemoryStateStore`), asserting the counter SURVIVES ACROSS RE-ARMS rather than that a dead-letter is reachable by some path. Fail-first verified by ablation: restore the `#clearReleaseAttempts(key)` line and only that case fails, `expected undefined to be 1` after 10 125 ms, while the three local cases still pass — which is what pins the discrimination to the durable path. 2. THE WRONG BUDGET WAS CHARGED. The generic arm of the drive's `.catch()` re-arms for dispatch, publishing and recovery failures as well as releases, and it charged all of them. That would dead-letter a work unit that was never stuck in a release loop. Charging is now confined to `#scheduleReleaseRetry`, whose every caller is a release failure: the three inside `#finishDurableRelease`, and `#completeIssue`'s catch once `releaseReasonForRetry` is set. The generic re-arm passes no charge at all. Pinned by a call-site audit rather than by a behavioural test, and that is deliberate. I could not reach that arm from a realistic fixture — forcing durable lifecycle reads to throw makes the agent-exit handler fail before any lifecycle retry is scheduled, so a test built that way passes whether or not the narrowing is present. Confirmed by ablation: with `releaseAttempt = true` restored, the fixture-based version still passed, and instrumenting it showed zero `durable dispatch lifecycle retry failed` warnings — the branch was never entered. Shipping that would have been a test that proves nothing, so the audit states the structure instead. Known gap, stated plainly: a release failure that THREW out of `#finishDurableRelease` would reach the generic arm and re-arm unbounded. Every failure path in that method returns `false` and schedules its own retry, so this is not a reachable shape today, and if one appears it degrades to the pre-existing unbounded behaviour rather than to a wrong dead-letter. 3. THE DEAD-LETTER LEAKED THE SLOT. Trading an unbounded 1 Hz spin for a permanently leaked in-flight record is not obviously the better failure: a spin is loud and self-describing, while a leaked slot silently reduces dispatch capacity until the process is restarted. Local completion never calls `batch.complete`, so exhaustion left the work unit in flight forever. `#releaseDeadLetteredSlot` now hands the batch slot back, drops any uncompensated claim, rewrites the in-flight registry, and admits whatever was queued behind it — a freed slot nothing is admitted into is only half the repair. The durable lifecycle is still deliberately RETAINED in `releasing`, so a successor or restart re-drives the same cleanup with a fresh budget; freeing a process-local slot is not a terminal phase and does not declare the work clean. The work unit therefore ends up recoverable, never merely abandoned. Must-fire asserts the slot is released after exhaustion. Fail-first by ablation: stub the call out and it fails with the work unit still in flight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
miyaontherelay
force-pushed
the
fix/reconcile-wedge-0825
branch
from
August 25, 2026 21:09
87dde1e to
514cca1
Compare
Contributor
|
@coderabbitai review Requested for exact head |
Contributor
Author
Round 2: rebased onto
|
| Finding | Status |
|---|---|
| P1 budget reset → bound never fired | Confirmed by reading and by test. Refund removed from the scheduler; new durable-path must-fire asserts the counter survives across re-arms |
| P1 wrong budget charged | Charging confined to #scheduleReleaseRetry; pinned by call-site audit, with an honest note on why a behavioural test was not possible |
| P1 dead-letter leaks the slot | #releaseDeadLetteredSlot frees the slot and retains the durable row — recoverable, not abandoned |
The first one was a real defect and the review was right: my own tests could not have caught it, because FakeFleetClient places locally and production places remotely, so every original case exercised a path with no success handler and therefore no reset.
The package check: inherited, and not the @agent-relay/sdk failure
Checked rather than assumed, and it is a different cause from the one #377 hit — that was No matching version found for @agent-relay/sdk@11.8.4; this is three failing tests. main itself is currently red on CI, and every test that failed on this PR is documented failing on main with no changes of mine:
Failing test on #379 (run 32894127383, sha 87dde1e) |
Also fails on main |
|---|---|
factory.test.ts > preserves the GitHub reporter on a Linear mirror and authorizes their escalation reply |
✅ run 32897421780, sha effd4c75 — same expected undefined to be 1 |
factory.test.ts > fences a Slack reply route that registers while the terminal drain is running |
✅ run 32897421780, sha effd4c75 |
cli/teammate-mcp.test.ts > exposes discover and bounded ask ... |
✅ run 32858119443, sha 46919f2a |
main's run also failed two more (tailscale-preview timing, atomic-json-file concurrency) that this PR did not — the set varies per run, which is the signature of load-sensitive timing tests on a shared runner rather than of a code defect.
My six budget tests all passed in that CI run, including on the CI runner's timing:
✓ stops re-arming a completion release that never succeeds, instead of spinning at 1 Hz forever 1027ms
✓ does not bound a completion release that succeeds 74ms
✓ still completes a release that fails a few times and then succeeds 287ms
Conclusion: inherited flakiness on a currently-red main, not caused by this change. I have not tried to fix it here.
Verification on the new base
factory.test.ts+public-health.test.ts: 693 passed, exit 0tsc --noEmit: 225 errors on base, 225 on branch — identical, zero in any file or line this PR touches- Each of the three fixes ablated independently and watched go red; verbatim output on the threads
On splitting
You offered the split. My recommendation is keep it as one PR: commit 1 (29aa19d, observability) was untouched by all three findings and is still clean, but commit 2 is now green with a discriminating test per finding, so splitting buys a rebase and a second review cycle for no remaining benefit. If you would rather land the observability half now regardless, say so and I will split it out in a few minutes — commit 1 is independent and cherry-picks cleanly.
Contributor
Author
|
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
1 participant
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.
Two commits against
origin/main(46919f2, 0.1.76). Do not merge — the principal owns the merge gate.What the evidence actually showed
The report was:
heartbeat.readinessReconcile.inFlightMsclimbing 1:1 with wall clock (58 s → 208 s → 268 s over ~3.5 min),state: "healthy"throughout, notimeoutMsfield, andno pid available to terminate ...repeating 15+ times in one payload. The conclusion drawn was "readinessReconcileis unbounded, and it is not the sweep #374 bounded".Three of those four inferences do not survive contact with the code.
1.
readinessReconcileIS the discovery sweep, and it IS boundedreadinessReconcileis the health stanza for the discovery sweep —sweep-budget.ts:17names it as such in #374's own module doc. Three deadlines are live on that path in 0.1.76:relayfileOperationTimeoutMsper call,readinessReconcileTimeoutMson the caller's wait, and #374's aggregatesweepBudgetMs.The pass was bounded. It was bounded at 90 minutes —
DEFAULT_DISCOVERY_SWEEP_BUDGET_MS = DEFAULT_READINESS_RECONCILE_TIMEOUT_MS = 90 * 60_000(schema.ts:62,84) — so at 268 s it had 89 minutes left to run, and no published field said so. The absence oftimeoutMsfrom the stanza was read as the absence of a bound from the code. That is a reporting defect, and it is commit 1.Adding a fourth timer here would have been the exact anti-pattern #374's commit message warns about by name ("bound the whole sweep, not one more call inside it"). Recovery time on this path is
sweepBudgetMs, which is already a config dial — no code change gets it.2.
state: "healthy"at 268 s is correct, not a reporting bugderivedReadinessReconcileState(public-health.ts:478) already re-derivesstalledfrominFlightMs > intervalMs * READINESS_RECONCILE_STALL_INTERVALS, and that constant is 10 (public-health.ts:64). The flip was due at 600 s; the observation window (14:37Z–14:41Z) closed 5.5 minutes early. Had the reads continued to 14:47Z the stanza would have saidstalledon its own.Lowering the threshold is the trivially wrong fix, and
public-health.ts:53-63says why: #36 measured a 61-minute post-boot reconcile while the Relayfile mirror hydrated, so a small multiple cries wolf on every cold container. Not changed.3.
missedPassesalready existedOn
FactoryPublicReadinessReconcileHealth(#295/#300). It was missing from the heartbeatFactoryReadinessReconcileStatus— which is the surface the report was quoting. Commit 1 puts it there.4. The missing PID is a co-symptom, and the retry loop is real
This one the brief got right in substance, though not in mechanism. Details below.
Commit 1 —
453acacpublish the bounds that can preempt a sweepDoes not add a bound. Publishes the ones that exist, so the misdiagnosis above is not available to the next reader.
src/types.tstimeoutMs,sweepBudgetMs,missedPassesonFactoryReadinessReconcileStatus;timeoutMs,sweepBudgetMsonFactoryPublicReadinessReconcileHealthsrc/orchestrator/factory.ts#readinessReconcileStatus()publishes all threesrc/orchestrator/public-health.tsreadinessReconcileHealth()+ the by-construction serializer pass both throughoptionalPositivetimeoutMsends the wait;sweepBudgetMsunwinds the sweep and hands the lease back. The second answers "when does this recover" — the question every reader of this stanza has actually been asking.Commit 2 —
87dde1ebound the completion release retryWhy the missing PID is not the loop.
#releaseAndTerminateAgents(factory.ts:9820) logsno pid available to terminatewhen#terminationRootsreturns{ pids: [], status: 'unresolved' }, then falls through. Nothing on that branch reachesfailed[]— only a throw from#fleet.release()that is notisAgentAlreadyGoneOnReleasedoes. The three agents were re-attempted because their release kept failing; the no-PID line was printed on the way past.Why it never ended.
#finishDurableReleasedoes not throw on a failed release — it returnsfalseand calls#scheduleReleaseRetry, re-arming atDISPATCH_LIFECYCLE_RETRY_MS= 1 000 ms, unbounded. Every re-arm therefore arrives on the resolved path, which is why the.catch()in both schedulers never bounded it, and why a bound written there would have been a fix that never fired. The budget is charged at the scheduling point instead.What a pass costs.
#terminationRootsruns once per agent in the release and once per agent again inside#writeInFlightRegistry(factory.ts:10086) — a process-table scan each — plus a durable read and write. Three agents ≈ ten scans and several state ops per second, indefinitely. #303 already measured this exact shape once, at 1477 state GETs in 111 s (factory.ts:458-467), and bounded the rate of the capacity-wait re-arm in response.Design choice: (a) bounded retries. Not (b), and not under uncertainty
The brief said to pick (a) if I could not tell a clean exit from a never-recorded PID. I can, and the code already does — so (a) is chosen on evidence, not on caution:
#terminationRoots(factory.ts:9910) returns'missing'for confirmed-gone (remote placement, or a process scan that came back missing and a resolver that agreed) and'unresolved'for could-not-determine (no resolver and no recorded pids, an ambiguous scan, a resolver that returned nothing, or one that threw). The error only fires on'unresolved'.So (b) is wrong twice over:
'unresolved'is definitionally "we could not determine". An ambiguous scan is literally more than one candidate matched. Reclassifying that as already-terminated means skipping termination of a process that may be alive, leaving orphans holding worktrees and slots.failed[].Release is also the opposite shape from #303's capacity wait, which is what makes bounding the count right here and wrong there: it is the last step of a work unit that is already finished — issue closed, writeback acknowledged, batch slot returned — so a release that has failed ten times is not waiting for anything.
Scoped so it cannot abandon work that was never failing
#scheduleDispatchLifecycleRetrytakes an explicitreleaseAttemptflag, soDispatchLifecycleCapacityErrorandDispatchLifecycleOwnedElsewhereErrorstill retry forever, exactly as Dispatch is permanently wedged: one agent-less lifecycle holds the only batch slot and cannot be reaped, so every queued row spins at 1 Hz forever #303 intended.dispatchLifecycleKey— the budget follows the work unit, not an agent, surface, or dispatcher (the AR-448 identity rule).Observability
Exhaustion logs at
errorand incrementsdispatchLifecycleReleaseAbandoned. Every layer of this failure so far has been invisible until someone read stderr by hand; a work unit whose cleanup this process has permanently given up on must not be inferable only from the absence of further log lines.Red-then-green, observed on the current base
Each fix reverted independently, its test watched go red, restored, watched go green. Verbatim.
Commit 1 — ablated (
git stash push src/orchestrator/public-health.ts src/types.ts, tests kept):Restored:
Test Files 1 passed (1) / Tests 55 passed (55)Commit 2 — ablated (
git checkout origin/main -- src/orchestrator/factory.ts, tests kept):Restored:
Tests 3 passed | 628 skipped (631)The red is unambiguous at any cadence:
dispatchLifecycleReleaseAbandoneddoes not exist onorigin/main. The wait can only end in its own deadline, because the loop re-arms for as long as the process lives — a property of the loop, not of a number chosen in the test.Both must-not-fire guards passed under the ablation too, which is what makes them guards rather than restatements of the fix.
A flake this PR introduced and then removed
The first version of the commit-2 suite exhausted a ten-attempt budget at the real 1 s floor: +41 s on
factory.test.ts. Run beside two other files it pushed an already-300 s combination over an edge and four unrelated tests began failing on timing (reopen-fence, Slack reply routes) — while the same three files passed onorigin/main, andfactory.test.tsalone passed 631/631 on the branch. This suite already carries #342 and #373; buying a fourth flake to test a fix for a spin is the wrong trade.dispatchLifecycleRetryMsis now a test-only port override, followingbabysitterWakeUnreachableRetryMs/startupAgentExitDrainTimeoutMs. Only the delay between attempts moves — the budget under test is the real one. Overhead is now +4 s and the four failures are gone:Suite and typecheck
Full suite:
Test Files 7 failed | 105 passed | 1 skipped (113),Tests 17 failed | 2280 passed | 1 skipped (2298).All 7 failing files fail identically on a clean
origin/maintree and are sandbox artifacts, not regressions — none is a file this PR touches:src/__tests__/dist-entrypoints.test.tsCannot find module '/dist/...'— needs a build.agentworkforce/agents/factory-maintainability/persona.test.tsCannot find package '@agentworkforce/review-kit'.agentworkforce/agents/factory-feature-guardian/agent.test.tsbindPreviewTransport is not a functionsrc/node/factory-persona-card.test.tsderiveAgentCard is not a functiontest/e2e/ask-a-teammate.test.ts,src/environments/load-harness.test.ts,src/git/agent-worktree.test.tsThe sandbox has no private-registry access, so
npm cicannot install the tree; tests ran against an overlay of nearest-available local packages (@agentworkforce/runtime,@agentworkforce/persona-kit,@relaycast/a2a), which is why the borrowed packages are missing newer exports.tsc --noEmit -p tsconfig.json: 225 errors on base, 225 on branch — identical. Zero insrc/types.ts,src/orchestrator/factory.ts,src/orchestrator/public-health.ts, or in any line this PR added.No conflict with #377
Both PRs touch
factory.tsandfactory.test.ts, so I checked rather than assumed. Closest approach is 12 lines (field declarations, #377 at base 809-813 vs this at 825); the test-file hunks are at 517/20708 for #377 and EOF (31348) here.git merge-tree --write-treeof the two branches exits 0 with no conflict.What this does not do
sweepBudgetMs, a config dial defaulting to 90 minutes. Tightening it is a policy decision with a documented cost — fix(orchestrator): bound the whole sweep, not one more call inside it #374 measured fix(factory): replay existing Slack triage answers #36's cold-mirror hydration at 61 minutes, and a budget below that makes a slow boot a loop that never progresses. I did not touchfactory.config.json.#scheduleAbandonedDispatchRetry(factory.ts:11673), which re-arms on the same 1 s floor. Same shape, different path, not in the reported evidence.🤖 Generated with Claude Code