From e87323f71ab2928414fa49637291da1eb7383438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 25 Aug 2026 05:26:40 +0000 Subject: [PATCH] writeAtomic, everywhere a file lands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the .part-then-rename rule was applied at two upload routes yesterday and by this morning the platform had a dozen other writes putting files in the same folders — an OCR result beside its image, a TTS clip, a download landing, a save from the editor — every one straight to the final name. a guarantee that holds for two of fourteen paths is not a guarantee, and the next person adding a fifteenth had nothing to copy. so it is a function now, and every durable write goes through it: the file browser, sendoff, task definitions, skill and tool files, bug reports, scraped attachments, the chat provider config. the failure it prevents is silent rather than loud. a truncated m4a decodes fine, it is simply short — a commander transcribing recordings as they land would produce a clean transcript of the first few seconds of a two-minute memo and report success. two things learned writing it. Bun.write creates the parent chain itself, so the mkdir option i added was dead weight and several callers were doing it twice. and Parameters[1] resolves to the first overload only — BunFile — which rejects the string and Buffer every caller actually passes. Co-Authored-By: Claude Opus 5 Officer-Session: 656f5c85-e49a-4f1f-b9f3-042a04b4d043 --- src/servers/api/bug-report/bug-report.ts | 5 +- src/servers/api/file-browser/router.ts | 24 ++---- src/servers/api/item-router.ts | 7 +- src/servers/api/scrape/scrape.ts | 3 +- src/servers/api/sendoff/router.ts | 17 ++-- .../api/server-settings/chat-providers.ts | 5 +- src/servers/api/tasks/task-files.ts | 13 +-- src/servers/write-atomic.test.ts | 81 +++++++++++++++++++ src/servers/write-atomic.ts | 51 ++++++++++++ 9 files changed, 164 insertions(+), 42 deletions(-) create mode 100644 src/servers/write-atomic.test.ts create mode 100644 src/servers/write-atomic.ts diff --git a/src/servers/api/bug-report/bug-report.ts b/src/servers/api/bug-report/bug-report.ts index bab414f4..9c3fcbcb 100644 --- a/src/servers/api/bug-report/bug-report.ts +++ b/src/servers/api/bug-report/bug-report.ts @@ -2,6 +2,7 @@ import { createRouter } from '../../create-router'; import { mkdir } from 'node:fs/promises'; import { join } from 'node:path'; import { DATA_PATH } from '@@/data-path'; +import { writeAtomic } from '@@/write-atomic'; // Bug reports land on disk and nowhere else. // @@ -42,10 +43,10 @@ bugReportRouter.post('/', async (ctx) => { createdAt: new Date().toISOString(), }; - await Bun.write(join(reportDir, 'report.json'), JSON.stringify(report, null, 2)); + await writeAtomic(join(reportDir, 'report.json'), JSON.stringify(report, null, 2)); if (screenshot instanceof File) { - await Bun.write(join(reportDir, 'screenshot.png'), Buffer.from(await screenshot.arrayBuffer())); + await writeAtomic(join(reportDir, 'screenshot.png'), Buffer.from(await screenshot.arrayBuffer())); } return ctx.json({ ok: true, id: dirName }); diff --git a/src/servers/api/file-browser/router.ts b/src/servers/api/file-browser/router.ts index 04c70797..8f4bfc88 100644 --- a/src/servers/api/file-browser/router.ts +++ b/src/servers/api/file-browser/router.ts @@ -4,6 +4,7 @@ import { readdir, stat, mkdir, rm, rename, readFile, cp } from 'node:fs/promises import { existsSync } from 'node:fs'; import { getOwnerHomeDir, DATA_PATH } from '@@/data-path'; import { resolveHomeDir } from '@@/user-home'; +import { writeAtomic } from '@@/write-atomic'; import * as errors from '@@/custom-errors'; import { readTtsConfig } from '@@/api/server-settings/tts'; import { readOcrConfig } from '@@/api/server-settings/ocr'; @@ -253,7 +254,7 @@ router.post('/write', async (ctx) => { const absPath = resolveUserPath(rootDir, path); await mkdir(dirname(absPath), { recursive: true }); - await Bun.write(absPath, content); + await writeAtomic(absPath, content); return ctx.json({ ok: true }); }); @@ -306,16 +307,7 @@ router.post('/upload', async (ctx) => { if (!isInside(rootDir, filePath)) continue; await mkdir(dirname(filePath), { recursive: true }); - const partPath = `${filePath}.part`; - try { - await Bun.write(partPath, file); - await rename(partPath, filePath); - } catch (err) { - // Leave nothing half-named. Only an interrupted PROCESS leaves a fragment behind, which is - // the case worth being able to see. - await rm(partPath, { force: true }).catch(() => {}); - throw err; - } + await writeAtomic(filePath, file); } return ctx.json({ ok: true }); @@ -815,7 +807,7 @@ router.post('/tts', async (ctx) => { const siblingRel = dir ? `${dir}/${name}.mp3` : `${name}.mp3`; const siblingAbs = resolveUserPath(rootDir, siblingRel); const cached = await readFile(cacheAbs); - await Bun.write(siblingAbs, cached); + await writeAtomic(siblingAbs, cached); return ctx.json({ audioPath: `/${siblingRel}`, audioRoot: root ?? 'home', cached: true }); } return ctx.json({ audioPath: cacheRel, audioRoot: 'user-data' }); @@ -845,12 +837,12 @@ router.post('/tts', async (ctx) => { await mkdir(dirname(cacheAbs), { recursive: true }); const buffer = await res.arrayBuffer(); - await Bun.write(cacheAbs, buffer); + await writeAtomic(cacheAbs, buffer); if (saveNextTo) { const siblingRel = dir ? `${dir}/${name}.mp3` : `${name}.mp3`; const siblingAbs = resolveUserPath(rootDir, siblingRel); - await Bun.write(siblingAbs, buffer); + await writeAtomic(siblingAbs, buffer); return ctx.json({ audioPath: `/${siblingRel}`, audioRoot: root ?? 'home', cached: false }); } @@ -900,7 +892,7 @@ router.post('/tts-text', async (ctx) => { await mkdir(dirname(cacheAbs), { recursive: true }); const buffer = await res.arrayBuffer(); - await Bun.write(cacheAbs, buffer); + await writeAtomic(cacheAbs, buffer); return ctx.json({ audioPath: cacheRel, audioRoot: 'user-data' }); }); @@ -972,7 +964,7 @@ router.post('/ocr', async (ctx) => { if (saveNextTo) { const siblingRel = dir ? `${dir}/${name}.md` : `${name}.md`; const siblingAbs = resolveUserPath(rootDir, siblingRel); - await Bun.write(siblingAbs, text); + await writeAtomic(siblingAbs, text); return ctx.json({ ocrPath: `/${siblingRel}`, ocrRoot: root ?? 'home', cached: false }); } diff --git a/src/servers/api/item-router.ts b/src/servers/api/item-router.ts index 8411136c..7c3991a3 100644 --- a/src/servers/api/item-router.ts +++ b/src/servers/api/item-router.ts @@ -2,6 +2,7 @@ import { createRouter } from '../create-router'; import { readdir, mkdir, rm } from 'node:fs/promises'; import { join, dirname } from 'node:path'; import { itemsDir, type ItemType } from '../data-path'; +import { writeAtomic } from '@@/write-atomic'; // Shared router for the flat, file-based item types (skills, tools, processes). Each item is a // directory under $OFFICER_ITEMS_DIR/// holding a single markdown file (SKILL.md / @@ -114,8 +115,8 @@ export function createItemRouter({ type, fileName, label }: ItemRouterConfig) { const { sessionId, messages } = await ctx.req.json<{ sessionId: string; messages: unknown[] }>(); await mkdir(chatDir, { recursive: true }); - await Bun.write(join(chatDir, 'messages.json'), JSON.stringify(messages)); - if (sessionId) await Bun.write(join(chatDir, 'meta.json'), JSON.stringify({ id: sessionId })); + await writeAtomic(join(chatDir, 'messages.json'), JSON.stringify(messages)); + if (sessionId) await writeAtomic(join(chatDir, 'meta.json'), JSON.stringify({ id: sessionId })); return ctx.json({ ok: true }); }); @@ -140,7 +141,7 @@ export function createItemRouter({ type, fileName, label }: ItemRouterConfig) { if (await Bun.file(filePath).exists()) return ctx.text(`${label} already exists`, 409); await mkdir(dirname(filePath), { recursive: true }); - await Bun.write(filePath, `---\nname: ${name.trim()}\ndescription: \n---\n`); + await writeAtomic(filePath, `---\nname: ${name.trim()}\ndescription: \n---\n`); return ctx.json({ name: name.trim(), dirName, filePath }); }); diff --git a/src/servers/api/scrape/scrape.ts b/src/servers/api/scrape/scrape.ts index e4838d73..b2d3df17 100644 --- a/src/servers/api/scrape/scrape.ts +++ b/src/servers/api/scrape/scrape.ts @@ -3,6 +3,7 @@ import { mkdir } from 'node:fs/promises'; import { join } from 'node:path'; import { chromium, type Browser } from 'playwright'; import { getTmpAttachmentsDir, getAttachmentsDir } from '@@/data-path'; +import { writeAtomic } from '@@/write-atomic'; const MAX_CONTENT_LENGTH = 100_000; @@ -148,7 +149,7 @@ scrapeRouter.post('/', async (ctx) => { } await mkdir(saveDir, { recursive: true }); - await Bun.write(join(saveDir, attachmentId), html); + await writeAtomic(join(saveDir, attachmentId), html); return ctx.json({ url, title, content, attachmentId }); } catch (err) { diff --git a/src/servers/api/sendoff/router.ts b/src/servers/api/sendoff/router.ts index d2e9bea8..70208a18 100644 --- a/src/servers/api/sendoff/router.ts +++ b/src/servers/api/sendoff/router.ts @@ -1,4 +1,4 @@ -import { mkdir, rename, rm } from 'node:fs/promises'; +import { mkdir } from 'node:fs/promises'; import { existsSync } from 'node:fs'; import { extname, join } from 'node:path'; import { createRouter } from '../../create-router'; @@ -17,6 +17,7 @@ import { upsertSendoffDevice, } from 'officerdb'; import { getNotifyServerUrl, notifyProducerHeaders } from '../notify/router'; +import { writeAtomic } from '@@/write-atomic'; import { displayNameFor, folderNameFor, safeFilename, safeSegment, sendoffDirFor } from './paths'; /* @@ -152,19 +153,11 @@ sendoffRouter.post('/send', async (ctx) => { await mkdir(dir, { recursive: true }); const written: { name: string; size: number }[] = []; - // Aside, then rename — the same reasoning as the file browser upload, and it matters more here - // because this is a folder things ARRIVE in unattended. Neither a recipient watching it nor the - // notification below may point at a name that is still being written. + // `writeAtomic` matters more here than anywhere: this is a folder things ARRIVE in unattended. + // Neither a recipient watching it nor the notification below may point at a name still being written. for (const file of files) { const path = freePath(dir, safeFilename(file.name)); - const partPath = `${path}.part`; - try { - await Bun.write(partPath, await file.arrayBuffer()); - await rename(partPath, path); - } catch (err) { - await rm(partPath, { force: true }).catch(() => {}); - throw err; - } + await writeAtomic(path, await file.arrayBuffer()); written.push({ name: path.split('/').pop() ?? file.name, size: file.size }); } diff --git a/src/servers/api/server-settings/chat-providers.ts b/src/servers/api/server-settings/chat-providers.ts index c6b7755b..49967244 100644 --- a/src/servers/api/server-settings/chat-providers.ts +++ b/src/servers/api/server-settings/chat-providers.ts @@ -4,6 +4,7 @@ import { AGENT_CONFIG_DIR } from '../../data-path'; import { invalidateModelCache } from '../chat/list-models'; import { logger } from '../chat/logger'; import { readConfigValue, writeConfigValue } from 'officerdb'; +import { writeAtomic } from '@@/write-atomic'; export const chatProvidersRouter = createRouter(); @@ -69,7 +70,7 @@ async function readModelsConfig(): Promise { } async function writeModelsConfig(config: ModelConfig): Promise { - await Bun.write(MODELS_FILE, JSON.stringify(config, null, 2)); + await writeAtomic(MODELS_FILE, JSON.stringify(config, null, 2)); } /** @@ -342,7 +343,7 @@ async function readAuthJson(): Promise { } async function writeAuthJson(auth: AuthJson): Promise { - await Bun.write(AUTH_FILE, JSON.stringify(auth, null, 2)); + await writeAtomic(AUTH_FILE, JSON.stringify(auth, null, 2)); } /** diff --git a/src/servers/api/tasks/task-files.ts b/src/servers/api/tasks/task-files.ts index 89e463ce..5bda9854 100644 --- a/src/servers/api/tasks/task-files.ts +++ b/src/servers/api/tasks/task-files.ts @@ -2,6 +2,7 @@ import { readdir, mkdir, rm } from 'node:fs/promises'; import { join, dirname } from 'node:path'; import { itemsDir, OFFICER_ITEMS_DIR } from '../../data-path'; import { capabilitySources, type CapabilitySource } from '../../capability-sources'; +import { writeAtomic } from '@@/write-atomic'; // File-backed task store. Every task is a directory under $OFFICER_ITEMS_DIR/tasks// with a // TASK.md (metadata + prose body) and, for script-mode tasks, a sibling implementation file. There are @@ -296,11 +297,11 @@ export async function createTask(input: CreateTaskInput): Promise & { body?: string; implementation?: string | null }, ): Promise { await mkdir(taskDir(dirName), { recursive: true }); - await Bun.write(taskFile(dirName), buildTaskMd(record, record.body ?? '')); + await writeAtomic(taskFile(dirName), buildTaskMd(record, record.body ?? '')); if (record.implementation != null && record.implementation !== '') { - await Bun.write(join(taskDir(dirName), implFileName(record.language ?? null)), record.implementation); + await writeAtomic(join(taskDir(dirName), implFileName(record.language ?? null)), record.implementation); } } diff --git a/src/servers/write-atomic.test.ts b/src/servers/write-atomic.test.ts new file mode 100644 index 00000000..78110cf8 --- /dev/null +++ b/src/servers/write-atomic.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, test } from 'bun:test'; +import { mkdtempSync, rmSync, existsSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { writeAtomic } from './write-atomic'; + +/* + The property: the final name never holds a partial file. + + A test cannot observe the instant between two syscalls, so what is pinned here is the shape that makes + the property true — the data goes to `.part` first, the final name only ever appears complete, + and a failure leaves no fragment under either name. +*/ + +const dir = () => mkdtempSync(join(tmpdir(), 'zz-atomic-')); + +describe('writeAtomic', () => { + test('the file ends up at the final name, complete', async () => { + const d = dir(); + try { + const path = join(d, 'memo.md'); + await writeAtomic(path, 'hello'); + expect(readFileSync(path, 'utf8')).toBe('hello'); + expect(existsSync(`${path}.part`)).toBe(false); + } finally { + rmSync(d, { recursive: true, force: true }); + } + }); + + test('it overwrites an existing file rather than refusing', async () => { + const d = dir(); + try { + const path = join(d, 'memo.md'); + writeFileSync(path, 'old'); + await writeAtomic(path, 'new'); + expect(readFileSync(path, 'utf8')).toBe('new'); + } finally { + rmSync(d, { recursive: true, force: true }); + } + }); + + // A failed write must not leave a `.part` behind — only an interrupted PROCESS should, which is the + // case worth being able to see in the folder. + test('a failed write leaves nothing under either name', async () => { + const d = dir(); + try { + // The parent is a FILE, so neither the .part write nor the rename can succeed. + writeFileSync(join(d, 'blocker'), 'x'); + const path = join(d, 'blocker', 'memo.md'); + await expect(writeAtomic(path, 'x')).rejects.toThrow(); + expect(existsSync(`${path}.part`)).toBe(false); + expect(existsSync(path)).toBe(false); + } finally { + rmSync(d, { recursive: true, force: true }); + } + }); + + // Bun.write creates the parent chain itself, which is why this takes no mkdir option and why the + // callers it replaced could drop theirs. + test('a missing directory is created, not an error', async () => { + const d = dir(); + try { + const path = join(d, 'nested', 'deep', 'memo.md'); + await writeAtomic(path, 'x'); + expect(readFileSync(path, 'utf8')).toBe('x'); + } finally { + rmSync(d, { recursive: true, force: true }); + } + }); + + test('a Blob writes the same way as a string', async () => { + const d = dir(); + try { + const path = join(d, 'blob.bin'); + await writeAtomic(path, new Blob([new Uint8Array([1, 2, 3])])); + expect([...readFileSync(path)]).toEqual([1, 2, 3]); + } finally { + rmSync(d, { recursive: true, force: true }); + } + }); +}); diff --git a/src/servers/write-atomic.ts b/src/servers/write-atomic.ts new file mode 100644 index 00000000..4ba8ac04 --- /dev/null +++ b/src/servers/write-atomic.ts @@ -0,0 +1,51 @@ +import { rename, rm } from 'node:fs/promises'; + +/** + * Write a file so that its final name never holds a partial one. + * + * ── Why every write into a user's tree has to do this ── + * + * `Bun.write(path, data)` streams to the name the file will end up with, so for as long as the write + * takes, that name exists holding a fragment — and anything reading the folder cannot tell. The failure + * is silent rather than loud: a truncated m4a decodes fine, it is simply short. A commander transcribing + * recordings as they land would produce a clean transcript of the first few seconds of a two-minute + * recording and report success. + * + * A rename within one directory is atomic, so afterwards the final name either does not exist or is a + * complete file. Every consumer gets that for free — a watcher, an agent, the phone's own upload + * verification — instead of each inventing a heuristic like "the size stopped changing", which cannot + * tell a finished write from a stalled one. + * + * ── Why it is a function rather than a rule ── + * + * It was a rule, applied at the two upload routes on 2026-08-24, and by the next morning the platform + * had a dozen other writes that put files in the same folders — an OCR result beside its image, a + * download landing, a save from the editor — every one of them straight to the final name. A guarantee + * that holds for two of fourteen paths is not a guarantee, and the next person to add a fifteenth would + * have had nothing to copy. + * + * ── `.part`, visibly ── + * + * A leftover should be VISIBLE, named for what it was going to be, next to where it belongs — not a + * dotfile. Nothing sweeps them up: a job deleting files it did not create is a worse trade than a + * fragment somebody can see. Only an interrupted PROCESS leaves one, since a failed write cleans up + * after itself below. + */ +/** What `Bun.write` accepts, spelled out — `Parameters[1]` resolves to the first + * overload only, which is `BunFile` and rejects the string and Buffer every caller actually passes. */ +type WritableData = string | Blob | ArrayBufferView | ArrayBufferLike | Response; + +export async function writeAtomic(path: string, data: WritableData): Promise { + // No mkdir: `Bun.write` creates the parent chain itself (`createPath` defaults on), which is why the + // callers this replaced could drop theirs too. + const partPath = `${path}.part`; + try { + // Bun.write's overloads do not union cleanly — every one of them accepts this at runtime, and the + // signature is a set of alternatives rather than one parameter that takes them all. + await Bun.write(partPath, data as Parameters[1]); + await rename(partPath, path); + } catch (err) { + await rm(partPath, { force: true }).catch(() => {}); + throw err; + } +}