Skip to content

test: fix Windows fixture lifetimes - #224

Merged
steipete merged 1 commit into
mainfrom
fix/windows-fixture-lifecycle
Sep 4, 2026
Merged

test: fix Windows fixture lifetimes#224
steipete merged 1 commit into
mainfrom
fix/windows-fixture-lifecycle

Conversation

@steipete

@steipete steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Windows CI exposed two fixture-lifetime failures: recursively copying the built package consumed the process-exit test's five-second deadline, and large traversal/copy-move corpora shared one 30-second deadline. Vitest does not cancel timed-out filesystem promises, so teardown could remove directories while those operations were still running, producing ENOTEMPTY.

Move physical-package setup into a separately budgeted suite hook, and serialize/drain suite-owned work before removing its directories. Preserve the independent physical package import, all twelve manager domains, every lock-cleanup assertion, and the child's four-second liveness guard. Run all 96 traversal and 48 copy/move payloads in their original order with shared fixture state and a deadline per payload. Production code, permission checks, native boundaries, global test timeouts, and coverage thresholds are unchanged.

Validation:

  • Focused suite: 159 tests passed, including fixture setup failure, operation failure, serialization, and teardown ordering.
  • Injecting a six-second package copy into the unchanged baseline reproduces its five-second timeout and confirms teardown races the copy. The candidate passes the same probe, reports no cleanup race, removes the fixture, and executes the real built-package child in 344 ms.
  • Windows Node 22 and 24 CI now run that deterministic slow-copy proof in addition to the complete checks.
  • CI=1 pnpm check, pnpm docs:site, and git diff --check passed. The complete local fallback check passed 5,038 tests (2,427 platform/native-dependent cases skipped); hosted CI covers native and Windows paths.
  • Full candidate P2 autoreview: scoped-clean, no actionable findings.

This is a focused follow-up to the postmerge Windows failures after #220. The earlier PowerShell permission-inspection timeout passed its retry; its production deadline and fail-closed behavior remain intact. No further blind retry was used for the known fixture failure.

Exact-head validation at d263be6bc84eda96e6a84e34c7b3e87ac03ee9c3:

Observed Windows proof (open step 7 in each linked job):

Runtime Real delayed-copy run Observed result
Node 22 terminal log 7.45 s, including the injected 6 s delay; child case 346 ms One physical copy completed; copied: true, teardownRacedCopy: false; test passed.
Node 24 terminal log 7.21 s, including the injected 6 s delay One physical copy completed; copied: true, teardownRacedCopy: false; test passed.

Both successful runs include the fixture-absence assertion after teardown, independent-package assertion, twelve lock-domain cleanup assertions, and unchanged four-second child deadline. These completed Windows logs supply the runtime evidence requested in the earlier automated review.

No failures or reruns on this repair candidate. Prepared for maintainer review; not merged.

Move the physical package copy into suite setup and drain tracked fixture
operations before teardown. Preserve the child liveness deadline and all
independent-package and lock-domain cleanup assertions.

Split the ordered traversal and copy/move corpora into per-payload cases
without changing their shared state or boundary checks. Add lifecycle
regressions and a Windows proof that delays the real package copy past the
ordinary test timeout.
@steipete
steipete requested a review from a team as a code owner September 4, 2026 09:09
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 4, 2026
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 5:34 AM ET / 09:34 UTC.

ClawSweeper review

What this changes

The PR moves slow shared test fixture setup into a separately timed lifecycle, drains pending filesystem work before cleanup, splits two adversarial corpora into per-payload tests, and adds a Windows slow-copy CI proof.

Merge readiness

Ready for maintainer review

Keep open: this focused repair remains necessary because current main retains the timeout-prone fixture shape; the branch adds a scoped lifecycle guard with exact-head Windows proof and no actionable defect found.

Priority: P2
Reviewed head: d263be6bc84eda96e6a84e34c7b3e87ac03ee9c3

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A focused test-infrastructure repair with direct Windows fault-injection evidence and no identified correctness or security defect.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The production-adjacent behavior is the real built-package copy and child-process lock-cleanup path; linked exact-head Windows Node 22 and 24 traces inject the six-second copy, then report completed copy, non-racing teardown, fixture removal, and passing tests.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The production-adjacent behavior is the real built-package copy and child-process lock-cleanup path; linked exact-head Windows Node 22 and 24 traces inject the six-second copy, then report completed copy, non-racing teardown, fixture removal, and passing tests.
Evidence reviewed 4 items Current-main baseline: The PR base retains the physical-package copy inside the ordinary test body, so main does not yet contain the separate fixture-lifetime protection.
Fixture drain implementation: The introduced helper serializes fixture operations, preserves operation failures for Vitest, and waits for the tracked work before cleanup.
Exact-head Windows behavior proof: The supplied PR body links successful Windows Node 22 and 24 terminal runs at the reviewed head, each injecting a six-second package copy and reporting one completed copy with teardownRacedCopy false; the provided check set is complete and successful.
Findings None None.
Security None None.

How this fits together

The test suite creates temporary filesystem fixtures and runs built-package child processes to validate lock cleanup and path confinement. The new fixture lifecycle tracks setup and test work so teardown only removes directories after that work has settled, with Windows CI exercising the delayed-copy case.

flowchart TD
  A[Filesystem test suites] --> B[Shared fixture lifecycle]
  B --> C[Prepare fixture work]
  C --> D[Run ordered test cases]
  D --> E[Drain pending work]
  E --> F[Remove temporary directories]
  F --> G[Windows slow-copy CI proof]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 9 files; production +0, test/harness net +196, workflow +4 The change is confined to test infrastructure, contributor guidance, release notes, and one CI proof step.
Windows proof coverage 2 Windows runtimes Exact-head Node 22 and Node 24 runs exercised the injected six-second copy scenario.

Technical review

Best possible solution:

Land the scoped fixture-lifecycle repair so Windows cleanup assertions remain deterministic without changing filesystem runtime behavior.

Do we have a high-confidence way to reproduce the issue?

Yes—source-reproducible: the reviewed slow-copy configuration defines the delayed package-copy scenario, and the supplied exact-head Windows runs report the repaired teardown behavior.

Is this the best way to solve the issue?

Yes. Tracking and draining the shared fixture's actual work before cleanup is the narrowest maintainable repair while retaining the child-process liveness limit and boundary assertions.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against dc6daf69ca00.

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The production-adjacent behavior is the real built-package copy and child-process lock-cleanup path; linked exact-head Windows Node 22 and 24 traces inject the six-second copy, then report completed copy, non-racing teardown, fixture removal, and passing tests.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The production-adjacent behavior is the real built-package copy and child-process lock-cleanup path; linked exact-head Windows Node 22 and 24 traces inject the six-second copy, then report completed copy, non-racing teardown, fixture removal, and passing tests.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: This repairs a bounded Windows CI fixture-lifetime failure without changing the package's runtime filesystem contract.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The production-adjacent behavior is the real built-package copy and child-process lock-cleanup path; linked exact-head Windows Node 22 and 24 traces inject the six-second copy, then report completed copy, non-racing teardown, fixture removal, and passing tests.
  • proof: sufficient: Contributor real behavior proof is sufficient. The production-adjacent behavior is the real built-package copy and child-process lock-cleanup path; linked exact-head Windows Node 22 and 24 traces inject the six-second copy, then report completed copy, non-racing teardown, fixture removal, and passing tests.

Evidence

What I checked:

  • Current-main baseline: The PR base retains the physical-package copy inside the ordinary test body, so main does not yet contain the separate fixture-lifetime protection. (test/sidecar-lock-process-exit.test.ts:136, dc6daf69ca00)
  • Fixture drain implementation: The introduced helper serializes fixture operations, preserves operation failures for Vitest, and waits for the tracked work before cleanup. (test/helpers/suite-fixture.ts:13, d263be6bc84e)
  • Exact-head Windows behavior proof: The supplied PR body links successful Windows Node 22 and 24 terminal runs at the reviewed head, each injecting a six-second package copy and reporting one completed copy with teardownRacedCopy false; the provided check set is complete and successful. (scripts/slow-package-copy.setup.ts:35, d263be6bc84e)
  • Feature-history routing: The physical-package test originated in the merged sidecar-lock work; current line 156 remains attributable to that source commit, whose recorded parent lacks this new file. (test/sidecar-lock-process-exit.test.ts:156, a41b18dd37fd)

Likely related people:

  • unknown: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-04T09:16:46.563Z sha d263be6 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 4, 2026
@steipete
steipete merged commit 15d313d into main Sep 4, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant