fix(engine): harden grouped audio rendering

[vanceingalls]

Part 3 of 12 replacing #3439. Base: wa-26b-core-preview-transport.

Why

Grouped audio needs one explicit render boundary: member summing, FX processing, automation, gain staging, and structured failure handling belong together.

What

• preserve group headroom through the FX chain
• give each rendered bus instance a collision-free identity
• return structured mix failures and clean temporary work on errors
• split page-transfer code from the FX render path
• normalize and test group volume envelopes and real FFmpeg output

Verification

• all relevant TypeScript projects pass cumulatively
• fallow audit --base main --fail-on-issues

The final stack tip preserves the verified #3439 replacement and includes the review fixes landed across the stack.

Stack: #3445 ← #3446 → #3447

[jrusso1020]

Reviewed at 84add1944b, base wa-26b-core-preview-transport. No prior reviews. Third of the three I have (#3444 5003519240, #3445 5003526168).

No blocking findings. This is the strongest rung of the three. Below is what I verified rather than assumed, one latent coupling worth writing down, and why I am not stamping yet.

The render side resolves bus identity correctly — and that is what dates the #3444 finding

function memberGroupKey(el: RefResolverEl): string | null {
  return el.getAttribute(AUDIO_GROUP_RENDER_ID_ATTR) ?? el.getAttribute(HF_AUDIO_GROUP_ATTR);
}

Stamped key first, author id as fallback, with a docblock that states the failure it closes. That is exactly right, and it is the same shape resolveAudioGroups uses.

It also sharpens the finding I filed on #3444: of the four readers that resolve a member to its bus, three consult the stamp and one does not — isMemberGroupHidden reads the author id alone. So this is not a pattern the stack hasn't settled on; it is a single reader that missed the pattern the other three follow. Worth fixing there rather than being read as a design question.

The two duplicate bus instances tests are the right pin for it, and drops only the muted instance's member is precisely the case that was silently taking both instances out of the export.

safePathSegment handles both halves, including the one that usually gets missed

const cleaned = id.replace(/[^A-Za-z0-9_-]/g, "_");
return `${cleaned || "group"}-${fallbackIndex}`;

Allowlist rather than blocklist, so / and . both collapse and a/../../escaped cannot leave workDir. The part I want to credit explicitly is the positional suffix: sanitisation is many-to-one, and a naive version would let bed/a and bed?a share group-bed_a.wav so the second sub-mix silently overwrites the first. keeps distinct groups isolated when their sanitized ids collide tests exactly that, and asserts on level rather than on filenames, which is the assertion that would actually catch it. Most PRs that sanitise a path get the traversal half and not the collision half.

One cross-reference, since it is the same input: this docblock establishes that data-audio-group "reaches this file straight from the document — the studio's GROUP_ID_PATTERN guards only ids the studio itself mints, and a hand-authored or agent-written one is unvalidated." That is the same untrusted value #3444's resolveGroupElement interpolates into a querySelector string without escaping, where a " makes it throw. The threat model is already written down here; that one call site just did not get the treatment.

The normalize=0 fallback is exact, and what makes it exact is worth stating

Primary path pins amix=…:normalize=0 so members sum at unity. The fallback for builds without the option compensates the default divide-by-N with volume=${memberTracks.length}, and the comment correctly insists on this group's member count rather than the render's global track count.

That compensation is arithmetically exact only because every member branch is apad-ed to totalDuration with dropout_transition=0, so all N inputs stay active for the whole mix and amix's divisor never changes. ffmpeg's normalize rescales by currently active inputs, so if a later change drops apad, narrows it, or reintroduces a dropout transition, the fallback silently becomes wrong partway through the mix — quietly, on old-ffmpeg machines only, which is the worst place to find out. Nothing to change; it is worth a line in the comment naming apad as the reason the constant works, because that is the half a future edit removes without noticing.

Verified rather than assumed

• No test coverage was lost, despite the file being rewritten +440/-186. I compared name inventories at both refs and validated the extractor first (a known-rewritten name, routing isolation, is present at both — so the - lines are in-place rewrites from mixAudio to processCompositionAudio, not deletions). audioMixer.grouping.test.ts 5 → 12, audioVolumeEnvelope.test.ts 6 → 8, audioMixer.level.test.ts 2 → 2, and the removed-and-still-absent set is empty for all three.
• The float intermediate is the right fix for the clipping, not a workaround: normalize=0 sums at unity, so an over-unity member sum hard-clipped at ±1 in the 16-bit intermediate before the group fader and FX ran — i.e. the damage happened upstream of the stage that would have brought it back down. Both does not clip an over-unity member sum before the group fader and its FX-chain twin cover it.
• The automation-degradation retry mirrors the defence mixAudioTracks already had and that this path forked without. Grouped, a dense envelope failing ffmpeg's expression evaluator used to take the whole composition's audio down; now it degrades to base volume like the ungrouped path. Good instance of rule-of-thumb "the sibling implementation already solved it".
• Structured failures + rmSync on the temp dirs — a malformed chain on a bus previously threw straight out of processCompositionAudio, past the workDir cleanup.

Why I am not stamping this yet

Seven of the eight required contexts — Build, Typecheck, Test, Test: runtime contract, Semantic PR title, both windows jobs — have never run at this head, because ci.yml filters on branches: [main] and this rung is based on wa-26b. That is by design and self-repairing (edited is in the workflow's types:, which re-fires on the base flip back to main), so there is nothing to fix.

But it means an approval from me right now would carry no CI evidence whatsoever for a +1035/-450 change to the audio render path — including the ffmpeg integration tests in this very PR, which have not executed. Happy to stamp on a re-ping once it retargets and the matrix is real; hyperframes sets require_last_push_approval: true, so a stamp given then will not silently ride onto later pushes either.

— Review by Rames (pr-review), James's assistant

[vanceingalls]

Addressed the non-blocking documentation feedback: the fallback now explains why its compensation is exact—every member is padded/trimmed to the same total duration and dropout transition is zero, so all N inputs remain active. The grouped mixer suite passes (12 tests) and the engine typecheck is green.

[vanceingalls]

CI follow-up: the sanitized-group-id collision test launches two sequential FFmpeg mixes. It used Vitest’s 5s unit-test default, which Windows consistently reached; timeout teardown then raced open WAV handles and produced EBUSY. This PR now gives that integration test a 30s budget. The focused test passes locally, and the fix is placed here where the test first enters the stack; #3447–#3455 were cascaded.

[jrusso1020]

Re-review at 4ed7032a2e. My earlier review (5003530694) was pinned to 84add1944b, which the restack replaced; two commits of substance since — beb41a04 "explain grouped mix fallback invariant" and 4ed7032a "allow grouped mixes to finish on Windows".

I had no blockers here, so this is a check on the two things that did change plus the one cross-PR item I raised.

The normalize-compensation comment is now an argument — and it holds

The fallback branch grew a rationale:

Every member is padded and trimmed to totalDuration above, and dropout_transition=0 keeps all N inputs active for that whole span. amix's default normalize therefore divides by exactly N throughout.

A docblock asserting a filter-graph invariant is the assertion under test, not evidence for it, so I read the graph it is claiming about. buildInputFilters emits, per member:

[i:a]atrim=0:<trim>,<volume>,adelay=<ms>|<ms>,apad,asetpts=N/SR/TB,atrim=0:<totalDuration>[ai]

apad then atrim=0:totalDuration makes every branch exactly totalDuration long regardless of its own start or length, so no input ever drops out, dropout_transition=0 has nothing to transition, and the divisor is N for the entire span. volume=<memberTracks.length> compensates exactly. The comment is accurate, and it is compensating on this group's member count rather than the render's global track count, which is the part that would have been silently wrong.

Worth noting this is the legacy branch only — the primary path is normalize=0 with no compensation at all, which is why pcm_f32le matters there.

The 30s timeout

keeps distinct groups isolated when their sanitized ids collide (audioMixer.grouping.test.ts:467-499) now carries }, 30_000). That is a real multi-pass ffmpeg test against vitest's 5s default, so raising it is right rather than papering over something — and it is the test I singled out last time as the assertion that would actually catch a sanitisation collision, so I would rather it ran slowly than flaked out.

The cross-PR item is closed

I flagged here that data-audio-group "reaches this file straight from the document" unvalidated, and that the same value was being interpolated into a querySelector string in resolveGroupElement on #3444 — where a " made it throw. #3444 at 62b6d35b now compares the raw attribute instead of building a selector, with a test using vo"\instance. Closed at the reader, not at one call site.

Nothing open from me on this rung.

— Review by Rames (pr-review), James's assistant

[jrusso1020]

Re-review at a678fede07 (prior review 5003703286 @ 4ed7032a2e).

Restack only — no content change on this rung. All 6 files in the PR's own file list are blob-identical across 4ed7032a2e → a678fede07, checked by blob sha per path rather than by reading the diff, so the prior review transfers exactly.

The finding it closed stands: the new amix comment is an argument, so I checked it against the filter graph it describes — apad plus atrim=0:totalDuration on every branch means no input drops out, the divisor stays N throughout, and volume=N compensates exactly.

No blockers.

— Review by Rames (pr-review), James's assistant

[jrusso1020]

Approving at a678fede07, the exact head of my re-review above — that review is the audit; this is the stamp <@vanceingalls> asked for in Slack.

No blockers on the code. The one nit I raised on #3445 (the mock gain defaulting to 1, so the toBe(1) solo assertions are weaker than they read) is non-blocking and that code is deleted at #3454.

Scope of this approval, stated so it is checkable: it covers a678fede073b639caf9367b8309817fe2ff00a2b only. main runs require_last_push_approval: true, so if this head moves the PR returns to REVIEW_REQUIRED rather than carrying my stamp forward — re-ping me and I will re-review at the new head.

— Review by Rames (pr-review), James's assistant

The base branch was changed.

[miguel-heygen]

Independent exact-head pass at a678fed.\n\nI audited the grouped render boundary: members resolve through the compiler stamp, group filenames are collision-safe, every branch is padded and trimmed to the same composition span before the normalize fallback, and the float intermediate preserves headroom until group FX/fader processing. Structured failures, browser leases, page handles, filter-script directories, and canonical producer work directories all have cleanup ownership; the canonical producer also removes its outer work tree unconditionally.\n\nNon-blocking follow-up: audioMixer.ts:1497-1498 rethrows a group AudioFxRenderError before the function-local workDir cleanup at 1517-1520. The canonical producer path is still safe because renderOrchestrator owns an outer unconditional cleanup, but a direct engine caller can retain its supplied work directory on that rare fatal path. Routing the rethrow through a local cleanup would make the exported function self-consistent.\n\nAll current required checks are green.\n\nVerdict: APPROVE\nReasoning: Group summing, FX, automation, gain staging, collision handling, and production cleanup are sound; the remaining direct-caller cleanup edge does not affect the canonical render path.\n\n— Magi