fix: tighten streaming failure handling from re-review
Addresses the CodeRabbit + Codex re-review of the prior commit.
- Normalize a rejected append (channel/handler error, not just a
{ success: false } result) into appendError, so the write queue never
rejects and isStreaming() stays consistent after a failure (CodeRabbit).
- Handle a rejected open-stream IPC the same as a failed open: fall back
to in-memory buffering instead of leaving the recorder stuck "pending"
with an unhandled rejection (CodeRabbit).
- Discard a streamed webcam whose write failed even when the screen save
succeeds. The cleanup gate is now per-recorder, so a webcam omitted from
a successful screen-only save no longer leaks its stream and partial
file (Codex).
Adds tests for the rejected-append and rejected-open paths.
Verified: tsc --noEmit clean; biome clean; vitest 182/182.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -90,6 +90,29 @@ describe("createRecorderHandle", () => {
|
|||||||
expect(decode(await blob.arrayBuffer())).toBe("abc");
|
expect(decode(await blob.arrayBuffer())).toBe("abc");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("falls back to in-memory buffering when the open IPC call rejects", async () => {
|
||||||
|
const openRecordingStream = vi.fn(async () => {
|
||||||
|
throw new Error("ipc channel closed");
|
||||||
|
});
|
||||||
|
stubElectronAPI({
|
||||||
|
openRecordingStream,
|
||||||
|
appendRecordingChunk: vi.fn(async () => ({ success: true })),
|
||||||
|
});
|
||||||
|
|
||||||
|
const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm");
|
||||||
|
const fake = driver(handle);
|
||||||
|
|
||||||
|
fake.emit(new Blob(["a"]));
|
||||||
|
await tick(); // open rejects -> treated as a failed open, keep buffering
|
||||||
|
fake.emit(new Blob(["b"]));
|
||||||
|
fake.stop();
|
||||||
|
|
||||||
|
const blob = await handle.recordedBlobPromise;
|
||||||
|
expect(handle.isStreaming()).toBe(false);
|
||||||
|
expect(blob.size).toBe(2);
|
||||||
|
expect(decode(await blob.arrayBuffer())).toBe("ab");
|
||||||
|
});
|
||||||
|
|
||||||
it("waits for in-flight chunk writes before stop resolves (no truncation)", async () => {
|
it("waits for in-flight chunk writes before stop resolves (no truncation)", async () => {
|
||||||
let releaseAppend: () => void = () => undefined;
|
let releaseAppend: () => void = () => undefined;
|
||||||
const appendGate = new Promise<void>((resolve) => {
|
const appendGate = new Promise<void>((resolve) => {
|
||||||
@@ -142,6 +165,26 @@ describe("createRecorderHandle", () => {
|
|||||||
expect(handle.isStreaming()).toBe(false);
|
expect(handle.isStreaming()).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("treats a rejected append the same as a failed write", async () => {
|
||||||
|
stubElectronAPI({
|
||||||
|
openRecordingStream: vi.fn(async () => ({ success: true })),
|
||||||
|
appendRecordingChunk: vi.fn(async () => {
|
||||||
|
throw new Error("kernel said no");
|
||||||
|
}),
|
||||||
|
closeRecordingStream: vi.fn(async () => ({ success: true })),
|
||||||
|
});
|
||||||
|
|
||||||
|
const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm");
|
||||||
|
const fake = driver(handle);
|
||||||
|
|
||||||
|
await tick();
|
||||||
|
fake.emit(new Blob(["a"]));
|
||||||
|
fake.stop();
|
||||||
|
|
||||||
|
await expect(handle.recordedBlobPromise).rejects.toThrow(/kernel said no/);
|
||||||
|
expect(handle.isStreaming()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
it("buffers in memory and never opens a stream when no file name is given", async () => {
|
it("buffers in memory and never opens a stream when no file name is given", async () => {
|
||||||
const openRecordingStream = vi.fn(async () => ({ success: true }));
|
const openRecordingStream = vi.fn(async () => ({ success: true }));
|
||||||
stubElectronAPI({
|
stubElectronAPI({
|
||||||
|
|||||||
+28
-14
@@ -61,10 +61,17 @@ export function createRecorderHandle(
|
|||||||
if (appendError || !fileName || !api?.appendRecordingChunk) {
|
if (appendError || !fileName || !api?.appendRecordingChunk) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const buffer = await chunk.arrayBuffer();
|
// Capture both outcomes — a `{ success: false }` result and an outright
|
||||||
const result = await api.appendRecordingChunk(fileName, buffer);
|
// rejection (channel/handler error) — into appendError, so writeChain
|
||||||
if (!result.success) {
|
// never rejects and isStreaming() stays consistent after a failure.
|
||||||
appendError = new Error(result.error ?? "Failed to write recording chunk to disk");
|
try {
|
||||||
|
const buffer = await chunk.arrayBuffer();
|
||||||
|
const result = await api.appendRecordingChunk(fileName, buffer);
|
||||||
|
if (!result.success) {
|
||||||
|
appendError = new Error(result.error ?? "Failed to write recording chunk to disk");
|
||||||
|
}
|
||||||
|
} catch (error) {
|
||||||
|
appendError = error instanceof Error ? error : new Error(String(error));
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
@@ -74,18 +81,25 @@ export function createRecorderHandle(
|
|||||||
? api.openRecordingStream(fileName)
|
? api.openRecordingStream(fileName)
|
||||||
: Promise.resolve({ success: false });
|
: Promise.resolve({ success: false });
|
||||||
|
|
||||||
void openPromise.then((result) => {
|
void openPromise.then(
|
||||||
if (result.success) {
|
(result) => {
|
||||||
streamOpened = true;
|
if (result.success) {
|
||||||
mode = "streaming";
|
streamOpened = true;
|
||||||
for (const chunk of memoryChunks) {
|
mode = "streaming";
|
||||||
enqueueWrite(chunk);
|
for (const chunk of memoryChunks) {
|
||||||
|
enqueueWrite(chunk);
|
||||||
|
}
|
||||||
|
memoryChunks.length = 0;
|
||||||
|
} else {
|
||||||
|
mode = "buffering";
|
||||||
}
|
}
|
||||||
memoryChunks.length = 0;
|
},
|
||||||
} else {
|
() => {
|
||||||
|
// The IPC call itself rejected (channel or handler error). Treat it the
|
||||||
|
// same as a failed open: keep buffering in memory so nothing is lost.
|
||||||
mode = "buffering";
|
mode = "buffering";
|
||||||
}
|
},
|
||||||
});
|
);
|
||||||
|
|
||||||
const recordedBlobPromise = new Promise<Blob>((resolve, reject) => {
|
const recordedBlobPromise = new Promise<Blob>((resolve, reject) => {
|
||||||
recorder.ondataavailable = (event: BlobEvent) => {
|
recorder.ondataavailable = (event: BlobEvent) => {
|
||||||
|
|||||||
@@ -328,10 +328,11 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
|
|||||||
window.electronAPI?.setRecordingState(false);
|
window.electronAPI?.setRecordingState(false);
|
||||||
|
|
||||||
void (async () => {
|
void (async () => {
|
||||||
// Set once the recording is safely stored. Until then any disk stream
|
// Each disk stream must end up either saved or explicitly discarded.
|
||||||
// is still open, so the finally block closes it and removes the partial
|
// store-recorded-session finalizes the streams included in a successful
|
||||||
// file on the discard or error paths.
|
// save; the finally block discards everything else.
|
||||||
let savedToDisk = false;
|
let storeSucceeded = false;
|
||||||
|
let webcamIncludedInSave = false;
|
||||||
try {
|
try {
|
||||||
const screenBlob = await activeScreenRecorder.recordedBlobPromise;
|
const screenBlob = await activeScreenRecorder.recordedBlobPromise;
|
||||||
if (discardRecordingId.current === activeRecordingId) {
|
if (discardRecordingId.current === activeRecordingId) {
|
||||||
@@ -364,6 +365,7 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
|
|||||||
webcamVideoData = new ArrayBuffer(0);
|
webcamVideoData = new ArrayBuffer(0);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
webcamIncludedInSave = webcamVideoData !== undefined;
|
||||||
|
|
||||||
const result = await window.electronAPI.storeRecordedSession({
|
const result = await window.electronAPI.storeRecordedSession({
|
||||||
screen: {
|
screen: {
|
||||||
@@ -383,8 +385,8 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
|
|||||||
console.error("Failed to store recording session:", result.message);
|
console.error("Failed to store recording session:", result.message);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
// store-recorded-session has flushed and closed the disk streams.
|
// store-recorded-session has flushed and closed the saved streams.
|
||||||
savedToDisk = true;
|
storeSucceeded = true;
|
||||||
|
|
||||||
if (result.session) {
|
if (result.session) {
|
||||||
await window.electronAPI.setCurrentRecordingSession(result.session);
|
await window.electronAPI.setCurrentRecordingSession(result.session);
|
||||||
@@ -396,12 +398,15 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
|
|||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error("Error saving recording:", error);
|
console.error("Error saving recording:", error);
|
||||||
} finally {
|
} finally {
|
||||||
if (!savedToDisk) {
|
// Discard any recorder whose data was not part of a successful save
|
||||||
// Discarded, or failed before a successful save — close any
|
// — a discarded run, a failed save, or a webcam whose disk write
|
||||||
// dangling disk streams and remove their partial files so a
|
// failed (so it was omitted while the screen still saved) — so no
|
||||||
// cancelled or failed run doesn't leak a descriptor or orphan.
|
// stream or partial file is left open or orphaned.
|
||||||
|
if (!storeSucceeded) {
|
||||||
await activeScreenRecorder.discard().catch(() => undefined);
|
await activeScreenRecorder.discard().catch(() => undefined);
|
||||||
await activeWebcamRecorder?.discard().catch(() => undefined);
|
}
|
||||||
|
if (activeWebcamRecorder && !(storeSucceeded && webcamIncludedInSave)) {
|
||||||
|
await activeWebcamRecorder.discard().catch(() => undefined);
|
||||||
}
|
}
|
||||||
if (finalizingRecordingId.current === activeRecordingId) {
|
if (finalizingRecordingId.current === activeRecordingId) {
|
||||||
finalizingRecordingId.current = null;
|
finalizingRecordingId.current = null;
|
||||||
|
|||||||
Reference in New Issue
Block a user