Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: closing the queued-message journal database before fixture cleanup. Description check The description is complete and focused. It explains the Windows EBUSY failure, the teardown order change, the reason for using the existing close helper, testing performed, and the lack of UI impact.… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and focused. It explains the Windows EBUSY failure, the teardown order change, the reason for using the existing close helper, testing performed, and the lack of UI impact. It also records that no linked issue exists, which is acceptable if the author is a maintainer.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one file, test-only: the shared queued-message test fixture now closes its journal database before deleting its temporary directory.
- Close the journal handle before
rm—dispose()callscloseTestJournalHostDatabase(root)betweenhost.flushAllStreamedEvents()andrm(root, { recursive, force }), so the Windows EBUSY on the SQLite DB/WAL no longer blocks teardown.
The fix is correct and complete for its scope. The record store opened by openTestAgentSessionRecordStore(root) is rows in the same journal host database, so closing by root covers both handles; closeTestJournalHostDatabase is idempotent, so the double-dispose() cases stay safe; and a close failure still propagates before deletion, as the description states. crashRestartHostProcess re-opens through the same tracked root, so a single close in teardown is right.
ℹ️ The same open-handle teardown remains in the sibling REST rig
The structurally identical fixture structured-agent-session-rest-test-rig.ts deletes its temp root without closing the journal database it opened, so the same Windows EBUSY class likely persists there. Out of scope for this PR (which explicitly does not claim a full-suite result) — noting it only so the class isn't assumed fully retired.
Technical details
# Same open-DB-before-rm shape in the REST test rig
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-agent-session-rest-test-rig.ts:199` — `dispose()` flushes streamed events then `rm(root)`, but never calls `closeTestJournalHostDatabase(root)`, though `hostFor` opened that root's DB at line 173.
## Required outcome
- The REST rig's teardown closes `root`'s journal host database before removing the directory, matching the queued rig.
## Open questions for the human
- Is chasing the full Windows teardown class intended, or is this PR deliberately limited to the queued fixture (sibling tracked separately)?deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…anup The queued-message test rig deleted its temp state directory while the shared journal SQLite handle was still open, so teardown failed with EBUSY on Windows. Close the rig root's journal database after the host drains, then remove the directory.
b99d4ac to
f7220d5
Compare
nwparker
changed the title
ELI5
A test helper was deleting its scratch folder while a database file inside it was still open. Windows won't delete a file that's still open, so the tests failed during cleanup. Now the helper closes the database first and then deletes the folder.
What Changed
createQueuedMessageTestRig().dispose()flushed the host's streamed events and then ranrm(root, { recursive: true, force: true }). The journal SQLite database opened for that root byopenTestJournalHostDatabasewas still open. On Windows thatrmfails withEBUSYon the database or its WAL file.dispose()calls the existingcloseTestJournalHostDatabase(root)after the host drains and before therm. That closes the handle registered for this rig's root and removes it from the shared test registry. Errors from flushing or closing still surface before anything is deleted.The change is one test fixture file:
structured-agent-session-queued-message-rig.test-fixture.ts. No production code is touched.Why
The fixture opens the database through the shared test-support registry, so it should also close it through that registry. That helper already exists, so this adds no new cleanup path. The other options were retrying
rmonEBUSYor relying on a global after-all close. Both would hide the problem rather than fix the order of teardown.Linked Issue
None. This is test-only cleanup and no issue is filed for it.
Visual Proof
N/A. This is a test fixture teardown fix with no UI or user-facing behavior change.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
macOS: ran all 41 test files that use
createQueuedMessageTestRig. All 382 tests passed.Windows (contributor's run, Node 24.21.0): the baseline case reproduced
EBUSYduring teardown. With the fix, that case and all 33 tests in the file pass.No new test was added. The existing suites that use this fixture are the regression coverage, and they fail on Windows without the fix.
Static gates on the touched file pass: anti-slop oxlint, code-quality native plugins, ast-grep, changed-code quality, and oxfmt.
Review
Agent skill upstream boundary
Notes
Test-only change. It improves Windows test reliability and doesn't affect remote SSH, mobile, or backwards compatibility.
Original fix by @toodeceptive in #25478. Their authorship is kept on the landing commit.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)