fix: don't cache rejected oniguruma WASM load promise (fixes #326825) - #326830
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
Prevents TextMate tokenization from becoming permanently broken for a session when vscode-oniguruma WASM loading fails transiently by avoiding caching a rejected initialization promise.
Changes:
- Wrap
vscode-onigurumaimport + WASM load in atry/catch. - Reset
_vscodeOnigurumatonullon failure so later calls can retry initialization.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/vs/workbench/services/textMate/browser/textMateTokenizationFeatureImpl.ts:394
- This four-line inline comment exceeds the repository's hard limit of one line for comments inside method bodies. Keep only the non-obvious cache rationale in a single line.
// Do not cache a rejected promise: loading the WASM can fail with a transient
// error (e.g. "Failed to fetch" while the window is being torn down). Caching
// the rejection would permanently break tokenization for the rest of the
// session and cause the same error to be re-reported for every language.
Summary
TypeError: Failed to fetchis thrown fromTextMateTokenizationFeature._loadVSCodeOnigurumaWASMwhen the renderer fetches the localonig.wasmasset (e.g. while editors are being restored during window startup/teardown). The fetch itself is a transient/environmental failure, but the memoization in_getVSCodeOnigurumacaches the rejected promise permanently:this._vscodeOnigurumais assigned the rejecting async IIFE and never reset on failure. As a result, a single transient blip poisons TextMate tokenization for the entire session — every subsequent language whose tokenizer is created re-awaits the same rejected promise, and each one re-reports the error throughonUnexpectedError. This amplification explains the hits-per-user ratio (218 hits / 125 users in 1.129.1) and why the bucket appears in essentially every shipped version.The fix stops the amplification: on failure we clear the cache so a later request can retry, while still re-throwing so the genuine failure is reported once (never silenced).
Fixes #326825
Recommended reviewer:
@alexdimaCulprit Commit
Not identified via
git blame— the workspace is a shallow (fetch-depth=1) clone, so line history is unavailable. The memoization pattern is long-standing; the bucket history shows hits across every tracked version (1.110 → 1.129), so this is not a single-commit regression but a long-lived latent amplifier. The recentfeat: restore asar for node_modules (#324897)change altered the builtonig.wasmresource path (node_modules.asar.unpacked/...) and is a plausible contributor to the recent uptick in transient fetch failures, but it is not the root cause of the telemetry amplification.Code Flow
flowchart TD A[LanguageService.requestRichLanguageFeatures] --> B[TokenizationRegistry.getOrCreate] B --> C[factory.resolve -> _create] C --> D[TextMateTokenizationFeature.createSupport] D --> E[_createTokenizationSupport try/catch] E --> F[_getOrCreateGrammarFactory] F --> G[_getVSCodeOniguruma - memoized] G --> H[_loadVSCodeOnigurumaWASM: fetch onig.wasm] H -- Failed to fetch --> I[rejected promise cached in _vscodeOniguruma] I -- reused by every later language --> G E -- catch --> J[onUnexpectedError -> telemetry, once per language]Affected Files
src/vs/workbench/services/textMate/browser/textMateTokenizationFeatureImpl.ts—_getVSCodeOniguruma()memoization now clears the cached promise on failure.Repro Steps
onig.wasmfetch to fail transiently (network hiccup, resource unavailable during window teardown/reload, or a slow/aborted request while the window is closing).Failed to fetchis reported repeatedly (once per language whose tokenizer is requested), rather than a single transient report followed by recovery.How the Fix Works
Chosen approach (
textMateTokenizationFeatureImpl.ts,_getVSCodeOniguruma): wrap the memoized async loader in atry/catchthat, on failure, resetsthis._vscodeOniguruma = nullbefore re-throwing. This fixes the defect at the data producer — the memoization cache is where the permanently-broken state is created — rather than at the crash site (_loadVSCodeOnigurumaWASM). Thecatchdoes not silence anything: it re-throws so_createTokenizationSupport's existingcatchstill routes the genuine failure toonUnexpectedErrorexactly once, preserving the telemetry pipeline. After this change a transientFailed to fetchno longer poisons the whole session; the next tokenizer request re-attempts the load and can succeed.Alternatives considered:
_createTokenizationSupport's catch (e.g. treatingFailed to fetchlike the benign missing-grammar case): rejected because it hides a real failure from telemetry instead of fixing the state-caching defect that causes the repeated reports._loadVSCodeOnigurumaWASM: rejected because it masks the symptom at the bottom of the stack and does not address the cache poisoning that produces the amplification.Recommended Owner
@alexdima— owns the editor tokenization / TextMate service area (src/vs/workbench/services/textMate,src/vs/editor/common/tokenizationRegistry.ts).