Uh oh!
There was an error while loading. Please reload this page.
fix(cli): stop Ctrl-C from stranding the local maintenance lock - #556
Merged
Conversation
The maintenance lock is released by a `finally` inside promise land, so whether it survives a Ctrl-C is decided entirely by the Effect boundary above it — and every one of those boundaries was a bare `Effect.tryPromise`. `Effect.tryPromise` is interruptible, and interruption ABANDONS the promise rather than cancelling it: `callbackOptions` marks the async resumed, aborts its signal and unwinds the fiber without waiting. Under `BunRuntime.runMain` a Ctrl-C during `maple archive create` therefore tore the process down while `withMaintenanceLock` was still mid-write, so its `finally` never ran and `<dataDir>.maple-maintenance-lock` survived carrying a plausible owner record. The next run only recovered because `acquireMaintenance` quarantines a provably dead PID — recovery by luck, one PID reuse away from a hard failure. `maintenanceOperation` makes that boundary uninterruptible, which is enough on its own: an interrupt is recorded on the fiber and not delivered, the fiber stays parked until the promise settles, the lock's `finally` releases, and `setInterruptibleTrue` re-raises the recorded interrupt afterwards. The 12 call sites that reach the lock now use it — nine in `archive.ts` (create, gc, reconcile, rebuild-catalog, expire, retire, three calibration paths) and three in `schema.ts` (migrate preview/apply, abandon). Nothing inside gc.ts, retention.ts, listing.ts, generation.ts or local-store-migrations.ts changes. The rejected alternative was an `acquireRelease` bracket with an interruptible body. It releases the lock promptly while the abandoned promise keeps writing — strictly worse than the bug. The work was never abortable; the only real choice was whether Effect waits for it. The cost is deliberate and documented: Ctrl-C is honoured when the operation finishes, not immediately. That is the right trade for the only writer of a lock the next process must trust, and it does not weaken the calibration watchdog, which kills with an untrappable SIGKILL (both `killSignal` and `process.kill(-pgid, ...)`) — the crash case the on-disk journals already reconcile. To make Ctrl-C prompt again, those promise bodies would have to observe the `AbortSignal` that `try` already passes them. Tested end to end against real processes and real signals: `native-maintenance-sigint-probe.sh` SIGINTs a worker holding the lock under real `runMain` and asserts BOTH arms, so it cannot pass vacuously — the bare shape must strand the lock (exit 130, operation abandoned) and the fixed shape must release it (exit 130, operation completed). The unit tests drive `fiber.interruptUnsafe()`, which is exactly what runMain's SIGINT handler calls; removing `Effect.uninterruptible` fails them.
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
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 freeto 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.
What
maintenanceOperation— an uninterruptible Effect boundary for promise-land work that takes the maintenance lock internally — replacing bareEffect.tryPromiseat the 12 call sites that reach the lock: nine incommands/archive.ts(create, gc, reconcile, rebuild-catalog, expire, retire, and three calibration paths) and three incommands/schema.ts(migrate preview/apply, abandon).Nothing inside
gc.ts,retention.ts,listing.ts,generation.tsorlocal-store-migrations.tschanges.Why
The maintenance lock is released by a
finallyinside promise land, so whether it survives a Ctrl-C is decided entirely by the Effect boundary above it — and every one of those boundaries was a bareEffect.tryPromise.Effect.tryPromiseis interruptible, and interruption abandons the promise rather than cancelling it:callbackOptionsmarks the async resumed, aborts its signal, and unwinds the fiber without waiting. UnderBunRuntime.runMaina Ctrl-C duringmaple archive createtherefore tore the process down whilewithMaintenanceLockwas still mid-write, so itsfinallynever ran and<dataDir>.maple-maintenance-locksurvived carrying a plausible owner record.The next run only recovered because
acquireMaintenancequarantines a provably dead PID — recovery by luck, one PID reuse away from a hard failure.Effect.uninterruptibleis enough to fix it on its own: the interrupt is recorded on the fiber and not delivered, the fiber stays parked until the promise settles, the lock'sfinallyreleases, andsetInterruptibleTruere-raises the recorded interrupt afterwards.Reviewer notes
The rejected alternative. An
acquireReleasebracket with an interruptible body was the first instinct and is wrong: it releases the lock promptly while the abandoned promise keeps writing — strictly worse than the bug. The work was never abortable; the only real choice was whether Effect waits for it.The deliberate cost. Ctrl-C is honoured when the operation finishes, not immediately. That is the right trade for the only writer of a lock the next process must trust. To make Ctrl-C prompt again, the promise bodies would have to observe the
AbortSignalthattryalready passes them — until they do, an interruptible boundary only deletes the lock out from under work that keeps running. This is documented onmaintenanceOperationitself.The calibration watchdog is not defanged. It kills with an untrappable SIGKILL on both paths (
killSignal: "SIGKILL"andprocess.kill(-pgid, "SIGKILL")), which is the crash case the on-disk journals already reconcile.Behaviour claims were read out of the runtime, not assumed —
callbackOptions,interruptUnsafe, andsetInterruptibleTrueinpackages/effect/src/internal/effect.tsat the pinned4.0.0-rc.108.Testing
test/native-maintenance-sigint-probe.shSIGINTs a real worker process holding the lock under realBunRuntime.runMain, and asserts both arms so it cannot pass vacuously:test/maintenance-operation.test.ts(3 tests) drives interruption throughfiber.interruptUnsafe()— exactly what runMain's SIGINT handler calls — and covers the ordinary success and failure release paths. Mutation-checked: deletingEffect.uninterruptiblefails it with "THE FIX: the lock was released".tsc --noEmitclean, 70 targeted tests pass, oxlint reports nothing on the changed files.Not run locally: the native probes that need a bundled binary + chDB + duckdb (
native-archive-smoke.sh,native-local-store-migration.sh). They exercise the converted commands against real archives and are worth a CI run.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.