fix(js): always fall back to non-streaming WebAssembly.instantiate#445
Merged
fix(js): always fall back to non-streaming WebAssembly.instantiate#445
Conversation
Contributor
WalkthroughAdds a Justfile recipe and patch script that modify wasm-bindgen–generated JS to always log a warning and fall back to WebAssembly.instantiate when instantiateStreaming fails, removes an unused EXPECTED_RESPONSE_TYPES constant, and verifies per-file patch results. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Browser
end
rect rgba(240,255,220,0.5)
participant WASMLoader as WASM Loader
participant Network
end
rect rgba(255,240,220,0.5)
participant WebAssemblyRuntime as WebAssembly
end
Browser->>WASMLoader: request module
WASMLoader->>Network: fetch(wasmURL)
Network-->>WASMLoader: Response (stream)
WASMLoader->>WebAssemblyRuntime: try WebAssembly.instantiateStreaming(response, imports)
alt instantiateStreaming succeeds
WebAssemblyRuntime-->>WASMLoader: Module/Instance
WASMLoader-->>Browser: initialize
else instantiateStreaming throws
WebAssemblyRuntime--xWASMLoader: throws
WASMLoader->>Console: console.warn("instantiateStreaming failed, falling back")
WASMLoader->>Network: response.arrayBuffer()
Network-->>WASMLoader: ArrayBuffer
WASMLoader->>WebAssemblyRuntime: WebAssembly.instantiate(buffer, imports)
WebAssemblyRuntime-->>WASMLoader: Module/Instance
WASMLoader-->>Browser: initialize
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/justfile (1)
55-55: Use([ \t]*)instead of(\s*)to capture only indentation, not preceding newlinesThe pattern
(\s*)at the start matches a preceding newline (from the previous line) plus the indentation spaces. While syntactically correct, using([ \t]*)(horizontal whitespace only) makes the intent explicit and avoids the ambiguity of capturing a newline that gets re-emitted twice via\1in the replacement string. This is particularly important for regex maintainability given the pattern's sensitivity to wasm-bindgen's exact code format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/justfile` at line 55, Update the regex r'(\s*)\} catch \(e\) \{\s*\n' so its leading capture only matches horizontal indentation, not newlines—replace the group (\s*) with ([ \t]*) in the regex literal in js/justfile (the pattern string used where the catch-block formatting is matched) so the backreference (\1) will contain only spaces/tabs and won't re-emit a preceding newline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/justfile`:
- Around line 43-69: The patcher currently uses re.sub which silently does
nothing if the regex doesn't match; update the fix-wasm-streaming-fallback
Python target to use re.subn for the big try/catch replacement and for the
EXPECTED_RESPONSE_TYPES removal ( operate on the same text variable ), check the
returned count for each substitution, and if any expected substitution count is
0, print a clear error mentioning the filename f and sys.exit(1) to fail the
build; also adjust the indentation capture group in the big regex from (\s*) to
([ \t]*) as suggested so it only matches spaces/tabs, keeping the rest of the
replacement logic and writing back the file when substitutions succeed.
---
Nitpick comments:
In `@js/justfile`:
- Line 55: Update the regex r'(\s*)\} catch \(e\) \{\s*\n' so its leading
capture only matches horizontal indentation, not newlines—replace the group
(\s*) with ([ \t]*) in the regex literal in js/justfile (the pattern string used
where the catch-block formatting is matched) so the backreference (\1) will
contain only spaces/tabs and won't re-emit a preceding newline.
sd2k
force-pushed
the
fix/wasm-streaming-fallback
branch
2 times, most recently
from
11a4b16 to
90d4ec1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/justfile (1)
82-82:⚠️ Potential issue | 🟡 MinorSilent success when no
.jsfiles are found inaugurs/.If
glob.glob("augurs/*.js")returns an empty list (e.g., wasm-pack output directory changed), theforloop body never executes,errorsstays empty, and the script exits 0 — silently skipping all patches with no signal.🐛 Proposed fix — assert the glob is non-empty
+ files = sorted(glob.glob("augurs/*.js")) + assert files, "fix-wasm-streaming-fallback: no .js files found in augurs/" - for f in sorted(glob.glob("augurs/*.js")): + for f in files:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/justfile` at line 82, The loop over glob.glob("augurs/*.js") can silently do nothing if no files match; before iterating, check the result of glob.glob("augurs/*.js") (e.g., assign to a variable like augur_files) and if it's empty, raise an exception or exit non-zero with a clear error message so the script fails fast instead of leaving errors empty; modify the code that currently iterates directly over glob.glob(...) and add this pre-check referencing augur_files and errors to ensure a non-empty set of files is required.
🧹 Nitpick comments (1)
js/justfile (1)
83-83: File handles opened withoutwithcontext managers.Both
open(f).read()andopen(f, "w").write(text)leave file descriptors unclosed until GC. In a short-lived script this is benign, but usingwithis the idiomatic and safe form, especially before thewwrite (ensures flush + close on exception).♻️ Proposed refactor
- text = open(f).read() + with open(f, encoding="utf-8") as fh: + text = fh.read()- open(f, "w").write(text) + with open(f, "w", encoding="utf-8") as fh: + fh.write(text)Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/justfile` at line 83, Replace plain open(...) calls with context managers: change the read site that uses open(f).read() to use a with open(f, "r") as fh: text = fh.read() pattern, and change the write site that uses open(f, "w").write(text) to with open(f, "w") as fh: fh.write(text); update occurrences of open(f).read() and open(f, "w").write(text) accordingly so file descriptors are guaranteed closed even on exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/justfile`:
- Around line 97-100: The current whole-file check if "throw e;" in text risks
false positives; update the logic that verifies the patch by restricting the
search to the __wbg_load function body (or the exact replaced region) instead of
scanning the entire text. Locate the patched region using the same markers or
pattern used when replacing __wbg_load (e.g., the function signature
"__wbg_load" and its closing brace) and only check that substring for "throw
e;"; if present within that region append the error to errors, otherwise allow
the file to be written. Ensure you reference the existing variables (text,
errors) and the __wbg_load replacement code path so behavior remains consistent.
---
Duplicate comments:
In `@js/justfile`:
- Line 82: The loop over glob.glob("augurs/*.js") can silently do nothing if no
files match; before iterating, check the result of glob.glob("augurs/*.js")
(e.g., assign to a variable like augur_files) and if it's empty, raise an
exception or exit non-zero with a clear error message so the script fails fast
instead of leaving errors empty; modify the code that currently iterates
directly over glob.glob(...) and add this pre-check referencing augur_files and
errors to ensure a non-empty set of files is required.
---
Nitpick comments:
In `@js/justfile`:
- Line 83: Replace plain open(...) calls with context managers: change the read
site that uses open(f).read() to use a with open(f, "r") as fh: text = fh.read()
pattern, and change the write site that uses open(f, "w").write(text) to with
open(f, "w") as fh: fh.write(text); update occurrences of open(f).read() and
open(f, "w").write(text) accordingly so file descriptors are guaranteed closed
even on exceptions.
js/justfile
Outdated
Comment on lines
97
to
100
| # Verify no throw e; remains in the __wbg_load function. | ||
| if "throw e;" in text: | ||
| errors.append(f"{f}: patched but 'throw e;' still present") | ||
| continue |
Contributor
There was a problem hiding this comment.
"throw e;" in text is a whole-file scan — risk of false positives.
After patching, if "throw e;" in text searches the entire file content. If any other function in the wasm-bindgen–generated JS (e.g., error-propagation helpers added in future wasm-bindgen versions) contains the literal throw e; outside of __wbg_load, the check fires on a correctly-patched file: the error is appended, the file is not written, and the build exits 1 even though the patch was applied correctly.
Scope the check to just the __wbg_load function body, or at minimum confirm the substring is absent only within the replaced region:
🐛 Proposed fix — verify only the patched region
- # Verify no throw e; remains in the __wbg_load function.
- if "throw e;" in text:
- errors.append(f"{f}: patched but 'throw e;' still present")
- continue
+ # Verify the specific catch block no longer contains throw e;
+ # by re-searching for the original pattern (should be absent after patching).
+ if CATCH_V1.search(text) or CATCH_V2.search(text):
+ errors.append(f"{f}: patched but original rethrow pattern still present")
+ continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/justfile` around lines 97 - 100, The current whole-file check if "throw
e;" in text risks false positives; update the logic that verifies the patch by
restricting the search to the __wbg_load function body (or the exact replaced
region) instead of scanning the entire text. Locate the patched region using the
same markers or pattern used when replacing __wbg_load (e.g., the function
signature "__wbg_load" and its closing brace) and only check that substring for
"throw e;"; if present within that region append the error to errors, otherwise
allow the file to be written. Ensure you reference the existing variables (text,
errors) and the __wbg_load replacement code path so behavior remains consistent.
wasm-bindgen generates code that rethrows when WebAssembly.instantiateStreaming fails and Content-Type is application/wasm. This breaks in environments where fetch() returns a Proxy-wrapped Response (e.g. OpenTelemetry instrumentation-fetch) that passes JS `instanceof Response` but fails the browser's internal slot checks in WebAssembly.instantiateStreaming/compileStreaming. Add a post-build step that patches the generated __wbg_load function to always fall back to the ArrayBuffer path (response.arrayBuffer() + WebAssembly.instantiate) instead of conditionally rethrowing. The streaming path is purely an optimization; the ArrayBuffer path is always correct. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sd2k
force-pushed
the
fix/wasm-streaming-fallback
branch
from
90d4ec1 to
7c68f15
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/justfile (1)
82-83: Add a guard against an empty glob result.If
augurs/*.jsis empty (e.g. allbuild-innertargets were somehow skipped or ran in a separate step), the loop body never executes,errorsremains empty, and the script exits 0 — silently succeeding without patching a single file. This also combines with the missing context-manager pattern on line 83 and line 101.🛡️ Proposed fix — empty-glob guard + context managers
- errors = [] - for f in sorted(glob.glob("augurs/*.js")): - text = open(f).read() + files = sorted(glob.glob("augurs/*.js")) + if not files: + print("fix-wasm-streaming-fallback: no .js files found in augurs/ — was the build run first?", file=sys.stderr) + sys.exit(1) + errors = [] + for f in files: + with open(f) as fh: + text = fh.read()- open(f, "w").write(text) + with open(f, "w") as fh: + fh.write(text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/justfile` around lines 82 - 83, Add an explicit guard for an empty glob result before iterating over sorted(glob.glob("augurs/*.js")): if the list is empty, log or raise an error and exit non‑zero so the script doesn't silently succeed; also replace open(f).read() with the context-manager pattern (with open(f, "r") as fh: text = fh.read()) and apply the same change where files are opened later (e.g., the other open(...) at line 101) so files are always closed; reference the glob call sorted(glob.glob("augurs/*.js")), the loop variable f, the variable text, and the errors accumulator when implementing the guard and exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/justfile`:
- Around line 88-93: The current logic appends an error when both CATCH_V2.subn
and CATCH_V1.subn return n == 0 even if the file is already patched; modify the
branch after those two attempts to detect an already-patched file (e.g., check
if CATCH_REPL or a unique marker is present in text) and if so skip adding the
error and continue, otherwise keep appending the existing error; adjust the code
around the variables CATCH_V2, CATCH_V1, CATCH_REPL, text, errors and f so the
recipe is idempotent when run against already-patched files.
---
Nitpick comments:
In `@js/justfile`:
- Around line 82-83: Add an explicit guard for an empty glob result before
iterating over sorted(glob.glob("augurs/*.js")): if the list is empty, log or
raise an error and exit non‑zero so the script doesn't silently succeed; also
replace open(f).read() with the context-manager pattern (with open(f, "r") as
fh: text = fh.read()) and apply the same change where files are opened later
(e.g., the other open(...) at line 101) so files are always closed; reference
the glob call sorted(glob.glob("augurs/*.js")), the loop variable f, the
variable text, and the errors accumulator when implementing the guard and exit
behavior.
Comment on lines
+88
to
+93
| text, n = CATCH_V2.subn(CATCH_REPL, text) | ||
| if n == 0: | ||
| text, n = CATCH_V1.subn(CATCH_REPL, text) | ||
| if n == 0: | ||
| errors.append(f"{f}: expected to patch __wbg_load but regex didn't match") | ||
| continue |
Contributor
There was a problem hiding this comment.
Idempotency gap: running the recipe standalone on already-patched files produces a misleading error.
When both CATCH_V2.subn and CATCH_V1.subn return n == 0 on a previously-patched file, the error appended is "expected to patch __wbg_load but regex didn't match", and the final message printed is "The wasm-bindgen output format may have changed. Update the regex in js/justfile." A developer retrying without a clean rebuild (e.g., after a partial build failure) would chase a phantom wasm-bindgen format change. A simple content-based already-patched check avoids the false alarm:
♻️ Proposed fix — distinguish already-patched from genuinely unrecognised format
if n == 0:
- errors.append(f"{f}: expected to patch __wbg_load but regex didn't match")
- continue
+ # If neither pattern is present either, the file may already be patched.
+ if not (CATCH_V1.search(text) or CATCH_V2.search(text)):
+ print(f" {f}: already patched or no matching pattern — skipping")
+ continue
+ errors.append(f"{f}: instantiateStreaming present but regex didn't match; "
+ f"wasm-bindgen output format may have changed")
+ continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/justfile` around lines 88 - 93, The current logic appends an error when
both CATCH_V2.subn and CATCH_V1.subn return n == 0 even if the file is already
patched; modify the branch after those two attempts to detect an already-patched
file (e.g., check if CATCH_REPL or a unique marker is present in text) and if so
skip adding the error and continue, otherwise keep appending the existing error;
adjust the code around the variables CATCH_V2, CATCH_V1, CATCH_REPL, text,
errors and f so the recipe is idempotent when run against already-patched files.
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
1 participant
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.
Summary
js/justfilethat patches wasm-bindgen's generated__wbg_loadfunction to always fall back to theArrayBuffer+WebAssembly.instantiatepath wheninstantiateStreamingfails, instead of conditionally rethrowingProblem
wasm-bindgen generates code that rethrows when
WebAssembly.instantiateStreamingfails andContent-Typeisapplication/wasm. This breaks in environments wherefetch()returns a Proxy-wrappedResponse(e.g.@opentelemetry/instrumentation-fetch) that passes JSinstanceof Responsebut fails the browser's internal C++ slot checks.This has been observed in Grafana instances with OpenTelemetry tracing enabled, causing
@bsull/augursWASM modules to fail to load with:The OTel fetch instrumentation wraps
globalThis.fetchand returns aProxy<Response>fromcreateResponseProxy(). This Proxy correctly forwards all JS-visible properties, but browsers (Firefox and Chrome) perform internal slot validation ininstantiateStreaming/compileStreamingthat rejects Proxy objects.Fix
The streaming path is purely an optimization — the
ArrayBufferpath (response.arrayBuffer()+WebAssembly.instantiate()) always works correctly regardless of whether the Response is a Proxy. The post-build Python script patches all generated JS files to replace the conditional rethrow with an unconditionalconsole.warn+ fallback.Test plan
just buildinjs/and verify all 10.jsfiles no longer containthrow e;orEXPECTED_RESPONSE_TYPES@opentelemetry/instrumentation-fetchactive🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes