From 206e11438795164ec2b5bdc5e7b49b3285a09dc7 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 17 May 2026 20:28:56 -0700 Subject: [PATCH] fix(security-classifier): await writer close before unlinking tmp on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier downloadFile() error-path cleanup hit a race: Node's createWriteStream lazily opens the FD and flushes buffered writes during destroy(), so a naive `fs.unlinkSync(tmp)` immediately after `writer.destroy()` hits ENOENT (file not yet on disk), then the writer's destroy finishes on the next tick and creates the file fresh — leaving the half-written tmp behind exactly as the original fix tried to prevent. The new sequence awaits the writer's 'close' event before unlinking, so the FD is fully torn down and no subsequent flush can re-create the path. Caught by browse/test/security-classifier-download-cleanup.test.ts in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/src/security-classifier.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/browse/src/security-classifier.ts b/browse/src/security-classifier.ts index 54f770c79..0c8304b66 100644 --- a/browse/src/security-classifier.ts +++ b/browse/src/security-classifier.ts @@ -156,9 +156,15 @@ export async function downloadFile(url: string, dest: string): Promise { }); fs.renameSync(tmp, dest); } catch (err) { - // Close the writer and drop the half-written tmp so we don't leak an FD - // or ship a truncated model file to the renameSync path on retry. - try { writer.destroy(); } catch { /* already destroyed */ } + // Drop the half-written tmp so we don't ship a truncated model file to + // a retry's renameSync. Wait for the writer to close fully before + // unlinking: Node's createWriteStream lazily opens the FD and flushes + // buffered writes during destroy(), so a naive unlinkSync hits ENOENT + // first and the writer re-creates the file on the next tick. + await new Promise((resolve) => { + writer.once('close', () => resolve()); + writer.destroy(); + }); try { fs.unlinkSync(tmp); } catch { /* nothing to clean */ } throw err; }