fix(tests): live-e2e の scratch 削除を検証付きへ — rmSync force:true の無音 no-op を解消 (#2154) - #2548
Merged
Conversation
`rmSync(root, { recursive: true, force: true })` swallows the transient ENOENT
a concurrent remover produces between the directory walk and the unlink. The
swallowed error turns the removal into a silent no-op, leaving the scratch root
behind while the caller believes it is gone — the mechanism behind the
probabilistic red in tests/integration/t-codex-exec-live-helper.test.ts.
Add tests/harness/live-e2e/testing/remove-tree-verified.ts, a synchronous
helper that removes, verifies with existsSync, retries up to three times, and
throws with the surviving path when the tree will not go. Non-ENOENT failures
(EACCES, EPERM, …) still propagate. It mirrors the async pattern already proven
in live-e2e/kiro-tui.ts #removeScratch, which is left unchanged.
Apply it at the seven synchronous scratch-removal callsites: the codex-exec
home cleanup closure, the five same-root cleanup paths (claude-sdk, codex,
claude-tui, claude, testing/fakes), and the kiro allocate-failure path.
Barrier-synchronised probe over 300 trials with four concurrent removers:
plain rmSync silently left the root behind 265 times; removeTreeVerified never
returned silently with a surviving root (0/300), failing loudly 8 times instead.
Refs #2154
Verified removal can now throw, and the setup-failure rollback paths ran it as
`catch (error) { cleanup(); throw error; }` — a cleanup throw would replace the
original setup error and lose the reason the rollback was running at all.
Wrap each rollback so both errors survive: on a cleanup failure the callsite
throws `AggregateError([setupError, cleanupError])`. When cleanup succeeds the
original error is rethrown unchanged, so existing rollback assertions are
untouched.
Also apply removeTreeVerified at the two allocate-failure paths missed in the
first pass (live-e2e/claude.ts, live-e2e/codex.ts) — same defect class as the
kiro allocate path already converted.
Cover the composition with an integration test that pins a subtree read-only so
the rollback fails with EACCES, and assert the AggregateError carries the
original setup failure first.
Refs #2154
|
Warning Review limit reached
Next review available in: 49 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Comment |
…2154) The retry and exhaustion branches of removeTreeVerified are only reachable when a concurrent remover wins the race, so no real-filesystem test can force them on demand and they landed uncovered in the patch coverage gate. Add a RemoveTreeIo seam (rm/exists/sleep) with the real filesystem as the default argument, so every callsite stays unchanged, and drive the branches from a test-side fake: first-attempt success, one retry, two retries, budget exhaustion (asserting the surviving path is in the message), and propagation of a removal error. The fake lives entirely in the test; no test-only branch enters the helper. The fake never touches the filesystem, so the new suite is a unit test; the existing real-filesystem suite stays in integration unchanged. Refs #2154
…aths (#2154) Both codex-exec setup paths carried the same eight-line rollback block, and the setupCodexExecHome copy is unreachable from a test: its only fallible statement is a mkdirSync on a directory the preceding mkdtempSync just created, so no external fixture can fail one without failing the other first. Those lines were the patch coverage gate's remaining violations. Extract rollbackOrAggregate into remove-tree-verified.ts, documented as always throwing: the original error is rethrown unchanged when cleanup succeeds, and only a failing cleanup produces the AggregateError. Both codex-exec callsites now delegate to it, so the composition is exercised once by the existing setupCodexExecProject rollback-failure test. Collapse the unreachable Home catch onto one line so it occupies a single lcov row, and record next to it why no fixture can reach it. Refs #2154
#2154) Sharing rollbackOrAggregate removed the duplicated composition but left each setup with its own catch clause, and the setupCodexExecHome one is unreachable from any fixture: its only fallible statement is a mkdirSync against a directory the preceding mkdtempSync just created. That clause stayed uncovered, and the patch coverage gate is zero-tolerance. Add withRollback(cleanup, work), which owns the catch, and run both setups through it. The clause now lives in one place that the existing setupCodexExecProject rollback tests exercise, so no unreachable catch remains in the patch. rollbackOrAggregate keeps its contract and still composes the errors. Value and exception semantics are unchanged: work's value is returned as-is, a successful cleanup rethrows the original error, and only a failing cleanup raises the AggregateError — pinned by the five existing helper tests. Patch coverage over the branch diff for both files: 34 measured added lines, 34 covered, 0 uncovered. Refs #2154
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.
概要
live-e2e scratch ツリーの削除が並行 remover 競合時に無音 no-op になる欠陥（#2154）を、検証付き削除ヘルパーで解消する。
Closes #2154
機序（クロスレビュー2名で確定済み）
rmSync(root, {recursive:true, force:true})は、並行 remover が walk と unlink の間にエントリを消したときの一過性 ENOENT を握り潰し、ツリーを残したまま正常 return する。呼び出し元は削除成功を検証していないため、t-codex-exec-live-helperのロールバック検証が確率的に赤（reviewer-1 の再現: 88/1000 @ remover 4本）。変更内容（tests/ のみ、packages/ 無変更）
tests/harness/live-e2e/testing/remove-tree-verified.ts（新設）: remove →existsSync検証 → 10ms 待機で最大3回 → 残存なら loud throw。非 ENOENT（EACCES 等）は伝播。様式はkiro-tui.ts #removeScratch（既存の実証済み async 版）の同期形codex-exec-live.ts+ live-e2e のclaude-sdk/codex×2 /claude-tui/claude×2 /kiro/testing/fakes（kiro-tuiは既に検証付きのため不変更）catch (error) { cleanup(); throw error; }形 5 callsite で、cleanup 失敗時にAggregateError([setupError, cleanupError])— 元エラーも cleanup 失敗もどちらも失わないt-remove-tree-verified.test.ts（happy / 冪等 / 削除不能ツリーで loud 失敗）+t-codex-exec-live-helperへ AggregateError 回帰ケース追加検証
git diff空）rmSync(force:true)= 265/300 無音残存 / ヘルパー = 0/300（8件は retry 枯渇の loud throw — 無音でない失敗は契約どおり）t-remove-tree-verified/t-codex-exec-live-helper/ typecheck / lint 全て exit 0、consumer integration 11ファイル 93 pass / 0 failgrep -rn "rmSync(.*recursive.*force" tests/harness/）: 置換9 / 対象外12（kiro-tui 既検証・lock 解放系・fixture teardown 系 — 分類は Issue クロスレビューと整合）。live-e2e scratch 削除クラスの取り残し 0スコープ外
tests/全体の同型 ~725 箇所（reviewer-2 指摘の広義クラス）は本 PR の所有境界外。live-e2e 面の閉包を先行し、広域は別判断。関連
#2154 / #1565（同根の過去事例・CLOSED）/ PR #2159（対象ファイルの改名元）
Note
Low Risk
Test-harness-only changes; no production packages touched. Slightly stricter teardown may surface real filesystem permission issues as loud failures instead of silent leaks.
Overview
Fixes flaky live-e2e / codex-exec helper tests where
rmSync(..., { recursive: true, force: true })could leave scratch dirs behind when concurrent removers race (silent no-op on swallowed ENOENT, #2154).Adds
removeTreeVerified: remove,existsSynccheck, up to three attempts with a short delay, then throw if the tree remains; non-ENOENT errors still propagate. Live-e2e harness andcodex-exec-livescratch teardown now call this instead of rawrmSync(nine sites).On setup rollback, failed cleanup no longer hides the original error:
AggregateErrorcarries both the setup failure and cleanup failure (rollback cleanup failed after setup failure).New coverage:
t-remove-tree-verified.test.tsand anAggregateErrorregression int-codex-exec-live-helper.test.ts.Reviewed by Cursor Bugbot for commit f42651f. Configure here.