Give boot-time restore a real cancellation path instead of an abandoning timeout - #499
Merged
Merged
Conversation
withRestoreTimeout races a boot-restore attempt against a timer and, on timeout, abandons the still-running attempt outright. If restoreDeploymentFromRecord later succeeds anyway, spawnWorkflowDeployment registers a live supervisor for an address the boot loop already recorded as a restore failure -- a genuinely live deployment whose durable record says it failed to restore. This asserts the fix ahead of the implementation: a restore that outlasts its timeout must still finish (never abandoned), and once it does, its record must be corrected rather than left claiming a live deployment failed. createHandshakeSpawner is extracted out of makeLifecycleFixture so this test can drive a real spawn/ready handshake on its own timing instead of one that throws or resolves synchronously; the fixture also grows optional materializeDeploymentClosure and restoreAttemptTimeoutMs overrides so a boot-restore test can use a multi-step closure and a millisecond-scale timeout instead of the real 30s default.
…ing timeout withRestoreTimeout raced restoreDeploymentFromRecord against a timer and walked away on timeout, with no way to tell the loser it had lost. The underlying restore kept running regardless -- including spawnWorkflowDeployment, which registers a live supervisor and spawns a real workflow-process child -- so a restore that finished after the timeout already fired left a genuinely live deployment whose durable record claimed it had failed to restore (CL-7215). withRestoreTimeout now hands the restore an AbortSignal that fires the instant the deadline wins. restoreDeploymentFromRecord checks it at two points where real cancellation is still possible: before the closure fetch (applyAtomic has no signal of its own, so bailing here is the only way to skip it) and before the spawn (Supervisor.spawn has none either). Past that second checkpoint the restore cannot be cancelled -- so once spawnWorkflowDeployment resolves, it checks whether the caller's own timeout had already fired and, if so, corrects the durable record instead of leaving that lie in place. The correction and the boot loop's own failure-recording catch both touch the same on-disk record, so they're serialized through a per-deployment in-memory lock (withDeploymentRecordLock): without it, the correction's disk read could land before the catch's disk write, observe nothing to clear, and no-op -- leaving the catch's write to permanently mismark a live deployment. The catch also consults activeSupervisors (in-memory, always set synchronously the instant a spawn actually succeeds) while holding that same lock, so a restore that raced ahead of its own timeout never gets a boot failure recorded against it in the first place. Together the two make the final on-disk state correct regardless of which side finishes first. RESTORE_ATTEMPT_TIMEOUT_MS is now overridable per router (restoreAttemptTimeoutMs) so a test can exercise this in milliseconds rather than the real 30s ceiling.
teardownDeployment's own two on-disk deployment-record writes (deleteWorkflowDeploymentRecord for a reclaiming undeploy, markWorkflowDeploymentRecordParked for a hibernate) were a third, unlocked writer touching deployment.json for a given deploymentId, alongside the two the CL-7215 boot-restore fix already serializes through withDeploymentRecordLock: the boot loop's restoreFailure write and a dangling (timed-out but still-running) restore's late-settle correction. Routing both teardown writes through the same lock closes the write-ordering half of that race. Closing the write ordering alone was not enough: a reclaiming teardown racing a dangling restore can still delete the record between when the boot loop scans it and when either writer runs, and two blind writers mishandled that: - recordWorkflowDeploymentRestoreFailure wrote the caller's stale in-memory record unconditionally, so a teardown that reclaimed the deployment first got its delete silently undone -- the boot loop's catch would resurrect deployment.json with a false restoreFailure for a deployment that no longer exists. It now re-reads from disk under the lock and no-ops when the record is already gone, matching markWorkflowDeploymentRecordParked's sibling shape. - restoreDeploymentFromRecord's late-settle correction only handled "record still exists but wrongly marked failed"; it had no branch for "record is gone entirely". If the reclaimed restore's own spawn later succeeded anyway, spawnWorkflowDeployment would register a live supervisor and re-register its routers with nothing durable backing them -- an orphaned live deployment invisible to any future boot scan, arguably worse than the original false-failure bug. It now distinguishes that outcome and unwinds the orphaned supervisor through the ordinary teardownDeployment(reclaimDirs: true) path. Two tests exercise the fixed race: a reclaiming teardown ahead of the boot loop's own failure write must not have its delete undone, and a teardown racing a restore whose spawn resolves afterward must not leave a live, record-less deployment behind.
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.
Summary
Fixes CL-7215:
withRestoreTimeoutraced a boot-time deployment restore against a timer and, on timeout, abandoned the still-running restore instead of cancelling it. If the restore later succeeded anyway,spawnWorkflowDeploymentregistered a live supervisor for an address the boot loop had already durably recorded as a restore FAILURE — a deployment recorded as failed that is actually live.restoreDeploymentFromRecordnow takes anAbortSignal, threaded fromwithRestoreTimeout's timer. Two cooperative cancellation checkpoints (throwIfRestoreAborted) sit before the closure fetch and before the spawn — the only two places real cancellation is possible, since neither@intx/tool-packaging'sapplyAtomicnor@intx/workflow-host'sSupervisor.spawnaccept a signal of their own.spawnWorkflowDeploymentresolves: if the timeout already fired, it reconciles the durable record against whatever's actually true on disk instead of leaving a lie in place.withDeploymentRecordLock) serializes every writer that can touch a given deployment'sdeployment.jsonaround this race: the boot loop's own failure-recording catch, the late-settle correction, andteardownDeployment's reclaim/park writes.recordWorkflowDeploymentRestoreFailurenow re-reads from disk and no-ops on a missing record; the late-settle correction now recognizes "record is gone" as its own outcome and unwinds the orphaned supervisor via the ordinaryteardownDeployment(reclaimDirs: true)path.RESTORE_ATTEMPT_TIMEOUT_MSis overridable per router (restoreAttemptTimeoutMs) so tests exercise the timeout path in milliseconds.CL-7219 verdict
CL-7219 (sources-rotation rollback correctness) lives in the same file but is a separate, unrelated code path — the single-step warm-keep sources-rotation handler's
currentSourcesrespawn-hint rollback, not the boot-restore lock. It does not fall out of this change; left as its own ticket.Test plan
cd apps/sidecar && bun run typecheck— cleancd apps/sidecar && bun test— 223 pass, 0 fail (run twice)bunx prettier --checkon all touched filesbun run check:structuralfrom repo root — all checks passDO NOT MERGE.