Skip to content

fix(native-sidecar): keep queued child services progressing beneath WASM parents - #2013

Open
ankssjain wants to merge 4 commits into
rivet-dev:mainfrom
ankssjain:fix/python-shell-vfs-requests
Open

ankssjain wants to merge 4 commits into
rivet-dev:mainfrom
ankssjain:fix/python-shell-vfs-requests

Conversation

@ankssjain

@ankssjain ankssjain commented Sep 29, 2026 •

Copy link
Copy Markdown

Fixes #2012.

process.exec('python -c "print(42)"') starts Python but times out while Python initializes its filesystem bridge. The WASM parent's poller leaves internal requests for the supervisor, but the supervisor skips every child of a WASM parent. Direct Python execution avoids this path.

There is also a shared progress-accounting problem: claiming an owned JavaScript or Python service returns Value::Null, which the attached and detached descendant pumps treat as no progress. With multiple queued requests and one coalesced notification, a request can remain stranded without a continuation wake-up.

This change:

  • Uses the existing internal-event classification to admit queued requests and completions beneath WASM parents, including JavaScript and Python. preserve_pull_owned_events keeps stdout, stderr, and exit delivery with the parent.
  • Reports owned-service claims explicitly from the common descendant poller, independently of whether it returns a parent-facing event.
  • Counts those claims as progress and against the VM/child work budgets in both attached and detached pumps. The loops drain ready services within their limits and retain a continuation notification when yielding.
  • Tests a single coalesced producer notification, ordered exactly-once claims, service capacity, VM/child fairness limits, quiescence after draining, and preservation of shell-owned output. The JavaScript tests cover attached/detached children and ordinary/WASM parents; the Python tests cover filesystem requests and socket completions.

The supervisor always supplies Some(owned_javascript_services) to the descendant poller. A JavaScript request is therefore enqueued and counted before returning Value::Null; the compatibility path's inline JavaScript handler is bypassed. A mixed-queue regression places process.signal_state before a Python filesystem request and socket completion and verifies progress from one coalesced notification, both in one batch and across one-request turns. This checks the call-path distinction raised in the follow-up review.

Related: #2007 also changes admission of JavaScript process-control requests beneath WASM parents. This PR now covers that admission through the shared internal-event classification; its other dispatch refactoring is separate. The two PRs overlap at the WASM-parent guard.

Validation

  • Before the scheduling correction (commit 0a701f7), both generic JavaScript regressions fail after claiming 1 of 3 queued requests. The strengthened Python regressions fail on the missing continuation notification and on claiming only 1 of 2 queued services.
  • cargo test --locked -p agentos-native-sidecar --test service -- child_event_claim_tests wasm_parent_ --test-threads=1: all 24 tests pass. This includes the mixed JavaScript/Python regression and existing capacity, cancellation, output/exit ordering, deferred socket, and child-write deadline checks.
  • cargo build --locked -p agentos-sidecar --bin agentos-sidecar succeeds.
  • Eleven checks pass against the rebuilt sidecar with the published 0.2.22 SDK on Linux arm64 / Node 24: direct Python, shell Python, explicit Bash/python3, piped Python stdin, output ordering, separate stdout/stderr, shell → Node → Python, shell → Node → Bash → Python, an ordinary grep/sed pipeline, guest file setup, and a Python file edit verified through the filesystem API. These probes use the command-registration workaround documented in Python launched through guest shell stalls on queued filesystem RPCs #2012.
  • Rust formatting and git diff --check pass.

Full workspace and agent-session suites have not been run. Local builds use cached V8 bridge assets and omit the optional bundled TypeScript compiler. The command-registration workaround and mixed API/guest file consistency issue are separate from this event scheduling fix.

@ankssjain
ankssjain marked this pull request as ready for review September 29, 2026 21:24

@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 0a701f7.


🔴 High · Rearm the pump when another queued Python event remains

A claimed Python service returns Value::Null, so this path hits continue before setting emitted_this_round or incrementing work. The outer loop therefore stops after one request, and because the claim count is still below max_service_claims, neither this function nor pump_process_events_nowait restores a notification permit. If the WASM poller requeues two Python events before the supervisor consumes its coalesced Notify edge, the second request remains at the front indefinitely and the child can stay blocked. The new test masks this by calling the pump twice directly. Treat a claimed service as progress so the turn continues up to the service limit, or explicitly rearm when another eligible event remains; the regression test should consume one coalesced notification and verify the second request gets a continuation edge.

Original location: "crates/native-sidecar/src/execution/child_process.rs":2805 (new side, not submitted inline).

@ankssjain ankssjain changed the title fix(native-sidecar): service Python RPCs beneath WASM parents fix(native-sidecar): keep queued child services progressing beneath WASM parents Sep 30, 2026

@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 5716557.

Comment thread crates/native-sidecar/src/execution/child_process.rs Outdated

@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 medium-severity finding

Reviewed commit 6cb3b41.

Comment thread crates/native-sidecar/src/execution/child_process.rs Outdated
Count in-place JavaScript completions as bounded internal work, preserve pull-owned events in order, and retain upstream JavaScript RPC ownership. Add real completion-queue and ownership regressions; merge main at 6a6e18d.

@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 medium-severity finding

Reviewed commit b476af3.


🟠 Medium · Make the RPC ownership classifier the single source of truth

This classification now controls whether the supervisor or the WASM parent's pull loop owns an RPC, but poll_owned_descendant_javascript_child_process still repeats the same method-name list inline when deciding what to requeue. If either list changes alone, one side can skip a request that the other side also believes it does not own, leaving the synchronous RPC parked. Use this helper in the pull-loop branch as well (with the existing process.signal_state fast path kept explicit) so the ownership boundary cannot drift.

Original location: "crates/native-sidecar/src/execution/child_process.rs":2640 (new side, not submitted inline).

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.

Python launched through guest shell stalls on queued filesystem RPCs

1 participant