Repository navigation
test(native-chat): cancel the journal import test's tick before teardown - #24071
Conversation
The test re-arms a setImmediate tick while the copy runs and stopped it with a flag. A tick already queued when the import resolved still ran, and when the afterEach teardown closed the database first it threw journal_closed as an unhandled error, failing a CI shard with every test green. Clear the queued tick instead.
 |
![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
- Cancel the queued journal-import tick instead of flagging it — the test now holds the
setImmediatehandle inpendingand callsclearImmediate(pending)afterimportPerSessionJournalresolves, so no tick can read the database after teardown closes it. Replaces thecopying = falseflag, which could not stop an already-queued callback.
The mechanism is sound: tick re-arms into the same pending variable, so exactly one handle is ever live, and the await continuation runs after the current macrotask completes — clearImmediate therefore reliably cancels the last queued tick. between.length > 0 still relies on the import yielding at least one macrotask, but that reliance predates this change. Verified with a focused run: journal-per-session-import.test.ts 22/22 pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe commit-synchronization test now schedules recurring Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to A rejected import can still leave a test tick running into teardown, as before this change. The PR fixes cleanup after successful imports and introduces no new merge risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 6b5e60c8-8e06-4360-b840-475a4f9d7e8c
📒 Files selected for processing (1)
src/main/native-chat/agent-session-journal/journal-per-session-import.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| }) | ||
| copying = false | ||
| // A flag would still let the queued tick read the database after teardown closes it. | ||
| clearImmediate(pending) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cancel the tick when the import rejects.
If importPerSessionJournal rejects, execution skips clearImmediate(pending). The queued tick then reschedules itself and continues reading the database. Put the cancellation in a finally block so a failed import cannot leave the loop running during teardown.
Proposed fix
- await importPerSessionJournal({
- database,
- identity: IDENTITY,
- legacyDirectory: legacyDir(),
- batchRows: 1
- })
- // A flag would still let the queued tick read the database after teardown closes it.
- clearImmediate(pending)
+ try {
+ await importPerSessionJournal({
+ database,
+ identity: IDENTITY,
+ legacyDirectory: legacyDir(),
+ batchRows: 1
+ })
+ } finally {
+ clearImmediate(pending)
+ }
1 participant
Fixes: a flaky unit test that can fail a CI shard even when every test passes. Scope: one test in
src/main/native-chat/agent-session-journal/journal-per-session-import.test.ts. No app code changes and no user-facing change. The test came in with #23613.ELI5
A test kept poking the database in a loop while a copy ran, and told the loop to stop with a "please stop" flag. One more poke was already queued when the flag went up. If the test's cleanup closed the database before that last poke ran, the poke hit a closed database and crashed. Now the test cancels the queued poke instead of asking it to stop.
What Changed
The test "commits copy batches unsynced, and every other commit synced" re-arms a
setImmediatetick while the import copies, recording the database's sync level between batches. It used to stop with acopying = falseflag, but a tick queued before the flag flipped still ran and read the database. When theafterEachteardown closed the database first, that read threwjournal_closedas an unhandled error. Vitest counts that as a failure, so shard 2/5 of PR #23806's CI failed with all 19,428 tests green (run 36649661726, attempt 1).Now the test keeps the handle of the queued tick and calls
clearImmediateon it as soon as the import resolves, so nothing touches the database after the test's own assertions.Why
Cancelling the queued callback removes the race; a flag can only stop the tick after it has already run once more. Other options were worse: waiting a turn before the assertions would still leave the ordering to the event loop, and closing the database later in teardown would change shared test support for one test.
Linked Issue
None (maintainer fix for a CI flake).
Visual Proof
N/A: test-only change with no UI or behavior change.
Testing
journal_closed, unhandled); the fixed test passes with the same forced close. The probe was not committed.journal-per-session-import.test.ts: 22/22 pass. Main-process typecheck, oxlint, oxfmt and the changed-code quality gate pass.Notes
Test-only; no effect on security, platforms, SSH, mobile, compatibility or performance.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)