Skip to content

fix(host-call): enforce parked execution timeout - #2329

Open
chaliy wants to merge 1 commit into
mainfrom
2026-08-21-propose-fix-for-parked-host-call-timeout-issue
Open

fix(host-call): enforce parked execution timeout#2329
chaliy wants to merge 1 commit into
mainfrom
2026-08-21-propose-fix-for-parked-host-call-timeout-issue

Conversation

@chaliy

@chaliy chaliy commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • Parked host-call executions stored an unpolled future, so the execution timeout could not fire while the host held a request, allowing a script to retain interpreter state indefinitely.
  • The change ensures wall-clock deadlines and resource limits continue to protect executions while they are parked waiting for host responses.

Description

  • Drive the execution future on an independent driver (Tokio native / spawn_local on wasm) so the timeout can elapse even when the host is not polling next_event(); implement a completion channel to report results or timed-out drop.
  • Reject late resumes and discard timed-out sessions by sending completion metadata (timed-out -> no recoverable Bash) and returning a clear error from resume() when the request is no longer active.
  • Add an AbortHandle to abort the driver when the handle is dropped, and introduce spawn_execution helpers plus a small ExecutionHandle state refactor.
  • Update an integration test to assert autonomous timeout behavior, update docs/threat-model (TM-DOS-098), and add the wasm helper dependency; primary changes in crates/bashkit/src/host_call.rs, crates/bashkit/tests/integration/host_call_execution_tests.rs, crates/bashkit/Cargo.toml, and docs/knowledge files.

Testing

  • Ran the integration suite for host-call behavior: cargo test -p bashkit --test integration host_call_execution_tests -- --nocapture and the focused timeout test suspended_host_call_remains_inside_the_execution_timeout, both passed (all host-call lifecycle tests OK).
  • Ran static checks and CI-style validations: cargo fmt --check, cargo clippy -p bashkit --all-targets -- -D warnings, just check-okf, and just check-doc-links, all succeeded.
  • Verified wasm compatibility with rustup target add wasm32-unknown-unknown and cargo check -p bashkit --target wasm32-unknown-unknown, and validated the parallel bench build invocation cargo bench -p bashkit --bench parallel_execution --no-run; these checks succeeded.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 21, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit aa47985 Commit Preview URL Aug 21 2026, 11:02 PM

chaliy commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed in depth and rebased onto main. Not merging this one — as written it breaks the non-JS wasm embedding that landed in #2311, and the fix is a design decision rather than a mechanical one.

The blocker

spawn_execution needs a task spawner. The PR supplies two:

#[cfg(not(all(target_arch = "wasm32", target_os = "unknown")))]  // tokio::spawn
#[cfg(all(target_arch = "wasm32", target_os = "unknown"))]       // wasm_bindgen_futures::spawn_local

But wasm32-unknown-unknown is not synonymous with "JS host" in this repo — that is the whole premise of knowledge/runtimes/non-js-wasm.md and of the wasm_js feature, whose comment in crates/bashkit/Cargo.toml reads:

These three are JS-backed (wasm-bindgen imports), so they are optional and pulled in by wasm_js. A non-JS wasm embedder gets time_compat::host_clock instead and links none of them: their imports would be unresolvable in a runtime with no JS engine.

wasm-bindgen-futures is precisely such a crate, and the PR adds it unconditionally:

wasm-bindgen-futures = "0.4"   # not optional, not behind wasm_js

The PR is also based on a pre-wasm_js main — it still carries gloo-timers = "0.3" without optional = true, so this conflicts directly on rebase.

Evidence

I rebased, resolved the conflict the charitable way (made wasm-bindgen-futures optional and added dep:wasm-bindgen-futures to the wasm_js feature), and ran the same build the hyperlight CI job runs:

$ cd examples/hyperlight
$ RUSTFLAGS='--cfg getrandom_backend="custom"' cargo build --release --target wasm32-unknown-unknown

error[E0433]: cannot find module or crate `wasm_bindgen_futures`
    |     ^^^^^^^^^^^^^^^^^^^^ use of unresolved module or unlinked crate `wasm_bindgen_futures`
error: could not compile `bashkit` (lib) due to 1 previous error

Same command on main:

    Finished `release` profile [optimized] target(s) in 1m 19s

So this is a real regression, not a rebase artifact. Left un-gated as the PR has it, it is worse: the crate links and the guest imports JS symbols that a no-JS runtime cannot resolve — which the build script's own import check exists to catch.

Why this needs a decision, not a patch

On a no-JS wasm runtime there is no executor to spawn onto. That is not a gating oversight; it is fundamental to the approach. You cannot make an execution future progress while the host is parked if nothing is driving it. So one of these has to be chosen:

  1. Per-target behaviour. Keep the spawned driver where a spawner exists (native, JS wasm) and fall back to today's caller-polled future on non-JS wasm. Costs two ExecutionHandle code paths in security-sensitive async code, and makes "timeout armed while parked" a target-dependent property that the threat model has to state explicitly.
  2. Drop host-call support on non-JS wasm, compiling start_execution out there.
  3. Leave it as-is on main. Worth weighing: today the deadline is enforced, just on the next host poll. The unbounded-retention case needs a host that holds the handle and never polls again — a host bug, bounded by one session, and already released by dropping the handle. TM-DOS-098 is currently marked MITIGATED on that basis.

There is also a second consequence worth calling out independently of wasm: tokio::spawn requires a tokio runtime, so start_execution would begin panicking outside one. Today the handle is runtime-agnostic — the caller polls it. For a library that advertises an async-first API, that is a meaningful narrowing.

I have the rebase (conflicts resolved, wasm-bindgen-futures correctly gated) ready on my side and can push it behind whichever option you pick. I did not push it, since every version of it still fails the hyperlight CI job until the design question is settled.

The rest of the PR is good — the Abortable/oneshot restructuring is clean, and the tightened test asserting resume() fails with "no longer active" and into_bash() errors after timeout is a genuine improvement.


Generated by Claude Code

chaliy commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Re-checked this against current main (now at 4b634fd, 56 commits ahead of this PR's base). The standing review's conclusion holds, and I found one merge-safety hazard worth adding.

The green checkmarks on this PR are misleading

The head commit has only 7 check runs: CodeQL (×5 languages), Analyze (actions), and Workers Builds: bashkit. The CI workflow never ran on this PR — none of its jobs appear on the head commit.

That matters because the job which would catch this regression is in CI:

# Guards the `wasm_js`-off configuration, which no other job covers: a
# stray JS-backed dependency only shows up as an unresolvable import here.
name: WASM component (no JS, no WASI)

So the PR reads as "all checks passed" while the one job designed to catch exactly this class of failure never executed. Please don't take the green state as evidence of mergeability here.

The blocker, re-confirmed structurally

No rebuild needed to confirm it — the configuration on current main is decisive:

  • examples/hyperlight/Cargo.toml builds the crate as bashkit = { path = ..., default-features = false, features = ["jq"] }no wasm_js.
  • On main, every JS-backed dep is gated: web-time, gloo-timers (now 0.4), and send_wrapper are all optional = true and pulled in only by wasm_js.
  • This PR adds wasm-bindgen-futures = "0.4" unconditionally under the wasm32-unknown-unknown target block, and spawn_execution references it on that whole target — not just the JS-backed configuration.

wasm32-unknown-unknown is not synonymous with "JS host" in this repo; that separation is the entire premise of knowledge/runtimes/non-js-wasm.md. The PR also still carries the pre-wasm_js gloo-timers = "0.3" (non-optional), so it conflicts on rebase regardless.

Still a design decision, not a patch

Gating the dependency correctly is mechanical, but it does not resolve the substance: on a no-JS wasm runtime there is no executor to spawn onto, so a spawned driver cannot exist there at all. The three options from the previous comment are unchanged, and I did not pick one unilaterally — each changes a documented contract:

  1. Per-target behaviour — makes "timeout armed while parked" a target-dependent security property that the threat model must state explicitly.
  2. Drop host-call support on non-JS wasm — compiles start_execution out there.
  3. Leave as-is on main — the deadline is already enforced on the next host poll; TM-DOS-098 is MITIGATED on that basis.

There is also the non-wasm consequence: tokio::spawn requires an active tokio runtime, so start_execution would begin panicking outside one. Today ExecutionHandle is runtime-agnostic because the caller polls it — that is a meaningful narrowing of an async-first public API.

I did not rebase-and-push, because every version of this still fails the hyperlight job until the design question is settled, and AGENTS.md is unambiguous that nothing merges with CI red. Happy to implement whichever option you choose.

Unrelated but checked in the same pass: advisories are clean — cargo audit reports 0 vulnerabilities across all four lockfiles (the two unmaintained warnings are already suppressed with rationale in deny.toml), and npm audit on site/ reports 0.


Generated by Claude Code

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant