Repository navigation
fix(macros): reject instead of panicking when a queue or email handler returns Err - #1115
Open
PedroRosalba wants to merge 1 commit into
Open
PedroRosalba wants to merge 1 commit into
PedroRosalba wants to merge 1 commit into
Conversation
…r returns Err Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The queue and email glue in
worker-macros/src/event.rsturn a handlerErrinto a panic:For a queue consumer, returning
Erris the ordinary way to say "retry this batch". On the default build (panic=abort) the panic is a Wasm trap, though. It breaks every call in flight in the same isolate, andshim.jsthen reinitializes the instance on the next entry, so static state is lost.Local repro (wrangler 4.151.2, std target,
worker-build --release): a fetch awaits a 5 sDelayand then reads astatic AtomicU64, and while it waits a batch is delivered to the consumer.Err(..)RuntimeError: null function, or "Cannot invoke closure from previous WASM instance" if it resumes after the reset)batch.retry_all(); Ok(())Ok(())Fix
Return the error as a rejected promise, as the
connectglue in the same file already does:The runtime still retries the whole batch, as the docs describe ("If the queue() function throws, or the promise returned by it ... rejected, then the entire batch will be considered a failure and will be retried"), and it still counts as a failed invocation. The difference is that the instance survives. The email handler gets the same change.
Tests
Adds a queue test: the consumer returns
Errthe first time it sees afail-once:message and remembers that in a static. The test checks that the batch is retried and succeeds on the second delivery (which only happens if the static survived), and that a message received before theErris still in the existingGLOBAL_QUEUE_STATE.Before the fix the new test fails (and the run logs the instance crashes); after it, the full suite passes.
cargo fmtandclippyare clean. Email has no test harness here, so that change is covered by review only.Not tested: the
--panic-unwindbuild, where the panic should already be caught (worker/src/init.rs:39-42).Related: #166 (panics killing later requests; a 2023 comment there hit it from a queue consumer), #1090 (errors under panic=unwind).
Tested on macOS (Apple Silicon) only.