Wasmtime GitHub notifications bot (Nov 21 2025 at 22:33): Wasmtime GitHub notifications bot (Nov 21 2025 at 22:33):fitzgen opened PR #12070 from fitzgen:oom-test to bytecodealliance:main:
Baby steps towards https://github.com/bytecodealliance/wasmtime/issues/12069
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:33):fitzgen requested alexcrichton for a review on PR #12070.
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:33):fitzgen requested wasmtime-fuzz-reviewers for a review on PR #12070.
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:33):fitzgen requested wasmtime-default-reviewers for a review on PR #12070.
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:47):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:47):alexcrichton created PR review comment:
Given how low-level allocation is and how high-level printing/backtraces are, I think it'd probably be best to remove this. I'd imagine that any error-related to OOM would already be aborting and/or something in a context where we could get a stack trace, so logging/printing a new one here seems ok to defer to that style of debugging. Basically I'd be worried that the
OutsideOomTestis not sufficient to block any possible malloc anywhere to be able to print/backtrace on that call.Could also wait to see if it's a problem too, and it may not end up being a problem.
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:47):alexcrichton created PR review comment:
This feels onerous to me to require which is to say that after one allocation fails all other allocations must be avoided entirely. I could imagine it being pretty reasonable that allocations are still attempted on some paths which are then also handled if they fail.
Would it make sense to hard-return null while
DidOomis true to "permanently OOM"? Or maybe only force one OOM but let other allocations keep going?
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:47):alexcrichton created PR review comment:
This feels perhaps a bit over-ambitious to me where we might have various edge cases where allocation failure can be handled. For example we might have a test saying "invoke a function twice" which invokes it a second time even if the first time failed, ensuring that an OOM on one call doesn't prevent some other call from happening (and reading corrupted state/etc). In such a situation the test would still return success, despite handling the OOM.
More generally given that the main goal here is "didn't abort on OOM" I'd think it'd be reasonable to transform this to "well the test returned to that means success, yay!"
Wasmtime GitHub notifications bot (Nov 21 2025 at 22:47):alexcrichton created PR review comment:
This'll likely want to be in a
Dropto avoid heavily crashing the process by accident in the case thattest_funcpanics
Wasmtime GitHub notifications bot (Nov 21 2025 at 23:15):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Nov 21 2025 at 23:15):bjorn3 created PR review comment:
Currently oom failure aborts without backtrace, just a panic message containing the amount of bytes it attempted to allocate.
Wasmtime GitHub notifications bot (Nov 21 2025 at 23:35):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 21 2025 at 23:35):alexcrichton created PR review comment:
Oh sure yeah, but that's a pretty easy event to break on in a debugger to get a backtrace
Wasmtime GitHub notifications bot (Nov 22 2025 at 02:15):github-actions[bot] commented on PR #12070:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Nov 24 2025 at 17:50):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Nov 24 2025 at 17:50):fitzgen created PR review comment:
Could also wait to see if it's a problem too, and it may not end up being a problem.
I'd prefer to do this, just because, while it is always possible to attach a debugger to get the stack trace, it is an extra step and I'd prefer to just have the log immediately if possible. Of course, if it does become a problem, we can remove it.
Wasmtime GitHub notifications bot (Nov 24 2025 at 17:56):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Nov 24 2025 at 17:56):fitzgen created PR review comment:
Agreed that this is pretty strict, but I think we should at least aim high and see if it is really too onerous before backing down. Especially as the effort begins, we are going to be testing smaller pieces of code where it probably is very reasonable to match this strict behavior. As we start testing larger and larger amounts of code at the same time, then we can loosen things up a bit, and when we do that, I'd like to do it via adding config knobs to
OomTestrather than loosening the requirements for all existing tests.But yeah I'd imagined that when we are in the "non-strict" mode, we would treat
DidOomasAlwaysOomFromNowOn, if we wanted to allow allocation to continue. If we want to allow subsequent allocations to potentially succeed instead, then I think we would want to enforce an available bytes limit, and let deallocations raise the available bytes again. Which is an interesting idea. Perhaps worth investigating in the future (but not in this PR obviously).
Wasmtime GitHub notifications bot (Nov 24 2025 at 18:42):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Nov 24 2025 at 18:42):fitzgen created PR review comment:
This feels perhaps a bit over-ambitious to me where we might have various edge cases where allocation failure can be handled.
Again, I'd prefer to start ambitious, and have that be the default, and then add knobs to loosen things up and be less strict that tests opt into.
For example we might have a test saying "invoke a function twice" which invokes it a second time even if the first time failed, ensuring that an OOM on one call doesn't prevent some other call from happening (and reading corrupted state/etc). In such a situation the test would still return success, despite handling the OOM.
Right and this kind of test is one where we would definitely twist that knob.
But, at least how I am imagining things now, most (all?) of these "retry" code paths will be in embedder code, not in the core Wasmtime APIs themselves. For example, instance allocation will just return an error on OOM, not try again N times or attempt to free anything before trying again; all of that logic really belongs in the embedder, IMO. So probably most of our tests can actually work in the stricter mode (but we should also test retrying to exercise things for when the embedder will do it).
Wasmtime GitHub notifications bot (Nov 24 2025 at 18:44):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Nov 24 2025 at 18:44):fitzgen created PR review comment:
That's a great point, will do in a minute.
Wasmtime GitHub notifications bot (Nov 24 2025 at 18:58):fitzgen updated PR #12070.
Wasmtime GitHub notifications bot (Nov 24 2025 at 18:58):fitzgen has enabled auto merge for PR #12070.
Wasmtime GitHub notifications bot (Nov 24 2025 at 19:48):fitzgen merged PR #12070.
Last updated: Dec 06 2025 at 07:03 UTC