Repository navigation
Remove a fork that a still-loading worker creates after drop_component looked - #3818
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request ensures that dropping an application with no active fork on disk (but which a worker might still be loading or could potentially fork) correctly records a pending branch removal. It introduces the declaresBranches helper to check the root configuration, speculatively records pending removals under certain conditions (e.g., if a worker pool may hold it), and adds robust integration and unit tests to verify these scenarios. It also ensures directory creation and fsyncing are properly handled when recording pending removals. No review comments were provided, so there is no feedback to address.
A worker still loading an application can create its first fork after drop_component's last look at the disk. No marker covered that fork and nothing listed the absent application again, so it stayed on disk with nothing logged. A discarding drop now records the removal before it changes anything whenever the root config declares branches, even with none on disk yet, in the default database's branches. One marked directory stands for all of an application's, and the retry already waits for every worker that may hold the application before it removes anything or clears the marker. So a late fork lands under a debt that survives a crash of main and a drop that fails after the application is gone, with no new state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…has not reported A worker loads from its own view of the root config, which can still declare branches the file no longer does, so the declaration alone could miss a fork. A worker that has not reported its load is the only one that can still create one, so its presence now records the removal too. The marker write flushes the entries naming its directory every time rather than only when it creates them, since an earlier drop that failed can have created the directory without that, and drops the separate directory helper. The end-to-end test counts the removal log line rather than matching one an earlier test may have left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d loads included A worker reports an application placed before a load deferred behind its preparation lock has run, so a reported worker can still create the first fork. The drop now records the removal whenever the pool may hold the application or a dedicated worker for it remains, the same checks its retention already makes, instead of only while a worker has not reported. The marker write also flushes the entry of the topmost directory it created, in case that is the storage root itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With threads.count 0 main is the only worker, and a load still running there can create the first fork after the drop looked, with no pool or dedicated worker for the drop to see. The retry now logs that it finished a removal rather than that it removed storage, since a marker recorded in case a fork appeared may have had none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every drop of a loaded shared application now writes under the default database's storage, so a full or read-only volume would have refused the drop of an application with no branches at all, where before it touched no database storage. Only a fork on disk or a declaration refuses the drop now; for the rest a failed record is logged and the drop goes on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main now holds each design note to a byte ceiling, and this note was at it, so the line about markers with no fork yet is shortened and the paragraph around it tightened to make room. The details stay in the dropApplication and recordPendingBranchRemoval docblocks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/late-fork-census-3758
branch
from
November 2, 2026 16:40
7779da4 to
262ea74
Compare
dawsontoth
marked this pull request as ready for review
November 2, 2026 17:10
kriszyp
approved these changes
Nov 2, 2026
| if (failures.length) { | ||
| throw new Error(`Could not record the owed removal of branch storage for '${appName}': ${failures.join('; ')}`); | ||
| } | ||
| if (!directories.length) { |
Member
There was a problem hiding this comment.
[minor] A newly created intermediate directory can leave the removal marker non-durable
This was referenced Nov 3, 2026
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.
⊙ Problem
drop_componentchecked disk to decide whether to remove a fork. A worker still loading the application could create its first fork after the drop's last look. That fork had no marker. The absent application was never listed again, so the fork stayed, unlogged, for a same-name redeploy to adopt.🤔 Does a rare race need a fix?
Yes. It needs a load running during the drop, but a preview's private data must not outlive its drop, and nothing logs it.
💡 Solution
A drop that discards branches now records the removal before changing anything if a fork could still appear. This applies when the root config declares branches or any thread may hold the application. The retry already waits for every holder, so the marker covers a late fork. A marker that covered nothing clears the same way.
⚖️ Alternatives
🤔 Why a durable marker instead of the recheck the issue suggested?
Framing-Verdict: better-alternative-exists (6b1407a5d94f)
I planned the issue's option 3, an in-memory recheck on each worker release. The planning review showed the retry already keeps an empty marker while anything may hold the application, so the marker is cheap. Unlike the recheck, it survives a crash of main or a drop that fails partway.
🤔 Is a marker for an application that never forked acceptable?
Yes. A plain shared application dropped without a restart keeps an empty marker and retries less often until the pool restarts. Nothing is deleted wrongly. Checking only unreported workers missed loads deferred past a report. Only a fork or a declaration can block a drop, so a full disk never blocks dropping a plain application.
Also considered
prepareBranches: races the drop unless every boot takes the component lock.keepBranchesdrop leaves that state on purpose.🔧 Changes
Fixes #3758. Part of #3042.
components/operations.ts:dropApplicationrecords the removal when the app has a fork or the newdeclaresBranchessays so, and refuses the drop if it cannot. When onlypoolMayHold,hadWorkerormainServesholds, a failed record is logged.declaresBranchesreads the root config file and answers true when it cannot, ashasRootConfigEntrydoes. The retry's log line now says it finished a removal, since a marker may have covered nothing.resources/branchDatabase.ts:recordPendingBranchRemovalcreates the default database's<app>directory when none exists, and flushes the entries naming each directory it marks.components/DESIGN.md: thedrop_componentnote records when a marker is written with no fork on disk.✅ Verification
branchRemovalRetry.test.js, a real worker thread): nine new tests; five fail onmain(late fork, stale declaration, deferred load, declared drop that cannot record, drop failing after removal). This harness puts the test and default databases in different roots, so the late-fork case also covers a fork outside its marker's root.branched-database-package.test.ts): dropping an application no worker loaded records and clears its removal, counted through the retry's log line.ad291755b): unit main and resources fail the same 4 tests on head and base; integration is 3086/0 against 3085/0. CI is green on the rebased head262ea74b5.prepareBranchesafter the drop's last look; the harness cannot pause a worker there. Also,hadWorkerandmainServesalone.🤖 Generated with Claude Code
🤖 Generated by Anthropic Claude (Opus 5.5); posted via @dawsontoth.
Release target: v5.4 — fixes code from #3600 and #3763, which is only on main (v5.4); not a backport
Related PRs: #3763 overlaps (merged; this builds on its retry and worker-release wake-ups)
Complexity: moderate
Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex,cursor-grok; late=cursor-grok:adjudicated; adjudicated=domain; declined=muse; rounds=8; full=4 @ 262ea74
Review-Attention: study ~13m (critical: branchDatabase.ts; decisions: speculative-marker-fail-open, marker-in-default-root, census-breadth, marker-home-default-root) @ 262ea74