feat(workers): report a failed worker creation separately from a failed module - #744
Open
johnstonmatt wants to merge 1 commit into
Open
johnstonmatt wants to merge 1 commit into
johnstonmatt wants to merge 1 commit into
Conversation
…ed module InvalidWorkerCreation covered three unrelated outcomes: the pool dropped the sender, the worker did not come up before the pool gave up, and the module threw while being evaluated. Only the last is the caller's own code failing, and an embedder had no way to tell them apart. The pool now sends a typed WorkerCreationTimeout instead of a message built from a string, so the two cases where no worker was produced can be matched on and reported as WorkerUnavailable. InvalidWorkerCreation keeps its name and now means only that evaluating the module failed. The request path lists the new variant explicitly rather than widening its match to a wildcard, so a future variant still has to be decided there. Two integration tests asserted the old creation-timeout message and now expect the new name.
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.
Stacked on #743. Same shape, for worker creation rather than cancellation.
InvalidWorkerCreationcovers three unrelated outcomes and only the last is the caller's own code failing:ext/workers/lib.rs:305WorkerUnavailableext/workers/lib.rs:314viapool.rs:379WorkerUnavailableext/workers/lib.rs:314viaworker_inner.rs:250InvalidWorkerCreationThe first two mean no worker was produced, so the module was never evaluated and nothing about it could have caused the failure. They share a name because they classify identically; the message still distinguishes them.
The pool now sends a typed
WorkerCreationTimeoutrather thananyhow!("worker did not respond in time"), so the second case is matchable instead of only being distinguishable by its text.Notes for review
InvalidWorkerCreationnarrows in meaning. It previously covered all three; it now means only that evaluating the module failed. Anything keying on it for the platform cases will stop seeing them.WorkerCreationTimeoutexplicitly and maps it toInvalidWorkerResponse, rather than widening to a wildcard. It cannot occur there, but listing it keeps the compiler forcing a decision when the next variant is added — which is how this change got caught in the first place.InvalidWorkerCreation: worker did not respond in timeand now expectWorkerUnavailable. Theworker boot errorassertions are deliberately untouched, since that case keeps its name.Verification
cargo checkclean forext_workers. Thebasecrate does not build in my environment (its build script links openblas, not installed), sopool.rsand the integration test updates need CI.Consumers need updating first
InvalidWorkerCreationnarrows here rather than staying put, so this is a breaking change for anything that maps it. Two of the three cases it used to cover now arrive asWorkerUnavailable, which an existing embedder will not recognise, and those requests will fall through to its unhandled-error path.So the ordering is: teach the caller
WorkerUnavailable, deploy that, then merge this.For Supabase's own ingress that is supabase/edge-functions-ingress#487, which should be deployed before this lands.