Bound dormant staged builds per component with deployment.stagingRetention.maxCount - #2531
Open
dawsontoth wants to merge 4 commits into
Open
Bound dormant staged builds per component with deployment.stagingRetention.maxCount#2531dawsontoth wants to merge 4 commits into
dawsontoth wants to merge 4 commits into
Conversation
…ntion_maxCount Step 4 of #2315. A journal-less `.deploy-staging/<id>` directory holding `.complete` and the owner's tree is a dormant build: built and validated, activated by nobody. Boot recovery used to remove every owned journal-less directory; it now keeps dormant builds and bounds them per component to `deployment_stagingRetention_maxCount` (default 5, 0 keeps none), newest by `.complete` mtime, ties broken by deployment id. Residue — a partial tree, a tree already moved live, a stale `.unsettled` — is still removed. Classification happens only under the owner's preparation lock, because activation writes `.complete` moments before its journal while holding it. Boot classifies inside the existing locked residue pass and prunes per owner afterwards, treating a held lock as the same deferral that pass records; the deploy path prunes inside the settlement scan it already runs under the lock, so a deploy pays one traversal. Each eviction re-checks for a journal first. Pruning never fails a component closed and never replaces a deploy's error. Nothing produces a dormant build yet beyond the crash window between `.complete` and the journal; step 6 (deploy from an existing aside) is the producer this bound exists for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review round 1 (gemini + domain): classifying a retained build under the owner's lock on every recovery pass re-took that lock per directory, forever, so a healthy component could lose the 250 ms probe to its sibling threads at boot and be deferred with nothing in progress. Dormant builds are now catalogued without the lock; only an owner over its bound takes it, once, and each eviction is re-derived under it. `dropComponentDirectory` reclaims the dropped component's dormant builds, since no later deploy of that name will. Comment volume trimmed; catalog append no longer copies the array. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cale-free Review round 2: the boot prune sliced its kept set from the unlocked catalog, so a build activated or staged between the scan and the lock could hold or miss a slot; the over-quota owner is now re-catalogued under its lock first. Deployment-id ties compare code points rather than `localeCompare`, so two processes with different locales pick the same victim. Comment trims. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review round 3 (codex + gemini): re-reading the whole staging root once per over-bound owner while holding its lock could make sibling boot passes lose the 250 ms probe and defer healthy components. `pruneDormantBuilds` now re-derives the catalogued directories it was handed before choosing the kept set, so the boot path reads only that owner's entries under the lock. Adds a test that a journaled activation is settled rather than retained. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a retention mechanism for dormant staged builds under .deploy-staging per component, bounded by the deployment_stagingRetention_maxCount configuration parameter (defaulting to 5). The pruning of these builds is integrated into boot recovery, live deployment, and component deletion paths. The review feedback suggests ensuring that temporary directories created during unit tests are reliably cleaned up using a finally block to prevent resource leaks when assertions fail.
dawsontoth
marked this pull request as ready for review
September 8, 2026 18:26
Contributor
|
Reviewed; no blockers found. |
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.
Step 4 of #2315. Adds
deployment.stagingRetention.maxCount(default5,0keeps none, coerced like its sibling) and bounds, per component, how many dormant staged builds survive under<componentsRoot>/.deploy-staging— a dormant build being a journal-less deployment directory that holds.completeand the owner's tree: built and validated, activated by nobody.The premise this step was planned on moved during step 1's review:
maindiscards a rejected or failed candidate immediately, and boot recovery removed every owned, journal-less staging directory. So nothing intentionally leaves a dormant build behind today — only the crash window between.completeand the journal write does. Step 6 (deploy from an existing aside) is the producer this bound exists for; this PR lands the bound and the knob so that step has a contract to build on, and makes recovery stop destroying the state step 6 will rely on.What changes on disk: boot recovery keeps dormant builds instead of removing them, and bounds them per owner to the newest
maxCountby.completemtime (ties broken by deployment id, so concurrent worker passes evict the same build). Residue — a partial tree, a tree already moved live, a directory carrying a stale.unsettled— is still removed exactly as before. The deploy path applies the same bound at the start of a deploy, inside the settlement scanprepareApplicationalready runs under the component lock, so a deploy pays one traversal of the staging root rather than two.drop_componentreclaims the dropped component's dormant builds, since no later deploy of that name will.For the human reviewer
.complete+ tree + no journal, with removal decided under the owner's lock — not a new marker. The planning review proposed an explicit "staged/dormant" marker written by a stage-only operation, arguing.completeproves durability but not dormancy because activation writes it moments before the journal. That is true of an unlocked read, which is why an unlocked read is only a candidate here and nothing is removed without re-deriving it under the owner's preparation lock, where a complete, journal-less directory cannot be mid-activation: activation holds that lock from.completethrough the swap. A marker would duplicate.completefor the one producer step 6 adds. Reversible at zero migration cost: no dormant builds exist in the wild yet, so step 6 can introduce a marker if its stage-only mode wants one. Cost of a "no" here: step 6 writes and retention reads one more control file.componentLoaderalready acts on, and no.unsettledis written: "do not delete" is not "safe to load". The alternative — skip and log on timeout — is the more available reading; the conservative one was kept because it only arises while an owner is over quota, which the same pass then clears.0would preserve today's on-disk behaviour exactly but make step 6's default useless and force a later default change.5is the reviewed feat(deploy): two-phase stage/activate for deploy_component #1849 default. The only behaviour visible today: a.completebuild whose journal was never written (crash in that window) is retained up to the bound at boot instead of removed. Cost of a "no": one line and the docs..completemtime as the order; no pin for a build a deploy is about to read. The domain review's ledger raised all three. Count is what the issue specifies and matches feat(deploy): two-phase stage/activate for deploy_component #1849; disk is the constraint that bites, so amaxSizesibling may be worth adding once step 6 produces real trees — cheap to add beside this knob, awkward to replace it..completemtime does not survive a timestamp-normalising restore; a monotonic id or a recorded stamp can replace it before any retained build is something an operator rolls back to. Pruning before the build has nothing to pin today because no deploy reads a retained build; step 6 must pin its source or prune after..deploy-staging/<id>; nothing names it, so it cannot be told from a build that has not reached extraction yet. The fix — writing the sidecar at the start of the build — would make every respawning worker defer that component for the wholenpm install, a step-1 decision this step does not reopen. Documented in DESIGN.md rather than fixed.Verification
unitTests/components/deployStagingRetention.test.js(15 tests): a complete build and afile:symlink build survive boot while a partial tree and a swept-but-not-removed directory are removed; per-component bound to the newest N with a neighbour's builds not counted; completion-time tie broken by id;maxCount: 0keeps none; a stale.unsettledon a journal-less directory is removed so workers stop refusing it; a journaled activation is settled rather than retained however dormant it looks; a component within its bound takes no lock, so a held lock does not defer it; an over-quota owner whose lock is held is deferred, with nothing deleted and no verdict written;dropComponentDirectoryreclaims the dropped component's dormant builds and leaves a neighbour's alone; the eviction re-check keeps a build that acquired a journal since the scan; an unremovable build is logged, not thrown; the deploy path bounds its own component, leaves its neighbours alone, and still surfaces the deploy's own error; getter coercion (unset/blank/boolean/array/NaN/negative → 5; numeric string, float floor, explicit 0). The 43 existingdeployActivationtests still pass unchanged.integrationTests/deploy/staging-retention.test.tsstarts Harper withdeployment.stagingRetention.maxCount: 1through the harness's config override, plants two dormant builds beside the running instance, deploys the component once through the operations API, and asserts the deploy landed and only the newest planted build survives. Passes locally. The wholeintegrationTests/deploy/folder also passes locally.unitTests/components/*.test.jsrun as one suite locally: 829 pass; the 7 failures (applicationSpawnprocess-group timing,RuntimeModuleTrackermacOS/private/varrealpath) reproduce identically on a cleanorigin/mainworktree and touch nothing in this diff.tsc --noEmit,oxlint,prettier --checkclean on the touched files; the full unit and integration matrix is CI's.deploymentsection to the configuration reference (this knob plus the previously undocumentedpayloadRetention.maxSize) and a 5.3 release note.Complexity: medium
Review-Coverage: authored=claude; ran=gemini; adjudicated=domain; blocked=codex(exit-1); declined=cursor-grok,cursor-composer; rounds=4 @ b8a8d80
Human-Review-Need: 3 (decisions: retain-before-producer, count-bound-not-size-bound, mtime-as-completion-order, deferral-on-held-lock) @ b8a8d80