fix(core,sidecar): route sidecar events by VM ownership, not execution id alone - #1926
Open
WyvernMonarch wants to merge 1 commit into
Open
fix(core,sidecar): route sidecar events by VM ownership, not execution id alone#1926WyvernMonarch wants to merge 1 commit into
WyvernMonarch wants to merge 1 commit into
Conversation
…n id alone
odw-0a6. Every AgentOs built from one sidecar handle listens on the SAME
shared native process, so `_handleSidecarEvent` sees every tenant VM's
events. It dispatched purely on `executionId`, and the mappers
`mapExecutionOutputEvent` / `mapExecutionCompletedEvent` drop `ownership`
before the handler ever sees it — so a colliding id delivered VM B's stdout
and exit into VM A's handlers and language-process bookkeeping.
Collisions were not hypothetical: ids were minted as
`operation-{now_ms:x}-{counter:x}` from a PER-VM counter checked only
against that VM's own map, so K VMs fanning out all minted
`operation-{ms}-1` in the same millisecond.
Two fixes, both at the root rather than per call site:
- One ownership guard at the top of `_handleSidecarEvent`, before the
mappers. Every id-keyed lookup downstream — including the in-flight
`_onExecutionOutput("*")` / `_onExecutionCompleted("*")` admission
subscriptions — is then VM-local by construction, so no per-caller
filtering is needed. Session- and connection-scoped events are not
VM-specific and still pass through.
- Mint from a process-wide `AtomicU64` instead of the per-VM counter, which
closes the collision at the source. `VmState::next_public_execution_id`
had no other reader and is deleted.
Also unblocks the `--lib` test build: `vm.rs` referenced
`crate::execution::javascript::rpc::error_code` through the private
`javascript` module, so `cargo test --lib` failed to compile on HEAD.
Routed through the existing `pub(crate)` re-export.
Member
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
2 participants
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.
The bug
AgentOs._handleSidecarEventdispatchesexecution_output/execution_completedpurely onexecutionId, over a shared sidecar client. Execution ids are minted per VM asoperation-{now_ms:x}-{per-VM counter:x}, and the uniqueness loop only checks that VM's own map — there is no process-wide registry.So two VMs whose Nth execution is admitted in the same millisecond mint the identical id. That is not an exotic race: the common fan-out (create K VMs together, each runs execution #1) puts every VM at the same counter ordinal simultaneously.
Impact when it fires: stdout/stderr chunks and completion (exit code, outcome) from VM B are delivered to VM A's handlers and process objects — a cross-VM data leak, not just a mixed-up log line.
The fix
Two layers, both small:
Ownership guard at the single shared entry point.
_handleSidecarEventnow drops any vm-scoped frame whoseevent.ownership.vm_idis not this instance's VM, beforemapExecutionOutputEvent/mapExecutionCompletedEventdiscardownership. Because the guard sits in the one shared entry point, every id-keyed lookup downstream is VM-local by construction — including the in-flight"*"admission subscriptions, which needed no separate patch. Session- and connection-scoped events are not VM-specific and still pass through.This mirrors the pattern already used in the same file for the VM-readiness wait, which filters
ownership.scope === "vm" && ownership.vm_id === nativeVm.vmId.Harden the mint at the source.
language_execution.rsnow formats the public id from a process-wideAtomicU64instead of the per-VM counter, so ids are unique across VMs by construction.VmState::next_public_execution_idhad no other reader and is removed.Why it is safe
cargo build -p agentos-native-sidecaris clean, and the newpackages/core/tests/cross-vm-execution-event-isolation.test.tscovers the dispatch filter (2 tests, passing).Found while running many concurrent VMs from one host process; happy to adjust the test's shape if you would rather drive it from a decoded
EventFramefixture than the instance-level probe.