fix(bundle): atomic re-commit via per-entry aside (compatible with #162)#196
fix(bundle): atomic re-commit via per-entry aside (compatible with #162)#196SahilRakhaiya05 wants to merge 2 commits into
Conversation
SahilRakhaiya05
requested review from
ruili-testsprite and
zeshi-du
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangescommitBundle aside/rollback rework
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
src/lib/bundle.commit.test.ts (1)
72-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for failure after a new artifact is installed.
The rollback test only fails on the first staged rename (
result.json), so no artifact is installed before the throw. It does not exercise the case where a new artifact installs successfully and a later rename (e.g.meta.json) fails — which is where a stitched old-meta/new-artifact bundle can survive rollback (see thecommitBundlerollback comment). Consider adding a case that throws on themeta.jsonrename and asserts that no new-run artifact (e.g.result.json) remains alongside the restored oldmeta.json.🤖 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.commit.test.ts` around lines 72 - 92, The current rollback test in commitBundle only covers a failure on the first staged rename, so it never verifies rollback after a new artifact has already been installed. Add a second case in src/lib/bundle.commit.test.ts using commitBundle and the existing renameMock/seedBundleDirs helpers that throws on the meta.json rename after allowing result.json to install, then assert the prior bundle is fully restored and no new-run artifact like result.json remains alongside the old meta.json.
🤖 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 578-627: The rollback logic in the bundle commit flow does not
remove newly installed destination files when a later rename fails, so a failed
update can leave mixed bundle contents behind. Update the rollback path in
src/lib/bundle.ts to track every destination written by the commit sequence in
the main try block, especially the steps directory and top-level files renamed
from tmpDir, and ensure rollback deletes those installed targets before
restoring aside entries. Keep the fix localized around rollback, asideIfPresent,
and the rename/unlink steps so the bundle returns to a clean pre-commit state on
any error.
---
Nitpick comments:
In `@src/lib/bundle.commit.test.ts`:
- Around line 72-92: The current rollback test in commitBundle only covers a
failure on the first staged rename, so it never verifies rollback after a new
artifact has already been installed. Add a second case in
src/lib/bundle.commit.test.ts using commitBundle and the existing
renameMock/seedBundleDirs helpers that throws on the meta.json rename after
allowing result.json to install, then assert the prior bundle is fully restored
and no new-run artifact like result.json remains alongside the old meta.json.
🪄 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: 6ee143ef-3954-461d-8a1b-f9362b62f177
📒 Files selected for processing (2)
src/lib/bundle.commit.test.tssrc/lib/bundle.ts
| const rollback = async (): Promise<void> => { | ||
| 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); | ||
| }; | ||
|
|
||
| // (4) Top-level files (result/failure/code/video). meta.json renames | ||
| // LAST; track it separately. | ||
| 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)); | ||
| } | ||
| try { | ||
| const topLevel = files.filter(f => !f.startsWith('steps/')); | ||
| const newTopLevelSet = new Set(topLevel); | ||
| newTopLevelSet.add('meta.json'); | ||
|
|
||
| const existing = await readdir(dir).catch(() => [] as string[]); | ||
| for (const entry of existing) { | ||
| if (entry === '.tmp') continue; | ||
| if (!isBundleOwnedEntry(entry)) continue; | ||
|
|
||
| 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant structure in src/lib/bundle.ts and inspect the surrounding logic.
ast-grep outline src/lib/bundle.ts --view expanded || true
echo '--- bundle.ts around the commit/rollback section ---'
sed -n '500,700p' src/lib/bundle.ts
echo '--- tests mentioning partial/rollback/bundle integrity ---'
rg -n "partial|rollback|asideIfPresent|commitBundle|bundle integrity|stitch|meta.json|result.json" src -g '*.{ts,tsx,js,mjs,cjs}' -A 4 -B 4Repository: TestSprite/testsprite-cli
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tests specifically around bundle commit failure modes.
rg -n "result\.json|meta\.json|steps/|\.partial|rollback|rename" tests src -g '*.{ts,tsx,js,mjs,cjs}' -A 6 -B 6
# If tests exist in src/lib, inspect them with line numbers.
if [ -f src/lib/bundle.test.ts ]; then
echo '--- src/lib/bundle.test.ts ---'
sed -n '1,260p' src/lib/bundle.test.ts
fi
if [ -f src/lib/bundle.spec.ts ]; then
echo '--- src/lib/bundle.spec.ts ---'
sed -n '1,260p' src/lib/bundle.spec.ts
fiRepository: TestSprite/testsprite-cli
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the bundle-related helper definitions referenced by the commit path.
ast-grep outline src/lib/bundle.ts --view expanded | sed -n '1,220p'
echo '--- helper definitions ---'
rg -n "function asideIfPresent|const asideIfPresent|async function asideIfPresent|dirExists|isBundleOwnedEntry|asideLog|tmpDir|asideDir" src/lib/bundle.ts -A 5 -B 5
echo '--- any exact-run integrity enforcement nearby ---'
rg -n "snapshotId|run-id|integrity|mismatch|reject" src/lib/bundle.ts src -g '*.{ts,tsx,js,mjs,cjs}' -A 4 -B 4Repository: TestSprite/testsprite-cli
Length of output: 50382
Rollback must clean up newly installed bundle files (src/lib/bundle.ts:578-627)
If a later rename fails after result.json or steps/ has already been moved in, rollback() restores the aside entries but leaves those new files behind, which can mix two runs in the same bundle. Track installed destinations and delete them during rollback.
🤖 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 578 - 627, The rollback logic in the bundle
commit flow does not remove newly installed destination files when a later
rename fails, so a failed update can leave mixed bundle contents behind. Update
the rollback path in src/lib/bundle.ts to track every destination written by the
commit sequence in the main try block, especially the steps directory and
top-level files renamed from tmpDir, and ensure rollback deletes those installed
targets before restoring aside entries. Keep the fix localized around rollback,
asideIfPresent, and the rename/unlink steps so the bundle returns to a clean
pre-commit state on any error.
Source: Path instructions
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
This is very close — the aside/rollback design is a real improvement over the old commit path (which had no rollback at all), and the #162 foreign-file contract is correctly preserved. One ordering issue before it can land:
meta.json must be the FIRST entry moved aside, not wherever readdir happens to place it.
The old commit path removed the old meta.json first, deliberately: its presence is the bundle's completion signal (§3/§7.3 — "no meta → bundle absent or mid-write, refuse to consume"). In this version the aside loop walks readdir(dir) order, which is platform-arbitrary. If steps/ is moved aside before meta.json, there's a window where a concurrent reader sees a present meta.json (⇒ "bundle complete") pointing at steps that are already gone — exactly the torn read the meta-first ordering existed to prevent.
Fix is one line: await asideIfPresent('meta.json') before the loop (and skip it inside the loop). Install-side ordering is already right (meta lands last).
Nit, non-blocking: asideDir lives in dirname(dir), so a read-only parent with a writable --out dir now fails the commit where it previously succeeded. The failure is caught and rolled back cleanly, so I'm fine with it — just noting the behavior change.
Happy to merge as soon as the meta-first ordering is in.
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
New Features
--outdirectories that already include unrelated files.Bug Fixes
Tests