test(opencode): retry missing session directory cleanup#511
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function removeSessionProjectDirectory in export.test.ts to encapsulate directory removal logic with retry attempts, specifically targeting potential file system issues on Windows. The direct calls to fs.rm within the test suite have been updated to use this new helper. I have no feedback to provide as there were no review comments.
Root cause: - The post-merge Windows advisory job after PR #511 still failed in unit-windows-opencode-session with EBUSY while deleting a git-backed temp project directory. - The export tests were deleting a session project directory after that directory had already been loaded into InstanceStore. - The lifecycle contract was incomplete: app-owned resources for a loaded directory must be released before the fixture removes the directory from disk. Raising fs.rm retries alone would only hide timing, not release the owned handles. Fix: - Added InstanceStore.disposeDirectory(directory) as a no-load disposal path: resolve the directory, look up the existing entry, return if it is not loaded, await the deferred load, remove failed loads, and dispose successful loads through the existing disposal machinery. - Threaded that API through instance-runtime and the public Instance.disposeDirectory wrapper, including cleanup of the module-level directory cache. - Updated export test cleanup to call Instance.disposeDirectory(dir) before deleting loaded session project fixture directories. - Kept the Windows fs.rm retry budget at 30 after review, while leaving non-Windows at 5, so external transient OS locks remain tolerated after app-owned resources are released. Scope boundary: - Product export behavior is unchanged. - The current call site is test fixture cleanup only, but the disposal API lives in the production instance layer because the lifecycle contract belongs there. - disposeDirectory intentionally does not bootstrap or mutate missing directories. Verification: - Red test confirmed InstanceStore lacked disposeDirectory before implementation. - Instance store focused tests: 7 pass, 0 fail. - Export focused tests: 32 pass, 0 fail. - Combined focused tests: 39 pass, 0 fail. - Typecheck: bun run --cwd packages/opencode typecheck passed. - Diff check: git diff --check passed. - Windows advisory workflow_dispatch run 25605476612 passed, including unit-windows-opencode-session in 8m31s. - Final PR checks passed after review follow-up: CodeQL, CodeRabbit, analyze-js-ts, e2e-artifacts, smoke-macos-arm64, typecheck, unit-app, unit-desktop, unit-opencode, commit-lint, and dependency-review. Review closure: - Gemini flagged that reducing Windows fs.rm retries from 30 to 5 could reintroduce flaky cleanup if a lock came from outside InstanceStore. - Commit 521f9d8 restored the Windows-specific 30 retry budget while keeping Instance.disposeDirectory as the root-cause release step. - The review thread was replied to and resolved. CodeRabbit left no actionable code comments; its remaining note was a rate-limit/status comment. Follow-up: - If Windows EBUSY recurs, investigate remaining non-Instance lock sources such as git fsmonitor, antivirus or indexing, runner filesystem timing, or other process handles. Do not treat a larger retry budget as the primary fix. PR: #513 Follow-up: PR #511 post-merge Windows advisory failure
Summary
Adds a small helper in
packages/opencode/test/session/export.test.tsto delete the synthetic session project directory with retry support, then uses it in the two export tests that intentionally remove the original session directory before exporting.Why
The post-merge
windows-advisoryworkflow failed twice inunit-windows-opencode-sessionwith the same Windows-only cleanup error:The failure happened before the export assertion, at
export.test.ts:195, while the test was deleting a git-backed temp directory to simulate a missing session instruction directory. The original NSIS desktop failure from PR #509 is fixed; this is a separate advisory failure in the opencode session export test harness.Relevant runs:
EBUSYsignature: https://github.com/Astro-Han/pawwork/actions/runs/25602372221Related Issue
No dedicated issue. This follows up the post-merge
windows-advisoryfailure ondev.Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please check that the fix stays scoped to the failing Windows cleanup path and does not weaken the export assertions themselves. The export behavior remains unchanged.
Risk Notes
Low. Test-only change. The only behavior change is adding retry budget to deletion of the intentionally removed git-backed temp session project directory. Windows runner verification still requires the advisory job or equivalent Windows unit job because this local machine is macOS.
How To Verify
Screenshots or Recordings
Not applicable. No visible UI change.
Checklist
dev, and my PR title and commit messages use Conventional Commits in English