v0.3: fix OCR continuation crash + rework indexer progress UX - #3
Merged
Merged
Conversation
Two ship-blockers for v0.3:
1. EXC_BREAKPOINT crash in OCRRecognizer.recognize(cgImage:)
Both the VNRecognizeTextRequest completion handler and the
VNImageRequestHandler.perform() catch block can signal a result
to the same checked continuation. Under rare Vision timing — seen
reliably at ~2.7k files into a scan of a ~4.7k-file folder — both
fire, double-resuming the continuation and crashing the process
with "CheckedContinuation.resume(throwing:) called twice".
Routed both paths through a tiny ContinuationGate wrapper: NSLock-
protected one-shot flag, forwards the first resume and drops
subsequent ones. Can't actually log the second resume from inside
the callback without re-introducing the same race, so we eat it.
2. "Looks like it's starting from scratch on each relaunch"
initialScan was reporting progress as "Indexing N / TOTAL" across
ALL discovered files — including the tens of thousands that just
needed a mtime+size fingerprint check (microseconds each). Mixing
fast fingerprint skips with slow OCR passes made the counter look
like a fresh re-index even though the DB was persistent.
Split initialScan into four clearly-labelled phases:
1. enumerate — walk the disk once, collect image candidates
2. reconcile — drop rows for files no longer on disk
3. fingerprint — filter to genuinely new/changed files using the
(mtime, size) fingerprint already stored per row
4. OCR — Vision pass on that shorter list
Progress enum reflects the phases:
.idle
.enumerating(folders: Int) — brief walk, no usable progress
.indexing(done: Int, total: Int) — OCR queue, total == new files
only (total=0 = fully resumed)
.watching(indexed: Int) — steady state
Menu bar status strings match:
Scanning folder…
OCR'ing 0 / 2003 new images — only new work
Up to date — when total is 0 after resume
4,762 screenshots indexed
Net effect: second launch after a full scan shows "Up to date"
almost instantly rather than rolling a progress counter through
the entire library.
Tests: 35 still green — the fingerprint check in ScreenshotStore
was already tested; the Progress enum change has no test coverage
(it's UI state), and the OCR gate's threadsafety is in the same
category as the thread-safety guarantees the Vision callbacks
already carry.
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: GitButler <gitbutler@gitbutler.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a rare OCR crash caused by double-resuming a CheckedContinuation in the Vision pipeline, and refactors Indexer progress reporting so relaunches with an already-populated DB don’t misleadingly count through the entire library.
Changes:
- Add a thread-safe one-shot
ContinuationGateto prevent double-resume crashes inOCRRecognizer.recognize(cgImage:). - Split initial scan work into clearer phases and update
Indexer.Progressto distinguish enumeration vs OCR indexing vs steady-state watching. - Update menu bar and app state UI mapping to reflect the new progress model.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Sources/VistaCore/OCRRecognizer.swift | Prevents rare Vision timing from resuming continuations twice by gating resumes. |
| Sources/VistaCore/Indexer.swift | Reworks initial scan phases and progress reporting so “total” reflects only new OCR work. |
| Sources/Vista/MenuBarContentView.swift | Updates status line messaging for new progress states (enumerating/indexing/watching). |
| Sources/Vista/AppState.swift | Adjusts derived indexed-count logic to align with new progress cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+27
| /// DB populated typically shows total=0 → we skip straight to | ||
| /// `.watching`. |
There was a problem hiding this comment.
The Progress case docs say that when the DB is populated you’ll have total=0 and “skip straight to .watching”, but initialScan() still yields .indexing(done: 0, total: 0) before emitting .watching. Either adjust the emitted progress (skip .indexing when total == 0) or update this comment so it matches the actual state transitions.
Suggested change
| /// DB populated typically shows total=0 → we skip straight to | |
| /// `.watching`. | |
| /// DB populated may briefly emit `.indexing(done: 0, total: 0)` | |
| /// before transitioning to `.watching`. |
Comment on lines
+213
to
+215
| let attrs = try? fm.attributesOfItem(atPath: url.path) | ||
| let mtime = (attrs?[.modificationDate] as? Date) ?? Date() | ||
| let size = (attrs?[.size] as? NSNumber)?.int64Value ?? 0 |
There was a problem hiding this comment.
In Phase 3 the fingerprint filter uses try? fm.attributesOfItem(...) and then defaults mtime to Date() when attributes can’t be read. That masks attribute-read failures and can incorrectly force files into the OCR queue (and then fail again in indexFile). Consider either (a) using the URL resource values you already requested during enumeration (mtime/size) so you don’t need a second filesystem stat, or (b) handling the attribute lookup failure explicitly (log + skip) instead of defaulting to Date().
Suggested change
| let attrs = try? fm.attributesOfItem(atPath: url.path) | |
| let mtime = (attrs?[.modificationDate] as? Date) ?? Date() | |
| let size = (attrs?[.size] as? NSNumber)?.int64Value ?? 0 | |
| let attrs: [FileAttributeKey: Any] | |
| do { | |
| attrs = try fm.attributesOfItem(atPath: url.path) | |
| } catch { | |
| VistaLog.log("Skipping fingerprint check for \(url.path): failed to read file attributes (\(error))") | |
| continue | |
| } | |
| let mtime = (attrs[.modificationDate] as? Date) ?? .distantPast | |
| let size = (attrs[.size] as? NSNumber)?.int64Value ?? 0 |
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
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
OCRRecognizer.recognize(cgImage:)was double-resuming itsCheckedContinuationunder rare Vision timing — both theVNRecognizeTextRequestcompletion handler ANDhandler.perform()'s catch block could signal a result, tripping the runtime assertion. Reproduced at ~2.7k files into a 4.7k-file scan. Wrapped both paths in aContinuationGate(NSLock-protected one-shot) so the second resume is dropped.Indexing N / 4762even when the DB was already populated, because fast fingerprint-checks and slow OCR passes shared the same counter. SplitinitialScaninto four named phases (enumerate, reconcile, fingerprint, OCR) andIndexer.Progressinto.enumerating / .indexing / .watching, withtotalon.indexingnow meaning "new work only". A resumed launch with a fully-populated DB showsUp to datealmost instantly instead of rolling a counter through the whole library.Test plan
swift test --parallel— 35 tests greenOCR'ing N / total new imagescounts upUp to dateimmediately (no counter through the library)