GodlyDonuts opened PR #13708 from GodlyDonuts:async-cancel-waitable-set-trap to bytecodealliance:main:
Fixes #13690.
[component-model#647] tightened the trap rules for synchronous stream/future
operations: a synchronousread/writemust trap if the waitable it operates
on has already been added to a waitable set. That PR extended the same rule to
{stream,future}.cancel-{read,write}andsubtask.cancel, but Wasmtime only
implemented it forread/write. As a result the upstream
trap-if-sync-and-waitable-set.wasttest hits anunreachableon the cancel
cases today instead of trapping.Why it was missing
The check lives in
wait_for_event, the single point every blocking
synchronous read/write funnels through before suspending:if waitable.common(state)?.set.is_some() { bail!(Trap::WaitableSyncAndAsync); }The cancel paths don't reliably reach it. When there is nothing pending to wait
on,cancel_read/cancel_writetake an early return with aCancelledcode,
andsubtask.cancelcan resolve a starting/finished subtask without ever
suspending. In those cases the waitable-set check was never run, so the cancel
succeeded where the spec requires a trap.Fix
Pull the check into a small helper on
Waitable:fn trap_if_in_waitable_set(&self, state: &mut ConcurrentState) -> Result<()> { if self.common(state)?.set.is_some() { bail!(Trap::WaitableSyncAndAsync); } Ok(()) }
wait_for_eventnow calls it (no behavior change), and the three synchronous
cancel entry points —cancel_read,cancel_write, andsubtask_cancel—
call it up front, guarded by!async_to match the neighbouring
check_blockingguards. The async variants are deliberately untouched: being a
member of a waitable set is the whole point of an asynchronous cancel.Testing
Added
tests/misc_testsuite/component-model/async/cancel-sync-and-waitable.wast
covering all five operations (future.cancel-{read,write},
stream.cancel-{read,write}, andsubtask.cancel). Each starts a pending
operation, joins the waitable to a fresh set, and asserts the synchronous
cancel traps; this mirrors the upstreamtrap-if-sync-and-waitable-set.wast. I
verified each case regresses (returns a cancel code / completes normally rather
than trapping) when its individual guard is removed.
cargo test --test wast --features component-model-asyncand the cancellation
integration tests intests/all/component_model/async.rsboth pass.[component-model#647]: https://github.com/WebAssembly/component-model/pull/647
GodlyDonuts requested cfallin for a review on PR #13708.
GodlyDonuts requested wasmtime-core-reviewers for a review on PR #13708.
cfallin unassigned cfallin from PR #13708 Trap on sync stream/future cancel and subtask.cancel when in a waitable set.
cfallin requested dicej for a review on PR #13708.
cfallin commented on PR #13708:
I'm going to redirect review to someone who knows much more about the CM async machinery (hope that's ok @dicej!).
GodlyDonuts edited PR #13708:
Fixes #13690.
[component-model#647] tightened the trap rules for synchronous stream/future
operations: a synchronousread/writemust trap if the waitable it operates
on has already been added to a waitable set. That PR extended the same rule to
{stream,future}.cancel-{read,write}andsubtask.cancel, but Wasmtime only
implemented it forread/write. As a result the upstream
trap-if-sync-and-waitable-set.wasttest hits anunreachableon the cancel
cases today instead of trapping.Why it was missing
The check lives in
wait_for_event, the single point every blocking
synchronous read/write funnels through before suspending:if waitable.common(state)?.set.is_some() { bail!(Trap::WaitableSyncAndAsync); }The cancel paths don't reliably reach it. When there is nothing pending to wait
on,cancel_read/cancel_writetake an early return with aCancelledcode,
andsubtask.cancelcan resolve a starting/finished subtask without ever
suspending. In those cases the waitable-set check was never run, so the cancel
succeeded where the spec requires a trap.Fix
Pull the check into a small helper on
Waitable:fn trap_if_in_waitable_set(&self, state: &mut ConcurrentState) -> Result<()> { if self.common(state)?.set.is_some() { bail!(Trap::WaitableSyncAndAsync); } Ok(()) }
wait_for_eventnow calls it (no behavior change), and the three synchronous
cancel entry points —cancel_read,cancel_write, andsubtask_cancel—
call it up front, guarded by!async_to match the neighbouring
check_blockingguards. The async variants are deliberately untouched: being a
member of a waitable set is the whole point of an asynchronous cancel.Testing
Added
tests/misc_testsuite/component-model/async/cancel-sync-and-waitable.wast
covering all five operations (future.cancel-{read,write},
stream.cancel-{read,write}, andsubtask.cancel). Each starts a pending
operation, joins the waitable to a fresh set, and asserts the synchronous
cancel traps; this mirrors the upstreamtrap-if-sync-and-waitable-set.wast. I
verified each case regresses (returns a cancel code / completes normally rather
than trapping) when its individual guard is removed.Depends on bytecodealliance/wit-bindgen#1638
Enforcing this trap surfaces a latent bug in wit-bindgen's async runtime:
cancel_inter_task_stream_readissues a synchronousstream.cancel-readon its
inter-task wakeup stream while that stream is still in the task's waitable set
(it callsremove_waitableonly afterwards). That is exactly the pattern this
trap now (correctly) rejects, so everywasi:http@0.3.0(p3) program built with
the currentwit-bindgentraps during ordinary operation — which is what the
red p3 CI jobs here are.bytecodealliance/wit-bindgen#1638 reorders that to leave the set before
cancelling, matching the unregister-then-cancel ordering the general
WaitableOperation::cancelpath already uses. With that change applied locally
(via[patch.crates-io]), the full p3 suites pass again:
wasmtime-wasi-httpp3: 25 passedwasmtime-wasip3: 27 passedcomponent-model/asyncwast (incl. the new test): 192 passedSo this PR needs a
wit-bindgenrelease containing #1638 and a corresponding
version bump before its p3 jobs go green. Happy to fold the bump in here once
that's available, or sequence it however you prefer.[component-model#647]: https://github.com/WebAssembly/component-model/pull/647
:thumbs_up: dicej submitted PR review:
LGTM, thanks!
CI is failing, and it seems to point to
wit_bindgen::rt's internal machinery for handling inter-task communication. I suspect a change will be needed in that project in order to get those tests passing. I'm on vacation at the moment (and should not have opened my computer at all :sweat_smile: ), but I can take a closer look when I get back if nobody beats me to it.
GodlyDonuts commented on PR #13708:
Thanks @dicej! That matches what I found — the trap fires in
wit-bindgen'scancel_inter_task_stream_read, which issues a synchronousstream.cancel-readon the inter-task wakeup stream while it's still in the task's waitable set (it callsremove_waitableonly afterwards).I opened bytecodealliance/wit-bindgen#1638 to fix that: it reorders the two so the stream leaves the waitable set before the synchronous cancel, matching the unregister-then-cancel ordering the general
WaitableOperation::cancelpath already uses. With that patched in locally ([patch.crates-io]), the full p3 suites pass again —wasmtime-wasi-httpp3 (25),wasmtime-wasip3 (27), andcomponent-model/asyncwast incl. the new test (192).So this just needs a
wit-bindgenrelease containing #1638 and the corresponding bump here. Happy to fold the bump into this PR once it's available — no rush, enjoy your vacation. :slight_smile:
GodlyDonuts edited a comment on PR #13708:
Thanks @dicej! That matches what I found, the trap fires in
wit-bindgen'scancel_inter_task_stream_read, which issues a synchronousstream.cancel-readon the inter-task wakeup stream while it's still in the task's waitable set (it callsremove_waitableonly afterwards).I opened bytecodealliance/wit-bindgen#1638 to fix that: it reorders the two so the stream leaves the waitable set before the synchronous cancel, matching the unregister-then-cancel ordering the general
WaitableOperation::cancelpath already uses. With that patched in locally ([patch.crates-io]), the full p3 suites pass again,wasmtime-wasi-httpp3 (25),wasmtime-wasip3 (27), andcomponent-model/asyncwast incl. the new test (192).So this just needs a
wit-bindgenrelease containing #1638 and the corresponding bump here. Happy to fold the bump into this PR once it's available, no rush, enjoy your vacation. :slight_smile:
github-actions[bot] added the label wasmtime:api on PR #13708.
GodlyDonuts requested dicej for a review on PR #13708.
GodlyDonuts commented on PR #13708:
wit-bindgen#1638 is merged, thanks @alexcrichton. Only thing left here is a wit-bindgen release that carries it plus a matching version bump in this PR. I'll put the bump up as soon as there's something on crates.io to point at. Verified locally that the p3 suites go green with the fix in, so CI should be clean once it's bumped.
alexcrichton commented on PR #13708:
It's taken a bit of time (apologies and thanks for the patience @GodlyDonuts), but I've got a bump for wit-bindgen over at https://github.com/bytecodealliance/wasmtime/pull/13844. Once that's merged could you rebase this and then I'd be happy to r+ and merge
GodlyDonuts commented on PR #13708:
sounds good to me
:thumbs_up: dicej submitted PR review:
LGTM, thanks!
CI is failing, and it seems to point to
wit_bindgen::rt's internal machinery for handling inter-task communication. I suspect a change will be needed in that project in order to get those tests passing. I'm on vacation at the moment (and should not have opened my computer at all :sweat_smile: ), but I can take a closer look when I get back if nobody beats me to it.
GodlyDonuts updated PR #13708.
GodlyDonuts commented on PR #13708:
It's taken a bit of time (apologies and thanks for the patience @GodlyDonuts), but I've got a bump for wit-bindgen over at #13844. Once that's merged could you rebase this and then I'd be happy to r+ and merge
done
alexcrichton commented on PR #13708:
Thanks! I think the CI failure here is legitimate, and might require some small tweaks to the test?
I can take a look at that tomorrow if nobody beats me to it.
dicej updated PR #13708.
alexcrichton added PR #13708 Trap on sync stream/future cancel and subtask.cancel when in a waitable set to the merge queue.
:check: alexcrichton merged PR #13708.
alexcrichton removed PR #13708 Trap on sync stream/future cancel and subtask.cancel when in a waitable set from the merge queue.
Last updated: Jul 29 2026 at 05:03 UTC