Repository navigation
fix(runtime): publish an isolate's Caches under the registry lock - #512
Closed
adrian-niculescu wants to merge 1 commit into
Closed
adrian-niculescu wants to merge 1 commit into
adrian-niculescu wants to merge 1 commit into
Conversation
Runtime::NotifyMemoryPressure runs on a global queue and, holding the isolate registry lock, reads the Caches slot of every registered isolate to get its gate id. CreateIsolate registers the isolate, but Runtime::Init stored the slot afterwards without the lock. A memory pressure event that arrives while a main or worker isolate is starting therefore reads the slot on one thread while the starting thread writes it. A reader that finds the slot empty gets the stand-in cache, whose gate id matches no gate, and skips the isolate, so the practical outcome is benign. What is left is an unsynchronized read of a pointer while it is published. Init now stores the slot under the registry lock. Teardown already removes the isolate from the registry under that lock before Caches::Remove clears the slot.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: publishing an isolate's Caches under the registry lock to fix the runtime synchronization issue. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checked the cache slot with care
While isolatesMutex_ guarded it there Runtime::Init began Caches::Init ran
And the lock kept both readers aware
Comment @coderabbitai help to get the list of available commands.
adrian-niculescu
marked this pull request as ready for review
October 13, 2026 20:00
edusperoni
added a commit
that referenced
this pull request
Oct 13, 2026
Setting up a runtime took CreateIsolate, Init and, for workers, SetWorkerId, assembled by hand at each call site. The isolate joined the live registry in CreateIsolate, before Init wrote its Caches slot, so a thread that found it there (the memory-pressure handler, #512) could read that slot while it was being written. CreateIsolate could also be called twice on one Runtime, and a Runtime deleted before Init left its isolate registered and never disposed. - Runtime(const RuntimeOptions&) creates the isolate, homed on the calling thread, and takes the limits and worker id up front, so IsWorker() is correct during a worker's Init. The caller still locks the isolate and can set it up (WatchHeapLimit) before calling Init(). - Init() publishes the isolate to the registry only after writing its Caches slot. With the teardown already unregistering before clearing it, a thread that finds an isolate in the registry may read its slots under the lock. - A Runtime deleted before Init disposes its unpublished isolate. - CreateIsolate and SetWorkerId are gone. Reported in #512.
Contributor
Author
|
Closed in favor of #513, which fixes the same race differently: the isolate is added to the registry only after its |
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.
If the system reports memory pressure while the app or a worker is still starting, the pressure handler and the starting thread touch the same isolate slot without synchronization. I have not seen this cause a failure and I would rate it low severity: it is a data race on a pointer, found while reading the code added in #507.
Runtime::NotifyMemoryPressureruns on a global queue and readsCaches::Get(isolate)for every registered isolate while holding the registry lock.CreateIsolateregisters the isolate, butRuntime::Initstored theCachesslot later, outside that lock. In that window the handler normally finds the slot empty, gets the stand-in cache whose gate id matches no gate, and skips the isolate, which is harmless. The read is still unsynchronized with the write that publishes the pointer.Initnow stores the slot under the registry lock. Teardown already follows the same rule: it removes the isolate from the registry under the lock beforeCaches::Removeclears the slot.To see the window, I put a sleep before
Caches::Initand simulated memory warnings on the simulator during launch: the handler ran for the registered isolate and read the slot before it was set. No test added, since the overlap depends on timing. The TestRunner suite passes.Summary by CodeRabbit