Skip to content

test(core): cover VM disposal after guest fetch - #2014

Open
RitikaxG wants to merge 1 commit into
rivet-dev:mainfrom
RitikaxG:codex/fix-vm-dispose-timer-worker
Open

RitikaxG wants to merge 1 commit into
rivet-dev:mainfrom
RitikaxG:codex/fix-vm-dispose-timer-worker

Conversation

@RitikaxG

@RitikaxG RitikaxG commented Sep 30, 2026 •

Copy link
Copy Markdown
  • Add a fresh-sidecar regression for a guest fetch() whose response body is fully consumed, then require VM disposal before the 5-second shutdown deadline.
  • Run the regression in the required Core PR test lane. It failed on the pre-fix base at 5,047 ms with one VM timer task retained and passes on current main after upstream commit a6c668c made timer-wheel initialization process-owned.

Related: #1989

This covers the completed-response disposal path. The unread-response evaluation delay needs separate investigation.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 1 high-severity finding

Reviewed commit f26c94d.

Comment thread crates/native-sidecar/src/state.rs Outdated
let mut javascript = JavascriptExecutionEngine::new(process_runtime);
javascript.set_event_notify(Some(Arc::clone(&event_notify)));
let mut python = PythonExecutionEngine::new(runtime.clone());
let mut python = PythonExecutionEngine::new(vm_runtime.clone());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 High · Python can still bind the global timer wheel to one VM

PythonExecutionEngine::new(vm_runtime.clone()) constructs its embedded JavascriptExecutionEngine with this VM-scoped context. Python executions are ultimately started through that engine, whose LocalBridgeState passes the engine context to TimerWheel::get; if Python is the first runtime to schedule a JS/bridge timer, the process-global wheel is therefore still spawned under this VM's admission scope and dies when the VM is disposed, leaving later VMs with the same dead singleton. WasmExecutionEngine::new(vm_runtime) on the next line has the same path. Give both wrapper engines the process runtime for their embedded JavaScript host while retaining the VM runtime for guest execution, and cover first-use through Python/Wasm before disposing the VM.

@RitikaxG
RitikaxG force-pushed the codex/fix-vm-dispose-timer-worker branch from f26c94d to e5aa7f3 Compare October 1, 2026 07:02

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues found

Reviewed commit e5aa7f3.

@RitikaxG RitikaxG changed the title fix(execution): keep JavaScript timer worker process-scoped test(core): cover VM disposal after guest fetch Oct 1, 2026
@WellDunDun

Copy link
Copy Markdown

Follow-up from a credential-free comparison using the official v0.2.22 Linux x86_64 native sidecar and matching @rivet-dev/agentos-core@0.2.22, with no upstream edits.

The release binary checksum matched 64ebdd6cbfc5acb0ca2857c5f95646c8b8b24e7778284ab527986402410be62f. Each control used a fresh sidecar and VM, the supported AGENTOS_SIDECAR_BIN override, one CPU / 2 GiB, and a 40-second parent deadline. The published option schema uses hostFunction: 'deny'; no host functions or provider credentials were supplied.

  • Offline disposal passed in 369 ms.
  • Fully consumed public HTTP response evaluation succeeded, but disposal took 5,268 ms and emitted ERR_AGENTOS_VM_TEARDOWN_DEADLINE with one active task and zero outstanding capabilities. Child exit was zero, but shutdown was not clean.
  • Unread public response evaluation did not finish within 40 seconds; the owned child process group was stopped.

As of October 8, npm latest and the latest GitHub native release still identify 0.2.22. We understand this PR reports the consumed-response regression passing on main after a6c668c, with the unread-response path separate.

Is there a compatible published native build containing the timer ownership fix for the 0.2.22 interface, or is migration to the newer process runtime API required? What supported path should a Rivet actor integration use to verify both cleanup cases without carrying another custom upstream patch?

No credentials, actor identifiers, transcripts or user data are included here. Hosted inference was not used for this comparison.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants