From f7c92d9eb949d8f16462b30e8cfe0a94ac0ffee9 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 4 Sep 2026 00:26:08 -0700 Subject: [PATCH] Live: the shader teardown can no longer race its own construction The new cycling assertion caught a real defect on CI: vite8-react-insert reached CYCLING with #impeccable-live-shader still painted over the page. showShaderOverlay is async. It appends its canvas synchronously, then awaits createImageBitmap and finishes the GL setup before it publishes shaderState. hideShaderOverlay returned early on a null shaderState, so a teardown that landed inside that window did nothing, and the construction then published itself over a session that had already left GENERATING, with no teardown left to run. The scroll tick kept repositioning it, which is why the CI page.html shows the canvas sized from the capture rect but styled to the cycling anchor. Every teardown now bumps a shader epoch before it does anything else, and a construction pins the epoch it owns and abandons its canvas (releasing the GL context) at every point past an await and before any publish, including both bitmap-fallback publishes. A teardown also drops a shader node that no shaderState owns, so an already-orphaned canvas cannot survive one. Reproduced by widening the append-to-publish window: with a 400ms delay after uiAppend, vite8-react-insert failed with the CI error and the probe showed the teardown arriving at CYCLING with shaderState still null. The same run passes with this change, as does a 1500ms window on insert and plain. Locally that window is about 4ms, which is why it only showed on a slower runner. The four remaining setLiveState('CYCLING') sites that did not lower the loader now do: the SSE done handler (the one route that can reach CYCLING from GENERATING), the Svelte republish remount, and the two accept failure recoveries. The e2e assertion already waits up to 5s for the shader to clear, so it was never racing a legitimate teardown; it is left as it is. Co-Authored-By: Claude Code Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY --- skill/scripts/live-browser.js | 47 ++++++++++++++++++++++- tests/live-browser-source.test.mjs | 61 ++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index 10353bc95..cbbf3a7ce 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -5820,6 +5820,7 @@ return; } setLiveState('CYCLING'); + hideShaderOverlay(); showOrUpdateCyclingBar(); saveSession(); completeParameterGenerationIfReady(); @@ -7166,6 +7167,7 @@ if (arrivedVariants >= expectedVariants && expectedVariants > 0) { if (state === 'GENERATING') { setLiveState('CYCLING'); + hideShaderOverlay(); showOrUpdateCyclingBar(); disableInlineEdit(); refreshParamsPanel(); @@ -7226,6 +7228,7 @@ pendingAcceptedSession = null; awaitingAcceptResult = null; setLiveState('CYCLING'); + hideShaderOverlay(); updateBarContent('cycling'); showToast('Could not complete accept cleanup. Try Accept again.', 5000); break; @@ -8328,6 +8331,15 @@ void main() { // matches the original off-white risograph paper. const SHADER_PAPER_FALLBACK = [0.975, 0.965, 0.955]; let shaderState = null; // { canvas, gl, program, texture, rafId, startTime } + // showShaderOverlay is async: it appends its canvas, then awaits + // createImageBitmap and the GL setup before it publishes shaderState. A + // teardown that landed inside that window found shaderState still null, + // returned, and then watched the construction publish itself over a session + // that had already left GENERATING, with no teardown left to run. That is + // the generating loader frozen over a page that already cycles (issue #719). + // Every teardown bumps this epoch; a construction abandons its own canvas as + // soon as it sees the epoch move. + let shaderEpoch = 0; // The element's effective background tone, used as the uniform halftone // ground so content dissolves into dots over it. Unlike resolveCanvasBackground @@ -8474,14 +8486,28 @@ void main() { }); } + /** Drop a shader node no shaderState owns (an abandoned construction). */ + function removeStrayShaderNode() { + const stray = uiGetById(PREFIX + '-shader'); + if (stray) stray.remove(); + } + function hideShaderOverlay() { - if (!shaderState) return; + // Bump first, unconditionally: this is what tells an in-flight + // showShaderOverlay to abandon itself rather than publish over a session + // that has already moved on. + shaderEpoch += 1; + if (!shaderState) { + removeStrayShaderNode(); + return; + } if (shaderState.rafId) cancelAnimationFrame(shaderState.rafId); if (shaderState.canvas) shaderState.canvas.remove(); if (shaderState.objectUrl) URL.revokeObjectURL(shaderState.objectUrl); const lose = shaderState.gl?.getExtension?.('WEBGL_lose_context'); try { lose?.loseContext(); } catch {} shaderState = null; + removeStrayShaderNode(); } function showShaderBitmapFallback(canvas, blob) { @@ -8506,6 +8532,16 @@ void main() { async function showShaderOverlay(el, blob, rect, paper) { hideShaderOverlay(); if (!blob || !el) return; + // hideShaderOverlay just bumped the epoch, so this run owns it until the + // next teardown. Every step past an await re-checks before it publishes. + const epoch = shaderEpoch; + const abandoned = (node, gl) => { + if (epoch === shaderEpoch) return false; + node.remove(); + const lose = gl?.getExtension?.('WEBGL_lose_context'); + try { lose?.loseContext(); } catch {} + return true; + }; const canvas = document.createElement('canvas'); canvas.id = PREFIX + '-shader'; const dpr = Math.min(window.devicePixelRatio || 1, 2); @@ -8528,6 +8564,7 @@ void main() { if (!gl) { // WebGL unavailable: use the captured bitmap as a background overlay so // the user still sees something meaningful during generation. + if (abandoned(canvas, null)) return; showShaderBitmapFallback(canvas, blob); return; } @@ -8567,16 +8604,22 @@ void main() { } // Upload the screenshot as a texture + if (abandoned(canvas, gl)) return; let bitmap; try { bitmap = await createImageBitmap(blob); } catch (err) { console.warn('[impeccable] shader bitmap decode failed:', err); + if (abandoned(canvas, gl)) return; const lose = gl.getExtension?.('WEBGL_lose_context'); try { lose?.loseContext(); } catch {} showShaderBitmapFallback(canvas, blob); return; } + if (abandoned(canvas, gl)) { + if (bitmap.close) bitmap.close(); + return; + } texture = gl.createTexture(); gl.bindTexture(gl.TEXTURE_2D, texture); gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE); @@ -8595,6 +8638,7 @@ void main() { const paperRgb = paper || resolvePaperRgb(el); const reduced = window.matchMedia('(prefers-reduced-motion: reduce)').matches; + if (abandoned(canvas, gl)) return; shaderState = { canvas, gl, program, texture, rafId: 0, startTime: performance.now(), reduced }; function frame() { if (!shaderState) return; @@ -8674,6 +8718,7 @@ void main() { .catch(() => { if (pendingAcceptedSession?.id === acceptedSessionId) pendingAcceptedSession = null; setLiveState('CYCLING'); + hideShaderOverlay(); showOrUpdateCyclingBar(); showToast('Could not confirm accept with the live server. Session kept for recovery; try Accept again.', 5000); }); diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index f2aaa0b46..c87db08f6 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -564,6 +564,67 @@ describe('live-browser source contracts', () => { ); }); + it('never leaves a shader behind when the teardown races its construction (#719)', () => { + // showShaderOverlay appends its canvas, then awaits createImageBitmap and + // the GL setup before it publishes shaderState. A teardown inside that + // window found shaderState null, returned, and then watched the + // construction publish itself over a session that had already reached + // CYCLING, with no teardown left to run. On a slow runner that is the + // generating loader frozen over a page that already cycles. + const hideStart = SOURCE.indexOf('function hideShaderOverlay()'); + const hide = SOURCE.slice(hideStart, SOURCE.indexOf('\n function ', hideStart + 10)); + assert.match( + hide, + /shaderEpoch \+= 1;[\s\S]{0,120}?if \(!shaderState\) \{/, + 'the epoch must be bumped before the no-state early return, or an in-flight construction never hears about the teardown', + ); + assert.match(hide, /removeStrayShaderNode\(\);/, 'a teardown must also drop a shader node no state owns'); + + const showStart = SOURCE.indexOf('async function showShaderOverlay('); + const show = SOURCE.slice(showStart, SOURCE.indexOf('\n async function handleAccept', showStart)); + assert.match(show, /const epoch = shaderEpoch;/, 'the construction must pin the epoch it owns'); + assert.match( + show, + /const abandoned = \(node, gl\) => \{[\s\S]{0,80}?if \(epoch === shaderEpoch\) return false;[\s\S]{0,200}?return true;/, + 'abandoning must remove the canvas and release the GL context', + ); + assert.match( + show, + /if \(abandoned\(canvas, gl\)\) return;\n shaderState = \{ canvas, gl, program, texture,/, + 'the publish must be guarded by the epoch it pinned', + ); + const awaitIdx = show.indexOf('await createImageBitmap(blob)'); + assert.ok(awaitIdx > 0, 'createImageBitmap is the await this guards'); + assert.ok( + show.indexOf('if (abandoned(canvas, gl))', awaitIdx) > awaitIdx, + 'the bitmap await must be followed by an abandonment check', + ); + for (const call of ['showShaderBitmapFallback(canvas, blob);']) { + let at = show.indexOf(call); + assert.ok(at > 0, call); + while (at > 0) { + const before = show.slice(Math.max(0, at - 220), at); + assert.match(before, /abandoned\(canvas, (?:gl|null)\)/, 'every fallback publish must be epoch guarded'); + at = show.indexOf(call, at + 1); + } + } + }); + + it('lowers the shader on every route that sets CYCLING (#719)', () => { + // resumeSession reaches CYCLING through setLiveState(resumedState), which + // its own block covers; every literal site has to lower the loader too. + const sites = [...SOURCE.matchAll(/setLiveState\('CYCLING'\);/g)].map((m) => m.index); + assert.ok(sites.length >= 8, `expected the known CYCLING sites, saw ${sites.length}`); + for (const at of sites) { + const after = SOURCE.slice(at, at + 260); + assert.match( + after, + /hideShaderOverlay\(\);/, + `a setLiveState('CYCLING') at offset ${at} does not lower the generating shader`, + ); + } + }); + it('prefers the wrapper that actually holds variants over the first match (#719)', () => { // A target inside a `.map()` renders one wrapper per item, and an agent // that relocates the wrapper out of the shared primitive live-wrap