Breakout Task keeps running and can report completion twice after it finishes #1641

Activity

1. joshunrau

   added

   BugType: existing behavior is wrongType: existing behavior is wrong

   Priority: LowNice to have, cosmetic or speculativeNice to have, cosmetic or speculative

   Difficulty: LowIsolated change, about 2 hours or lessIsolated change, about 2 hours or less

   Area: InstrumentsInstrument authoring API, bundling, runtime, rendering, measures and the built-in libraryInstrument authoring API, bundling, runtime, rendering, measures and the built-in library

   on Oct 5, 2026
2. joshunrau commented on Oct 26, 2026

   

   joshunrau

   on Oct 26, 2026

   CollaboratorAuthor

   More actions

   Still reproduces on main (d86740e). packages/instrument-library/src/interactive/DNP_BREAKOUT_TASK/index.ts has not changed since 4caaa70 (2025-06-30), so the cited lines still hold.

   A loss also reports twice

   The second done does not need a win. The ball that is re-served at Lives: -1 (:187-199) keeps breaking bricks, and when it breaks the last one, collisionDetection calls onComplete again with livesRemaining: -1 (:111-116). In a Node port of draw with no input, the game is lost 1064 frames after Start with 14 bricks broken, and the second done follows 182 frames (about 3 s at 60 Hz) later. Every scale getScale can pick lands within a few frames of that. z.number().int() (:10) accepts -1, so a first save that fails or takes longer than those 3 s is followed by a saved livesRemaining: -1. On the win path the second done comes after livesRemaining more misses, not always three: a win with one life left reports again on the next miss.

   The playground example apps/playground/src/instruments/examples/interactive/Interactive-With-Vanilla/index.ts has the same loop and listeners (:51-53, :87, :160, :182), so it breaks the "call done exactly once" rule in packages/instrument-guidelines/AGENTS.md:541 too. Fix it in the same PR or a follow-up. It cannot get a unit test either way: the playground project runs in node and has no environment for /runtime/v1 (apps/playground/AGENTS.md, Tests).

   On the suggested fix

   • cancelAnimationFrame would cancel nothing. Both onComplete calls run inside draw, before :211 requests the next frame. Guard the request instead: if (!finished) requestAnimationFrame(draw).
   • draw renders (:170-175) before it updates (:176-210), so only stopping the loop freezes the canvas one step early: the last brick still drawn with Score: 14, or Lives: 1. Have complete draw the frame once more without stepping.
   • One AbortController replaces the three removeEventListener calls: pass { signal } to each addEventListener and call abort() in complete. It also covers the pointermove listener that Breakout Task paddle does not follow a finger dragged across a touch screen #1672 proposes. happy-dom 20.11.15 supports signal.

   Tests

   In DNP_BREAKOUT_TASK.test.ts:

   • The proposed key-press test does not test the listeners. Once no frame is requested nothing is drawn, so lastPaddleX() is the same whether or not they were removed. Assert instead that the signal passed in every call in addDocumentListener.mock.calls is aborted.
   • Keep the afterEach loop. Most tests stop mid-game, where the listeners are still attached.
   • Check nextFrame in both completion tests. With the final redraw, the loss test can also expect the last fillText call to be ('Lives: 0', 415, 20).

   e2e, in testing/src/specs/instrument-completion.spec.ts: the demo seeds the Breakout Task (apps/api/src/demo/demo.service.ts:72), and a game with no input is lost. Press Start with the keyboard so no mousemove moves the paddle. Answer the first POST /v1/instrument-records with a 500 through page.route, which keeps the iframe mounted. Wait for that POST, then 10 s more, and assert that no second POST was sent; on main one is. Use a 500, not a slow response: #1668's fix ignores a second done while the first save is pending, so with a slow response the test would pass without this fix. The loss alone takes about 18 s, so with the session setup and the wait the test exceeds Playwright's 30 s default (testing/playwright.config.ts sets no test timeout). Give it its own with test.describe.configure({ timeout }), as gateway-assignment.spec.ts does. #1825 also appends to this spec, so expect a conflict.

   #1640, #1669 and #1672 change the same source and test file. One PR for all four saves three rebases.

   _{Triage note written with Claude Code. Code references point at main @ d86740e.}