Repository navigation
fix(plugins): rollback renames the broken target aside; git runs foreground-only (#978) - #981
Merged
Merged
Conversation
…-rename; git runs foreground-only Closes #978. On CI the upgrade rollback failed with ENOTEMPTY renaming the backup into place: `restoreFromJournal` removed the broken target and renamed the backup into the hole, and a writer the failed step left behind (git's detached `gc --auto` after the fetch/merge) re-populated the path in between. The rename error replaced the real failure and the previous install stayed stranded in the backup — the opposite of the transaction's promise. - Restore renames the broken target ASIDE first (a directory rename is atomic whatever is inside it), puts the backup back, and deletes the discard pile and the backup dir LAST and best-effort (`removeLeftover`: retried, logged, never thrown). A leftover sits under the hidden backup prefix and `recoverPluginDir` sweeps it at the next operation or boot. The committed path uses the same best-effort delete for its backup dir — a commit is durable; cleanup can never fail the operation. - `replacePluginDir` always throws the operation's own error; a rollback that stumbles is logged beside it. - Every git Bakin spawns goes through `gitArgs` (`packages/core/src/net/git-args.ts`: gc.auto=0, gc.autoDetach=false, maintenance.auto=false) — the plugin upgrade lanes (`git()` in upgrade-gate, replacing `run('git', …)` and the raw execFileSync calls), upgrade-check, the agent-package source fetcher and the github source cache — so no housekeeping outlives a command in a directory about to be renamed or deleted. Tests: an undeletable subtree in the broken target (stand-in for the straggling writer) — the previous tree comes back byte for byte, the real error is thrown, the leftover is hidden from the loader and swept by the next boot recovery; a committed upgrade whose backup cannot be deleted still commits; the git config is honored by a real git. Both transaction tests fail against the previous rollback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nup; gitArgs everywhere (review) Codex review of #981, five findings: - Partial cleanup could erase the recovery journal: a recursive rmSync of the backup dir is not atomic — it could delete journal.json, then fail on an undeletable subtree, and the next recovery would misread the leftover (synthesizing `backup: false` and discarding the RESTORED target, or on the committed path restoring a half-deleted backup over the new install). A finished backup dir is now renamed WHOLE into `.bakin-trash-<id>-<stamp>/` (`retireBackupDir`) before any delete; trash carries no meaning and is never read, so a half-deleted trash dir cannot mislead anything. `sweepPluginTrash` deletes parked trash best-effort at boot and before every operation. The ledger row is restored BEFORE the backup is retired. - The committed-cleanup branch of `recoverPluginDir` still used a throwing rmSync, which blocked the next explicit operation while the backup was undeletable; it retires the backup the same way, so the transaction location is always free for the next operation. Pinned by a test that runs a second upgrade past parked trash, and a boot test with a committed journal and an undeletable backup. - The permission-based tests are skipped as root (where 0o555 binds nothing) instead of passing vacuously, and teardown unlocks the tree before deleting so a failing run cannot leak into /tmp. - "Every git Bakin spawns goes through gitArgs" was not true: the run-dir worktree runner, the install provenance reads and the git plugin spawned git raw. All wrapped; `tests/architecture/git-foreground-args.test.ts` scans src/, packages/, plugins/ and cli/ for any raw git spawn. - Docs: ledger-before-cleanup ordering, trash semantics, detached gc named as the suspected (not proven) writer, the two renames described as not an atomic exchange, and no retry claim (Bun ignores rmSync retry options). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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
Closes #978 — the CI flake surfaced by #975 (
ENOTEMPTYrenaming the backup into place during a failed-build rollback).restoreFromJournalremoved the broken target and renamed the backup into the hole; a writer the failed step left behind re-populated the path between the two calls (the suspect: git's detachedgc --autoafter the lane's fetch/merge). The rename error replaced the real failure and the previous install stayed stranded in the backup.<backup>/discard-*first (atomic whatever is inside it), the backup goes back, the ledger row is restored — and only then is the backup dir retired: renamed whole into.bakin-trash-<id>-<stamp>/and deleted best-effort. Trash carries no meaning and is never read, so a partial delete can never produce an ambiguous backup dir (the review's finding: a recursive delete could dropjournal.jsonfirst, then fail, and the next recovery would misread what was left).sweepPluginTrashruns at boot and before every operation; the committed path and the committed-cleanup recovery branch retire the same way, so the transaction location is always free for the next operation.gitArgs(packages/core/src/net/git-args.ts:gc.auto=0,gc.autoDetach=false,maintenance.auto=false) fronts every git Bakin spawns — plugin upgrade lanes and upgrade-check, agent-package source fetcher, github source cache, run-dir worktrees, install provenance reads, the git plugin.tests/architecture/git-foreground-args.test.tsfails on any rawspawn('git', …)in runtime code.Verification
tests/core/plugins/replace-transaction.test.ts: a read-only subtree stands in for the straggling writer (skipped as root, where it would not bind) — previous tree back byte for byte, real error thrown, backup parked whole in trash, hidden from the loader, boot recovery a no-op while the trash is undeletable, swept once it is; a committed upgrade whose backup cannot be deleted still commits and the next upgrade proceeds past the parked trash; boot with a committed journal and an undeletable backup keeps the committed dir. The two original cases fail against the previous rollback. Teardown unlocks before deleting so a failing run cannot leak into/tmp.tests/core/net/git-args.test.ts(a real git honors the config),tests/architecture/git-foreground-args.test.ts(no raw spawns), lifecycle suites (upgrade flow/smoke/decline/artifact/bins, install source-swap/subpath/ref), install-lock, source-fetcher, clawhub fetch, github-source-cache, git-worktree, run-workspace, git plugin suites green; typecheck, lint (3 pre-existing warnings),check:cyclesgreen.Docs:
.claude/knowledge/plugin-system.md§ Replace transaction (ordering, trash semantics, two renames are not an exchange, no retry claim — Bun ignoresrmSyncretry options), CLAUDE.md plugin-lifecycle bullet.🤖 Generated with Claude Code