Retry a branched database removal that drop_component could not finish, instead of only logging the failure by dawsontoth · Pull Request #3600 · HarperFast/harper

dawsontoth · 2026-10-26T22:33:43Z

⊙ Problem

drop_component answers an isolated app's own worker, or a shared worker being replaced by a pool restart, before removing forks (branched databases). A failed removal was only logged; the fork stayed on disk. The preview expiry sweep also leaked forks and never lists that preview again. Previews drop forks all day, so leaks pile up (documentation#695).

🤔 A redeploy adopts a fork that is still owed a removal

❓ Your call: redeploying an app before its old fork is removed adopts the old rows, as today after a failed removal. Removal waits for its next drop. The alternative deletes the leftover fork before the redeploy loads. Keep adoption?

💡 Solution

A drop that does not keep its forks first writes a durable `removal-pending` file beside them.
Main retries each marked app after every drop, at start, and on a timer (1 minute, doubling to 1 hour) while one it could remove is left.
Only a completed removal or a keepBranches drop clears the file. Removal waits for redeployed apps or apps a worker may hold.

⚖️ Alternatives

Retry in memory with backoff (issue option 2) — a crash or restart forgets it, and a lasting failure still ends in a log line.
Report it for a CI job to poll (option 3) — preview CI drops and leaves; nobody polls.
Reap the forks of every app that is gone — keepBranches keeps an absent app's forks on purpose.
Remove the forks before answering — the asker, or the pool about to restart, still holds them.

🤔 The planning review's alternative was adopted

Framing-Verdict: better-alternative-exists (ecbdd691fc5c)

I planned retries at the next drop and start only. Codex showed a quiet node would keep its last failed fork forever. Adopted: a capped-backoff timer while removals remain, a marker recheck under the lock, and fsynced markers.

🔧 Changes

Closes #3463. Part of #3042.

resources/branchDatabase.ts — the marker: record (a durable write into every <app> directory, refused when the census cannot be read), clear (a durable unlink, then the empty directory goes), check, and list across storage roots, reporting when a root or marker could not be read. removeBranchesForApplication clears it only when every removal succeeded. branchRoots is factored out of branchDirectoriesFor, which now also returns the <app> directories.
components/operations.ts — dropApplication writes the marker, or clears it for keepBranches, before removeApplication, takes the census again afterwards for a fork a still-loading worker created, and starts a retry pass in finally. retryPendingBranchRemovals runs one pass at a time and arms the backoff timer while debt remains or a census was incomplete. retryBranchRemoval takes the app's lock without waiting (reclaiming a dead worker's ticket), rechecks the marker, presence and holders, then removes. The retention and failure messages say the removal is retried; early answers still say branchesRemoved: null.
bin/run.ts — a start runs a pass after startHTTPThreads, when main has settled interrupted deploys and every pool worker has reported what it loaded.
components/DESIGN.md — the drop_component note describes the marker and its retries.
Rollback: a build without this change ignores the marker. After rollback, a keepBranches drop leaves the file if the app's earlier removal failed. Re-upgrading then removes that fork. Delete the file by hand in that case.
Docs: documentation#750 (draft) still says a second drop removes leftover forks; its "Removing a fork" paragraph needs a sentence once this lands.

✅ Verification

Integration (branched-database-package.test.ts, Linux): an unremovable entry fails the removal after a self-drop and after a shared worker's restart: true drop, both answered branchesRemoved: null. A later drop of another app, or the next start, then removes the fork. Both fail on base with "the fork was left on disk".
Unit: a swept isolated: false preview's fork, and a failed removal with no later drop, are both reaped; both fail on base. Removing the timer, or the incomplete-census check, fails them again.
Full unit and integration gates: every failure also fails on base or passes alone (details below).
PR QA: skipped @ f808b1e — dispatch unavailable in this session.

Gates, engines, and the other new tests

Gates ran at 2373b688f in a node:24 container with CI's harper install. Results were compared by test name with the same gates on origin/main:

test:unit:main: 3 of 8302 fail. applicationSpawn "lingering same-group descendant" also fails on base. MQTT durable "resumes after retained records" fails in 2 of 3 base reruns too. logRotator "deletes rotated logs older than the configured retention" passed 3 of 3 alone on head and on base.
test:unit:resources: 1 of 4959 fails, "Subscribe read gate … refused upgrade closes with 3003", which also fails on base.
test:integration:all: 2766 pass, 1 fail. The failure was branched-database-package matching the old loaded message; it is fixed in d939eeaa0, and that file passes 14 of 14.
Lint, typecheck and Prettier are clean. npm run check:design-docs fails only on resources/DESIGN.md at 1002 lines, as it does on base; this change does not touch that file.

Forks are RocksDB-only: prepareBranches refuses branchedDatabases under LMDB, so on LMDB no fork exists, no marker is written, and the new tests skip, as the existing branch suites do. Under HARPER_STORAGE_ENGINE=lmdb, the three touched suites ran 37 tests, all passing, and skipped 92.

The integration drops are sent from an app resource on a worker, the way a replicated peer receives them; the operations API runs drop_component on main and answers a failure directly.

Also covered: a keepBranches drop cancels an owed removal; an app deployed again keeps its forks until its next drop; an unreadable branch root keeps the timer retrying; the marker survives a failed removal and goes with a completed one, and clearing it leaves the branches alone.

🤖 Generated with Claude Code

_{🤖 Generated by Anthropic Claude (Opus 5.5); posted via @dawsontoth.}

Release target: main — fixes code from #3339, which is only on main (v5.4); not a backport

Related PRs: #3339 overlaps (adds the early answers whose removal this makes durable), #3505 overlaps (its expiry sweep is one of the leaks this heals), 1 other independent
Complexity: complicated

Deferred findings

e593471114de655c minor reliability — components/operations.ts — unreadable root config read as deployed ends the retry timer — NOT FILED
5b66151ecffb1b3c nit correctness — components/operations.ts — retained-loaded answer while the drop's finally pass discards the fork when poolMayHold is false — NOT FILED
84c308c28af47b98 nit maintainability — components/operations.ts — added comments in the delta narrate code — NOT FILED
0f7a681a6ae4cd0c nit performance — components/operations.ts — every drop synchronously scans all branch roots before returning — NOT FILED

_{Review-Coverage: authored=claude; ran=gemini,codex,cursor-composer; adjudicated=domain; blocked=cursor-grok(no-receipt); declined=cursor-kimi,cursor-muse,muse; rounds=3; full=1 @ 633c4b3}

_{Review-Attention: deep ~45m (critical: branchDatabase.ts; decisions: do-less-alternative, keep-refuses-on-incomplete-census, late-marker-failure-aborts-drop, redeploy-adopts-owed-fork, drop-refuses-without-durable-debt, retry-triggers, answer-promises-cleanup; raised: degraded review) @ 633c4b3}

gemini-code-assist

Code Review

This pull request introduces a durable retry mechanism for branched database removals that fail during application drop operations. It implements a pending branch removal marker (removal-pending) written to disk before removal begins, allowing the system to track and retry pending removals on startup, after subsequent drops, or via a backoff timer. The reviewer suggested adding an outer catch block to the runBranchRemovalPasses function to prevent potential unhandled promise rejections from unexpected errors outside the inner try/catch block.

… early answer no longer leaks fork storage drop_component answers an isolated application's own worker, and a shared worker a pool restart replaces, before it removes the application's branched databases. A removal that failed after that answer was only logged on main, and the fork stayed on disk until someone read hdb.log and dropped the application again. The PR-preview expiry sweep leaked the same way: it logs a failed drop and never lists the preview again, and an isolated: false preview's fork is retained while the pool may hold it, with nothing to drop it later. A drop that does not keep its branches now records the owed removal as a durable `removal-pending` file beside them before it changes anything. Only a removal that completes, or a later keepBranches drop, clears it. Main retries every marked application under its component lock (rechecking the marker inside it) after each drop, once a start's pool has reported what it loaded, and on a capped backoff timer while one remains. An application deployed again, or one a worker may still hold, is left for later. Closes #3463 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Pre-push review fixes: a drop takes the branch census again after it removes the component, so a fork a still-loading worker created meanwhile is recorded too; a marker that cannot be written for an unreadable directory refuses the drop; a pass that cannot read a storage root or marker keeps the timer armed; the retry lock reclaims a dead worker's ticket; the pass handle clears in the same turn as its last request check; retention messages no longer claim the workers still hold the fork. Tests now await a failed pass before releasing the holder, assert the sweep's own marker, and cover an unreadable branch root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Seeding them rewrites the config, which left componentsRoot relative for the suites after this one and failed deployPreview's retention test in the components gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… one goes unrecorded Pre-push review round 2: a keepBranches drop clears the marker whether or not the census found forks, and clearing refuses a census it cannot read; a fork found only after the component was removed, whose marker cannot be written, fails the drop saying the component is gone. The swept-preview test awaits a failed pass before restoring permissions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…retry runner Its callers discard the promise, so a throw outside the per-pass catch (from isReadOnlyMode or the timer) would have been an unhandled rejection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

On Bun a replacement worker reports what it loaded only after its predecessor exits, so the retry pass a drop starts can remove the fork between two of the test's drops; the next drop then finds nothing and answers branchesRemoved: false, and the loop waited for a true it would never see. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

kriszyp

_{🤖 Reviewed with Codex}

kriszyp · 2026-10-28T16:24:42Z

+			async () => {
+				if (!hasPendingBranchRemoval(project)) return true;
+				if (pathPresent(componentPath) || hasRootConfigEntry(project)) return true;
+				if (manageThreads.poolMayHold(project) || manageThreads.workersForApplication(project).length > 0) {


major — Automatic cleanup overlooks the serving main thread.

kriszyp · 2026-10-28T16:24:42Z

+				else if (branched) recordPendingBranchRemoval(project);
+				if (present) await removeApplication(project, componentPath);
+				// a worker still loading it can have created one since
+				if (!branched && applicationHasBranchStorage(project)) {


major — First materialization after the last scan escapes cleanup (pre-existing).

kriszyp · 2026-10-28T16:24:42Z

+			componentPath,
+			async () => {
+				if (!hasPendingBranchRemoval(project)) return true;
+				if (pathPresent(componentPath) || hasRootConfigEntry(project)) return true;


minor — Transient root-config failure stops cleanup scheduling indefinitely.

kriszyp · 2026-10-28T16:24:43Z

+			clearTimeout(branchRemovalRetryTimer);
+			branchRemovalRetryTimer = undefined;
+			branchRemovalRetryDelayMs = branchRemovalRetryFirstMs;
+		} else if (!branchRemovalRetryTimer) {


minor — Worker release does not wake pending cleanup.

kriszyp · 2026-10-28T16:24:43Z

+		throw new Error(`Could not record the owed removal of branch storage for '${appName}': ${failures.join('; ')}`);
+	}
+	for (const directory of directories) {
+		writeFileDurably(


nit — A crash-stranded marker temporary file prevents directory cleanup.

…hat drop_component records it #3600 records a branched database removal a drop could not finish and retries it on main, so disable_previews no longer tells the operator to drop that storage by hand; a manual drop_component is for whatever else the drop's error names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…iews, per-app preview limits, and harper preview setup (#3617) * Enable PR previews from the cluster: enable_previews/disable_previews, per-app bounds, harper preview setup Adds three super-user operations that create and remove one application's PR-preview identity on the cluster, so the CLI and Studio's preview panel share one implementation: - enable_previews writes, create-only, the role and user <app>-ci-preview (a component_deploy grant for <app>-pr-*: isolated, hostSuffix, branchedDatabases required by default, preview required, no table permissions), the trust policy github-actions-<app>-preview pinning repository_id, the default branch's workflow_ref, event_name pull_request_target and the preview environment, and a cleanup policy scoped to drop_component in the preview-cleanup environment. - disable_previews revokes (policies, then user) before it drops each of the application's own previews with the replicated drop_component, then removes the user and role; a failed drop stops it with the identity revoked, and a rerun resumes. - get_preview_policy reads the identity back. The per-app policy is the grant's constraints: maxPreviews replaces deployment.previews.maxPerApplication for deploys under the grant, maxTtlDays clamps preview.expiresAt, and preview: required binds every deploy to preview metadata of an app the grant previews. The CLI gains harper preview setup (repository lookup, workflow-file safety checks, the HARPER_PREVIEW_TARGET variable, an optional starter workflow and private-repository credential) and harper preview teardown, and harper preview deploy now warns when the preview's URL does not answer from the preview. Part of #3042 Part of #3055 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bind preview teardown to each node's live preview releases, and close the review's gaps - disable_previews asks every node for its live releases and drops each preview release of the app on the node that holds it, only while it is that node's live release; a node that cannot answer stops it with the identity revoked. A non-preview redeploy under the name is never dropped, and expiry no longer hides a release a peer still holds. - A deploy admitted before the revocation is refused at its activation once its deployer's records changed; an activation by deployment id is checked against the build's recorded preview first, the already-live path included. - Under preview: required the grant covers only <app>-pr-<n>, for drops and reads too; enable caps maxTtlDays at 7 unless told otherwise. - One enable or disable at a time per node, and each policy's id and overlap are rechecked just before it is written. - A user named like the identity in another role stops disable; get_preview_policy reports a role that is not enable's; a branch given as refs/heads/ is refused. - The fork guard counts only a whole && term that admits same-repository pull requests, and checks every job holding the identity token. - The starter workflow drops what it deployed if the PR closed meanwhile, and its cleanup retries while a deploy holds the preview. - The reachability check counts the preview's own 5xx as an answer and names the grant when its maxPreviews refuses a deploy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Stop preview teardown on releases it cannot attribute, and settle the second review's minors - A peer's live <app>-pr-<n> release this node has no deployment row for yet stops disable_previews with the identity revoked, instead of being taken for a non-preview; the release census has a timeout, and a peer's drop answer is unwrapped as replicated answers are. - get_preview_policy reports a role whose grant is invalid or whose operations no longer allow the preview commands; a preview grant's names must each be <app>-pr-*. - The starter workflow drops after a deploy only when the PR reads as closed, and still checks when the deploy itself failed. - The reachability check no longer takes a proxy-labelled 404 or 500 as a routing failure: behind nginx the application's own answer carries nginx's Server header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Refuse a preview workflow whose fork guard cannot be verified, and close the closing review's minors - harper preview setup follows a job's needs into the job it waits on, and refuses, unless allow_unverified=true, a fork guard or PR-head checkout it cannot rule out; branch-pattern notes stay notes. - The deployer is checked again inside the preview admission lock, just before the swap. - A peer that does not answer the release census, or answers without a release list, counts as unreachable; list_releases reports the latter as an error rather than an empty node. - get_preview_policy reports extra operations; a preview grant's names need a non-empty app; a failed drop's error names the manual drop for branched storage it left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Treat success() as a status function in the fork guard, and report a stripped preview grant - A job whose condition calls success() runs past a skipped needed job, so it no longer inherits that job's fork guard. - get_preview_policy reports a preview grant that no longer requires isolated previews under a host suffix. - A failed drop's error also names a worker it left running; the design note states that a swap already under way, or a peer's replicated deploy, can still go live after a revocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Refuse preview setup it cannot check, keep a reopened PR's preview, and read status functions in any case - harper preview setup refuses, unless allow_unverified=true, when there is no workflow file to check and none is written: the workflow is what keeps forks and PR code away from the preview identity. - The starter workflow's cleanup checks that the PR is still closed before each retry, so a PR reopened meanwhile keeps its new preview. - The fork guard reads always(), success() and the rest in any case, as GitHub does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Fail the starter workflow's jobs when GitHub cannot say the PR's state A failed read of the pull request used to read as "not closed", so cleanup exited green having dropped nothing. The deploy job now fails on a failed read, and cleanup retries the read with the drop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Stop taking head.repo.fork as proof a pull request is from the same repository head.repo.fork describes the head repository alone: it is false for a PR from the root of the base's own fork network, and a deleted fork's null head passes !fork. The fork guard now accepts only a full_name comparison with the base, and reports the fork spellings as unverified. get_preview_policy also reports a trust policy that no longer pins event_name pull_request_target and an environment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Say that Harper retries a failed preview drop's branch removal, now that drop_component records it #3600 records a branched database removal a drop could not finish and retries it on main, so disable_previews no longer tells the operator to drop that storage by hand; a manual drop_component is for whatever else the drop's error names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

gemini-code-assist Bot reviewed Oct 26, 2026

View reviewed changes

Comment thread components/operations.ts Resolved

dawsontoth requested a review from kriszyp October 26, 2026 22:51

dawsontoth added this to the v5.4 milestone Oct 26, 2026

dawsontoth mentioned this pull request Oct 27, 2026

Enable PR previews from the cluster: enable_previews and disable_previews, per-app preview limits, and harper preview setup #3617

Merged

dawsontoth force-pushed the claude/fork-removal-retry-3463-7283dc branch from f808b1e to da1569a Compare October 27, 2026 14:07

dawsontoth marked this pull request as ready for review October 27, 2026 14:53

dawsontoth force-pushed the claude/fork-removal-retry-3463-7283dc branch from da1569a to 633c4b3 Compare October 28, 2026 15:57

dawsontoth and others added 8 commits October 28, 2026 14:06

Drop a comment that restates its function's name

cbf85d5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Leave system tables unseeded in the branch-removal retry tests

795697c

Seeding them rewrites the config, which left componentsRoot relative for the suites after this one and failed deployPreview's retention test in the components gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Match the reworded loaded-retention message in the branched-package test

ebdecfe

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dawsontoth force-pushed the claude/fork-removal-retry-3463-7283dc branch from 633c4b3 to cec3b6c Compare October 28, 2026 18:06

kriszyp reviewed Oct 29, 2026

View reviewed changes

kriszyp approved these changes Oct 29, 2026

View reviewed changes

kriszyp merged commit 6bce916 into main Oct 29, 2026
101 of 103 checks passed

kriszyp deleted the claude/fork-removal-retry-3463-7283dc branch October 29, 2026 13:56

This was referenced Oct 30, 2026

[Epic] PR preview deployments #3042

Open

Remove a fork that a still-loading worker creates after drop_component looked #3818

Draft

Navigation Menu

Repository navigation

Retry a branched database removal that drop_component could not finish, instead of only logging the failure - #3600

Retry a branched database removal that drop_component could not finish, instead of only logging the failure#3600
kriszyp merged 8 commits into
mainfrom
claude/fork-removal-retry-3463-7283dc

dawsontoth commented Oct 26, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

kriszyp left a comment

Uh oh!

kriszyp Oct 28, 2026

Uh oh!

kriszyp Oct 28, 2026

Uh oh!

kriszyp Oct 28, 2026

Uh oh!

kriszyp Oct 28, 2026

Uh oh!

kriszyp Oct 28, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dawsontoth commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⊙ Problem

🤔 A redeploy adopts a fork that is still owed a removal

💡 Solution

⚖️ Alternatives

🤔 The planning review's alternative was adopted

🔧 Changes

✅ Verification

Deferred findings

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

kriszyp left a comment

Choose a reason for hiding this comment

Uh oh!

kriszyp Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

kriszyp Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

kriszyp Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

kriszyp Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

kriszyp Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dawsontoth commented Oct 26, 2026 •

edited

Loading