fix(media): watch the background path refresh instead of firing it into the void - #255
Merged
Merged
Conversation
…to the void `findMediaLinksByFingerprint` is a read that writes: when the file has moved, it refreshes `lastKnownPath` so the next lookup is cheaper. Not awaiting that write is right — a lookup should not pay for it — but `void promise` is not fire-and-forget, it is fire-and-crash. Nothing was watching, so any failure surfaced as a process-level `unhandledRejection`. That is what made the CI Test job intermittent: vitest reports an unhandled rejection from OUTSIDE every test and fails the run, so the job went red under a green summary — 1628 passing tests and a stack pointing at a temp directory a finished suite had already removed (run 30935779600). In the packaged app the same rejection lands in the main process, where the recovery is worse than a missed refresh ever was. The refresh stays unawaited and now logs what it could not do, which is the same answer `readRegistry` already gives a registry it cannot read. While in there, `withWriteLock` drops its queue entry once the chain drains, guarded by an identity check so a writer that queued behind us is not stranded. The map is keyed by an arbitrary directory path and grew for the life of the process. This is the pattern `DocumentService.writeProject` already uses, so the two write queues now read the same. Two cases cover it. The one that fails on the old code makes the directory read-only, so the write fails deterministically while the read still works, and asserts that nothing escapes and the lookup still answers. The second reproduces the CI shape — remove the directory while the refresh is queued — and asserts only that no rejection escapes, since either side of that race is acceptable.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe registry now cleans drained write queues safely and catches asynchronous path-refresh failures. Tests cover failed writes, warning behavior, unhandled rejection prevention, and directory removal races. ChangesRegistry refresh handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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.
Summary
The CI
Testjob is intermittently red under a green summary — 137 files passed, 1628 tests passed, then the job exits non-zero. Run 30935779600 is the example this came from:The cause is not in the test.
findMediaLinksByFingerprintis a read that writes: when a file has moved, it refresheslastKnownPathso the next lookup can take a cheaper path. Not awaiting that write is the right call — a lookup should not pay for it — butvoid promiseis not fire-and-forget, it is fire-and-crash. Nothing was watching the promise, so any failure became a process-levelunhandledRejection.Vitest reports an unhandled rejection from outside every test and fails the run, which is why the job could be red with nothing failing. In the packaged app the same rejection lands in the main process, where the recovery is a good deal worse than the missed refresh it came from.
The refresh stays unawaited and now logs what it could not do — the same answer
readRegistryalready gives a registry it cannot read.While in there,
withWriteLockdrops its queue entry once the chain drains, guarded by an identity check so a writer that queued behind us is not stranded. The map is keyed by an arbitrary directory path and grew for the life of the process. This is the patternDocumentService.writeProjectalready uses, so the two write queues now read the same.Related issue
Refs #217 — found while reviewing that PR's CI, but unrelated to it and fixed separately here.
Type of change
Release impact
Desktop impact
Testing
Two cases, and only the first is a detector — that is stated in the test itself rather than left for a reader to discover.
logs a refresh it cannot write, and still answers the lookupmakes the directory read-only, so the read still works and only the write fails, deterministically. On the old code it fails withexpected [ …(1) ] to deeply equal []— one unhandled rejection captured. Skipped out loud when running as root, where the permission bit does not bite.survives the directory disappearing while the refresh is queuedreproduces the CI shape — remove the directory while the refresh is in the queue — and asserts only that no rejection escapes, since either side of that race is a legitimate outcome.Verified:
npm testthree consecutive full runs: 137 files, 1627 passed, no unhandled rejection.electron/media/mediaLinksRegistry.test.tsfive consecutive runs: 12 passed each time..catch()turns the first case red and leaves the other eleven green — the test fails for the reason it claims to.npx tsc --noEmit,npx tsc -p tsconfig.test.json --noEmit, and biome all clean.🤖 Generated with Claude Code
Summary by CodeRabbit