Skip to content

Fix panic on pollable drop - #706

Draft
WhiteAbeLincoln wants to merge 5 commits into
mainfrom
abe/issue-696-pollable-drop-panic
Draft

WhiteAbeLincoln wants to merge 5 commits into
mainfrom
abe/issue-696-pollable-drop-panic

Conversation

@WhiteAbeLincoln

@WhiteAbeLincoln WhiteAbeLincoln commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Under the component ABI, if cache::Entry::step handed out a pollable
and the pollable was later dropped before the entry instance was,
subsequent use of the entry resulted in a panic because the async item
had been dropped from the sandbox.

The compute service doesn't have this issue because it checks the kind
of associated AsyncItem in the sandbox when dropping a pollable. Some
kinds are owned by the pollable handle, and some are owned by another
resource, with the pollable acting as a weak handle.

Porting this fix directly from the compute service would result in
breaking changes in viceroy-lib's public API (though I don't know if
this API is intended to be public or it's accidental). Additionally,
adding more items to the AsyncItem enum would model types which aren't
yet supported by Viceroy. Instead, a boolean flag is added to the
PendingCacheTask struct to indicate whether this item is shared (and
so can't be dropped through a pollable handle) or owned solely by the
pollable.

Closes #696

@WhiteAbeLincoln
WhiteAbeLincoln force-pushed the abe/issue-696-pollable-drop-panic branch 4 times, most recently from 90edfc0 to fef4537 Compare September 15, 2026 20:41
@WhiteAbeLincoln
WhiteAbeLincoln marked this pull request as ready for review September 15, 2026 21:01
@WhiteAbeLincoln

WhiteAbeLincoln commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor Author

@harmony7 @cceckman-at-fastly I'd appreciate a review when you get time - no rush. I suggest you review commit by commit though since there's a bulk format of the test fixtures in there which will add a bunch of noise.

This PR adds support for wasm32-wasip2 test fixtures (since afaik the issue doesn't occur under an adapted wasip1 component). That's loosely based off of wasmtime's test-programs crate with the addition of a manifest to control the architecture (and maybe other build flags in the future? One of the tests was failing when built under opt = 1). This is more complex than just listing the fixture separately in the Makefile, but I preferred it since it ensures the test fixtures get automatically built when they've changed and you re-run a test, and it means a cargo build -p test-fixtures-artifacts works directly. I can simplify or split into a separate PR if this isn't acceptable.

The fix itself is in d6ef4ed. This is similar to the equivalent code in Compute & Charles's PR 600, though I was trying to avoid unnecessary breaking changes and so didn't add new variants to the AsyncItem enum.

@WhiteAbeLincoln
WhiteAbeLincoln force-pushed the abe/issue-696-pollable-drop-panic branch from fef4537 to 25718c8 Compare September 16, 2026 14:22
Test fixtures are now part of the main workspace instead of a separate
crate, which means that CI fails because they're not formatted
correctly. This is a one-off bulk reformat, CI will enforce future
changes are correct.
Under the component ABI, if cache::Entry::step handed out a pollable
and the pollable was later dropped before the entry instance was,
subsequent use of the entry resulted in a panic because the async item
had been dropped from the sandbox.

The compute service doesn't have this issue because it checks the kind
of associated AsyncItem in the sandbox when dropping a pollable. Some
kinds are owned by the pollable handle, and some are owned by another
resource, with the pollable acting as a weak handle.

Porting this fix directly from the compute service would result in
breaking changes in viceroy-lib's public API (though I don't know if
this API is intended to be public or it's accidental). Additionally,
adding more items to the `AsyncItem` enum would model types which aren't
yet supported by Viceroy. Instead, a boolean flag is added to the
`PendingCacheTask` struct to indicate whether this item is shared (and
so can't be dropped through a pollable handle) or owned solely by the
pollable.
@WhiteAbeLincoln
WhiteAbeLincoln force-pushed the abe/issue-696-pollable-drop-panic branch from 25718c8 to 30bbf08 Compare September 17, 2026 18:21
@WhiteAbeLincoln
WhiteAbeLincoln marked this pull request as draft September 17, 2026 20:46
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.

cache.entry.step returns a pollable that aliases the entry, so a guest which drops the pollable destroys the entry

1 participant