fix(cache): atomic writes to prevent torn-read deletion of valid cache entries (#1056)

## Summary

Fixes the failing `main` build caused by a ~10% flaky unit test
(`response-cache.test.ts` → `--fresh round-trip: stale entry is replaced
by
fresh response`) that exposed a **real production bug** in the HTTP
response
cache.

## Root cause

In `src/lib/response-cache.ts`, each cache write fires `cleanupCache()`
**fire-and-forget** at 10% probability (`CLEANUP_PROBABILITY = 0.1`). A
second
write to the **same key** overwrote the file with a **non-atomic**
`writeFile`.
The first write's async cleanup could then read the file
**mid-overwrite**
(torn read), fail to `JSON.parse` the truncated content, and **delete
it** as
"corrupted" in `collectEntryMetadata` — silently losing a valid cache
entry.

**Proof:** A 300-iteration repro failed ~10% of the time (matching
`CLEANUP_PROBABILITY`), and on every miss the cache directory was
**empty** —
the valid entry was deleted, not expired.

This is a genuine correctness bug beyond tests: any two rapid writes to
the same
cache key (paginated fetches, concurrent CLI invocations sharing the
cache dir)
can race so cleanup's torn read deletes a valid entry.

## Why PR checks didn't catch it

The flake is ~10%, so any single CI "Unit Tests" run passes ~90% of the
time.
PR #1051's Unit Tests check landed in the lucky 90%; the post-merge
`main` run
hit the unlucky 10%. The bug is **pre-existing** — the cleanup code
predates
#1051, which never touched `response-cache.ts`.

## Fix

1. **Atomic writes** — new `atomicWriteCacheFile()` writes to a unique
temp file
(`<key>.<pid>.<uuid>.tmp`) then `rename`s it into place. `rename` is
atomic on
POSIX (same filesystem) and near-atomic on Windows (same volume), so a
   concurrent reader sees a complete old or new file, never a torn one.
2. **Cleanup hardening** — `collectEntryMetadata()` now separates
failure modes:
- **transient read failure** (locking, AV scanner, ENOENT from a
concurrent
     sweep) → **skip**, never delete.
- **fully read but unparseable** → genuine corruption (atomic writes
preclude
     torn reads), so the file is marked expired and reclaimed by
     `deleteExpiredEntries`. This keeps corrupt files visible to
     `MAX_CACHE_ENTRIES` eviction instead of leaking past the count.
3. **Temp-file sweep** — `cleanupCache()` removes orphaned `.tmp` files
older
than 60s, so a crash between `writeFile` and `rename` can't leak temp
files.
4. **Diagnostics** — previously-silent catch blocks now `log.debug()`
the
   suppressed error (per AGENTS.md no-silent-catch rule).

## Verification

- Amplified repro: **300/300 pass** (was ~10% failing before).
- Actual test file: **10/10 runs green** (36 tests, +2 new regression
tests).
- `pnpm run lint`: clean. `pnpm exec tsc --noEmit`: exit 0.
- Full unit suite: 325 files, all passing (one unrelated pre-existing
property
  flake noted below).

## Out of scope (follow-up)

During full-suite runs I observed an unrelated, pre-existing flaky
**property**
test: `auto-paginate.property.test.ts:94` (counterexample `[393,392,98]`
— an
`autoPaginate` `nextCursor` trim bug when `total > limit`). It passes in
isolation and is independent of this change. Tracked as a follow-up.

Author: BYK

src/lib/response-cache.ts

@@ -14,12 +14,14 @@
  * @module
  */
 
-import { createHash } from "node:crypto";
+import { createHash, randomUUID } from "node:crypto";
 import {
   mkdir,
   readdir,
   readFile,
+  rename,
   rm,
+  stat,
   unlink,
   writeFile,
 } from "node:fs/promises";
@@ -30,8 +32,12 @@ import pLimit from "p-limit";
 import { getIdentityFingerprint } from "./db/auth.js";
 import { getConfigDir } from "./db/index.js";
 import { getEnv } from "./env.js";
+import { logger } from "./logger.js";
 import { recordCacheHit, withCacheSpan } from "./telemetry.js";
 
+/** Tagged logger for diagnostic visibility into best-effort cache operations. */
+const log = logger.withTag("response-cache");
+
 // ---------------------------------------------------------------------------
 // TTL tiers — used as fallback when the server sends no cache headers
 // ---------------------------------------------------------------------------
@@ -582,6 +588,38 @@ export async function storeCachedResponse(
   }
 }
 
+/**
+ * Atomically write a cache file by writing to a unique temp file in the same
+ * directory and renaming it into place.
+ *
+ * A plain `writeFile` is not atomic: a concurrent reader (e.g. the probabilistic
+ * {@link cleanupCache} sweep fired fire-and-forget by an earlier write) can read
+ * the file mid-write, fail to `JSON.parse` the truncated content, and delete it
+ * as "corrupted" — silently losing a valid cache entry. `rename` into place is
+ * atomic on POSIX (same filesystem) and near-atomic on Windows (same volume), so
+ * a concurrent reader sees either the complete old file or the complete new file
+ * rather than a half-written one.
+ *
+ * Best-effort cleanup of the temp file on failure; the caller treats write
+ * failures as non-fatal.
+ */
+async function atomicWriteCacheFile(
+  finalPath: string,
+  serialized: string
+): Promise<void> {
+  const tmpPath = `${finalPath}.${process.pid}.${randomUUID()}.tmp`;
+  try {
+    await writeFile(tmpPath, serialized, "utf-8");
+    await rename(tmpPath, finalPath);
+  } catch (error) {
+    await unlink(tmpPath).catch((cleanupError) => {
+      // The temp file may never have been created (e.g. writeFile failed).
+      log.debug("Failed to clean up cache temp file", cleanupError);
+    });
+    throw error;
+  }
+}
+
 /** Inputs for {@link writeResponseToCache}, bundled to stay under useMaxParams. */
 type WriteRequest = {
   key: string;
@@ -634,7 +672,7 @@ async function writeResponseToCache(req: WriteRequest): Promise<number> {
 
   const serialized = JSON.stringify(entry);
   await mkdir(getCacheDir(), { recursive: true, mode: 0o700 });
-  await writeFile(cacheFilePath(key), serialized, "utf-8");
+  await atomicWriteCacheFile(cacheFilePath(key), serialized);
 
   // Probabilistic cleanup to avoid unbounded cache growth
   if (Math.random() < CLEANUP_PROBABILITY) {
@@ -736,6 +774,14 @@ async function cleanupCache(): Promise<void> {
     throw error;
   }
 
+  // Sweep orphaned temp files left by a crash between writeFile and rename in
+  // {@link atomicWriteCacheFile}. Done regardless of whether any .json files
+  // exist so leaked temp files can never accumulate unbounded.
+  const tmpFiles = files.filter((f) => f.endsWith(".tmp"));
+  if (tmpFiles.length > 0) {
+    await deleteStaleTempFiles(cacheDir, tmpFiles);
+  }
+
   const jsonFiles = files.filter((f) => f.endsWith(".json"));
   if (jsonFiles.length === 0) {
     return;
@@ -750,6 +796,36 @@ async function cleanupCache(): Promise<void> {
   ]);
 }
 
+/**
+ * Age (ms) after which an orphaned `.tmp` file is considered abandoned.
+ *
+ * A live {@link atomicWriteCacheFile} writes then renames in well under a
+ * second, so any `.tmp` file older than this was left by a crashed process and
+ * is safe to remove. The generous threshold avoids racing a concurrent write.
+ */
+const STALE_TEMP_FILE_MS = 60_000;
+
+/** Delete `.tmp` files older than {@link STALE_TEMP_FILE_MS}. Best-effort. */
+async function deleteStaleTempFiles(
+  cacheDir: string,
+  tmpFiles: string[]
+): Promise<void> {
+  const cutoff = Date.now() - STALE_TEMP_FILE_MS;
+  await cacheIO.map(tmpFiles, async (file) => {
+    const filePath = join(cacheDir, file);
+    try {
+      const stats = await stat(filePath);
+      if (stats.mtimeMs < cutoff) {
+        await unlink(filePath).catch(() => {
+          // Already gone — another sweep or the owning process removed it.
+        });
+      }
+    } catch (error) {
+      log.debug("Failed to inspect cache temp file during sweep", error);
+    }
+  });
+}
+
 /** Metadata for a cache entry, used for cleanup decisions */
 type EntryMetadata = { file: string; createdAt: number; expired: boolean };
 
@@ -769,20 +845,37 @@ async function collectEntryMetadata(
 
   await cacheIO.map(jsonFiles, async (file) => {
     const filePath = join(cacheDir, file);
+
+    // Read and parse are handled separately so we can distinguish a transient
+    // read failure (skip — never delete) from genuine corruption (parse failed
+    // on a fully-read file — safe to delete). Atomic writes
+    // ({@link atomicWriteCacheFile}) guarantee readers never see a half-written
+    // file, so a parse failure here means real corruption, not a torn read —
+    // deleting was previously a data-loss bug when writes were non-atomic.
+    let raw: string;
+    try {
+      raw = await readFile(filePath, "utf-8");
+    } catch (error) {
+      // Transient read failure (locking, AV scanner, ENOENT from a concurrent
+      // sweep). Skip — a later sweep will reconsider the file.
+      log.debug("Skipping cache file with unreadable contents", error);
+      return;
+    }
+
     try {
-      const raw = await readFile(filePath, "utf-8");
       const entry = JSON.parse(raw) as CacheEntry;
       const expired =
         entry.expiresAt !== undefined
           ? now >= entry.expiresAt
           : now - entry.createdAt >
             FALLBACK_TTL_MS[classifyUrl(entry.url ?? "")];
       entries.push({ file, createdAt: entry.createdAt, expired });
-    } catch {
-      // Unparseable file — delete it
-      unlink(filePath).catch(() => {
-        // Best-effort cleanup of corrupted file
-      });
+    } catch (error) {
+      // Fully read but unparseable — genuine corruption. Mark expired so
+      // {@link deleteExpiredEntries} reclaims it; this also keeps eviction
+      // counts accurate (corrupt files are not invisible to MAX_CACHE_ENTRIES).
+      log.debug("Reclaiming corrupt cache file during cleanup", error);
+      entries.push({ file, createdAt: now, expired: true });
     }
   });
 

test/lib/response-cache.test.ts

@@ -470,6 +470,56 @@ describe("file structure", () => {
     expect(files.length).toBe(1);
     expect(files[0]).toMatch(/^[0-9a-f]{64}\.json$/);
   });
+
+  test("atomic write leaves no temp file behind on success", async () => {
+    await storeCachedResponse(
+      TEST_METHOD,
+      TEST_URL,
+      {},
+      mockResponse(TEST_BODY)
+    );
+
+    const cacheDir = join(getConfigDir(), "cache", "responses");
+    const files = await readdir(cacheDir);
+    // Exactly the final .json file — the temp file was renamed into place.
+    expect(files.every((f) => f.endsWith(".json"))).toBe(true);
+    expect(files.some((f) => f.endsWith(".tmp"))).toBe(false);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// Atomic write / torn-read regression (getsentry/cli#1056)
+//
+// A write fires cleanupCache() fire-and-forget at 10% probability. Before
+// atomic writes, a second write to the same key could be read mid-overwrite by
+// that cleanup sweep, fail to JSON.parse, and be deleted as "corrupted" —
+// losing a valid entry. This loop exercises the exact store→overwrite→read
+// sequence many times so the probabilistic cleanup is virtually guaranteed to
+// fire; every iteration must still serve the fresh value.
+// ---------------------------------------------------------------------------
+
+describe("atomic write regression", () => {
+  test("repeated overwrite-then-read never loses the entry", async () => {
+    for (let i = 0; i < 50; i++) {
+      await storeCachedResponse(
+        TEST_METHOD,
+        TEST_URL,
+        {},
+        mockResponse({ data: `stale-${i}` })
+      );
+      const freshBody = { data: `fresh-${i}` };
+      await storeCachedResponse(
+        TEST_METHOD,
+        TEST_URL,
+        {},
+        mockResponse(freshBody)
+      );
+
+      const cached = await getCachedResponse(TEST_METHOD, TEST_URL, {});
+      expect(cached).toBeDefined();
+      expect(await cached!.json()).toEqual(freshBody);
+    }
+  });
 });
 
 // ---------------------------------------------------------------------------