-
-
Notifications
You must be signed in to change notification settings - Fork 165
fix(mac): 【openscreen】録画終了時の保存失敗を救済する #571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| import fs from "node:fs/promises"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { isSalvageableNativeMacCapture, resolveNativeMacCaptureStop } from "./nativeMacCaptureStop"; | ||
|
|
||
| let dir: string; | ||
| const validVideoFixture = path.resolve("website/static/video/webcam.mp4"); | ||
|
|
||
| beforeEach(async () => { | ||
| dir = await fs.mkdtemp(path.join(os.tmpdir(), "native-mac-stop-")); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(dir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| function atom(type: string, payloadBytes: number) { | ||
| const result = Buffer.alloc(8 + payloadBytes); | ||
| result.writeUInt32BE(result.length, 0); | ||
| result.write(type, 4, 4, "ascii"); | ||
| return result; | ||
| } | ||
|
|
||
| async function writeMp4(name: string, atoms: Buffer[]): Promise<string> { | ||
| const filePath = path.join(dir, name); | ||
| await fs.writeFile(filePath, Buffer.concat(atoms)); | ||
| return filePath; | ||
| } | ||
|
|
||
| describe("isSalvageableNativeMacCapture", () => { | ||
| it("accepts an MP4 with a parseable video stream after helper exit", async () => { | ||
| await expect(isSalvageableNativeMacCapture(validVideoFixture, true)).resolves.toBe(true); | ||
| }); | ||
|
|
||
| it("rejects the former false positive with atom names but no video stream", async () => { | ||
| const filePath = await writeMp4("empty-shell.mp4", [ | ||
| atom("ftyp", 24), | ||
| atom("mdat", 2048), | ||
| atom("moov", 256), | ||
| ]); | ||
| await expect(isSalvageableNativeMacCapture(filePath, true)).resolves.toBe(false); | ||
| }); | ||
|
|
||
| it("does not inspect or admit a file while the helper may still be writing", async () => { | ||
| await expect(isSalvageableNativeMacCapture(validVideoFixture, false)).resolves.toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when the expected output is missing", async () => { | ||
| await expect(isSalvageableNativeMacCapture(path.join(dir, "missing.mp4"), true)).resolves.toBe( | ||
| false, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("resolveNativeMacCaptureStop", () => { | ||
| it("keeps the acknowledged stop path unchanged", async () => { | ||
| const waitForExit = vi.fn(async () => true); | ||
| const isSalvageable = vi.fn(async () => true); | ||
| await expect( | ||
| resolveNativeMacCaptureStop({ | ||
| preferredPath: "/recordings/preferred.mp4", | ||
| waitForStop: async () => "/recordings/acknowledged.mp4", | ||
| waitForExit, | ||
| isSalvageable, | ||
| }), | ||
| ).resolves.toEqual({ path: "/recordings/acknowledged.mp4", recovered: false }); | ||
| expect(waitForExit).not.toHaveBeenCalled(); | ||
| expect(isSalvageable).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns the preferred path when failed stop output is safely recoverable", async () => { | ||
| const stopError = new Error("helper closed before stopped event"); | ||
| await expect( | ||
| resolveNativeMacCaptureStop({ | ||
| preferredPath: validVideoFixture, | ||
| waitForStop: async () => { | ||
| throw stopError; | ||
| }, | ||
| waitForExit: async () => true, | ||
| }), | ||
| ).resolves.toEqual({ path: validVideoFixture, recovered: true, stopError }); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ["helper is still alive", false, true], | ||
| ["output is invalid", true, false], | ||
| ])("preserves the original stop failure when %s", async (_label, helperExited, valid) => { | ||
| const stopError = new Error("stop failed"); | ||
| await expect( | ||
| resolveNativeMacCaptureStop({ | ||
| preferredPath: "/recordings/incomplete.mp4", | ||
| waitForStop: async () => { | ||
| throw stopError; | ||
| }, | ||
| waitForExit: async () => helperExited, | ||
| isSalvageable: async () => valid, | ||
| }), | ||
| ).rejects.toBe(stopError); | ||
| }); | ||
|
|
||
| it("falls back to the preferred path when the acknowledgement names no output", async () => { | ||
| await expect( | ||
| resolveNativeMacCaptureStop({ | ||
| preferredPath: "/recordings/preferred.mp4", | ||
| waitForStop: async () => "", | ||
| waitForExit: async () => true, | ||
| }), | ||
| ).resolves.toEqual({ path: "/recordings/preferred.mp4", recovered: false }); | ||
| }); | ||
|
|
||
| it("fails when neither the acknowledgement nor the preferred path names an output", async () => { | ||
| await expect( | ||
| resolveNativeMacCaptureStop({ | ||
| preferredPath: null, | ||
| waitForStop: async () => "", | ||
| waitForExit: async () => true, | ||
| }), | ||
| ).rejects.toThrow("did not return an output path"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import fs from "node:fs/promises"; | ||
| import { createFile } from "mp4box"; | ||
|
|
||
| const MIN_SALVAGEABLE_MP4_BYTES = 1024; | ||
| const PARSE_CHUNK_BYTES = 1024 * 1024; | ||
|
|
||
| type ParsedMovie = { | ||
| hasMoov: boolean; | ||
| duration: number; | ||
| timescale: number; | ||
| videoTracks: Array<{ | ||
| codec: string; | ||
| duration: number; | ||
| timescale: number; | ||
| nb_samples: number; | ||
| video?: { width: number; height: number }; | ||
| }>; | ||
| }; | ||
|
|
||
| type PositionedArrayBuffer = ArrayBuffer & { fileStart: number }; | ||
|
|
||
| /** | ||
| * Parses the MP4 incrementally, without retaining `mdat`, and requires a real | ||
| * video sample table. Atom names alone are insufficient: a zero-filled `moov` | ||
| * shell looks superficially complete but cannot be opened by the editor. | ||
| */ | ||
| async function hasReadableVideoStream(filePath: string): Promise<boolean> { | ||
| let handle: Awaited<ReturnType<typeof fs.open>> | null = null; | ||
| try { | ||
| handle = await fs.open(filePath, "r"); | ||
| const stat = await handle.stat(); | ||
| if (!stat.isFile() || stat.size < MIN_SALVAGEABLE_MP4_BYTES) return false; | ||
|
|
||
| const parser = createFile(false); | ||
| let movie: ParsedMovie | null = null; | ||
| let parseFailed = false; | ||
| parser.onReady = (info) => { | ||
| movie = info as ParsedMovie; | ||
| }; | ||
| parser.onError = () => { | ||
| parseFailed = true; | ||
| }; | ||
|
|
||
| let offset = 0; | ||
| while (offset < stat.size) { | ||
| const bytesToRead = Math.min(PARSE_CHUNK_BYTES, stat.size - offset); | ||
| const chunk = Buffer.allocUnsafe(bytesToRead); | ||
| const { bytesRead } = await handle.read(chunk, 0, bytesToRead, offset); | ||
| if (bytesRead !== bytesToRead) return false; | ||
| const arrayBuffer = chunk.buffer.slice( | ||
| chunk.byteOffset, | ||
| chunk.byteOffset + bytesRead, | ||
| ) as PositionedArrayBuffer; | ||
| arrayBuffer.fileStart = offset; | ||
| parser.appendBuffer(arrayBuffer, offset + bytesRead === stat.size); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🤖 get_repo_knowledge executed:
Length of output: 47387 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n electron/recording/nativeMacCaptureStop.ts
printf '%s\n' '--- package/API binding references ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' 'appendBuffer|mp4box|MP4Box|hasReadableVideoStream|resolveNativeMacCaptureStop' electron package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -200Repository: getopenscreen/openscreen Length of output: 6169 🌐 Web query:
💡 Result: In mp4box.js, the appendBuffer(data) method is designed to assist in progressive file parsing. When you pass an ArrayBuffer to this method, it expects the buffer to have a fileStart property (a Number) that indicates the 0-based byte position of that buffer within the original file [1][2][3]. The appendBuffer method returns the file offset that is expected to be the fileStart value for the next chunk of data [1][2][3]. This return value is intended to help you manage the reading process, particularly in cases where the moov box is not located at the beginning of the file, allowing the parser to request specific parts of the file needed for successful decoding [1][2][3]. Example usage: var ab = getArrayBuffer; // Method returning your ArrayBuffer ab.fileStart = 0; // Set the starting position of this buffer var nextBufferStart = mp4boxfile.appendBuffer(ab); // Use the returned value for the next read [1][2][3] Note that while the returned offset is intended to guide the next request, users have reported in various GitHub issues that it is crucial to handle potential mismatches or loops in logic if the returned offset does not advance as expected, sometimes necessitating manual management of the offset based on your application's read logic [4][5][6]. Always ensure your ArrayBuffer is correctly assigned the fileStart property before calling the method [1][2]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
import json, urllib.request
url = "https://registry.npmjs.org/mp4box/2.3.0"
with urllib.request.urlopen(url, timeout=10) as r:
data = json.load(r)
print("name:", data["name"])
print("version:", data["version"])
print("repository:", data.get("repository"))
print("dist.tarball:", data["dist"]["tarball"])
PYRepository: getopenscreen/openscreen Length of output: 2448 Use the next offset returned by
🤖 Prompt for AI Agents |
||
| offset += bytesRead; | ||
| } | ||
| parser.flush(); | ||
|
|
||
| if (parseFailed || !movie) return false; | ||
| const parsedMovie = movie as ParsedMovie; | ||
| return ( | ||
| parsedMovie.hasMoov && | ||
| parsedMovie.duration > 0 && | ||
| parsedMovie.timescale > 0 && | ||
| parsedMovie.videoTracks.some( | ||
| (track) => | ||
| track.codec.length > 0 && | ||
| track.duration > 0 && | ||
| track.timescale > 0 && | ||
| track.nb_samples > 0 && | ||
| (track.video?.width ?? 0) > 0 && | ||
| (track.video?.height ?? 0) > 0, | ||
| ) | ||
| ); | ||
| } catch { | ||
| return false; | ||
| } finally { | ||
| await handle?.close().catch(() => undefined); | ||
| } | ||
| } | ||
|
|
||
| /** A file is never inspected while the helper may still be mutating it. */ | ||
| export async function isSalvageableNativeMacCapture( | ||
| filePath: string | null, | ||
| helperExited: boolean, | ||
| ): Promise<boolean> { | ||
| if (!filePath || !helperExited) return false; | ||
| return hasReadableVideoStream(filePath); | ||
| } | ||
|
|
||
| export type NativeMacCaptureStopResolution = { | ||
| path: string; | ||
| recovered: boolean; | ||
| stopError?: unknown; | ||
| }; | ||
|
|
||
| /** | ||
| * Keeps the normal acknowledgement path unchanged. If acknowledgement fails, | ||
| * recovery is allowed only after helper exit and successful media parsing. | ||
| */ | ||
| export async function resolveNativeMacCaptureStop(options: { | ||
| preferredPath: string | null; | ||
| waitForStop: () => Promise<string>; | ||
| waitForExit: () => Promise<boolean>; | ||
| isSalvageable?: (filePath: string | null, helperExited: boolean) => Promise<boolean>; | ||
| }): Promise<NativeMacCaptureStopResolution> { | ||
| try { | ||
| // The helper acknowledges the stop with `String(event.screenPath ?? target ?? "")`, | ||
| // so an acknowledgement carrying no usable path resolves an empty string. Falling | ||
| // back to the preferred path and refusing an empty one is what the call site did | ||
| // before recovery existed; without it a pathless acknowledgement is reported as a | ||
| // successful save and the session manifest is written for a file that is not there. | ||
| const acknowledgedPath = (await options.waitForStop()) || options.preferredPath; | ||
| if (acknowledgedPath) { | ||
| return { path: acknowledgedPath, recovered: false }; | ||
| } | ||
| } catch (stopError) { | ||
| const helperExited = await options.waitForExit(); | ||
| const isSalvageable = options.isSalvageable ?? isSalvageableNativeMacCapture; | ||
| if ( | ||
| !helperExited || | ||
| !(await isSalvageable(options.preferredPath, helperExited)) || | ||
| !options.preferredPath | ||
| ) { | ||
| throw stopError; | ||
| } | ||
| return { path: options.preferredPath, recovered: true, stopError }; | ||
| } | ||
| throw new Error("Native macOS capture did not return an output path."); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge getopenscreen/openscreen /tmp/coderabbit-repo-knowledge/getopenscreen-openscreen-086fd783/architecture /tmp/coderabbit-repo-knowledge/getopenscreen-openscreen-086fd783/conventionsLength of output: 47456
🏁 Script executed:
Repository: getopenscreen/openscreen
Length of output: 50380
🏁 Script executed:
Repository: getopenscreen/openscreen
Length of output: 21313
🌐 Web query:
site:nodejs.org/api child_process ChildProcess stdin writable stream error event unhandled💡 Result:
In Node.js, the
subprocess.stdinproperty of aChildProcessobject is a writable stream [1]. Like allEventEmitter-based objects in Node.js, writable streams emit an 'error' event when an error occurs during operations such as writing or piping data [2][3][4]. If an 'error' event is emitted on a writable stream and no listener is registered to handle it, the error is thrown, resulting in an uncaught exception that will typically cause the Node.js process to exit and crash [3][4][5]. This occurs because 'error' events are treated as special cases in the Node.js event emitter implementation; they do not propagate or get caught by surroundingtry...catchblocks if they occur asynchronously after the initial call stack has cleared [3][4][5]. To prevent this, you should always attach an 'error' listener to thesubprocess.stdinstream [4]. Example of proper handling: const { spawn } = require('node:child_process'); const subprocess = spawn('some-command'); subprocess.stdin.on('error', (err) => { console.error('An error occurred on stdin:', err); }); // Proceed with writing to stdin subprocess.stdin.write('some data'); Additionally, be aware that if the child process exits or closes the pipe unexpectedly, attempts to write to the stdin stream may trigger these errors [2][3]. As a best practice, always monitor the stream for errors to ensure process stability [4].Citations:
Handle
proc.stdinerrors before recovery.If the helper closes its command pipe while
proc.stdin.write("stop\n")is pending, the writable stream can emitEPIPE. The macOS capture drain has noproc.stdinerror listener, so the unhandled event can terminate the main process beforeresolveNativeMacCaptureStopruns. Add a persistent listener that records the error and rejects the stop wait, allowing the existing exit-and-MP4 recovery path to handle it. Add a regression test for a helper that exits while the stop command is written.🧰 Tools
🪛 ast-grep (0.45.2)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcessWithoutNullStreams, spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents