diff --git a/electron/electron-env.d.ts b/electron/electron-env.d.ts index 6de7a3cc4..838ef6e77 100644 --- a/electron/electron-env.d.ts +++ b/electron/electron-env.d.ts @@ -179,6 +179,8 @@ interface Window { session?: import("../src/lib/recordingSession").RecordingSession; message?: string; discarded?: boolean; + /** The take ended before it was stopped, but its recording was kept. */ + warning?: string; error?: string; }>; attachNativeMacWebcamRecording: (payload: { @@ -317,6 +319,8 @@ interface Window { success: boolean; session?: RecordingSession | null; canceled?: boolean; + /** Why this recording ended before it was stopped, when it did. */ + warning?: string; }>; findRecordingCamera: (videoPath: string) => Promise<{ success: boolean; diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 4e9ba5429..5837502da 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -81,6 +81,12 @@ import { findPipeWireCursorHelperPath } from "../native-bridge/cursor/recording/ import type { CursorRecordingSession } from "../native-bridge/cursor/recording/session"; import { toHelperRect } from "../native-bridge/helperCoordinates"; import { scoreDeviceNameMatch } from "../recording/deviceNameMatching"; +import { + type NativeMacCaptureExit, + nativeMacDiscardTargets, + sendNativeMacStopCommand, + waitForNativeMacCaptureStop, +} from "../recording/nativeMacCaptureStop"; import { isSalvageableFragmentedCapture, NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES, @@ -765,6 +771,25 @@ let nativeMacCursorRecordingStartMs = 0; let nativeMacPauseStartedAtMs: number | null = null; let nativeMacPauseRanges: Array<{ startMs: number; endMs: number }> = []; let nativeMacIsPaused = false; +/** + * How each macOS helper exited, recorded by its output drain on `close` — the + * point at which its output has been read in full. + */ +const nativeMacCaptureExits = new WeakMap(); +/** + * Each macOS helper's own output. The shared `nativeMacCaptureOutput` belongs to + * the current take; a helper whose stop timed out keeps talking after the next + * take has started, and its late `recording-stopped` must not settle that take. + */ +const nativeMacCaptureOutputs = new WeakMap(); +/** + * Why the last macOS take ended before it was stopped, keyed by the file it was + * kept in. Handed to whatever opens that recording next — the editor or the CLI — + * because the HUD that ran the stop closes as the editor opens. + */ +let nativeMacRecordingWarning: { screenVideoPath: string; message: string } | null = null; +/** True while stop-native-mac-recording runs: the take is ending on purpose. */ +let nativeMacStopInFlight = false; // Global frame of the region captured by the SCK helper (see getSelectedSourceBounds). let activeMacCaptureBounds: Rectangle | null = null; let linuxNativeCaptureSession: LinuxNativeCaptureSession | null = null; @@ -1488,40 +1513,67 @@ function inspectNativeMacCaptureOutput() { function attachNativeMacCaptureOutputDrain( proc: ChildProcessWithoutNullStreams, - onErrorDuringCapture: () => void, + onTakeEnded: () => void, ) { let lineBuffer = ""; - // Hooked here rather than on `nativeMacCaptureEvents`, which the stop wait + // Hooked here rather than on `nativeMacCaptureEvents`, which the start wait // replays from the buffer: the drain sees each line once, live. - const watchForMidCaptureError = createNativeMacMidCaptureErrorWatch( - () => nativeMacCaptureProcess === proc, - onErrorDuringCapture, + const watchLiveTake = createNativeMacMidCaptureErrorWatch( + () => nativeMacCaptureProcess === proc && !nativeMacStopInFlight, + onTakeEnded, ); const drain = (chunk: Buffer) => { const text = chunk.toString(); - nativeMacCaptureOutput += text; + nativeMacCaptureOutputs.set(proc, (nativeMacCaptureOutputs.get(proc) ?? "") + text); + // Only the current take's helper feeds the shared buffer and event bus that the + // start wait, the microphone check and the diagnostics bundle read. + const isCurrent = nativeMacCaptureProcess === proc; + if (isCurrent) { + nativeMacCaptureOutput += text; + } lineBuffer += text; const lines = lineBuffer.split(/\r?\n/); lineBuffer = lines.pop() ?? ""; for (const line of lines) { const event = tryParseNativeHelperEvent(line.trim()); if (event) { - dispatchNativeMacHelperEvent(event); - watchForMidCaptureError(event); + if (isCurrent) { + dispatchNativeMacHelperEvent(event); + } + watchLiveTake(event); } } }; - const cleanup = () => { + // Registered right after spawn, before the stop wait can listen for `close`, so + // the wait always finds the exit recorded when its own listener runs. + const onClose = (code: number | null, signal: NodeJS.Signals | null) => { + nativeMacCaptureExits.set(proc, { code, signal }); proc.stdout.off("data", drain); proc.stderr.off("data", drain); - proc.off("close", cleanup); - proc.off("error", cleanup); + watchLiveTake.exited(); }; proc.stdout.on("data", drain); proc.stderr.on("data", drain); - proc.once("close", cleanup); - proc.once("error", cleanup); + proc.once("close", onClose); + // A ChildProcess `error` with no listener throws in the main process. + proc.on("error", (error) => { + console.warn("[native-sck] helper process error:", error); + }); + // `sendNativeMacStopCommand` checks the pipe first, but the helper can still die + // between that check and the write. The main-process guard would swallow the + // EPIPE; a listener here keeps it from being raised as uncaught at all. + proc.stdin.on("error", (error) => { + console.warn("[native-sck] helper command pipe error:", error); + }); + // The output pipes too, as the Windows drain does: the guard only swallows a few + // codes, and any other stream error would take the main process down. + proc.stdout.on("error", (error) => { + console.warn("[native-sck] helper stdout error:", error); + }); + proc.stderr.on("error", (error) => { + console.warn("[native-sck] helper stderr error:", error); + }); } function waitForNativeMacCaptureStart(proc: ChildProcessWithoutNullStreams) { @@ -1571,64 +1623,6 @@ function waitForNativeMacCaptureStart(proc: ChildProcessWithoutNullStreams) { }); } -function waitForNativeMacCaptureStop(proc: ChildProcessWithoutNullStreams) { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - cleanup(); - reject( - new Error( - `Timed out waiting for native macOS capture to stop. Output path: ${ - nativeMacCaptureTargetPath ?? "unknown" - }. Output: ${nativeMacCaptureOutput.trim()}`, - ), - ); - }, 30_000); - - const inspect = (event: Record) => { - if (event.event === "recording-stopped") { - cleanup(); - resolve(String(event.screenPath ?? nativeMacCaptureTargetPath ?? "")); - return; - } - if (event.event === "error") { - cleanup(); - reject(new Error(String(event.message ?? event.code ?? "Native macOS capture failed"))); - } - }; - - const onOutput = (event: Record) => inspect(event); - const onClose = (code: number | null) => { - if (code === 0 && nativeMacCaptureTargetPath) { - cleanup(); - resolve(nativeMacCaptureTargetPath); - return; - } - cleanup(); - reject( - new Error( - nativeMacCaptureOutput.trim() || - `Native macOS capture exited with code=${code ?? "unknown"}`, - ), - ); - }; - const onError = (error: Error) => { - cleanup(); - reject(error); - }; - const cleanup = () => { - clearTimeout(timer); - nativeMacCaptureEvents.off("helper-event", onOutput); - proc.off("close", onClose); - proc.off("error", onError); - }; - - nativeMacCaptureEvents.on("helper-event", onOutput); - proc.once("close", onClose); - proc.once("error", onError); - inspectNativeMacCaptureOutput(); - }); -} - function setCurrentRecordingSessionState(session: RecordingSession | null) { currentRecordingSession = session; currentVideoPath = session?.screenVideoPath ?? null; @@ -2821,6 +2815,8 @@ export function registerIpcHandlers( nativeMacPauseStartedAtMs = null; nativeMacPauseRanges = []; nativeMacIsPaused = false; + nativeMacStopInFlight = false; + nativeMacRecordingWarning = null; activeMacCaptureBounds = null; const cursorStartTimeMs = Date.now(); @@ -2836,8 +2832,9 @@ export function registerIpcHandlers( stdio: ["pipe", "pipe", "pipe"], }); nativeMacCaptureProcess = proc; - // Drives the renderer's own stop, the same one the tray's Stop Recording - // sends: it clears the HUD and surfaces the helper's error as the result. + // When the take ends without the user — the helper reported an error or + // exited — this drives the renderer's own stop, the same one the tray's Stop + // Recording sends: it clears the HUD and surfaces the result. attachNativeMacCaptureOutputDrain(proc, () => { const hudWindow = getMainWindow(); if (hudWindow && !hudWindow.isDestroyed()) { @@ -3200,15 +3197,18 @@ export function registerIpcHandlers( return { success: false, error: "Native macOS capture is not running." }; } + nativeMacStopInFlight = true; try { completeNativeMacCursorPauseRange(); - const stoppedPathPromise = waitForNativeMacCaptureStop(proc); - proc.stdin.write("stop\n"); - const stoppedPath = await stoppedPathPromise; - const screenVideoPath = stoppedPath || preferredPath; - if (!screenVideoPath) { - throw new Error("Native macOS capture did not return an output path."); - } + // Listen before sending, so a helper that stops at once cannot slip past. + const stopResultPromise = waitForNativeMacCaptureStop({ + proc, + targetPath: preferredPath, + readOutput: () => nativeMacCaptureOutputs.get(proc) ?? "", + readExit: () => nativeMacCaptureExits.get(proc) ?? null, + }); + sendNativeMacStopCommand(proc); + const stopResult = await stopResultPromise; if (cursorCaptureMode === "editable-overlay") { await stopCursorRecording(); @@ -3217,12 +3217,39 @@ export function registerIpcHandlers( } if (discard) { pendingCursorRecordingData = null; - await Promise.all([ - fs.rm(screenVideoPath, { force: true }), - fs.rm(`${screenVideoPath}.cursor.json`, { force: true }), - ]); + await Promise.all( + nativeMacDiscardTargets(stopResult, preferredPath).map((target) => + fs.rm(target, { force: true }), + ), + ); + if (!stopResult.ok) { + console.warn("[native-sck] discarded a take whose stop did not complete", { + reason: stopResult.reason, + message: stopResult.message, + }); + } return { success: true, discarded: true }; } + if (!stopResult.ok) { + pendingCursorRecordingData = null; + console.error("Failed to stop native macOS recording:", { + reason: stopResult.reason, + message: stopResult.message, + helperExited: stopResult.exited, + output: (nativeMacCaptureOutputs.get(proc) ?? "").trim(), + }); + return { success: false, error: stopResult.message }; + } + const screenVideoPath = stopResult.screenVideoPath; + nativeMacRecordingWarning = stopResult.warning + ? { screenVideoPath, message: stopResult.warning } + : null; + if (stopResult.warning) { + console.warn("[native-sck] the take ended before it was stopped; its recording was kept", { + warning: stopResult.warning, + path: screenVideoPath, + }); + } if (cursorCaptureMode === "editable-overlay") { compactPendingCursorTelemetryPauseRanges(nativeMacPauseRanges); @@ -3250,12 +3277,14 @@ export function registerIpcHandlers( path: screenVideoPath, session, message: "Native macOS recording session stored successfully", + ...(stopResult.warning ? { warning: stopResult.warning } : {}), }; } catch (error) { console.error("Failed to stop native macOS recording:", error); await stopCursorRecording(); return { success: false, error: error instanceof Error ? error.message : String(error) }; } finally { + nativeMacStopInFlight = false; nativeMacCaptureProcess = null; nativeMacCaptureTargetPath = null; nativeMacCaptureRecordingId = null; @@ -4254,9 +4283,14 @@ export function registerIpcHandlers( }); ipcMain.handle("get-current-recording-session", () => { - return currentRecordingSession - ? { success: true, session: currentRecordingSession } - : { success: false }; + if (!currentRecordingSession) { + return { success: false }; + } + const warning = + nativeMacRecordingWarning?.screenVideoPath === currentRecordingSession.screenVideoPath + ? nativeMacRecordingWarning.message + : undefined; + return { success: true, session: currentRecordingSession, ...(warning ? { warning } : {}) }; }); // returns the webcam path (if any) for a given screen video by diff --git a/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts b/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts index c22b9c0e9..c6958cd4b 100644 --- a/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts +++ b/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts @@ -46,4 +46,37 @@ describe("createNativeMacMidCaptureErrorWatch", () => { expect(onError).not.toHaveBeenCalled(); }); + + /** Killed or crashed: no error line ever comes, only the process closing. */ + it("fires when the helper exits in the middle of a take", () => { + const onTakeEnded = vi.fn(); + const watch = createNativeMacMidCaptureErrorWatch(() => true, onTakeEnded); + + watch({ event: "recording-started" }); + watch.exited(); + + expect(onTakeEnded).toHaveBeenCalledTimes(1); + }); + + it("leaves an exit before recording started to the start wait", () => { + const onTakeEnded = vi.fn(); + const watch = createNativeMacMidCaptureErrorWatch(() => true, onTakeEnded); + + watch.exited(); + + expect(onTakeEnded).not.toHaveBeenCalled(); + }); + + /** The exit a stop causes is the stop working, not the take ending under the user. */ + it("ignores the exit of a take that is already being stopped", () => { + const onTakeEnded = vi.fn(); + let live = true; + const watch = createNativeMacMidCaptureErrorWatch(() => live, onTakeEnded); + + watch({ event: "recording-started" }); + live = false; + watch.exited(); + + expect(onTakeEnded).not.toHaveBeenCalled(); + }); }); diff --git a/electron/ipc/nativeMacMidCaptureErrorWatch.ts b/electron/ipc/nativeMacMidCaptureErrorWatch.ts index 4c62f6b7e..6c786d3f3 100644 --- a/electron/ipc/nativeMacMidCaptureErrorWatch.ts +++ b/electron/ipc/nativeMacMidCaptureErrorWatch.ts @@ -1,24 +1,35 @@ /** - * Decides, one live helper event at a time, when the macOS helper raised an - * error mid-take. + * Decides, as the macOS helper speaks and when it exits, whether a take ended + * without the user stopping it, so the HUD can stop with it. * * The start and stop waits only listen while they are pending, so an error the * helper raises between them (a dead writer, a stream that stopped) used to sit * in the buffer until the user pressed stop, while the HUD counted on for - * minutes (issue #621). Before `recording-started` the start wait owns the - * error, and a helper that is no longer the current process has nobody to stop. + * minutes (issue #621). A helper that dies without a word — killed, crashed — is + * the same take ending, and only its process `close` shows it. + * + * Before `recording-started` the start wait owns both. A take that is no longer + * live — another helper replaced it, or a stop is already running — has nothing + * left to stop. */ export function createNativeMacMidCaptureErrorWatch( - isCurrentProcess: () => boolean, - onError: () => void, + isLiveTake: () => boolean, + onTakeEnded: () => void, ) { let recordingStarted = false; - return (event: Record) => { + const watch = (event: Record) => { if (event.event === "recording-started") { recordingStarted = true; } - if (event.event === "error" && recordingStarted && isCurrentProcess()) { - onError(); + if (event.event === "error" && recordingStarted && isLiveTake()) { + onTakeEnded(); + } + }; + /** The helper process closed. */ + const exited = () => { + if (recordingStarted && isLiveTake()) { + onTakeEnded(); } }; + return Object.assign(watch, { exited }); } diff --git a/electron/recording/nativeMacCaptureStop.test.ts b/electron/recording/nativeMacCaptureStop.test.ts new file mode 100644 index 000000000..9855b7c2d --- /dev/null +++ b/electron/recording/nativeMacCaptureStop.test.ts @@ -0,0 +1,334 @@ +import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { PassThrough, Writable } from "node:stream"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + type NativeMacCaptureExit, + nativeMacDiscardTargets, + readNativeMacHelperEvents, + readNativeMacStopOutcome, + sendNativeMacStopCommand, + waitForNativeMacCaptureStop, +} from "./nativeMacCaptureStop"; + +const TARGET = "/rec/recording-1.mp4"; + +/** What the helper prints, one JSON event per line. */ +function line(event: Record) { + return `${JSON.stringify(event)}\n`; +} + +// The messages as the Swift helper really prints them: the interruption embeds the +// writer's NSError, and a stream the system stopped reports its NSError alone. +const WRITER_DIED_MESSAGE = + 'Recording stopped: the video file could not be written (video append: Error Domain=AVFoundationErrorDomain Code=-11800 "The operation could not be completed" UserInfo={NSLocalizedFailureReason=An unknown error occurred (-16364)}).'; +const CAPTURE_STOPPED_MESSAGE = + 'Error Domain=com.apple.ScreenCaptureKit.SCStreamErrorDomain Code=-3815 "The stream was stopped by the system." UserInfo={NSLocalizedDescription=The stream was stopped by the system.}'; + +const started = line({ event: "recording-started", width: 3840, height: 2160 }); +const stopped = line({ event: "recording-stopped", screenPath: TARGET }); +const writerDied = line({ + event: "error", + code: "writer-failed-during-capture", + message: WRITER_DIED_MESSAGE, +}); +const writerFailed = line({ + event: "error", + code: "writer-failed", + message: "Error Domain=AVFoundationErrorDomain Code=-11800", +}); +const captureStopped = line({ + event: "error", + code: "capture-stopped-with-error", + message: CAPTURE_STOPPED_MESSAGE, +}); + +/** + * Stands in for openscreen-screencapturekit-helper. `exitCode`/`signalCode` are + * real `ChildProcess` properties the code under test reads, and `exit()` records + * the exit the way the output drain in handlers.ts does before `close` reaches + * any later listener. + */ +class FakeHelper extends EventEmitter { + stdout = new PassThrough(); + stderr = new PassThrough(); + stdin: Writable; + written: string[] = []; + exitCode: number | null = null; + signalCode: NodeJS.Signals | null = null; + recordedExit: NativeMacCaptureExit | null = null; + + constructor() { + super(); + const written = this.written; + this.stdin = new Writable({ + write(chunk, _encoding, callback) { + written.push(chunk.toString()); + callback(); + }, + }); + } + + exit(code: number | null, signal: NodeJS.Signals | null = null) { + this.exitCode = code; + this.signalCode = signal; + this.recordedExit = { code, signal }; + this.emit("close", code, signal); + } +} + +function asProc(helper: FakeHelper) { + return helper as unknown as ChildProcessWithoutNullStreams; +} + +let helper: FakeHelper; + +beforeEach(() => { + vi.useFakeTimers(); + helper = new FakeHelper(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +function waitForStop(readOutput: () => string, timeoutMs?: number) { + return waitForNativeMacCaptureStop({ + proc: asProc(helper), + targetPath: TARGET, + readOutput, + readExit: () => helper.recordedExit, + timeoutMs, + }); +} + +describe("readNativeMacHelperEvents", () => { + it("reads an event glued behind stderr text", () => { + const events = readNativeMacHelperEvents(`2026-09-14 helper log line${stopped}`); + expect(events).toEqual([{ event: "recording-stopped", screenPath: TARGET }]); + }); + + it("skips plain text and objects that are not events", () => { + expect(readNativeMacHelperEvents('warming up\n{"not":"an event"}\n{broken\n')).toEqual([]); + }); +}); + +describe("readNativeMacStopOutcome", () => { + it("takes the path the helper reported", () => { + expect(readNativeMacStopOutcome(started + stopped, null, "/elsewhere.mp4")).toEqual({ + ok: true, + screenVideoPath: TARGET, + }); + }); + + it("falls back to the requested path when the report names none", () => { + expect( + readNativeMacStopOutcome( + line({ event: "recording-stopped" }), + { code: 0, signal: null }, + TARGET, + ), + ).toEqual({ ok: true, screenVideoPath: TARGET }); + }); + + /** + * The shape that used to lose a finalized take: capture stopped on its own, the + * helper finished the file anyway, and the stop rejected on the earlier error. + */ + it("keeps a take whose capture stopped on its own but whose file was finalized", () => { + expect(readNativeMacStopOutcome(started + captureStopped + stopped, null, TARGET)).toEqual({ + ok: true, + screenVideoPath: TARGET, + warning: `Recording ended early (${CAPTURE_STOPPED_MESSAGE}). The part recorded until then was saved.`, + }); + }); + + it("says why the take ended rather than quoting the raw writer error", () => { + expect(readNativeMacStopOutcome(started + writerDied + writerFailed, null, TARGET)).toEqual({ + ok: false, + reason: "helper-failed", + message: WRITER_DIED_MESSAGE, + exited: false, + }); + }); + + /** The replay that used to reject a stop before the helper had even been told to stop. */ + it("does not settle on an interruption alone while the helper is still running", () => { + expect(readNativeMacStopOutcome(started + writerDied, null, TARGET)).toBeNull(); + }); + + it("keeps the old behaviour for a helper that exits 0 without a word", () => { + expect(readNativeMacStopOutcome(started, { code: 0, signal: null }, TARGET)).toEqual({ + ok: true, + screenVideoPath: TARGET, + }); + }); + + /** Exit 0 after an interruption only means the command pipe closed; nothing was finalized. */ + it("does not call exit 0 after an interruption a finished recording", () => { + expect( + readNativeMacStopOutcome(started + writerDied, { code: 0, signal: null }, TARGET), + ).toEqual({ + ok: false, + reason: "helper-failed", + message: WRITER_DIED_MESSAGE, + exited: true, + }); + }); + + it("reports a killed helper by its signal instead of dumping its log", () => { + expect(readNativeMacStopOutcome(started, { code: null, signal: "SIGKILL" }, TARGET)).toEqual({ + ok: false, + reason: "helper-failed", + message: "The recorder stopped unexpectedly (signal SIGKILL).", + exited: true, + }); + }); + + it("refuses a finalized report with no path to point at", () => { + expect( + readNativeMacStopOutcome( + line({ event: "recording-stopped" }), + { code: 0, signal: null }, + null, + ), + ).toMatchObject({ ok: false, reason: "helper-failed" }); + }); +}); + +describe("waitForNativeMacCaptureStop", () => { + it("waits for the helper to exit even when an error is already buffered", async () => { + let output = started + writerDied; + let settled = false; + const pending = waitForStop(() => output).then((result) => { + settled = true; + return result; + }); + + await vi.advanceTimersByTimeAsync(1_000); + expect(settled).toBe(false); + + output += writerFailed; + helper.exit(0); + + await expect(pending).resolves.toMatchObject({ + ok: false, + message: WRITER_DIED_MESSAGE, + exited: true, + }); + }); + + it("resolves with the finalized path once the helper exits", async () => { + let output = started; + const pending = waitForStop(() => output); + + output += stopped; + helper.exit(0); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: TARGET }); + }); + + /** Node never re-emits `close`, so a dead helper must not cost the whole timeout. */ + it("settles immediately when the helper has already exited", async () => { + helper.exit(null, "SIGKILL"); + + const result = await waitForStop(() => started); + + expect(result).toEqual({ + ok: false, + reason: "helper-failed", + message: "The recorder stopped unexpectedly (signal SIGKILL).", + exited: true, + }); + expect(vi.getTimerCount()).toBe(0); + }); + + it("keeps a finalized recording when the helper never exits", async () => { + const pending = waitForStop(() => started + stopped, 30_000); + + await vi.advanceTimersByTimeAsync(30_000); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: TARGET }); + }); + + it("reports a timeout, without claiming the helper exited, when it never finishes", async () => { + const pending = waitForStop(() => started, 30_000); + + await vi.advanceTimersByTimeAsync(30_000); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "stop-timeout", + message: "The recorder did not finish saving in time.", + exited: false, + }); + }); + + it("reports a process error as a helper failure", async () => { + const pending = waitForStop(() => started); + + helper.emit("error", new Error("spawn EACCES")); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "helper-failed", + message: "spawn EACCES", + exited: false, + }); + }); +}); + +describe("sendNativeMacStopCommand", () => { + it("tells a running helper to stop", () => { + expect(sendNativeMacStopCommand(asProc(helper))).toBe(true); + expect(helper.written).toEqual(["stop\n"]); + }); + + it("does not write to a helper that already exited", () => { + helper.exit(null, "SIGKILL"); + expect(sendNativeMacStopCommand(asProc(helper))).toBe(false); + expect(helper.written).toEqual([]); + }); + + it("does not write to a command pipe that is already closed", () => { + helper.stdin.destroy(); + expect(sendNativeMacStopCommand(asProc(helper))).toBe(false); + }); +}); + +describe("nativeMacDiscardTargets", () => { + it("removes the file the helper finalized", () => { + expect(nativeMacDiscardTargets({ ok: true, screenVideoPath: "/rec/a.mp4" }, TARGET)).toEqual([ + "/rec/a.mp4", + "/rec/a.mp4.cursor.json", + ]); + }); + + /** The orphan: a take thrown away after its writer died. */ + it("removes the requested file when the stop failed", () => { + expect( + nativeMacDiscardTargets( + { ok: false, reason: "helper-failed", message: WRITER_DIED_MESSAGE, exited: true }, + TARGET, + ), + ).toEqual([TARGET, `${TARGET}.cursor.json`]); + }); + + it("removes the requested file when the stop timed out", () => { + expect( + nativeMacDiscardTargets( + { ok: false, reason: "stop-timeout", message: "timed out", exited: false }, + TARGET, + ), + ).toEqual([TARGET, `${TARGET}.cursor.json`]); + }); + + it("has nothing to remove when no file was ever requested", () => { + expect( + nativeMacDiscardTargets( + { ok: false, reason: "helper-failed", message: "x", exited: true }, + null, + ), + ).toEqual([]); + }); +}); diff --git a/electron/recording/nativeMacCaptureStop.ts b/electron/recording/nativeMacCaptureStop.ts new file mode 100644 index 000000000..42e665f47 --- /dev/null +++ b/electron/recording/nativeMacCaptureStop.ts @@ -0,0 +1,304 @@ +import type { ChildProcessWithoutNullStreams } from "node:child_process"; + +/** + * Stopping a native macOS (ScreenCaptureKit) recording, as a unit that can be + * tested — the macOS twin of `nativeWindowsCaptureStop.ts`, which exists for + * the same reason: `electron/ipc/handlers.ts` cannot be loaded from a test. + * + * # Why the stop settles on the helper's exit, not on its first error + * + * The helper speaks two kinds of error. `writer-failed-during-capture` and + * `capture-stopped-with-error` say that the take ended on its own; the helper + * then shuts down by itself (issue #621, PR #655) and still reports how that + * went, with `recording-stopped` or `writer-failed`. The stop used to reject on + * the first `"event":"error"` in the whole buffered output, so a take whose + * capture stopped but whose file was finalized was thrown away with an error, + * and a take whose writer died was rejected before the helper had finished + * writing anything down. + * + * The helper always exits once it has received `stop` and finished (its command + * loop runs `await recorder.stop(); exit(0)`), after its last word. So the one + * moment at which the output is complete and the file is no longer being + * written is the process's `close`. That is also where Windows settles. + */ + +/** Outer bound on a stop. Unchanged from the wait this replaced. */ +export const NATIVE_MAC_CAPTURE_STOP_TIMEOUT_MS = 30_000; + +/** How the helper process ended, as Node's `close` reports it. */ +export type NativeMacCaptureExit = { + code: number | null; + signal: NodeJS.Signals | null; +}; + +export type NativeMacCaptureStopResult = + | { + ok: true; + screenVideoPath: string; + /** Why the take ended before the user stopped it, when it did. */ + warning?: string; + } + | { + ok: false; + reason: "helper-failed" | "stop-timeout"; + message: string; + /** False when the helper was still running when the wait gave up. */ + exited: boolean; + }; + +/** + * Errors after which the helper stops itself and still reports a terminal + * outcome. They explain a stop; they never settle one. + */ +const INTERRUPTION_ERROR_CODES = new Set([ + "writer-failed-during-capture", + "capture-stopped-with-error", +]); + +type HelperEvent = Record; + +function parseHelperEvent(line: string): HelperEvent | null { + const candidates = [line]; + // stdout and stderr drain into one buffer chunk by chunk, so a stderr line can + // end up glued onto the front of a JSON event. + const braceIndex = line.indexOf("{"); + if (braceIndex > 0) { + candidates.push(line.slice(braceIndex)); + } + for (const candidate of candidates) { + try { + const parsed = JSON.parse(candidate); + if (parsed && typeof parsed === "object" && typeof parsed.event === "string") { + return parsed as HelperEvent; + } + } catch { + // Not an event; the helper log also carries plain text. + } + } + return null; +} + +export function readNativeMacHelperEvents(output: string): HelperEvent[] { + const events: HelperEvent[] = []; + for (const line of output.split(/\r?\n/)) { + const trimmed = line.trim(); + if (!trimmed) { + continue; + } + const event = parseHelperEvent(trimmed); + if (event) { + events.push(event); + } + } + return events; +} + +/** `Array.prototype.findLast`, which the project's ES library target predates. */ +function lastWhere(events: HelperEvent[], matches: (event: HelperEvent) => boolean) { + for (let index = events.length - 1; index >= 0; index -= 1) { + if (matches(events[index])) { + return events[index]; + } + } + return undefined; +} + +function messageOf(event: HelperEvent) { + if (typeof event.message === "string" && event.message.trim()) { + return event.message.trim(); + } + return typeof event.code === "string" ? event.code : "Native macOS capture failed"; +} + +function describeExit(exit: NativeMacCaptureExit) { + if (exit.signal) { + return `The recorder stopped unexpectedly (signal ${exit.signal}).`; + } + if (exit.code === 0) { + return "The recorder exited without reporting a finished recording."; + } + return `The recorder stopped unexpectedly (exit code ${exit.code ?? "unknown"}).`; +} + +/** + * What the helper's output says about a stop, or null while it has not said + * enough yet. `exit` is null while the helper is still running. + */ +export function readNativeMacStopOutcome( + output: string, + exit: NativeMacCaptureExit | null, + targetPath: string | null, +): NativeMacCaptureStopResult | null { + const events = readNativeMacHelperEvents(output); + const isInterruption = (event: HelperEvent) => + typeof event.code === "string" && INTERRUPTION_ERROR_CODES.has(event.code); + const stopped = lastWhere(events, (event) => event.event === "recording-stopped"); + const errors = events.filter((event) => event.event === "error"); + const interruption = errors.find(isInterruption); + const failure = lastWhere(errors, (event) => !isInterruption(event)); + const exited = exit !== null; + + // A finalized file outranks anything said before it: this is the take the + // user would otherwise lose. + if (stopped) { + const reportedPath = + typeof stopped.screenPath === "string" && stopped.screenPath ? stopped.screenPath : null; + const screenVideoPath = reportedPath ?? targetPath; + if (!screenVideoPath) { + return { + ok: false, + reason: "helper-failed", + message: "Native macOS capture did not return an output path.", + exited, + }; + } + if (!interruption) { + return { ok: true, screenVideoPath }; + } + const reason = messageOf(interruption).replace(/[.\s]+$/, ""); + return { + ok: true, + screenVideoPath, + warning: `Recording ended early (${reason}). The part recorded until then was saved.`, + }; + } + + if (failure) { + // The interruption is what the user can act on ("the video file could not be + // written"); the terminal error that follows it is the raw NSError behind it. + return { + ok: false, + reason: "helper-failed", + message: messageOf(interruption ?? failure), + exited, + }; + } + + if (!exit) { + return null; + } + + // Exit 0 with no terminal word is the command loop reaching stdin EOF without a + // `stop` — nothing in the app closes that pipe, and `stop()` always reports + // before its exit(0). It keeps the meaning the stop wait gave it before this + // module (the requested file), except after an interruption, when nothing + // finalized that file. + if (exit.code === 0 && !exit.signal && targetPath && !interruption) { + return { ok: true, screenVideoPath: targetPath }; + } + return { + ok: false, + reason: "helper-failed", + message: interruption ? messageOf(interruption) : describeExit(exit), + exited: true, + }; +} + +/** + * Waits for the helper to finish a stop and says how it went. + * + * Resolves rather than rejects, like the Windows wait: the caller needs to tell + * a helper failure apart from a timeout, and a discard has to proceed either + * way. + */ +export function waitForNativeMacCaptureStop(options: { + proc: ChildProcessWithoutNullStreams; + /** Path we asked the helper to write, used when it exits 0 without saying so. */ + targetPath: string | null; + /** The accumulated helper output; read lazily so late chunks are included. */ + readOutput: () => string; + /** + * How the helper exited, once its output drain has seen `close`. Read from the + * drain rather than from `proc.exitCode`, because `exit` can fire before the + * last output chunk has been read. + */ + readExit: () => NativeMacCaptureExit | null; + timeoutMs?: number; +}): Promise { + const { proc, targetPath, readOutput, readExit } = options; + const timeoutMs = options.timeoutMs ?? NATIVE_MAC_CAPTURE_STOP_TIMEOUT_MS; + + const settleFromExit = (exit: NativeMacCaptureExit): NativeMacCaptureStopResult => + readNativeMacStopOutcome(readOutput(), exit, targetPath) ?? { + ok: false, + reason: "helper-failed", + message: describeExit(exit), + exited: true, + }; + + // The helper may already be gone: killed or crashed. (One that stopped itself + // stays alive until it is sent `stop`.) Node never re-emits `close`, so waiting + // for one would burn the whole timeout on a recorder that has nothing left to say. + const alreadyExited = readExit(); + if (alreadyExited) { + return Promise.resolve(settleFromExit(alreadyExited)); + } + + return new Promise((resolve) => { + const onClose = (code: number | null, signal: NodeJS.Signals | null) => { + cleanup(); + resolve(settleFromExit(readExit() ?? { code, signal })); + }; + const onError = (error: Error) => { + cleanup(); + resolve({ + ok: false, + reason: "helper-failed", + message: error.message, + exited: proc.exitCode !== null || proc.signalCode !== null, + }); + }; + const cleanup = () => { + clearTimeout(timer); + proc.off("close", onClose); + proc.off("error", onError); + }; + + const timer = setTimeout(() => { + cleanup(); + // A helper that already reported a finalized file but has not exited still + // left a complete recording behind. It is not killed here: one that has not + // reported may be inside a slow finishWriting, and this helper has no + // shutdown ceiling that would make killing it safe. + const outcome = readNativeMacStopOutcome(readOutput(), null, targetPath); + resolve( + outcome ?? { + ok: false, + reason: "stop-timeout", + message: "The recorder did not finish saving in time.", + exited: false, + }, + ); + }, timeoutMs); + + proc.once("close", onClose); + proc.once("error", onError); + }); +} + +/** + * Sends `stop` to a helper that can still hear it. Returns whether it was sent. + * + * A helper that already exited has closed its command pipe, and writing to it + * raises EPIPE or ERR_STREAM_DESTROYED on `proc.stdin`. + */ +export function sendNativeMacStopCommand(proc: ChildProcessWithoutNullStreams): boolean { + if (proc.exitCode !== null || proc.signalCode !== null || !proc.stdin.writable) { + return false; + } + proc.stdin.write("stop\n"); + return true; +} + +/** + * The files a discarded take leaves behind, whatever its stop returned: a take + * thrown away after its writer died used to stay on disk as an orphan .mp4, + * because removing it depended on the stop having worked. + */ +export function nativeMacDiscardTargets( + result: NativeMacCaptureStopResult, + targetPath: string | null, +): string[] { + const screenVideoPath = result.ok ? result.screenVideoPath : targetPath; + return screenVideoPath ? [screenVideoPath, `${screenVideoPath}.cursor.json`] : []; +} diff --git a/src/cli/CliRecordRunner.tsx b/src/cli/CliRecordRunner.tsx index e19c451f9..f588caa4c 100644 --- a/src/cli/CliRecordRunner.tsx +++ b/src/cli/CliRecordRunner.tsx @@ -284,6 +284,9 @@ export function CliRecordRunner() { cursorDataPath: `${session.screenVideoPath}.cursor.json`, durationMs, ...(request?.projectOut ? { projectData: buildDefaultProject(session) } : {}), + // The take ended before it was stopped but was kept: a script must be + // able to tell that apart from a clean run. + ...(sessionResult.warning ? { warnings: [sessionResult.warning] } : {}), }); } catch (error) { await fail(error); diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 681a88031..fabfc3acb 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -380,7 +380,7 @@ export function NewEditorShell() { void (async () => { if (!window.electronAPI) return; try { - if (await importPendingRecording()) { + if (await importPendingRecording((warning) => toast.warning(warning))) { toast.success("Recording added to a new project"); return; } diff --git a/src/components/ai-edition/recordingImport.test.ts b/src/components/ai-edition/recordingImport.test.ts index 123c954af..9bc7c5557 100644 --- a/src/components/ai-edition/recordingImport.test.ts +++ b/src/components/ai-edition/recordingImport.test.ts @@ -55,6 +55,7 @@ const realActions = { function stubElectronApi( screenVideoPath: string | null, cursorCaptureMode: "editable-overlay" | "system" = "editable-overlay", + warning?: string, ) { let session: { screenVideoPath: string; @@ -63,7 +64,7 @@ function stubElectronApi( } | null = screenVideoPath ? { screenVideoPath, createdAt: 0, cursorCaptureMode } : null; const api = { getCurrentRecordingSession: vi.fn(async () => - session ? { success: true, session } : { success: false }, + session ? { success: true, session, ...(warning ? { warning } : {}) } : { success: false }, ), setCurrentRecordingSession: vi.fn(async (next: typeof session) => { session = next; @@ -93,6 +94,33 @@ describe("importPendingRecording", () => { expect(createProject).not.toHaveBeenCalled(); }); + // The HUD that stopped a take which ended on its own closes as the editor opens, + // so the editor has to be the one that says the recording was cut short. + it("hands over why the take ended early once the import succeeded", async () => { + stubElectronApi( + "/recordings/recording-1.mp4", + "editable-overlay", + "Recording ended early (stream stopped). The part recorded until then was saved.", + ); + const onWarning = vi.fn(); + + await expect(importPendingRecording(onWarning)).resolves.toBe(true); + + expect(onWarning).toHaveBeenCalledTimes(1); + expect(onWarning).toHaveBeenCalledWith( + "Recording ended early (stream stopped). The part recorded until then was saved.", + ); + }); + + it("says nothing about a take that was stopped normally", async () => { + stubElectronApi("/recordings/recording-1.mp4"); + const onWarning = vi.fn(); + + await importPendingRecording(onWarning); + + expect(onWarning).not.toHaveBeenCalled(); + }); + it("imports the recording into a new project and consumes the hand-off", async () => { const api = stubElectronApi("C:\\recordings\\recording-1.mp4"); diff --git a/src/components/ai-edition/recordingImport.ts b/src/components/ai-edition/recordingImport.ts index 975a4a144..39b0923bd 100644 --- a/src/components/ai-edition/recordingImport.ts +++ b/src/components/ai-edition/recordingImport.ts @@ -297,7 +297,14 @@ export async function maybeSaveFreshRecordingAutoZooms( * reopening the most recent project. Throws if the import itself fails, leaving * the session in place so a later mount can retry it. */ -export async function importPendingRecording(): Promise { +export async function importPendingRecording( + /** + * Called once the import succeeded, with why the take ended before it was + * stopped when it did. The HUD that ran the stop closes as the editor opens, so + * the editor is the only window left to say it. + */ + onWarning?: (message: string) => void, +): Promise { const api = window.electronAPI; if (!api) return false; @@ -305,6 +312,7 @@ export async function importPendingRecording(): Promise { const screenPath = result.success ? result.session?.screenVideoPath : undefined; if (!screenPath) return false; const cursorCaptureMode = result.success ? result.session?.cursorCaptureMode : undefined; + const warning = result.success ? result.warning : undefined; const label = screenPath.split(/[\\/]/).pop() || "Recording"; await useProjectStore.getState().createProject(`Recording ${new Date().toLocaleString()}`); @@ -348,5 +356,8 @@ export async function importPendingRecording(): Promise { if (latest) { await maybeSaveFreshRecordingAutoZooms(latest); } + if (warning) { + onWarning?.(warning); + } return true; }