From 668eda6cc87ad17330d40b70c9a0eea734817010 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 24 May 2026 23:34:26 -0400 Subject: [PATCH] fix(stack-files): atomic write via tmp+rename with optional exclusive mode (#1205) * fix(stack-files): atomic write via tmp+rename with optional exclusive mode writeStackFile and writeStackFileBuffer previously called fs.writeFile directly, which truncates the target then streams the new bytes. A crash, disk-full event, or process kill between the truncate and the write left the target with partial content and no easy way to detect the half-write at read time. A private writeStackFileAtomic helper stages every write into a sibling .sencho-tmp- file in the same directory, fsyncs, then promotes via fs.rename. A crash now leaves either the original target intact or a leftover .sencho-tmp file (cleaned up on the next failure path); a torn target file is no longer reachable through this path. The helper accepts an optional `exclusive: true` flag that swaps the final promote step from rename to link+unlink. link is atomic against EEXIST so a caller that needs "create only if not present" gets a race-free FILE_EXISTS error instead of a clobber. The upload route's overwrite-confirm flow (PR #1204) will wire this through in a follow-up so the existence check becomes authoritative. Behaviour for current callers (writeStackFile, writeStackFileBuffer, BlueprintService deploy) is unchanged: the non-exclusive default matches the prior fs.writeFile semantics from the caller's perspective. * fix(stack-files): tighten atomic write entropy + concurrent / failure tests Tmp suffix now uses crypto.randomBytes(6) so the per-process collision window is a true 48-bit space (Math.random().toString(36).slice(2,6) could drop leading zeros and narrow entropy unpredictably). Adds a short comment on the Windows link path noting NTFS / same-FS POSIX is required, both guaranteed by tmp+target being siblings. Two new tests close coverage gaps the first round missed: - a write step that throws (writeFile rejected) leaves no tmp leak and no partial target; - concurrent non-exclusive writers settle with at least one success and the final file is exactly one of the inputs (POSIX silently overwrites; Windows EPERMs the loser, both consistent). --- .../__tests__/filesystem-stack-paths.test.ts | 112 ++++++++++++++++++ backend/src/services/FileSystemService.ts | 77 +++++++++++- 2 files changed, 183 insertions(+), 6 deletions(-) diff --git a/backend/src/__tests__/filesystem-stack-paths.test.ts b/backend/src/__tests__/filesystem-stack-paths.test.ts index f0be0443..6ab7d3d8 100644 --- a/backend/src/__tests__/filesystem-stack-paths.test.ts +++ b/backend/src/__tests__/filesystem-stack-paths.test.ts @@ -237,6 +237,118 @@ describe('FileSystemService stack methods', () => { }); }); + // ── atomic write semantics ────────────────────────────────────────────── + + describe('atomic write semantics', () => { + it('does not leak .sencho-tmp-* files after a successful write', async () => { + const service = FileSystemService.getInstance(); + await service.writeStackFile(STACK, 'clean.txt', 'final'); + const dirEntries = await fs.readdir(stackDir); + const leftovers = dirEntries.filter(name => name.startsWith('clean.txt.sencho-tmp-')); + expect(leftovers).toEqual([]); + }); + + it('preserves the original target when the rename step throws', async () => { + const target = path.join(stackDir, 'crash.txt'); + await fs.writeFile(target, 'ORIGINAL'); + + const fsModule = await import('fs'); + const renameSpy = vi + .spyOn(fsModule.promises, 'rename') + .mockRejectedValueOnce(Object.assign(new Error('disk yanked'), { code: 'EIO' })); + + const service = FileSystemService.getInstance(); + await expect(service.writeStackFile(STACK, 'crash.txt', 'NEW')).rejects.toThrow(/disk yanked/); + + // Target keeps its original content. + const content = await fs.readFile(target, 'utf-8'); + expect(content).toBe('ORIGINAL'); + + // Tmp file is cleaned up. + const dirEntries = await fs.readdir(stackDir); + const leftovers = dirEntries.filter(name => name.startsWith('crash.txt.sencho-tmp-')); + expect(leftovers).toEqual([]); + + renameSpy.mockRestore(); + }); + + it('exclusive write to a fresh target succeeds', async () => { + const service = FileSystemService.getInstance(); + await service.writeStackFile(STACK, 'first.txt', 'hello', { exclusive: true }); + const content = await fs.readFile(path.join(stackDir, 'first.txt'), 'utf-8'); + expect(content).toBe('hello'); + }); + + it('exclusive write to an existing target throws FILE_EXISTS and preserves the original', async () => { + const target = path.join(stackDir, 'taken.txt'); + await fs.writeFile(target, 'KEEP'); + + const service = FileSystemService.getInstance(); + let caught: unknown = null; + try { + await service.writeStackFile(STACK, 'taken.txt', 'OVERWRITE', { exclusive: true }); + } catch (err) { + caught = err; + } + expect((caught as { code?: string })?.code).toBe('FILE_EXISTS'); + + const content = await fs.readFile(target, 'utf-8'); + expect(content).toBe('KEEP'); + + // Tmp file cleaned up after the link failure. + const dirEntries = await fs.readdir(stackDir); + const leftovers = dirEntries.filter(name => name.startsWith('taken.txt.sencho-tmp-')); + expect(leftovers).toEqual([]); + }); + + it('cleans up the tmp file when the write step throws', async () => { + const fsModule = await import('fs'); + const originalOpen = fsModule.promises.open; + const openSpy = vi + .spyOn(fsModule.promises, 'open') + .mockImplementationOnce(async (...args) => { + const fh = await originalOpen.apply(fsModule.promises, args as Parameters); + // Patch writeFile to throw, the close still runs via the inner finally. + (fh as unknown as { writeFile: () => Promise }).writeFile = () => + Promise.reject(Object.assign(new Error('write blew up'), { code: 'EIO' })); + return fh; + }); + + const service = FileSystemService.getInstance(); + await expect(service.writeStackFile(STACK, 'wfail.txt', 'NEW')).rejects.toThrow(/write blew up/); + + // Target was never created and the tmp was cleaned. + const dirEntries = await fs.readdir(stackDir); + expect(dirEntries).not.toContain('wfail.txt'); + const leftovers = dirEntries.filter(name => name.startsWith('wfail.txt.sencho-tmp-')); + expect(leftovers).toEqual([]); + + openSpy.mockRestore(); + }); + + it('concurrent non-exclusive writers to the same path leave one winning content and no tmp leaks', async () => { + const service = FileSystemService.getInstance(); + const inputs = ['A', 'B', 'C', 'D', 'E'].map(l => l.repeat(32)); + // POSIX rename is atomic and silently overwrites: every writer succeeds and + // the last-to-rename wins. Windows rename throws EPERM if the destination + // is open or being renamed by another process. Both are acceptable: the + // contract is "the final file is always exactly one of the inputs, never + // torn, and no tmp files leak". Settle individually and require at least + // one writer to have succeeded. + const results = await Promise.allSettled( + inputs.map(content => service.writeStackFile(STACK, 'race.txt', content)), + ); + expect(results.some(r => r.status === 'fulfilled')).toBe(true); + + const finalContent = await fs.readFile(path.join(stackDir, 'race.txt'), 'utf-8'); + expect(inputs).toContain(finalContent); + + const dirEntries = await fs.readdir(stackDir); + const leftovers = dirEntries.filter(name => name.startsWith('race.txt.sencho-tmp-')); + expect(leftovers).toEqual([]); + }); + }); + // ── deleteStackPath ───────────────────────────────────────────────────── describe('deleteStackPath', () => { diff --git a/backend/src/services/FileSystemService.ts b/backend/src/services/FileSystemService.ts index 29b26732..c97abb8c 100644 --- a/backend/src/services/FileSystemService.ts +++ b/backend/src/services/FileSystemService.ts @@ -1,5 +1,6 @@ import path from 'path'; import os from 'os'; +import crypto from 'crypto'; import { promises as fsPromises, createReadStream } from 'fs'; import type { Readable } from 'stream'; import { NodeRegistry } from './NodeRegistry'; @@ -699,16 +700,80 @@ export class FileSystemService { }; } - async writeStackFile(stackName: string, relPath: string, content: string): Promise { - const safePath = await this.resolveSafeStackPath(stackName, relPath); + /** + * Atomic write: stages the content in a sibling .tmp file in the same + * directory, fsyncs, then promotes it to the final path. A crash between + * the open and the rename leaves either the original target intact or a + * leftover .tmp file (cleaned up on next failure path), never a truncated + * target. + * + * `exclusive: true` uses link+unlink instead of rename so the create is + * race-free atomic: link fails with EEXIST if the target already exists, + * giving the caller a definitive "did not exist when we wrote it" signal. + * The non-exclusive default uses rename, which is atomic against partial + * reads but clobbers any existing target. + */ + private async writeStackFileAtomic( + safePath: string, + data: string | Buffer, + opts: { exclusive?: boolean } = {}, + ): Promise { await fsPromises.mkdir(path.dirname(safePath), { recursive: true }); - await fsPromises.writeFile(safePath, content, 'utf-8'); + // crypto.randomBytes gives a guaranteed-length high-entropy suffix; Math.random + // can drop leading zeros which narrows entropy unpredictably. + const suffix = `${process.pid}-${Date.now()}-${crypto.randomBytes(6).toString('hex')}`; + const tmpPath = `${safePath}.sencho-tmp-${suffix}`; + let stagedTmp = false; + try { + const fh = await fsPromises.open(tmpPath, 'wx'); + stagedTmp = true; + try { + await fh.writeFile(data); + await fh.sync(); + } finally { + await fh.close(); + } + if (opts.exclusive) { + // link() is atomic against EEXIST. Tmp and target are guaranteed to live + // in the same directory (same filesystem); link works on NTFS and any + // POSIX FS without elevated privileges. + try { + await fsPromises.link(tmpPath, safePath); + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === 'EEXIST') { + throw Object.assign(new Error('File already exists'), { code: 'FILE_EXISTS' as const }); + } + throw err; + } + } else { + await fsPromises.rename(tmpPath, safePath); + stagedTmp = false; + } + } finally { + if (stagedTmp) { + await fsPromises.unlink(tmpPath).catch(() => {}); + } + } } - async writeStackFileBuffer(stackName: string, relPath: string, buffer: Buffer): Promise { + async writeStackFile( + stackName: string, + relPath: string, + content: string, + opts?: { exclusive?: boolean }, + ): Promise { const safePath = await this.resolveSafeStackPath(stackName, relPath); - await fsPromises.mkdir(path.dirname(safePath), { recursive: true }); - await fsPromises.writeFile(safePath, buffer); + await this.writeStackFileAtomic(safePath, content, opts); + } + + async writeStackFileBuffer( + stackName: string, + relPath: string, + buffer: Buffer, + opts?: { exclusive?: boolean }, + ): Promise { + const safePath = await this.resolveSafeStackPath(stackName, relPath); + await this.writeStackFileAtomic(safePath, buffer, opts); } /**