Fix a crash caused by improper management of async workers#21
Merged
savetheclocktower merged 2 commits intomasterfrom Feb 16, 2026
Merged
Fix a crash caused by improper management of async workers#21savetheclocktower merged 2 commits intomasterfrom
savetheclocktower merged 2 commits intomasterfrom
Conversation
The theory of the crash, produced during a rubber-ducking session with Claude: * `find_words_with_subsequence_in_range` is called * a job is scheduled and placed in `outstanding_workers` * a call to `set_text_in_range` cancels a pending worker via `cancel_queued_workers` and `worker->Cancel` * the worker is never removed from `outstanding_workers` because that happens in `Execute` — and we cancelled before the job got that far * later, `set_text_in_range` triggers another call to `cancel_queued_workers` * the already-cancelled job from before is still present in the set * we try to call `Cancel` on it again, but the memory has been freed The fix is to take the code that removes a job from `outstanding_workers` and move it from `Execute` to `OnWorkComplete`. The latter is guaranteed to be called, no matter how a job finished. Because this is a theory produced by a man and his hallucinating robot, it's important to back it up with proof. The test added in the previous commit should now pass instead of crashing. So this fix works, even if there are slight flaws in the reasoning above.
confused-Techie
approved these changes
Feb 16, 2026
Member
confused-Techie
left a comment
There was a problem hiding this comment.
While not versed in C, the changes here are extremely minimal, simply moving existing coding in-line with what you've described.
Plus seeing tests added for exactly this, and that they are passing in CI makes me think we are 100% in the clear for this being a resolution.
Just prior to merging make sure to bump the version key in package.json so we can easily make a release from this, and with that change happy to approve and get this merged!
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.
This is the cause of at least one of the crashes reported in pulsar#1438, but hopefully all three.
The theory of the crash, produced during a rubber-ducking session with Claude:
TextBuffer::findWordsWithSubsequenceis called (this is used byautocomplete-plus’s built-in subsequence provider) and delegates tofind_words_with_subsequence_in_rangein the C++ codeoutstanding_workersTextBuffer::setTextInRangecallsset_text_in_rangein the C++; this can invalidate whatever datafind_words_with_subsequence_in_rangewas collecting, so it cancels the worker viacancel_queued_workersandworker->Cancel()outstanding_workersbecause that happens in the worker’sExecutemethod — and we could've cancelled before that method was calledset_text_in_rangetriggers another call tocancel_queued_workersCancelon it again, but the memory has been freed, and everything falls apartThe fix is to take the code that removes a job from
outstanding_workersand move it fromExecutetoOnWorkComplete. The latter is guaranteed to be called, no matter how a job finished. Also,Executeruns in its own thread, butOnWorkCompleteruns on the main thread, where alloutstanding_workersaccess should happen in the first place.Because this is a theory produced by a man and his hallucinating robot, it's important to back it up with proof. The first commit in this PR adds a failing test; the second commit makes the test pass. So if you're reviewing this PR:
1f1caff, runnpm run build:node, thennpm run test:nodeand ensure you see a failing test with a crashdumpb89da52, repeat these steps, and see a green test suite