ESD-1700: Harden the WASM fetch smoke check against transient network failures - #550
Open
Phil-Browne wants to merge 3 commits into
Open
ESD-1700: Harden the WASM fetch smoke check against transient network failures#550Phil-Browne wants to merge 3 commits into
Phil-Browne wants to merge 3 commits into
Conversation
… failures The Browser Fetch Smoke job makes one live API call with no retry, and when that call fails it reports the wrong reason. - scripts/wasm-smoke.mjs: inspect the CLI JSON error envelope before the array-shape assertion, and retry the whole attempt once after 5s. - internal/wasm/wasmhttp: append a rejected fetch cause to the error message, so Node bare "fetch failed" carries ETIMEDOUT / ENOTFOUND / ECONNREFUSED.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
==========================================
- Coverage 79.48% 79.48% -0.01%
==========================================
Files 193 193
Lines 18859 18859
==========================================
- Hits 14991 14990 -1
Misses 2820 2820
- Partials 1048 1049 +1 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the WASM “fetch transport” smoke check and improves WASM fetch error reporting so transient network failures are retried and, when they do fail, surface actionable root-cause details (e.g., ETIMEDOUT, ENOTFOUND) instead of misleading JSON-shape assertions.
Changes:
- Update
wasm-smoke.mjsto detect the CLI JSON error envelope before asserting array shape, and to retry once after a delay with explicit logging. - Enhance WASM fetch transport error strings to append rejection
causedetails when present. - Add WASM transport tests covering multiple
causeshapes to ensure the detailed error message is preserved.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/wasm-smoke.mjs | Adds error-envelope inspection and a single retry to reduce CI flakes and improve failure diagnosis. |
| internal/wasm/wasmhttp/transport.go | Appends fetch rejection cause details to transport errors for better troubleshooting. |
| internal/wasm/wasmhttp/transport_test.go | Adds table-driven tests to validate surfaced cause details for rejected fetches. |
Reading a property off the rejection value panics when it is not an object, and a panic on a js.FuncOf goroutine takes the whole WASM process down rather than just the one request. - Gate both fetch error handlers on Type() == js.TypeObject instead of ruling out undefined and null one at a time. - Cover null, undefined, string and number rejections with a regression test. - Rework the cause cases to the shapes real Node 22 fetch failures produce, including the dual-stack AggregateError whose message is empty and whose code carries the reason.
…uard Round 2 review found the "prefers the cause message" case used an ETIMEDOUT shape Node never produces, under a comment claiming the shapes were reproduced. Node 22 raises undici's own ConnectTimeoutError there (UND_ERR_CONNECT_TIMEOUT), so use that. Also tighten textCatch's message check to match the sibling fetch handler, and cover that path: it was changed with no test behind it.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/wasm/wasmhttp/transport.go:217
name.String()can panic if the rejected value has a non-stringnameproperty (syscall/js panics when calling String() on non-TypeString). Since this code is already hardening against unusual rejection shapes, the AbortError check should also gate onname.Type() == js.TypeStringbefore callingString().
if len(catchArgs) > 0 && catchArgs[0].Type() == js.TypeObject {
if name := catchArgs[0].Get("name"); !name.IsUndefined() && name.String() == "AbortError" {
isAbort = true
}
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.
The Browser Fetch Smoke job makes one live staging API call with no retry, and misreports it when it fails. On PR #531 a 10.5s connect timeout came out as
expected a non-empty JSON array, which describes neither the failure nor anything that PR changed. Seven sibling PRs passed the identical check in the same 20-second window, and it passed on rerun with no code change.Three things combined to produce that message: the script checked
result.errorbut the CLI puts errors in a JSON envelope insideresult.output; the script ran the command exactly once; and the WASM transport read only the JS error'smessage, which under Node is a barefetch failedwith the real reason (UND_ERR_CONNECT_TIMEOUT,ECONNREFUSED) sitting oncause.make wasm-smokebehaves the same locally and in CI.causeto the transport's error, preferring itsmessageand falling back to itscode.TestRoundTrip_FetchRejectionSurfacesCausecovers five shapes, all taken from real Node 22 failures.Type() == js.TypeObject. A non-object rejection reason used to panic onValue.Get, and a panic on ajs.FuncOfgoroutine kills the whole WASM process rather than just the request.No retry in the transport itself. That's browser product code and retry policy there is a separate call; this flake belongs to the CI harness.
Verified:
make wasm-smokeexits 0 (621 records). With no credentials andWASM_SMOKE_COMMAND='ports list --output json'it exits 1 reporting the login error, after one logged retry. A scratch copy of the script with a one-shot injected fault exits 0 after the retry. Both new panic guards were checked by reverting them and watching the tests panic withsyscall/js: call of Value.Get on string.What I could not verify: the retry window is a judgment call. Two attempts 5s apart covers a single undici connect timeout, but not a longer staging outage, and it doubles worst-case job time to roughly 125s against a 10-minute job timeout. If this job keeps flaking, a staging health gate beats more retries.
Worth a follow-up, not fixed here: the same panic class is still open on the success path, which this PR does not touch.
thenFuncreadsresponse.Get("status")andextractHeadersreadsheaders.Get("forEach")with no type guard, so afetchthat resolves with a non-object (a patched global or a service worker) still takes the process down. Reproduced both. Left alone because it's pre-existing and on the success path, so it wants its own ticket and its own tests.