Repository navigation
[Bug] Snapshot worker pool: no false 'worker died', real lost-chunk count (stacked on #513) - #520
Merged
mcop1 merged 1 commit intoSep 23, 2026
Conversation
… the real number of lost chunks pump() read a worker's output before checking whether it still ran. A worker that wrote its last answer and exited in between was taken for one that died with chunks pending, failing a healthy import. The running state is now checked first; everything written before the exit is read and handled afterwards. The "worker died" message read the pending count after abort() had cleared it and always said "0 chunk(s)"; the count is captured first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approved
The focused implementation correctly addresses both reported defects with appropriate regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes worker-pool shutdown handling and accurately reports chunks lost when a snapshot replay worker dies.
Changes:
- Processes buffered worker output before declaring an exited worker dead.
- Captures the pending chunk count before abort cleanup.
- Adds regression tests for both scenarios.
| File | Description |
|---|---|
src/Service/SearchIndex/Snapshot/WorkerPoolBulkDispatcher.php |
Corrects worker exit handling and lost-chunk reporting. |
tests/Unit/Service/SearchIndex/Snapshot/WorkerPoolBulkDispatcherTest.php |
Adds regression coverage for worker exit races and lost chunks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mcop1
merged commit Sep 23, 2026
7fc20e0
into
feature/index-snapshot-export-import
11 of 12 checks passed
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Changes in this pull request
Stacked on #513 (base branch
feature/index-snapshot-export-import); the worker pool only exists there. Merge this into #513's branch before #513 itself is squash-merged.WorkerPoolBulkDispatcher::pump()read a worker's output before checking whether it still ran. A worker that wrote its lastOKline and exited in between was taken for one that died with chunks pending, which failed a healthy import. The running state is now checked first; everything written before the exit is read and handled afterwards.abort()had cleared it and always said0 chunk(s). The count is captured before the abort.Additional info
Tests:
testAWorkerThatDiesReportsHowManyChunksWereLostfails without the fix (reported "0 chunk(s)").testAWorkerThatExitsRightAfterItsLastAnswerIsNotReportedAsDeadis a guard: the race window is microseconds, so it cannot be forced deterministically; the fix is by ordering. Locally: snapshot unit suite 68 OK, snapshot functional suite 37 OK, PHPStan and php-cs-fixer clean.Both findings come from Copilot's suppressed note on #513 and a code reading during the CI-hang investigation (the hang itself was the test helper, see #519).
🤖 Generated with Claude Code