From 99a78f3b8df9b493fb181bb73baff46ea1297833 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Thu, 13 Aug 2026 22:46:37 +0200 Subject: [PATCH] fix(recording): open the recording a failed stop left playable A failed stop stopped meaning a lost take the moment the Windows helper began writing fragmented MP4 (a6795d23), and nothing on the Electron side was told. The stop handler still tears the recording down and answers "The recording could not be saved" -- which is now false. The bytes are there, indexed, and play. Measured on installed 1.9.5-rc.1: kill wgc-capture.exe mid-recording, which is what the shutdown watchdog does via TerminateProcess in #252 / #292 / #327, and the file left behind holds 41 moof+mdat fragments with mvex present and no mfra. ffprobe reads 41.0s / 2460 packets at 1920x1080, and `ffmpeg -i f -f null -` decodes it end to end, exit 0, zero errors. Truncating the pre-fMP4 container at the same fraction leaves 59.5 MB no demuxer will touch; the fragmented one at 60% still plays 29s. The app threw the good one away anyway. So the failed-stop branch now asks whether the file is worth keeping instead of assuming it is not, and falls through into the ordinary save path when it is -- same manifest, same cursor telemetry, same media links, same editor. No new UI: from the user's side the recording simply opens, minus at most the last incomplete fragment. The question is answered by the `container` field the helper has been reporting since a6795d23 and nobody read. That is the only thing that can answer it: the fragmented sink degrades to the plain one rather than failing a recording, so the flavour is a per-run outcome, and a plain MP4 killed before Finalize() really is unreadable. Absent, as from any older helper, is not fragmented. Gated on the helper actually being dead. `exited: false` means it survived even the forced kill, and such a process still holds the MP4 open and may still be appending; handing that to the editor would trade an honest failure for a sharing violation on a moving file. The predicate lives in nativeWindowsCaptureStop.ts, next to the rest of the stop logic and for the same reason: handlers.ts calls app.getPath() at import time, so nothing in it can be reached from a test. It shares its size floor with the cleanup that deletes stubs, so the two agree by construction rather than by comment -- nothing is recovered that the tidy-up would have deleted, and nothing deleted that this would keep. Windows only. macOS fragments too and needs the same treatment, but it also has no already-exited fast path and an unguarded stdin write, so it is its own change. Linux writes a plain container on purpose and has nothing to salvage. --- electron/ipc/handlers.ts | 124 +++++++++++++----- .../nativeWindowsCaptureStop.test.ts | 41 ++++++ .../recording/nativeWindowsCaptureStop.ts | 37 ++++++ src/hooks/useScreenRecorder.ts | 6 +- 4 files changed, 172 insertions(+), 36 deletions(-) diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 14262b11a..d4c094869 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -73,6 +73,8 @@ import { findPipeWireCursorHelperPath } from "../native-bridge/cursor/recording/ import type { CursorRecordingSession } from "../native-bridge/cursor/recording/session"; import { toHelperRect } from "../native-bridge/helperCoordinates"; import { + isSalvageableFragmentedCapture, + NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES, terminateNativeWindowsCapture, waitForNativeWindowsCaptureStop, } from "../recording/nativeWindowsCaptureStop"; @@ -538,6 +540,12 @@ let nativeWindowsCursorRecordingStartMs = 0; let nativeWindowsPauseStartedAtMs: number | null = null; let nativeWindowsPauseRanges: Array<{ startMs: number; endMs: number }> = []; let nativeWindowsIsPaused = false; +/** + * The MP4 flavour the helper reported for THIS run, or null if it never said. + * Read at stop, not for reporting: it is what decides whether a capture that + * failed to finalize still left a playable file behind. + */ +let nativeWindowsCaptureContainer: string | null = null; /** Cuts a surviving helper's output loose so it cannot pollute the next recording. */ let nativeWindowsCaptureDrainCleanup: (() => void) | null = null; @@ -558,14 +566,17 @@ function resetNativeWindowsCaptureState() { nativeWindowsPauseStartedAtMs = null; nativeWindowsPauseRanges = []; nativeWindowsIsPaused = false; + nativeWindowsCaptureContainer = null; } -/** - * An MP4 the helper never indexed is a few bytes of header at most. Anything - * larger might be a real recording, and deleting one of those to tidy up after - * a failed stop is a far worse outcome than leaving a stray file behind. - */ -const NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES = 64 * 1024; +/** Reads the file, then defers the judgement to the tested predicate. */ +async function salvageNativeWindowsFragmentedCapture(screenVideoPath: string | null) { + if (!screenVideoPath) { + return false; + } + const stats = await fs.stat(screenVideoPath).catch(() => null); + return isSalvageableFragmentedCapture(nativeWindowsCaptureContainer, stats?.size ?? null); +} /** * Best-effort removal of the files a failed or discarded native Windows capture @@ -1344,6 +1355,13 @@ function readNativeWindowsEncoderSelection(output: string) { try { return JSON.parse(lastLine) as { video?: string; + // Which MP4 flavour the helper actually wrote, `fragmented-mp4` or + // `mp4`. It reports this because the fragmented sink degrades to the + // plain one rather than failing a recording, so the flavour is a + // per-run outcome and not a property of the version. This is the only + // thing that can answer "was this file supposed to survive a kill?", + // which is what `salvageNativeWindowsFragmentedCapture` asks. + container?: string; preferSoftwareEncoder?: boolean; }; } catch { @@ -2433,6 +2451,9 @@ export function registerIpcHandlers( : 0; const webcamFormat = readNativeWindowsWebcamFormat(nativeWindowsCaptureOutput); const encoderSelection = readNativeWindowsEncoderSelection(nativeWindowsCaptureOutput); + // Captured now because stop may have no helper left to ask. A helper + // killed mid-recording is exactly the case where this matters most. + nativeWindowsCaptureContainer = encoderSelection?.container ?? null; console.info("[native-wgc] capture started", { captureStartedAtMs, cursorOffsetMs: nativeWindowsCursorOffsetMs, @@ -2742,6 +2763,11 @@ export function registerIpcHandlers( } } + // Set when the helper failed its stop handshake but left a playable + // fragmented file. Reported so a bug report can tell a clean stop from a + // recovered one; the user-facing path is deliberately identical. + let recovered = false; + try { completeNativeWindowsCursorPauseRange(); const stopPromise = waitForNativeWindowsCaptureStop({ @@ -2763,35 +2789,62 @@ export function registerIpcHandlers( if (!stopResult.exited) { detachNativeWindowsCaptureOutputDrain(); } - await stopCursorRecording(); - // Same as the discard path. `startCursorRecording` clears this on - // the next recording anyway, so this is not what keeps the samples - // from being written next to someone else's video -- it just stops - // a lost take's telemetry from sitting in memory until then. - pendingCursorRecordingData = null; - // The helper never announced a finalized file, so what is on disk - // is almost certainly an unindexed stub, and leaving those behind - // just accumulates unplayable recordings the user cannot explain. - // Almost: size-gate it, because throwing away a recording to tidy - // up after a failed stop is the worse mistake of the two. - await removeNativeWindowsCaptureOutputs(preferredPath, preferredWebcamPath, { - onlyIfUnusable: true, - }); - // The helper log goes to console/diagnostics above, not into this - // string: it ends up in a toast, and pasting an entire capture log - // into the HUD tells the user nothing they can act on. - return { - success: false, - reason: stopResult.reason, - error: - stopResult.reason === "stop-timeout" - ? "Timed out waiting for native Windows capture to stop. The recording could not be saved." - : stopResult.message.split(/\r?\n/).filter(Boolean).at(-1) || - "Native Windows capture failed.", - }; + + // A failed stop stopped meaning a lost take when the helper started + // writing fragmented MP4. The file on disk is already playable, so + // the only thing standing between the user and their recording is + // this function deciding to throw it away and say so. Fall through + // into the normal save path instead: same manifest, same media + // links, same editor. From the user's side it simply worked, minus + // at most the last incomplete fragment. + // + // Only once the helper is actually dead. `exited: false` means it + // survived even the forced kill -- stuck somewhere `TerminateProcess` + // could not reach -- and on Windows such a process still holds the + // MP4 open and may still be appending to it. Handing that file to + // the editor trades an honest failure for a sharing violation on a + // file that is still moving, so a wedged helper keeps the old answer. + if (stopResult.exited && (await salvageNativeWindowsFragmentedCapture(preferredPath))) { + console.warn("[native-wgc] stop failed but the fragmented output is playable", { + reason: stopResult.reason, + path: preferredPath, + }); + recovered = true; + } else { + await stopCursorRecording(); + // Same as the discard path. `startCursorRecording` clears this on + // the next recording anyway, so this is not what keeps the samples + // from being written next to someone else's video -- it just stops + // a lost take's telemetry from sitting in memory until then. + pendingCursorRecordingData = null; + // Reaching here means the container was the plain one, whose only + // index is written by the `Finalize()` this stop never reached, so + // what is on disk really is an unindexed stub and leaving those + // behind just accumulates unplayable recordings the user cannot + // explain. Size-gate it anyway: throwing away a recording to tidy + // up after a failed stop is the worse mistake of the two, and the + // gate is the same one the salvage check above uses. + await removeNativeWindowsCaptureOutputs(preferredPath, preferredWebcamPath, { + onlyIfUnusable: true, + }); + // The helper log goes to console/diagnostics above, not into this + // string: it ends up in a toast, and pasting an entire capture log + // into the HUD tells the user nothing they can act on. + return { + success: false, + reason: stopResult.reason, + error: + stopResult.reason === "stop-timeout" + ? "Timed out waiting for native Windows capture to stop. The recording could not be saved." + : stopResult.message.split(/\r?\n/).filter(Boolean).at(-1) || + "Native Windows capture failed.", + }; + } } - const screenVideoPath = stopResult.screenVideoPath || preferredPath; + // Only a successful stop names the file; the salvage path above falls + // through with `ok: false` and nothing but the path we asked for. + const screenVideoPath = (stopResult.ok ? stopResult.screenVideoPath : null) || preferredPath; if (!screenVideoPath) { throw new Error("Native Windows capture did not return an output path."); } @@ -2833,7 +2886,10 @@ export function registerIpcHandlers( success: true, path: screenVideoPath, session, - message: "Native Windows recording session stored successfully", + recovered, + message: recovered + ? "Native Windows recording recovered from a failed stop" + : "Native Windows recording session stored successfully", }; } catch (error) { console.error("Failed to stop native Windows recording:", error); diff --git a/electron/recording/nativeWindowsCaptureStop.test.ts b/electron/recording/nativeWindowsCaptureStop.test.ts index 7ba34317c..900df9e16 100644 --- a/electron/recording/nativeWindowsCaptureStop.test.ts +++ b/electron/recording/nativeWindowsCaptureStop.test.ts @@ -3,6 +3,8 @@ import { EventEmitter } from "node:events"; import { PassThrough, Writable } from "node:stream"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { + isSalvageableFragmentedCapture, + NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES, readStoppedPath, terminateNativeWindowsCapture, waitForNativeWindowsCaptureStop, @@ -77,6 +79,45 @@ describe("readStoppedPath", () => { }); }); +describe("isSalvageableFragmentedCapture", () => { + const big = NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES * 8; + + // The whole point of the fragmented container, and the case that used to be + // deleted-or-disowned while the file on disk played perfectly (#252). + it("keeps a fragmented capture whose stop never finalized", () => { + expect(isSalvageableFragmentedCapture("fragmented-mp4", big)).toBe(true); + }); + + // The ablation. Same size, same failed stop, no index anywhere in the file: + // this one really is lost, and saying otherwise would open an empty editor. + it("does not pretend a plain MP4 survived the same failure", () => { + expect(isSalvageableFragmentedCapture("mp4", big)).toBe(false); + }); + + it("rejects a fragmented file too small to hold a complete fragment", () => { + expect( + isSalvageableFragmentedCapture("fragmented-mp4", NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES - 1), + ).toBe(false); + }); + + it("takes the floor itself as salvageable", () => { + expect( + isSalvageableFragmentedCapture("fragmented-mp4", NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES), + ).toBe(true); + }); + + // A helper predating the fragmented sink reports no container at all. Absent + // is not fragmented -- guessing here would resurrect the total loss. + it("refuses to guess when the helper never reported a container", () => { + expect(isSalvageableFragmentedCapture(null, big)).toBe(false); + expect(isSalvageableFragmentedCapture(undefined, big)).toBe(false); + }); + + it("rejects a file that is not there at all", () => { + expect(isSalvageableFragmentedCapture("fragmented-mp4", null)).toBe(false); + }); +}); + describe("waitForNativeWindowsCaptureStop", () => { it("resolves with the path the helper reported", async () => { let output = "Recording started\n"; diff --git a/electron/recording/nativeWindowsCaptureStop.ts b/electron/recording/nativeWindowsCaptureStop.ts index 95da2c19a..badec52d0 100644 --- a/electron/recording/nativeWindowsCaptureStop.ts +++ b/electron/recording/nativeWindowsCaptureStop.ts @@ -33,6 +33,43 @@ export const NATIVE_WINDOWS_CAPTURE_STOP_TIMEOUT_MS = 60_000; /** How long a killed helper gets to actually die before we escalate. */ const NATIVE_WINDOWS_CAPTURE_KILL_GRACE_MS = 2_000; +/** What `mf_encoder.h`'s `kContainerFormatFragmentedMp4` puts on the wire. */ +export const NATIVE_WINDOWS_FRAGMENTED_CONTAINER = "fragmented-mp4"; + +/** + * An MP4 the helper never indexed is a few bytes of header at most. Anything + * larger might be a real recording, and deleting one of those to tidy up after + * a failed stop is a far worse outcome than leaving a stray file behind. + */ +export const NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES = 64 * 1024; + +/** + * Did a stop that failed its handshake still leave a recording worth opening? + * + * Only the fragmented container can. A plain MP4 writes its one index in + * `Finalize()`, so a helper that never reached it leaves bytes no demuxer can + * read — the total loss issues #252 / #292 / #327 reported. A fragmented one + * writes `moov` up front and a self-describing `moof`+`mdat` pair about every + * second, so the same file plays up to the last complete fragment with nothing + * else needed. Which one a run used is not a property of the version: the + * fragmented sink degrades to the plain one rather than failing a recording, + * which is exactly why the helper reports the flavour it settled on. + * + * The size floor is shared with the cleanup that deletes unusable leftovers, so + * the two agree by construction: nothing is recovered that the tidy-up would + * have judged a stub, and nothing is deleted that this would have called a + * recording. + */ +export function isSalvageableFragmentedCapture( + container: string | null | undefined, + sizeBytes: number | null, +): boolean { + if (container !== NATIVE_WINDOWS_FRAGMENTED_CONTAINER) { + return false; + } + return sizeBytes !== null && sizeBytes >= NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES; +} + const RECORDING_STOPPED_PATTERN = /Recording stopped\. Output path: (.+)/; const STOP_TIMEOUT_EVENT_PATTERN = /"event":"stop-timeout"[^\n]*"step":"([^"]+)"/; diff --git a/src/hooks/useScreenRecorder.ts b/src/hooks/useScreenRecorder.ts index 9724c27ef..a6da24d74 100644 --- a/src/hooks/useScreenRecorder.ts +++ b/src/hooks/useScreenRecorder.ts @@ -591,8 +591,10 @@ export function useScreenRecorder(): UseScreenRecorderReturn { // disagreeing about whether anything was recording: the HUD kept // showing a stop button, and pressing it sent a second stop that // came back "Native Windows capture is not running." (issue #252). - // The recording is already lost either way -- what the user needs - // is to be able to start a new one. + // Reaching here now means the take really is unreadable -- a failed + // stop that left a playable fragmented file comes back `success` + // with a session and takes the editor path below, so this branch no + // longer decides the fate of a recoverable recording. clearNativeRecordingState(); return true; }