Repository navigation
Conversation
…released engine RuntimeBackgroundIndexingManager held its engine unowned since c96f229 broke their retain cycle, on the premise that the engine, which owns the manager, always outlives it. It does not: a batch's driving task keeps the manager alive, so an engine released mid-batch left the next runSingleIndex reading it, aborting the process in swift_abortRetainUnowned. The app's engine swap is safe only because the coordinator cancels every batch before it lets go of the old engine; nothing guards the other ways an engine can go away. The engine is now weak. expandDependencyGraph holds a strong reference for the whole walk; runSingleIndex takes one after its cancellation check and treats a missing engine as cancellation; runBatch stops dispatching once the engine is gone, so the batch ends with .batchCancelled and its remaining images cancelled. An image being loaded holds the engine for the call and finishes normally. batchWhoseEngineGoesAwayEndsCancelled releases the engine right after starting a 21-image batch; it aborted the test process before the fix.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Engine loss can still produce .batchFinished in empty-graph and last-dispatched-item paths instead of cancelling the batch.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Replaces an unsafe unowned indexing-engine reference with weak ownership to prevent crashes when engines are released mid-batch.
Changes:
- Adds weak-engine cancellation handling.
- Adds a regression test for engine release during indexing.
- Documents the incident and design decision.
| File | Description |
|---|---|
RuntimeBackgroundIndexingManagerTests.swift |
Adds engine-release regression coverage. |
RuntimeBackgroundIndexingManager.swift |
Uses weak engine ownership and cancellation checks. |
2026-10-01-background-indexing-engine-released-mid-batch.md |
Documents the defect and fix. |
Documentations/README.md |
Links the resolved issue. |
0002-background-indexing.md |
Records the ownership decision. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func expandDependencyGraph(rootPath: String, depth: Int) | ||
| async -> [RuntimeIndexingTaskItem] { | ||
| // Held for the whole walk, so the engine cannot go away halfway. | ||
| guard let engine else { return [] } |
| /// this manager alive through its driving task, so the engine can go away | ||
| /// first, and the batch's next load then read a released object and | ||
| /// aborted the process. A batch that finds the engine gone ends cancelled. | ||
| private weak var engine: (any RuntimeBackgroundIndexingEngineRepresenting)? |
This branch has not been deployed
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.
An engine released while one of its background indexing batches is running aborts the process:
Fatal error: Attempted to read an unowned reference but object … was already destroyed, on the batch's next image load.Cause
RuntimeBackgroundIndexingManageris owned by its engine (RuntimeEngine.backgroundIndexingManager) and has held the engineunownedsince c96f229 broke their retain cycle, on the premise that the engine always outlives the manager. It does not:startBatch's driving task keeps the manager alive while the batch runs, so when the engine goes away first, the nextrunSingleIndexreads it.The app's engine swap does not hit this:
RuntimeBackgroundIndexingCoordinator.handleEngineSwapcancels every batch on the old manager from a task that holds the old engine, andrunSingleIndexchecks for cancellation before it reads the engine, serialized on the manager's actor. Nothing guards the other ways an engine can go away — the coordinator'sdeinitstops its event pumps but does not cancel batches.Fix
engineis nowweak(a strong reference would bring the retain cycle back):expandDependencyGraphholds a strong reference for the whole walk and returns no items when the engine is gone.runSingleIndextakes a strong reference after its cancellation check and records a missing engine as a cancelled item.runBatchstops dispatching once the engine is gone, so the batch ends with.batchCancelledand its remaining images cancelled.An image being loaded holds the engine for the duration of the call and finishes normally.
Verified
RuntimeBackgroundIndexingManagerTests.batchWhoseEngineGoesAwayEndsCancelledstarts a 21-image batch on a mock engine and releases the engine right afterstartBatchreturns. Before the fix the test process aborted with the error above; after it the batch ends with.batchCancelled.main's pins: 435 of 436 pass. The one failure,ConnectionTransportRegressionTests' DirectTCP trailing-message case, passes in each of three runs of that suite on its own; it is a load-dependent timeout of the IPC tests under the full parallel run, unrelated to this change.Write-up:
Documentations/ResolvedIssues/2026-10-01-background-indexing-engine-released-mid-batch.md, plus a decision-log row in proposal 0002, whose design text still described theunownedreference.The same pattern in the Find navigator's corpus store, which exists only on
feature/find-navigator, was fixed there the same day. This PR and #117 both add a line at the top of the resolved-issues list inDocumentations/README.md, so whichever merges second has a one-line conflict to resolve.