Skip to content

fix(sound): guard recording lifecycle - #870

Open
martin-henz wants to merge 2 commits into
masterfrom
replace-pr-839
Open

fix(sound): guard recording lifecycle#870
martin-henz wants to merge 2 commits into
masterfrom
replace-pr-839

Conversation

@martin-henz

Copy link
Copy Markdown
Member

Re-opens #839, which was automatically closed (not merged) when its base branch conductor-migration was deleted after being merged into master via #680. This branch cherry-picks the original commits from @11suixing11 unchanged, onto current master.

Summary

  • reserve one in-flight recording slot synchronously when record or record_for accepts a request
  • make record's returned stop function idempotent so repeated calls reuse one stopRecording() promise
  • release recording state after successful completion and after startRecording/stopRecording failures
  • guard cleanup with a per-recording generation token so a stale stop() cannot release a newer reservation

Fixes #799

Why

record and record_for schedule delayed work before io().startRecording() runs. The existing activePlayCount check prevents recording during playback, but there was no state reserved for a recording request itself. Two quick calls could therefore schedule independent timeout chains and eventually attempt overlapping recordings against the same tab-side recorder.

The PR reserves recording state at call time, so a second request fails before either timeout chain reaches startRecording().

Review Follow-up

CodeRabbit flagged a stale cleanup edge case: after startRecording() rejects and releases the reservation, invoking the old returned stop() could settle a second cleanup after a newer recording had reserved the mic.

reserveRecording() now returns a generation token, and releaseRecording() only clears state when that token still identifies the active reservation. The regression test covers this exact sequence:

  1. the first record() fails during startup
  2. a newer recording reserves the mic
  3. the stale stop() from the failed first recording is invoked
  4. the newer reservation remains active

Testing (from original PR, re-verify on CI)

  • yarn workspace @sourceacademy/bundle-sound test (71 passed)
  • yarn workspace @sourceacademy/bundle-sound tsc
  • yarn workspace @sourceacademy/bundle-sound lint

Original author: @11suixing11

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sound: record()/record_for() don't guard against overlapping/concurrent recording sessions

2 participants