Skip to content

Fix current-thread leak on concurrent lower error paths#13918

Closed
SebTardif wants to merge 1 commit into
bytecodealliance:mainfrom
SebTardif:fix/restore-thread-on-lower-error
Closed

Fix current-thread leak on concurrent lower error paths#13918
SebTardif wants to merge 1 commit into
bytecodealliance:mainfrom
SebTardif:fix/restore-thread-on-lower-error

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

Summary

Temporary set_thread switches used during concurrent component lowering (so guest realloc / callbacks see the correct thread) were not restored on error paths. An early ? / bail! after set_thread left the store's current thread pointing at the temporary thread, so later concurrent operations could run with the wrong thread context.

This adds with_store_thread / restore_thread_after that always restore the previous thread (preferring the original work error if restore also fails), and converts every temporary save/restore pair in this class.

Why this is needed

#12736 correctly established that stream/future lowering can call guest realloc and therefore must set the current thread first. That PR also added restore on the success path. Error paths that return before the matching restore still leave the wrong thread installed.

Concrete example in futures_and_streams::lower when T::MAY_REQUIRE_REALLOC is true:

  1. set_thread(caller_thread) installs the caller.
  2. Alignment / bounds checks or linear_store_list_to_memory can fail with ?.
  3. The matching set_thread(old_thread) never runs.

The same pattern existed for guest↔guest future/stream copy, event delivery callbacks, stackless/stackful guest calls, async return adapters, host-task result lowering, and explicit thread start.

Fix

  • StoreOpaque::restore_thread_after always restores after a work Result.
  • Free function with_store_thread for the common scoped form.
  • AsStoreOpaque for StoreContextMut so the helper works at those call sites.
  • Converted all temporary set_thread pairs found by grepping crates/wasmtime/src/runtime/component.

Permanent thread transitions (enter/exit sync call, host task create, fiber resume bookkeeping) are unchanged.

Testing

Unit tests in with_store_thread_tests:

  • Restores the previous current thread on success.
  • Restores the previous current thread when work returns an error (red-green: skipping restore fails the assertion with got None after installing a host marker thread).
  • Prefer work errors over restore errors.
cargo test -p wasmtime --features component-model-async --lib with_store_thread
# 3 passed

Also ran other concurrent unit tests in the crate (cargo test -p wasmtime --features component-model-async --lib concurrent).

Related

  • Introduced with incomplete restore coverage in #12736 (merged 2026-03-11). Review discussion there already asked about restoring the previous thread after the temporary switch.
  • Related current-thread API work: #13887, #13759.

When set_thread temporarily switches the store's current thread for
guest realloc (or similar), early returns via ? left the wrong thread
active. Introduce with_store_thread / restore_thread_after so the
previous thread is always restored, preferring the original work error
if restore also fails. Apply the helper at every temporary set_thread
pair in concurrent lowering, copy, callback delivery, stackful/stackless
calls, async return, host lower, and thread start.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested a review from a team as a code owner July 21, 2026 02:07
@SebTardif
SebTardif requested review from pchickey and removed request for a team July 21, 2026 02:07
@github-actions github-actions Bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jul 21, 2026
@pchickey
pchickey requested review from alexcrichton and removed request for pchickey July 21, 2026 17:28
@alexcrichton

Copy link
Copy Markdown
Member

I'd advise against blindly following LLM advice when it comes to the deep internals of the runtime, especially an async runtime. It's intentional that error paths right now do not have the same degree of cleanup as successful paths because errors indicate traps after which a component is entirely locked down and unable to be used again.

Are you able to write tests that misbehave using just wasm? Tests for the internal state aren't too interesting since they're not guaranteed to be exposed by the public API.

@alexcrichton

Copy link
Copy Markdown
Member

er, sorry, didn't mean to close

@alexcrichton alexcrichton reopened this Jul 21, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@alexcrichton

It's intentional that error paths right now do not have the same degree of cleanup as successful paths because errors indicate traps after which a component is entirely locked down and unable to be used again.

Thanks for the context. That tracks: these host-side errors (alignment/bounds in lower, callback failures) propagate via ? through poll_until, so the event loop exits before anything reads the stale thread state.

Are you able to write tests that misbehave using just wasm?

No. The event loop terminates on error before anything could observe the stale state.

Closing this; the cleanup asymmetry is consistent with the design.

@SebTardif SebTardif closed this Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:api Related to the API of the `wasmtime` crate itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants