stop the music player playing two tracks at once

begin() only sets `started` after a fetch and a full decode, so anything that
called it during that window saw started === false and started a second decode
of the same track. both finished, both called startBuffer, and only one of the
two sources ended up in `cur`.

starting a queue from a paused player did this every time: the host commits a
new queue and playing: true in one render, its queue effect calls
load(autoplay) -> begin, and its playing effect then calls play() -> begin
again, same generation.

it compounds, which is why it sounded like three songs and not two. the twin
keeps its own onended, so at the boundary advance() ran twice: the index jumped
two tracks and a second source was promoted while the first was still sounding.

three changes. begin() refuses re-entry for a generation it is already running.
onended only advances the queue if the source that ended is the one in `cur`.
and every source is registered in a `live` set, because cur/nxt is what the
engine reasons about while `live` is what it is responsible for silencing — an
untracked web audio node cannot be stopped by anything except closing the
context.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

src/workspaces/officerdev/src/MusicPlayer/gapless-engine.test.ts

@@ -0,0 +1,191 @@
+import { describe, expect, test, beforeEach, afterEach } from 'bun:test';
+import { GaplessEngine, type EngineTrack } from './gapless-engine';
+
+// These tests exist because the failure they cover is inaudible to the code and obvious to the ear: two
+// sources playing the same track at once. Nothing throws, no state looks wrong, `cur` points at a real
+// node that is really playing — there is simply a second one nobody is holding. The only way to catch it
+// is to count the nodes that were started.
+
+type StartCall = { when: number; offset: number };
+
+class FakeSource {
+  buffer: AudioBuffer | null = null;
+  onended: (() => void) | null = null;
+  started: StartCall | null = null;
+  stopped = false;
+  disconnected = false;
+  constructor(private ctx: FakeContext) {}
+  connect(): void {}
+  disconnect(): void {
+    this.disconnected = true;
+  }
+  start(when = 0, offset = 0): void {
+    if (this.started) throw new Error('InvalidStateError: already started');
+    this.started = { when, offset };
+    this.ctx.started.push(this);
+  }
+  stop(): void {
+    if (!this.started) throw new Error('InvalidStateError: not started');
+    this.stopped = true;
+  }
+  /** What the browser does at the end of the buffer (or at stop()) — the engine's advance trigger. */
+  end(): void {
+    this.onended?.();
+  }
+}
+
+class FakeContext {
+  currentTime = 0;
+  state: 'running' | 'suspended' | 'closed' = 'running';
+  destination = {};
+  started: FakeSource[] = [];
+  createGain() {
+    return { gain: { value: 1 }, connect: () => {} };
+  }
+  createBufferSource() {
+    return new FakeSource(this) as unknown as AudioBufferSourceNode & FakeSource;
+  }
+  async resume(): Promise<void> {
+    this.state = 'running';
+  }
+  async suspend(): Promise<void> {
+    this.state = 'suspended';
+  }
+  async close(): Promise<void> {
+    this.state = 'closed';
+  }
+  async decodeAudioData(): Promise<AudioBuffer> {
+    // Decoding is the slow step the bug hides inside — a macrotask is enough to model "not instant".
+    await new Promise((r) => setTimeout(r, 0));
+    return { duration: 100 } as AudioBuffer;
+  }
+}
+
+/**
+ * Sources producing sound RIGHT NOW. The `when <= currentTime` clause is not a detail: a gapless engine
+ * always has the next track already started, scheduled at the current track's end. Counting those as
+ * audible would make the healthy state look like the bug.
+ */
+const audible = (ctx: FakeContext) =>
+  ctx.started.filter((s) => !s.stopped && s.started !== null && s.started.when <= ctx.currentTime);
+
+let ctx: FakeContext;
+const originalFetch = globalThis.fetch;
+const originalCtor = (globalThis as Record<string, unknown>).AudioContext;
+
+const QUEUE: EngineTrack[] = [
+  { key: 'a', url: '/stream/a' },
+  { key: 'b', url: '/stream/b' },
+  { key: 'c', url: '/stream/c' },
+];
+
+// Let every pending decode + its continuation run.
+const settle = async () => {
+  for (let i = 0; i < 8; i++) await new Promise((r) => setTimeout(r, 0));
+};
+
+beforeEach(() => {
+  ctx = new FakeContext();
+  (globalThis as Record<string, unknown>).AudioContext = function () {
+    return ctx;
+  };
+  globalThis.fetch = (async () => ({
+    ok: true,
+    arrayBuffer: async () => new ArrayBuffer(8),
+  })) as unknown as typeof fetch;
+});
+
+afterEach(() => {
+  globalThis.fetch = originalFetch;
+  (globalThis as Record<string, unknown>).AudioContext = originalCtor;
+});
+
+describe('GaplessEngine', () => {
+  test('load(autoplay) followed by play() starts the track exactly once', async () => {
+    // The host's queue effect and playing effect both fire in the same commit when a queue starts from a
+    // paused player. This is that commit, and it used to produce two sources of the same track.
+    const engine = new GaplessEngine({});
+    engine.load(QUEUE, 0, true);
+    engine.play();
+    await settle();
+
+    expect(audible(ctx)).toHaveLength(1);
+    engine.destroy();
+  });
+
+  test('repeated play() during the decode does not stack sources', async () => {
+    const engine = new GaplessEngine({});
+    engine.load(QUEUE, 0, true);
+    engine.play();
+    engine.play();
+    engine.play();
+    await settle();
+
+    expect(audible(ctx)).toHaveLength(1);
+    engine.destroy();
+  });
+
+  test('pause then play while still decoding does not stack sources', async () => {
+    // The impatient-user path: nothing is audible yet because the file is still downloading, so the play
+    // button gets hit again.
+    const engine = new GaplessEngine({});
+    engine.load(QUEUE, 0, true);
+    engine.pause();
+    engine.play();
+    await settle();
+
+    expect(audible(ctx)).toHaveLength(1);
+    engine.destroy();
+  });
+
+  test('a stale source ending does not advance the queue', async () => {
+    // Defence in depth: even if a source outlives its bookkeeping, only `cur` may move the index.
+    const seen: number[] = [];
+    const engine = new GaplessEngine({ onIndex: (i) => seen.push(i) });
+    engine.load(QUEUE, 0, true);
+    await settle();
+
+    const first = audible(ctx)[0]!;
+    engine.skipTo(2); // bumps the generation and stops `first`
+    await settle();
+    first.end(); // the browser still delivers its onended
+
+    expect(seen).toEqual([]); // skipTo is a user action; only a NATURAL boundary reports an index
+    engine.destroy();
+  });
+
+  test('switching queues leaves nothing from the old one sounding', async () => {
+    const engine = new GaplessEngine({});
+    engine.load(QUEUE, 0, true);
+    await settle();
+    engine.load([{ key: 'z', url: '/stream/z' }], 0, true);
+    await settle();
+
+    expect(audible(ctx)).toHaveLength(1);
+    expect(audible(ctx)[0]!.started).not.toBeNull();
+    engine.destroy();
+  });
+
+  test('a natural boundary advances the index exactly once', async () => {
+    const seen: number[] = [];
+    const engine = new GaplessEngine({ onIndex: (i) => seen.push(i) });
+    engine.load(QUEUE, 0, true);
+    await settle();
+
+    const cur = audible(ctx)[0]!;
+    cur.end();
+    await settle();
+
+    expect(seen).toEqual([1]);
+    engine.destroy();
+  });
+
+  test('destroy() silences every source it started', async () => {
+    const engine = new GaplessEngine({});
+    engine.load(QUEUE, 0, true);
+    await settle();
+    engine.destroy();
+
+    expect(audible(ctx)).toHaveLength(0);
+  });
+});

src/workspaces/officerdev/src/MusicPlayer/gapless-engine.ts

@@ -38,6 +38,11 @@ export class GaplessEngine {
 
   private cur: AudioBufferSourceNode | null = null;
   private nxt: AudioBufferSourceNode | null = null;
+  // EVERY source node this engine has started and that has not yet ended. `cur`/`nxt` are the two it is
+  // reasoning about; this is the set it is responsible for silencing. They diverged once — a duplicate
+  // begin() left a source playing that nothing held a reference to, so nothing could ever stop it — and
+  // an orphan in Web Audio is unstoppable and inaudible to the code. Add here, stop from here.
+  private live = new Set<AudioBufferSourceNode>();
   private curBaseTime = 0; // ctx time that corresponds to position 0 of the current track
   private curDuration = 0;
   private nextStartAt = 0; // ctx time the scheduled `nxt` will begin (= current track's end)
@@ -45,6 +50,9 @@ export class GaplessEngine {
   private started = false; // has the current track's source actually been started?
   private playing = false;
   private gen = 0; // bumped on any disruptive change; stale async/onended callbacks check it and bail
+  // The generation a begin() is currently in flight for, or -1. `gen` alone cannot express this: it marks
+  // a change, and two begin() calls for the SAME generation are exactly the case that must be refused.
+  private beginGen = -1;
   private raf = 0;
 
   private cb: EngineCallbacks;
@@ -201,8 +209,25 @@ export class GaplessEngine {
     void this.preloadNext(gen);
   }
 
-  /** Start the current track at `pendingOffset` (decoding first if needed). */
+  /**
+   * Start the current track at `pendingOffset` (decoding first if needed).
+   *
+   * Re-entrant calls for the same generation are REFUSED, and that guard is the whole reason this method
+   * is not simply idempotent-by-gen. `started` only becomes true at the very end, after a fetch and a
+   * full decode — seconds, for a long track. Anything that calls begin() in that window sees
+   * `started === false` and starts a second, parallel decode of the same track, and both finish and both
+   * call startBuffer(): two sources of the same audio playing at once, only one of them in `cur`.
+   *
+   * That was not a rare race. Starting a queue from a paused player fired it every single time: the host
+   * commits a new queue and `playing: true` together, its queue effect calls load(autoplay) → begin, and
+   * its playing effect then calls play() → begin again, same generation. The audible result compounds —
+   * the untracked twin keeps its own onended, so at the track boundary advance() runs twice, the index
+   * jumps two tracks and a second source is promoted while the first is still sounding.
+   */
   private async begin(gen: number): Promise<void> {
+    if (this.beginGen === gen) return;
+    this.beginGen = gen;
+    try {
       const ctx = this.ensureCtx();
       if (ctx.state === 'suspended') await ctx.resume();
       if (gen !== this.gen) return;
@@ -214,6 +239,11 @@ export class GaplessEngine {
       if (gen !== this.gen || !buf) return;
       this.startBuffer(buf, this.pendingOffset, gen);
       void this.preloadNext(gen);
+    } finally {
+      // Released even on the bail paths: a decode that fails must not leave this generation permanently
+      // unable to start, or a failed track would wedge the player until something bumped `gen`.
+      if (this.beginGen === gen) this.beginGen = -1;
+    }
   }
 
   private startBuffer(buf: AudioBuffer, offset: number, gen: number): void {
@@ -224,9 +254,8 @@ export class GaplessEngine {
     const startAt = ctx.currentTime;
     const off = Math.min(Math.max(0, offset), buf.duration);
     src.start(startAt, off);
-    src.onended = () => {
-      if (gen === this.gen) this.advance();
-    };
+    this.live.add(src);
+    src.onended = () => this.onSourceEnded(src, gen);
     this.cur = src;
     this.curDuration = buf.duration;
     this.curBaseTime = startAt - off; // position = ctx.currentTime - curBaseTime
@@ -237,12 +266,19 @@ export class GaplessEngine {
   /** Decode index+1 and schedule it to begin exactly when the current track ends. */
   private async preloadNext(gen: number): Promise<void> {
     if (this.nxt) {
+      const stale = this.nxt;
+      this.live.delete(stale);
       try {
-        this.nxt.onended = null;
-        this.nxt.stop();
+        stale.onended = null;
+        stale.stop();
       } catch {
         /* not started */
       }
+      try {
+        stale.disconnect();
+      } catch {
+        /* already disconnected */
+      }
       this.nxt = null;
     }
     const forIndex = this.index;
@@ -257,13 +293,26 @@ export class GaplessEngine {
     src.buffer = buf;
     src.connect(this.gain!);
     src.start(boundary, 0);
-    src.onended = () => {
-      if (gen === this.gen) this.advance();
-    };
+    this.live.add(src);
+    src.onended = () => this.onSourceEnded(src, gen);
     this.nxt = src;
     this.nextStartAt = boundary;
   }
 
+  /**
+   * A source finished. Only the one the engine considers CURRENT may drive the queue forward.
+   *
+   * Without the identity check, any source that outlives its bookkeeping still advances the queue when it
+   * ends — so one stray node does not just play unwanted audio, it desynchronises the index for
+   * everything after it. `cur` is the single source of truth for "what is playing"; ending anything else
+   * is bookkeeping, not an event.
+   */
+  private onSourceEnded(src: AudioBufferSourceNode, gen: number): void {
+    this.live.delete(src);
+    if (gen !== this.gen || this.cur !== src) return;
+    this.advance();
+  }
+
   /** Fired at a track boundary (its source ended): move to the next track. */
   private advance(): void {
     if (this.index + 1 >= this.queue.length) {
@@ -293,9 +342,9 @@ export class GaplessEngine {
     }
   }
 
+  /** Silence everything this engine has started. Iterates `live`, not just cur/nxt — see its declaration. */
   private stopSources(): void {
-    for (const s of [this.cur, this.nxt]) {
-      if (!s) continue;
+    for (const s of this.live) {
       try {
         s.onended = null;
         s.stop();
@@ -308,6 +357,7 @@ export class GaplessEngine {
         /* already disconnected */
       }
     }
+    this.live.clear();
     this.cur = null;
     this.nxt = null;
   }