Correct misleading crash-recovery comment (release pair) - #234
Open
ryanbr wants to merge 1 commit into
Open
Conversation
Fixes a documented-but-absent safety property in the IVS WASM worker crash-recovery comment at vaft.user.js + vaft-ublock-origin.js. The old comment claimed 'existing reload cooldown prevents runaway restart loops' — verifiably false; the crash handler calls doTwitchPlayerTask(..., 'early') directly on the main thread, bypassing the worker-side cooldown logic in processM3U8. Rewritten to honestly state: per-worker 'crashed' flag dedupes the multi-event burst from one crash; no cross-worker storm guard is implemented (intentionally — observed crashes have all been single + recoverable; revisit only if field logs ever show clustered crashes >=2 within ~60s). Lists all three observed WASM-error variants. No behavioral change — comment text only. Testing pair untouched (already has the full circuit-breaker + accurate comment from the v651 testing-direct work, intentional release-vs-testing divergence). No version bump per the no-per-PR-version convention; bullet added under ## Unreleased for the next roll-up. Co-Authored-By: Claude Opus 4.7 (1M context) <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.
Comment-only docfix on the release pair. No behavioral change, no version bump (per the no-per-PR-version convention) — bullet added under
## Unreleasedfor the next roll-up.What the old comment said (false)
The IVS WASM worker
errorhandler invaft.user.js+vaft-ublock-origin.jspreviously claimed:That's verifiably false. The crash handler calls
doTwitchPlayerTask(false, true, 'early')directly on the main thread. The reload-cooldown logic (tooSoonSinceLastReload, escalation, 30s/90s) lives in the worker'sprocessM3U8reload-decision path — which crash-recovery never traverses.doTwitchPlayerTaskitself has no throttle. So a hypothetical persistently-crashing WASM would currently produce an unboundedcrash → uncooled hard reload → fresh worker → crashloop, not a cooldown-throttled one.This misleading comment was what originally pulled me into the circuit-breaker thread (closed PR #230). Fixing the comment regardless removes the documented-but-absent safety property trap.
What the new comment says (accurate)
index out of bounds,indirect call signature mismatch,indirect call to null(the v82 and v83 logs gave us the latter two).crashedflag dedupes the multi-event burst from one crash — and is per-closure (does NOT survive the reload).Scope
vaft.user.js+vaft-ublock-origin.js: comment block rewritten (6 lines → 10 lines, both files synced).CHANGELOG.md: bullet under## Unreleased→### Documentation.acorn-clean on both release files. Single squashed commit.
🤖 Generated with Claude Code