Skip to content

Commit 409085b

Browse files
committed
fix(display): close the video writer when the audio writer's close throws
recordScreenAndAudioToFiles closed its two writers in sequence: audioWritten = await audioOut.close(); await videoOut.close(); M4aFileWriter finalizes by appending the moov box, ending the stream, then reopening the finished file to patch mdat's length field. Any of those can fail on a full disk or a reaped temp directory, long after every frame is already safely on disk. When one does, the video writer's close is skipped. The cost is a leaked file descriptor. Without close() the write stream is never ended, and Node does not release descriptors on garbage collection, so it lives until the process exits. The error the stream is holding also goes unread, since AnnexBFileWriter only surfaces that from write() or close(). The video file itself is intact. Node's write queue drains on its own once chunks are handed to write(), the recording loop awaits every write, and Annex-B is an elementary stream with no index or footer to finalize, so a file ending after the last frame is valid and playable. Verified by writing 15 MB and never calling close(): every byte was on disk. Closing the video writer from a finally makes it independent of the audio writer's failure. The audio error still propagates when the video close succeeds. If both throw the video error supersedes, which is standard finally behaviour and an acceptable edge for a path that has already failed. Covered by a unit test that stubs the audio writer's close to throw and asserts the video writer was still closed. Reverting to sequential closes fails it with 0 closes instead of 1.
1 parent 0bc93d0 commit 409085b

2 files changed

Lines changed: 70 additions & 3 deletions

File tree

src/services/ios/display/recording/av-capture.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -196,8 +196,16 @@ export async function recordScreenAndAudioToFiles(
196196
log.debug(`Failed to stop cleanly: ${error instanceof Error ? error.message : String(error)}`);
197197
});
198198
await audioCapture.stop().catch((): void => undefined);
199-
audioWritten = await audioOut.close();
200-
await videoOut.close();
199+
// The video writer closes even when the audio writer throws. M4aFileWriter
200+
// finalizes by reopening the finished file to patch mdat's length, so it can
201+
// fail on a full disk long after every frame is safely on disk. Sequencing
202+
// the two closes would leak the video file descriptor for the life of the
203+
// process, and the error its stream is holding would never be read.
204+
try {
205+
audioWritten = await audioOut.close();
206+
} finally {
207+
await videoOut.close();
208+
}
201209
}
202210

203211
const audioDurationMs = aacEldDurationMs(audioWritten.sampleCount);

test/unit/display/recording/av-capture.spec.ts

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,15 @@ import assert from 'node:assert/strict';
22
import {mkdtemp, rm} from 'node:fs/promises';
33
import {tmpdir} from 'node:os';
44
import {join} from 'node:path';
5-
import {after, before, describe, it} from 'node:test';
5+
import {type TestContext, after, before, describe, it} from 'node:test';
66

77
import type {DisplayService, MediaStreamAnswer} from '../../../../src/services/ios/display/index.js';
88
import {recordScreenAndAudioToFiles} from '../../../../src/services/ios/display/recording/av-capture.js';
9+
import {mockImport} from '../../../helpers/mock-module.js';
10+
11+
const AV_CAPTURE_MODULE = '../../../../src/services/ios/display/recording/av-capture.js';
12+
const M4A_WRITER_MODULE = '../../../../src/services/ios/display/audio/m4a-writer.js';
13+
const SCREEN_CAPTURE_MODULE = '../../../../src/services/ios/display/video/screen-stream-capture.js';
914

1015
/** A stub service that negotiates without a device and counts teardowns. */
1116
function makeStubService(): {service: DisplayService; stopCalls: () => number} {
@@ -60,4 +65,58 @@ describe('recordScreenAndAudioToFiles', function () {
6065

6166
assert.strictEqual(stopCalls(), 2, 'both captures must be stopped before the error propagates');
6267
});
68+
69+
it('closes the video writer even when closing the audio writer throws', async function (t: TestContext) {
70+
// M4aFileWriter finalizes by reopening the finished file to patch mdat's
71+
// length, so it can fail on a full disk after every frame is already on
72+
// disk. Sequencing the closes would skip the video one, leaking its file
73+
// descriptor and leaving the error its stream holds unread.
74+
let videoCloseCalls = 0;
75+
const closeFailure = new Error('failed to patch the mdat header');
76+
77+
class StubM4aFileWriter {
78+
static async create(): Promise<StubM4aFileWriter> {
79+
return new StubM4aFileWriter();
80+
}
81+
async write(): Promise<void> {
82+
return undefined;
83+
}
84+
async close(): Promise<never> {
85+
throw closeFailure;
86+
}
87+
}
88+
89+
class StubAnnexBFileWriter {
90+
constructor(path: string) {
91+
void path;
92+
}
93+
async write(): Promise<void> {
94+
return undefined;
95+
}
96+
async close(): Promise<void> {
97+
videoCloseCalls += 1;
98+
}
99+
}
100+
101+
const {recordScreenAndAudioToFiles: record} = await mockImport<{
102+
recordScreenAndAudioToFiles: typeof recordScreenAndAudioToFiles;
103+
}>(t, AV_CAPTURE_MODULE, import.meta.url, {
104+
[M4A_WRITER_MODULE]: {M4aFileWriter: StubM4aFileWriter},
105+
// Merged over the real exports, so ScreenStreamCapture stays intact.
106+
[SCREEN_CAPTURE_MODULE]: {AnnexBFileWriter: StubAnnexBFileWriter},
107+
});
108+
109+
const {service} = makeStubService();
110+
await assert.rejects(
111+
() =>
112+
record(service, {
113+
videoPath: join(directory, 'screen-close.h265'),
114+
audioPath: join(directory, 'audio-close.m4a'),
115+
durationMs: 50,
116+
}),
117+
/failed to patch the mdat header/,
118+
);
119+
120+
assert.strictEqual(videoCloseCalls, 1, 'the video writer must be closed even though the audio close threw');
121+
});
63122
});

0 commit comments

Comments
 (0)