fix(export): make audio finalization timeouts progress-aware#283
Merged
webadderall merged 5 commits intowebadderallorg:mainfrom Apr 20, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new finalization timeout module with workload- and progress-aware idle watchdogs and tests; replaces fixed finalization timers in both video exporters with duration/workload-aware timeouts and optional progress-aware watchdog wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Exporter
participant withFinalizer as Finalizer
participant Muxer
participant Watchdog
Exporter->>withFinalizer: call withFinalizationTimeout(promise, stage, workload, effectiveDurationSec, progressAware)
Note right of Exporter: compute totalTimeout via getExportFinalizationTimeoutMs(...)
withFinalizer->>Watchdog: start idleTimer = getExportFinalizationIdleTimeoutMs(...) (if progressAware)
withFinalizer->>Muxer: run finalization step (workload: audio/default)
Muxer-->>Exporter: report progress
Exporter->>Watchdog: refreshProgress()
Watchdog-->>withFinalizer: idle window expired -> reject ("without observable progress")
Muxer-->>withFinalizer: finalization completes -> resolve, clear timers, onWatchdogChanged(null)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/exporter/finalizationTimeout.ts (1)
18-29: Nit: redundant null-coalescing andMath.max(0, …)after the finite+positive guard.Once execution passes the
Number.isFinite(effectiveDurationSec) || (effectiveDurationSec ?? 0) <= 0guard on line 18,effectiveDurationSecis known to be a finite number that is strictly greater than zero. The latereffectiveDurationSec ?? 0(line 24) andMath.max(0, …)can never take their fallback branches.♻️ Optional simplification
- if (!Number.isFinite(effectiveDurationSec) || (effectiveDurationSec ?? 0) <= 0) { + if ( + typeof effectiveDurationSec !== "number" || + !Number.isFinite(effectiveDurationSec) || + effectiveDurationSec <= 0 + ) { return BASE_FINALIZATION_TIMEOUT_MS; } // Audio finalization work scales with the output timeline, so long exports need // more headroom without making unrelated finalization hangs wait longer. - const safeEffectiveDurationSec = Math.max(0, effectiveDurationSec ?? 0); const adaptiveTimeoutMs = BASE_FINALIZATION_TIMEOUT_MS + - safeEffectiveDurationSec * AUDIO_TIMEOUT_HEADROOM_PER_OUTPUT_SECOND_MS; + effectiveDurationSec * AUDIO_TIMEOUT_HEADROOM_PER_OUTPUT_SECOND_MS; return Math.min(adaptiveTimeoutMs, MAX_AUDIO_FINALIZATION_TIMEOUT_MS);This also lets TS narrow
effectiveDurationSectonumberinside the branch, which is more type-honest than threadingnumber | null | undefinedthroughMath.max.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/finalizationTimeout.ts` around lines 18 - 29, Remove the redundant null-coalescing and Math.max calls: after the guard that checks Number.isFinite(effectiveDurationSec) and > 0, treat effectiveDurationSec as a finite number and compute adaptiveTimeoutMs directly as BASE_FINALIZATION_TIMEOUT_MS + effectiveDurationSec * AUDIO_TIMEOUT_HEADROOM_PER_OUTPUT_SECOND_MS, then return Math.min(adaptiveTimeoutMs, MAX_AUDIO_FINALIZATION_TIMEOUT_MS); remove the safeEffectiveDurationSec and any uses of (effectiveDurationSec ?? 0) or Math.max(0, …) so TS can narrow effectiveDurationSec to number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/exporter/finalizationTimeout.ts`:
- Around line 18-29: Remove the redundant null-coalescing and Math.max calls:
after the guard that checks Number.isFinite(effectiveDurationSec) and > 0, treat
effectiveDurationSec as a finite number and compute adaptiveTimeoutMs directly
as BASE_FINALIZATION_TIMEOUT_MS + effectiveDurationSec *
AUDIO_TIMEOUT_HEADROOM_PER_OUTPUT_SECOND_MS, then return
Math.min(adaptiveTimeoutMs, MAX_AUDIO_FINALIZATION_TIMEOUT_MS); remove the
safeEffectiveDurationSec and any uses of (effectiveDurationSec ?? 0) or
Math.max(0, …) so TS can narrow effectiveDurationSec to number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abaf1300-82f5-4217-904c-77d05c616cb7
📒 Files selected for processing (4)
src/lib/exporter/finalizationTimeout.test.tssrc/lib/exporter/finalizationTimeout.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.ts
meiiie
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/finalizationTimeout.test.ts (1)
58-76: Consider expanding idle-timeout coverage.The idle helper is only asserted for
{default},{audio, 1_200}, and{audio, 2_700}. Two gaps worth covering while you're here:
getExportFinalizationIdleTimeoutMswith invalid audio durations (0,NaN) — symmetric to the invalid-duration case you already added forgetExportFinalizationTimeoutMs. This locks down the value that the consumer'sprogressAware && idleTimeoutMstruthy check in both exporters depends on (see the comment onvideoExporter.ts).- An end-to-end test using Vitest fake timers that exercises the watchdog path in
awaitWithFinalizationTimeoutitself: verify that no-progress causes rejection at the idle window and that calling the refresh hook resets it. That’s the behavior the PR title actually promises and it isn’t covered today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/finalizationTimeout.test.ts` around lines 58 - 76, Add two new tests in finalizationTimeout.test.ts: (1) assert getExportFinalizationIdleTimeoutMs returns the default idle timeout for invalid audio durations (e.g., effectiveDurationSec: 0 and effectiveDurationSec: NaN) similar to the existing invalid-duration tests for getExportFinalizationTimeoutMs; reference getExportFinalizationIdleTimeoutMs in these assertions. (2) add an end-to-end Vitest fake-timers test that exercises awaitWithFinalizationTimeout: use vi.useFakeTimers(), call awaitWithFinalizationTimeout with an idle window derived for a workload (via getExportFinalizationIdleTimeoutMs), verify that advancing timers past the idle window without calling the provided refresh hook causes the promise to reject, and verify that calling the refresh hook before the window expires resets the watchdog so advancing timers further does not reject; reference awaitWithFinalizationTimeout and the refreshProgress callback to locate the implementation to test. Ensure to restore real timers after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 379-444: The current awaitWithFinalizationTimeout implementation
uses a truthy check (progressAware && idleTimeoutMs) which breaks when
getExportFinalizationIdleTimeoutMs returns 0; change the guard to an explicit
null/number check so the idle watchdog is enabled for 0 (e.g. progressAware &&
idleTimeoutMs !== null && idleTimeoutMs !== undefined), and ensure idleTimeoutMs
is treated as a number where used. Also consider deduplicating this logic by
extracting the timer/watchdog plumbing into a shared helper (e.g.
finalizationTimeout.withFinalizationTimeout) used by both
VideoExporter.awaitWithFinalizationTimeout and
ModernVideoExporter.awaitWithFinalizationTimeout so callers can receive the
returned watchdog (to set activeFinalizationProgressWatchdog and allow
reportFinalizingProgress to call refreshProgress) and the tests cover the
behavior once.
---
Nitpick comments:
In `@src/lib/exporter/finalizationTimeout.test.ts`:
- Around line 58-76: Add two new tests in finalizationTimeout.test.ts: (1)
assert getExportFinalizationIdleTimeoutMs returns the default idle timeout for
invalid audio durations (e.g., effectiveDurationSec: 0 and effectiveDurationSec:
NaN) similar to the existing invalid-duration tests for
getExportFinalizationTimeoutMs; reference getExportFinalizationIdleTimeoutMs in
these assertions. (2) add an end-to-end Vitest fake-timers test that exercises
awaitWithFinalizationTimeout: use vi.useFakeTimers(), call
awaitWithFinalizationTimeout with an idle window derived for a workload (via
getExportFinalizationIdleTimeoutMs), verify that advancing timers past the idle
window without calling the provided refresh hook causes the promise to reject,
and verify that calling the refresh hook before the window expires resets the
watchdog so advancing timers further does not reject; reference
awaitWithFinalizationTimeout and the refreshProgress callback to locate the
implementation to test. Ensure to restore real timers after the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97428f2e-d8fc-4f75-b3f6-08b21941ab3f
📒 Files selected for processing (4)
src/lib/exporter/finalizationTimeout.test.tssrc/lib/exporter/finalizationTimeout.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/exporter/finalizationTimeout.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/exporter/videoExporter.ts (1)
880-887:⚠️ Potential issue | 🟠 MajorOnly refresh the idle watchdog when progress actually advances.
Line 885 refreshes on every finalization progress event, so repeated callbacks with the same
renderProgress/audioProgresscan keep a stalled audio finalization alive indefinitely. Track the last observed finalization progress and refresh only when the normalized value increases.🐛 Suggested direction
private reportFinalizingProgress( totalFrames: number, renderProgress: number, audioProgress?: number, ) { - this.activeFinalizationProgressWatchdog?.refreshProgress(); + const safeRenderProgress = Math.max(0, Math.min(renderProgress, 100)); + const safeAudioProgress = + typeof audioProgress === "number" + ? Math.max(0, Math.min(audioProgress, 1)) + : undefined; + const progressed = + safeRenderProgress > this.lastFinalizationRenderProgress || + (safeAudioProgress !== undefined && + safeAudioProgress > this.lastFinalizationAudioProgress); + + if (progressed) { + this.activeFinalizationProgressWatchdog?.refreshProgress(); + this.lastFinalizationRenderProgress = safeRenderProgress; + if (safeAudioProgress !== undefined) { + this.lastFinalizationAudioProgress = safeAudioProgress; + } + } this.reportProgress(totalFrames, totalFrames, "finalizing", renderProgress, audioProgress); }This also needs matching private fields initialized to sentinel values and reset in
cleanup().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 880 - 887, The routine reportFinalizingProgress is refreshing this.activeFinalizationProgressWatchdog on every call even when renderProgress/audioProgress haven't advanced; add private fields (e.g., lastFinalizingRenderProgress = -1 and lastFinalizingAudioProgress = -1) to track the last normalized progress, update them only when the new normalized renderProgress or audioProgress is greater than the stored value, and call this.activeFinalizationProgressWatchdog?.refreshProgress() only when at least one progressed; also reset those sentinel fields in cleanup() so subsequent runs start fresh.src/lib/exporter/modernVideoExporter.ts (1)
1154-1160:⚠️ Potential issue | 🟠 MajorGate watchdog refreshes on monotonic progress.
Line 1159 refreshes the idle watchdog for every finalization callback. If an audio stage repeatedly emits the same progress value, the watchdog will not detect a no-progress stall. Refresh only when normalized
renderProgressoraudioProgressadvances.🐛 Suggested direction
private reportFinalizingProgress( totalFrames: number, renderProgress: number, audioProgress?: number, ) { - this.activeFinalizationProgressWatchdog?.refreshProgress(); + const safeRenderProgress = Math.max(0, Math.min(renderProgress, 100)); + const safeAudioProgress = + typeof audioProgress === "number" + ? Math.max(0, Math.min(audioProgress, 1)) + : undefined; + const progressed = + safeRenderProgress > this.lastFinalizationRenderProgress || + (safeAudioProgress !== undefined && + safeAudioProgress > this.lastFinalizationAudioProgress); + + if (progressed) { + this.activeFinalizationProgressWatchdog?.refreshProgress(); + this.lastFinalizationRenderProgress = safeRenderProgress; + if (safeAudioProgress !== undefined) { + this.lastFinalizationAudioProgress = safeAudioProgress; + } + } this.reportProgress(totalFrames, totalFrames, "finalizing", renderProgress, audioProgress); }Add/reset the corresponding sentinel fields alongside the existing finalization state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernVideoExporter.ts` around lines 1154 - 1160, The watchdog is being refreshed every time reportFinalizingProgress is called regardless of monotonic progress; change reportFinalizingProgress to track previous normalized progress values (e.g., add class fields like lastFinalRenderProgress and lastFinalAudioProgress), compute normalized renderProgress and audioProgress, and only call this.activeFinalizationProgressWatchdog?.refreshProgress() when the normalized renderProgress or audioProgress is strictly greater than its respective stored sentinel; update the sentinels when you observe advancement, and still call this.reportProgress(totalFrames, totalFrames, "finalizing", renderProgress, audioProgress) on every invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 1154-1160: The watchdog is being refreshed every time
reportFinalizingProgress is called regardless of monotonic progress; change
reportFinalizingProgress to track previous normalized progress values (e.g., add
class fields like lastFinalRenderProgress and lastFinalAudioProgress), compute
normalized renderProgress and audioProgress, and only call
this.activeFinalizationProgressWatchdog?.refreshProgress() when the normalized
renderProgress or audioProgress is strictly greater than its respective stored
sentinel; update the sentinels when you observe advancement, and still call
this.reportProgress(totalFrames, totalFrames, "finalizing", renderProgress,
audioProgress) on every invocation.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 880-887: The routine reportFinalizingProgress is refreshing
this.activeFinalizationProgressWatchdog on every call even when
renderProgress/audioProgress haven't advanced; add private fields (e.g.,
lastFinalizingRenderProgress = -1 and lastFinalizingAudioProgress = -1) to track
the last normalized progress, update them only when the new normalized
renderProgress or audioProgress is greater than the stored value, and call
this.activeFinalizationProgressWatchdog?.refreshProgress() only when at least
one progressed; also reset those sentinel fields in cleanup() so subsequent runs
start fresh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69741ce9-e569-4b51-a287-276aebc1bd8a
📒 Files selected for processing (4)
src/lib/exporter/finalizationTimeout.test.tssrc/lib/exporter/finalizationTimeout.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/exporter/finalizationTimeout.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/exporter/videoExporter.ts (1)
712-729:⚠️ Potential issue | 🟠 MajorKeep the native session cancellable until finalization resolves.
Line 713 clears
this.nativeExportSessionIdbefore the timednativeVideoExportFinishcall. If the new audio-aware timeout rejects,cleanup()can no longer callnativeVideoExportCancel, leaving the native export session/process running.🛠️ Proposed fix
const sessionId = this.nativeExportSessionId; - this.nativeExportSessionId = null; const result = await this.awaitWithFinalizationTimeout( window.electronAPI.nativeVideoExportFinish(sessionId, { audioMode: audioPlan.audioMode, @@ "native export finalization", audioPlan.audioMode === "none" ? "default" : "audio", ); + this.nativeExportSessionId = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 712 - 729, The code clears this.nativeExportSessionId before awaiting awaitWithFinalizationTimeout(window.electronAPI.nativeVideoExportFinish(...)), which prevents cleanup() from calling nativeVideoExportCancel with the correct id if the timed call rejects; keep the native session cancellable by preserving the session id until the finalization promise settles — e.g., store sessionId in a local const (sessionId already exists) and only set this.nativeExportSessionId = null after the await resolves or in the finally block, or modify cleanup()/nativeVideoExportCancel to accept the stored local sessionId so nativeVideoExportCancel can be invoked even if this.nativeExportSessionId was cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/finalizationTimeout.ts`:
- Around line 78-87: The renderProgress clamping at normalizedRenderProgress
lacks a finite-number guard and can become NaN; change its computation to mirror
audioProgress handling by checking typeof renderProgress === "number" &&
Number.isFinite(renderProgress) and only then clamp to Math.max(0,
Math.min(renderProgress, 100)), otherwise treat it as null (or fallback to
state.lastRenderProgress when computing nextRenderProgress); update
normalizedRenderProgress and nextRenderProgress logic (variables:
normalizedRenderProgress, nextRenderProgress, state.lastRenderProgress) so NaN
values don't overwrite the last known progress.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 1154-1156: The cleanup path nulls audioProcessor without
cancelling in-flight work, allowing timed-out audio finalization to continue;
update the cleanup() logic (and the branch that handles withFinalizationTimeout
/ activeFinalizationProgressWatchdog) to, if this.audioProcessor is non-null,
call this.audioProcessor.cancel() before nulling it so any running audio
finalization is signalled to stop; ensure the cancel() call is safe to call
unconditionally (AudioProcessor.cancel()), then clear
activeFinalizationProgressWatchdog and set this.audioProcessor = null.
---
Outside diff comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 712-729: The code clears this.nativeExportSessionId before
awaiting
awaitWithFinalizationTimeout(window.electronAPI.nativeVideoExportFinish(...)),
which prevents cleanup() from calling nativeVideoExportCancel with the correct
id if the timed call rejects; keep the native session cancellable by preserving
the session id until the finalization promise settles — e.g., store sessionId in
a local const (sessionId already exists) and only set this.nativeExportSessionId
= null after the await resolves or in the finally block, or modify
cleanup()/nativeVideoExportCancel to accept the stored local sessionId so
nativeVideoExportCancel can be invoked even if this.nativeExportSessionId was
cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 570f32c0-cd54-4b65-9c01-12fbe8636296
📒 Files selected for processing (4)
src/lib/exporter/finalizationTimeout.test.tssrc/lib/exporter/finalizationTimeout.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/exporter/finalizationTimeout.test.ts
- src/lib/exporter/modernVideoExporter.ts
This was referenced Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make audio-heavy export finalization stages progress-aware instead of relying only on a fixed wall-clock timeout.
Root Cause
Local profiling showed that long exports spend most of their finalization time in audio work rather than muxer finalization itself. A larger fixed timeout helps, but it still treats healthy long-running audio work and a genuinely stuck finalization stage the same way.
Changes
Why This Is Safer
This narrows the follow-up fix to the bottleneck identified by profiling:
Validation
npx vitest --run src/lib/exporter/finalizationTimeout.test.tsnpx tsc --noEmitnpx @biomejs/biome check src/lib/exporter/finalizationTimeout.ts src/lib/exporter/finalizationTimeout.test.ts src/lib/exporter/videoExporter.ts src/lib/exporter/modernVideoExporter.tsnpx tsc && npx vite buildmodern-progress-aware-1776653676126.mp4legacy-progress-aware-1776653829973.mp4Related
Summary by CodeRabbit
New Features
Tests