diff --git a/packages/coding-agent/src/modes/controllers/input-controller.ts b/packages/coding-agent/src/modes/controllers/input-controller.ts index 72e793a82..f8792151b 100644 --- a/packages/coding-agent/src/modes/controllers/input-controller.ts +++ b/packages/coding-agent/src/modes/controllers/input-controller.ts @@ -747,6 +747,18 @@ export class InputController { this.ctx.clearEditor(); this.ctx.lastSigintTime = now; } + // Sync-flush the session JSONL so in-flight writes survive a hard exit. + // The TUI consumes Ctrl+C as a key event in raw mode, so postmortem's + // process-level SIGINT handler never fires. The second press still + // funnels through shutdown() which awaits its own async flush — the + // sync flush here is a superset that also covers the first-press case. + try { + this.ctx.sessionManager.flushSync(); + } catch (err) { + logger.warn("session-manager sync flush on Ctrl+C failed", { + error: err instanceof Error ? err.message : String(err), + }); + } } handleCtrlD(): void { diff --git a/packages/coding-agent/src/session/indexed-session-storage.ts b/packages/coding-agent/src/session/indexed-session-storage.ts index f09343adb..36d5508fa 100644 --- a/packages/coding-agent/src/session/indexed-session-storage.ts +++ b/packages/coding-agent/src/session/indexed-session-storage.ts @@ -414,6 +414,13 @@ class IndexedSessionStorageWriter implements SessionStorageWriter { await this.flush(); } + fsyncSync(): void { + // Indexed storage has no real fd to fsync; drain the pending chain + // synchronously is not possible, so this is a no-op. The async flush() + // above already ensures durability for the indexed backend. + if (this.#error) throw this.#error; + } + async close(): Promise { if (this.#closed) return; this.#closed = true; diff --git a/packages/coding-agent/src/session/session-manager.ts b/packages/coding-agent/src/session/session-manager.ts index 7c3ef3f91..8083ed424 100644 --- a/packages/coding-agent/src/session/session-manager.ts +++ b/packages/coding-agent/src/session/session-manager.ts @@ -1527,6 +1527,17 @@ class NdjsonFileWriter { } } + /** Synchronously fsync the underlying file descriptor to physical disk. */ + fsyncSync(): void { + if (this.#closed) return; + if (this.#error) throw this.#error; + try { + this.#writer.fsyncSync(); + } catch (err) { + throw this.#recordError(err); + } + } + /** Close the writer, flushing all data. */ async close(): Promise { if (this.#closed || this.#closing) return; @@ -2592,6 +2603,115 @@ export class SessionManager { if (this.#persistError) throw this.#persistError; } + /** + * Synchronously flush all in-memory entries to disk and fsync. + * Use when the process may exit before an async flush settles (e.g. Ctrl+C + * in the TUI, where raw mode consumes the keystroke so postmortem's SIGINT + * handler never fires). + * + * Hot path: the persist writer is open and flushed, so a single fsyncSync + * pushes the page-cache data to physical disk. + * + * Cold path: entries are only in memory (session just started, or a rewrite + * is pending). Writes all entries to a temp file, fsyncs, and atomically + * renames over the session file — then re-opens an append writer so the + * hot path resumes on subsequent `_persist` calls. + */ + flushSync(): void { + if (!this.persist || !this.#sessionFile) return; + if (this.#persistError) throw this.#persistError; + + // Hot path: writer is open and all entries have been written via writeSync. + // Just fsync the fd — the data is already in the kernel page cache. + if ( + this.#persistWriter?.isOpen() && + this.#flushed && + !this.#needsFullRewriteOnNextPersist + ) { + this.#persistWriter.fsyncSync(); + return; + } + + // Cold path: write all in-memory entries to a temp file and atomically + // replace the session file. This is safe to run even when an async + // rewrite is queued on #persistChain: the async task won't progress + // while we're on the sync call stack, and the file we produce is a + // superset of whatever the async rewrite would write. + const dir = path.resolve(this.#sessionFile, ".."); + const tempPath = path.join(dir, `.${path.basename(this.#sessionFile)}.${Snowflake.next()}.tmp`); + const fd = fs.openSync(tempPath, "w"); + try { + for (const entry of this.#fileEntries) { + const persisted = prepareEntryForPersistenceSync(entry, this.#blobStore); + const line = `${JSON.stringify(persisted)}\n`; + fs.writeSync(fd, line); + } + fs.fsyncSync(fd); + } finally { + fs.closeSync(fd); + } + + // Atomic replace (with EPERM retry for Windows) + try { + fs.renameSync(tempPath, this.#sessionFile); + } catch (err) { + if (!hasFsCode(err, "EPERM")) { + try { + fs.unlinkSync(tempPath); + } catch { + /* best effort */ + } + throw toError(err); + } + // Windows: move the old file aside, then rename + const backupPath = path.join(dir, `${path.basename(this.#sessionFile)}.${Snowflake.next()}.bak`); + try { + fs.renameSync(this.#sessionFile, backupPath); + } catch (moveAsideErr) { + if (isEnoent(moveAsideErr)) { + fs.renameSync(tempPath, this.#sessionFile); + return; + } + try { + fs.unlinkSync(tempPath); + } catch { + /* best effort */ + } + throw toError(err); + } + try { + fs.renameSync(tempPath, this.#sessionFile); + } catch (replaceErr) { + // Roll back + try { + fs.renameSync(backupPath, this.#sessionFile); + } catch { + /* best effort */ + } + throw toError(replaceErr); + } + try { + fs.unlinkSync(backupPath); + } catch { + /* best effort */ + } + } + + // Re-open the persist writer in append mode so the hot path resumes. + if (this.#persistWriter) { + // The old writer is stale (pointed at the pre-rewrite file or was + // mid-close). Close it asynchronously — it's a no-op if already + // closed, and we don't want to block on draining its queue. + void this.#persistWriter.close().catch(() => {}); + } + this.#persistWriter = new NdjsonFileWriter(this.storage, this.#sessionFile, { + onError: err => this.#recordPersistError(err), + }); + this.#persistWriterPath = this.#sessionFile; + this.#flushed = true; + this.#needsFullRewriteOnNextPersist = false; + } + /** Close the persistent writer after flushing all pending data. */ async close(): Promise { if (!this.#persistWriter) return; diff --git a/packages/coding-agent/src/session/session-storage.ts b/packages/coding-agent/src/session/session-storage.ts index 30f8420d8..1f318f9bc 100644 --- a/packages/coding-agent/src/session/session-storage.ts +++ b/packages/coding-agent/src/session/session-storage.ts @@ -23,6 +23,11 @@ export interface SessionStorageWriter { writeLineSync(line: string): void; flush(): Promise; fsync(): Promise; + /** + * Synchronously fsync the underlying file descriptor. Returns once the data + * is on the physical disk. Throws synchronously on I/O error. + */ + fsyncSync(): void; close(): Promise; getError(): Error | undefined; } @@ -118,6 +123,16 @@ class FileSessionStorageWriter implements SessionStorageWriter { } } + fsyncSync(): void { + if (this.#closed) throw new Error("Writer closed"); + if (this.#error) throw this.#error; + try { + fs.fsyncSync(this.#fd); + } catch (err) { + throw this.#recordError(err); + } + } + async close(): Promise { if (this.#closed) return; this.#closed = true; @@ -291,6 +306,11 @@ class MemorySessionStorageWriter implements SessionStorageWriter { if (this.#error) throw this.#error; } + fsyncSync(): void { + // No-op for in-memory storage + if (this.#error) throw this.#error; + } + async close(): Promise { if (this.#closed) return; this.#closed = true; diff --git a/packages/coding-agent/test/image-webp-exclusion.test.ts b/packages/coding-agent/test/image-webp-exclusion.test.ts new file mode 100644 index 000000000..e8f5dfa7d --- /dev/null +++ b/packages/coding-agent/test/image-webp-exclusion.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { getBundledModels } from "@oh-my-pi/pi-catalog/models"; +import { + modelLacksWebpSupport, + normalizeModelContextImages, + webpExclusionForModel, +} from "@oh-my-pi/pi-coding-agent/utils/image-loading"; + +// 1x1 red PNG seed, upscaled + re-encoded as WebP at test time so no binary +// fixture is checked in. Bun.Image sniffs format from bytes. +const RED_1X1_PNG_BASE64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC"; + +async function makeRedWebP(width: number, height: number): Promise { + const seed = Buffer.from(RED_1X1_PNG_BASE64, "base64"); + const upscaled = await new Bun.Image(seed) + .resize(width, height, { filter: "nearest" }) + .webp({ quality: 90 }) + .bytes(); + return Buffer.from(upscaled).toBase64(); +} + +describe("modelLacksWebpSupport", () => { + test("flags the local + cloud Ollama providers", () => { + expect(modelLacksWebpSupport({ provider: "ollama", api: "openai-responses" })).toBe(true); + expect(modelLacksWebpSupport({ provider: "ollama-cloud", api: "ollama-chat" })).toBe(true); + }); + + test("flags the ollama-chat api even behind a custom provider id", () => { + // A proxy/custom provider still routes images through Ollama's STB decoder. + expect(modelLacksWebpSupport({ provider: "my-local-ollama", api: "ollama-chat" })).toBe(true); + }); + + test("leaves WebP-capable providers and undefined untouched", () => { + expect(modelLacksWebpSupport({ provider: "anthropic", api: "anthropic-messages" })).toBe(false); + expect(modelLacksWebpSupport({ provider: "openai", api: "openai-responses" })).toBe(false); + expect(modelLacksWebpSupport(undefined)).toBe(false); + }); + + test("webpExclusionForModel yields true|undefined so the OMP_NO_WEBP fallback survives", () => { + // `true` forces exclusion for Ollama... + expect(webpExclusionForModel({ provider: "ollama", api: "openai-responses" })).toBe(true); + // ...but a capable model returns `undefined` (NOT `false`), so resizeImage's + // env fallback still applies instead of being overridden off. + expect(webpExclusionForModel({ provider: "openai", api: "openai-responses" })).toBeUndefined(); + }); +}); + +describe("normalizeModelContextImages model-aware WebP exclusion", () => { + const prior = Bun.env.OMP_NO_WEBP; + + beforeEach(() => { + delete (Bun.env as Record).OMP_NO_WEBP; + }); + + afterEach(() => { + if (prior === undefined) delete (Bun.env as Record).OMP_NO_WEBP; + else Bun.env.OMP_NO_WEBP = prior; + }); + + test("re-encodes a WebP image out of WebP for an Ollama-family model", async () => { + const [ollama] = getBundledModels("ollama-cloud"); + expect(ollama).toBeDefined(); + const webp = { type: "image" as const, data: await makeRedWebP(200, 200), mimeType: "image/webp" }; + + const result = await normalizeModelContextImages([webp], { model: ollama }); + expect(result).toHaveLength(1); + const mime = result![0]!.mimeType; + expect(mime).not.toBe("image/webp"); + expect(["image/png", "image/jpeg"]).toContain(mime); + }); + + test("keeps WebP for a WebP-capable model when OMP_NO_WEBP is unset", async () => { + const [anthropic] = getBundledModels("anthropic"); + expect(anthropic).toBeDefined(); + const webp = { type: "image" as const, data: await makeRedWebP(200, 200), mimeType: "image/webp" }; + + const result = await normalizeModelContextImages([webp], { model: anthropic }); + + expect(result?.[0]?.mimeType).toBe("image/webp"); + }); +}); diff --git a/packages/coding-agent/test/session-manager-close-race.test.ts b/packages/coding-agent/test/session-manager-close-race.test.ts index e5d5b71e1..58ea9b01d 100644 --- a/packages/coding-agent/test/session-manager-close-race.test.ts +++ b/packages/coding-agent/test/session-manager-close-race.test.ts @@ -52,6 +52,9 @@ class CloseHoldingStorage implements SessionStorage { fsync() { return inner.fsync(); }, + fsyncSync() { + inner.fsyncSync(); + }, async close() { const gate = Promise.withResolvers(); gates.push(gate);