Recovered: fix(bundle): atomic re-commit via per-entry aside (#196 by @SahilRakhaiya05)#246
Recovered: fix(bundle): atomic re-commit via per-entry aside (#196 by @SahilRakhaiya05)#246jangjos-128 wants to merge 2 commits into
Conversation
Walkthrough
ChangesBundle commit flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant commitBundle
participant BundleDirectory
participant AsideDirectory
participant StagedDirectory
commitBundle->>AsideDirectory: Move existing bundle-owned entries
commitBundle->>BundleDirectory: Install staged artifacts
StagedDirectory-->>BundleDirectory: Provide new bundle files
commitBundle->>AsideDirectory: Remove aside entries
commitBundle->>BundleDirectory: Restore entries if installation fails
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/bundle.ts`:
- Around line 601-632: Update the commit/rollback flow around rollback(), the
steps rename, the beforeMeta loop, and the final meta.json rename to track every
newly installed path, including steps and top-level artifacts with no prior
aside entry. On any failure, have rollback remove those tracked paths before
restoring aside’d entries, so the directory returns to its prior complete bundle
state.
- Around line 624-627: Make the post-commit cleanup in the bundle installation
flow best-effort by preventing failures from rm(tmpDir, { recursive: true,
force: true }) from propagating into the rollback handler. Update the cleanup
near the commit completion signal so tmpDir removal is handled like asideDir
removal, while preserving the existing cleanup order and rollback behavior for
failures occurring before commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a86fdbac-2091-4c41-9d75-e7af12c5fb87
📒 Files selected for processing (2)
src/lib/bundle.commit.test.tssrc/lib/bundle.ts
| const isStale = entry !== 'steps' && entry !== '.partial' && !newTopLevelSet.has(entry); | ||
| const willReplace = entry === 'steps' || newTopLevelSet.has(entry); | ||
| if (isStale || willReplace) { | ||
| await asideIfPresent(entry); | ||
| } | ||
| } | ||
|
|
||
| // (5) meta.json LAST → atomic completion signal. | ||
| if (metaIdx >= 0) { | ||
| await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json')); | ||
| } | ||
| const stepsTmp = join(tmpDir, 'steps'); | ||
| const stepsDir = join(dir, 'steps'); | ||
| if (await dirExists(stepsTmp)) { | ||
| await rename(stepsTmp, stepsDir); | ||
| } | ||
|
|
||
| // .partial from a prior aborted run is now stale. Remove it so an | ||
| // agent inspecting the dir sees only the fresh bundle. | ||
| await unlink(join(dir, '.partial')).catch(() => undefined); | ||
| const metaIdx = topLevel.indexOf('meta.json'); | ||
| const beforeMeta = metaIdx >= 0 ? topLevel.filter((_, i) => i !== metaIdx) : topLevel; | ||
| for (const file of beforeMeta) { | ||
| await rename(join(tmpDir, file), join(dir, file)); | ||
| } | ||
|
|
||
| // Clean up the now-empty tmp dir. | ||
| await rm(tmpDir, { recursive: true, force: true }); | ||
| if (metaIdx >= 0) { | ||
| await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json')); | ||
| } | ||
|
|
||
| await unlink(join(dir, '.partial')).catch(() => undefined); | ||
| await rm(tmpDir, { recursive: true, force: true }); | ||
| // Best-effort aside cleanup — failures must not roll back a committed bundle. | ||
| await rm(asideDir, { recursive: true, force: true }).catch(() => undefined); | ||
| } catch (err) { | ||
| await rollback(); | ||
| await rm(tmpDir, { recursive: true, force: true }).catch(() => undefined); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Rollback doesn't remove newly-installed artifacts that had no prior aside entry.
rollback() only undoes entries recorded in asideLog, which is populated exclusively by asideIfPresent for paths that already existed in dir. Any top-level artifact that is genuinely new (e.g. result.json installed where the prior bundle only had failure.json, or steps/meta.json on a first-ever commit) is renamed into dir by the beforeMeta loop / steps rename / final meta.json rename (Lines 608-622) but never tracked for removal on failure.
If a later step in that same sequence throws, rollback() (Lines 579-585) restores the old aside'd entries correctly, but leaves the just-installed new artifact(s) sitting in dir — the directory ends up as a mix of old + new content instead of atomically reverting to "the prior complete bundle" as the docstring promises (Lines 557-558). None of the three tests exercise this because seedBundleDirs only ever pre-creates entries that already exist in the old bundle, so the failing rename in the rollback test (result.json) never actually lands before it throws.
🐛 Proposed fix: track newly-installed paths and clean them up on failure
const asideLog: Array<{ asidePath: string; restorePath: string }> = [];
+ const installedTopLevel: string[] = [];
const asideIfPresent = async (entry: string): Promise<void> => {
...
};
const rollback = async (): Promise<void> => {
+ for (const target of installedTopLevel) {
+ await rm(target, { recursive: true, force: true }).catch(() => undefined);
+ }
for (const { asidePath, restorePath } of [...asideLog].reverse()) {
await rm(restorePath, { recursive: true, force: true }).catch(() => undefined);
await rename(asidePath, restorePath).catch(() => undefined);
}
await rm(asideDir, { recursive: true, force: true }).catch(() => undefined);
};
try {
...
if (await dirExists(stepsTmp)) {
await rename(stepsTmp, stepsDir);
+ installedTopLevel.push(stepsDir);
}
const metaIdx = topLevel.indexOf('meta.json');
const beforeMeta = metaIdx >= 0 ? topLevel.filter((_, i) => i !== metaIdx) : topLevel;
for (const file of beforeMeta) {
- await rename(join(tmpDir, file), join(dir, file));
+ const dest = join(dir, file);
+ await rename(join(tmpDir, file), dest);
+ installedTopLevel.push(dest);
}
if (metaIdx >= 0) {
- await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json'));
+ const dest = join(dir, 'meta.json');
+ await rename(join(tmpDir, 'meta.json'), dest);
+ installedTopLevel.push(dest);
}Would also be worth adding a test that seeds an old bundle with a different top-level artifact set than the new one (e.g. old has failure.json, new has result.json), fails mid-install, and asserts the new artifact doesn't linger. Want me to draft that test?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isStale = entry !== 'steps' && entry !== '.partial' && !newTopLevelSet.has(entry); | |
| const willReplace = entry === 'steps' || newTopLevelSet.has(entry); | |
| if (isStale || willReplace) { | |
| await asideIfPresent(entry); | |
| } | |
| } | |
| // (5) meta.json LAST → atomic completion signal. | |
| if (metaIdx >= 0) { | |
| await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json')); | |
| } | |
| const stepsTmp = join(tmpDir, 'steps'); | |
| const stepsDir = join(dir, 'steps'); | |
| if (await dirExists(stepsTmp)) { | |
| await rename(stepsTmp, stepsDir); | |
| } | |
| // .partial from a prior aborted run is now stale. Remove it so an | |
| // agent inspecting the dir sees only the fresh bundle. | |
| await unlink(join(dir, '.partial')).catch(() => undefined); | |
| const metaIdx = topLevel.indexOf('meta.json'); | |
| const beforeMeta = metaIdx >= 0 ? topLevel.filter((_, i) => i !== metaIdx) : topLevel; | |
| for (const file of beforeMeta) { | |
| await rename(join(tmpDir, file), join(dir, file)); | |
| } | |
| // Clean up the now-empty tmp dir. | |
| await rm(tmpDir, { recursive: true, force: true }); | |
| if (metaIdx >= 0) { | |
| await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json')); | |
| } | |
| await unlink(join(dir, '.partial')).catch(() => undefined); | |
| await rm(tmpDir, { recursive: true, force: true }); | |
| // Best-effort aside cleanup — failures must not roll back a committed bundle. | |
| await rm(asideDir, { recursive: true, force: true }).catch(() => undefined); | |
| } catch (err) { | |
| await rollback(); | |
| await rm(tmpDir, { recursive: true, force: true }).catch(() => undefined); | |
| throw err; | |
| } | |
| const asideLog: Array<{ asidePath: string; restorePath: string }> = []; | |
| const installedTopLevel: string[] = []; | |
| const asideIfPresent = async (entry: string): Promise<void> => { | |
| ... | |
| }; | |
| const rollback = async (): Promise<void> => { | |
| for (const target of installedTopLevel) { | |
| await rm(target, { recursive: true, force: true }).catch(() => undefined); | |
| } | |
| for (const { asidePath, restorePath } of [...asideLog].reverse()) { | |
| await rm(restorePath, { recursive: true, force: true }).catch(() => undefined); | |
| await rename(asidePath, restorePath).catch(() => undefined); | |
| } | |
| await rm(asideDir, { recursive: true, force: true }).catch(() => undefined); | |
| }; | |
| try { | |
| ... | |
| if (await dirExists(stepsTmp)) { | |
| await rename(stepsTmp, stepsDir); | |
| installedTopLevel.push(stepsDir); | |
| } | |
| const metaIdx = topLevel.indexOf('meta.json'); | |
| const beforeMeta = metaIdx >= 0 ? topLevel.filter((_, i) => i !== metaIdx) : topLevel; | |
| for (const file of beforeMeta) { | |
| const dest = join(dir, file); | |
| await rename(join(tmpDir, file), dest); | |
| installedTopLevel.push(dest); | |
| } | |
| if (metaIdx >= 0) { | |
| const dest = join(dir, 'meta.json'); | |
| await rename(join(tmpDir, 'meta.json'), dest); | |
| installedTopLevel.push(dest); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/bundle.ts` around lines 601 - 632, Update the commit/rollback flow
around rollback(), the steps rename, the beforeMeta loop, and the final
meta.json rename to track every newly installed path, including steps and
top-level artifacts with no prior aside entry. On any failure, have rollback
remove those tracked paths before restoring aside’d entries, so the directory
returns to its prior complete bundle state.
| await unlink(join(dir, '.partial')).catch(() => undefined); | ||
| await rm(tmpDir, { recursive: true, force: true }); | ||
| // Best-effort aside cleanup — failures must not roll back a committed bundle. | ||
| await rm(asideDir, { recursive: true, force: true }).catch(() => undefined); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Unguarded rm(tmpDir, ...) can trigger a rollback of an already-committed bundle.
By Line 625, meta.json has already been renamed into dir (Line 621) — per this function's own contract, that's the completion signal and the commit is logically done. Yet rm(tmpDir, { recursive: true, force: true }) on Line 625 has no .catch(), unlike the aside cleanup right below it. If it throws (e.g. EACCES/EBUSY on some entry inside .tmp, a locked file, etc.), the error propagates to the catch block, which calls rollback() — reverting the just-installed meta.json/steps/artifacts back to the old aside'd content, and then re-throwing. A caller sees a failure and (per the docstring) will assume nothing changed, but the new bundle was actually live for a moment and any concurrent reader may have already observed it — and now it's silently un-committed due to an unrelated tmp-directory cleanup failure.
The comment on Line 626 ("failures must not roll back a committed bundle") already states the intended invariant for aside cleanup — the same reasoning applies to tmpDir cleanup at this point, since it also runs after the point of no return.
🐛 Proposed fix
await unlink(join(dir, '.partial')).catch(() => undefined);
- await rm(tmpDir, { recursive: true, force: true });
- // Best-effort aside cleanup — failures must not roll back a committed bundle.
+ // Best-effort cleanup — the bundle is already committed once meta.json lands;
+ // failures here must not roll back an already-successful commit.
+ await rm(tmpDir, { recursive: true, force: true }).catch(() => undefined);
await rm(asideDir, { recursive: true, force: true }).catch(() => undefined);A regression test simulating rm failing for tmpDir (not the aside dir) after a successful install would help lock this in.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await unlink(join(dir, '.partial')).catch(() => undefined); | |
| await rm(tmpDir, { recursive: true, force: true }); | |
| // Best-effort aside cleanup — failures must not roll back a committed bundle. | |
| await rm(asideDir, { recursive: true, force: true }).catch(() => undefined); | |
| await unlink(join(dir, '.partial')).catch(() => undefined); | |
| // Best-effort cleanup — the bundle is already committed once meta.json lands; | |
| // failures here must not roll back an already-successful commit. | |
| await rm(tmpDir, { recursive: true, force: true }).catch(() => undefined); | |
| await rm(asideDir, { recursive: true, force: true }).catch(() => undefined); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/bundle.ts` around lines 624 - 627, Make the post-commit cleanup in
the bundle installation flow best-effort by preventing failures from rm(tmpDir,
{ recursive: true, force: true }) from propagating into the rollback handler.
Update the cleanup near the commit completion signal so tmpDir removal is
handled like asideDir removal, while preserving the existing cleanup order and
rollback behavior for failures occurring before commit.
2 participants
Summary
Re-running
test failure get --out <dir>can deletemeta.jsonmid-commit; a failed re-commit then leaves the directory without a usable bundle.This redesigns
commitBundleto be compatible with #162 (isBundleOwnedEntry):--outmay contain the user's unrelated files, so a whole-directory swap is not safe.Approach
<dir>/.tmp/(unchanged).<dir>.aside.<uuid>/directory.tmp/;meta.jsonlands last (completion signal)Foreign files (e.g.
notes.txt,src/) are never touched.Testing
npx vitest run src/lib/bundle.commit.test.ts- rollback, foreign-file preservation, aside cleanupnpx vitest run src/lib/bundle.test.ts -t "commit sweep"- existing fix(bundle): preserve unrelated pre-existing files in --out dir (stop data-loss sweep) #162 ownership guardnpm run typecheckandnpm run lint- passSplit from #8 per review feedback.
Summary by CodeRabbit