fix(persistence): close TOCTOU window in find* walkers (review followup)

src/cli/commands/summarise.ts

@@ -48,6 +48,7 @@ export const summariseCommand = new Command()
   .example("Show recent activity", "swamp summarise")
   .example("Activity from the last day", "swamp summarise --since 1d")
   .example("Activity from the last hour", "swamp summarise --since 1h")
+  .example("Cap detail output on a large repo", "swamp summarise --limit 10")
   .option(
     "--repo-dir <dir:string>",
     "Repository directory (env: SWAMP_REPO_DIR)",
@@ -56,7 +57,7 @@ export const summariseCommand = new Command()
     default: "7d",
   })
   .option(
-    "--limit <count:number>",
+    "--limit <n:number>",
     "Cap per-group run details (counts still reflect all matching runs)",
   )
   .action(async function (options) {

src/infrastructure/persistence/unified_data_repository_test.ts

@@ -750,3 +750,88 @@ Deno.test("markDirty is not called on read paths", async () => {
     }
   }
 });
+
+// ============================================================================
+// findAllGlobalSince — mirrors the workflow-run repo tests so the three
+// implementations of the same two-stage filter stay in lockstep.
+// ============================================================================
+
+async function withDataRepo(
+  fn: (
+    repo: FileSystemUnifiedDataRepository,
+    tmpDir: string,
+  ) => Promise<void>,
+): Promise<void> {
+  const tmpDir = await Deno.makeTempDir();
+  try {
+    const catalogStore = new CatalogStore(join(tmpDir, "_catalog.db"));
+    const repo = new FileSystemUnifiedDataRepository(
+      tmpDir,
+      undefined,
+      catalogStore,
+    );
+    await fn(repo, tmpDir);
+  } finally {
+    if (Deno.build.os === "windows") {
+      await Deno.remove(tmpDir, { recursive: true }).catch(() => {});
+    } else {
+      await Deno.remove(tmpDir, { recursive: true });
+    }
+  }
+}
+
+Deno.test("findAllGlobalSince: returns only in-window data items", async () => {
+  await withDataRepo(async (repo) => {
+    const old = makeData("old-data");
+    await repo.save(testType, "model-1", old, new TextEncoder().encode("x"));
+
+    const oldDate = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000);
+    const oldMetadataPath = repo.getMetadataPath(
+      testType,
+      "model-1",
+      "old-data",
+      1,
+    );
+    await Deno.utime(oldMetadataPath, oldDate, oldDate);
+
+    const fresh = makeData("fresh-data");
+    await repo.save(testType, "model-1", fresh, new TextEncoder().encode("y"));
+
+    const cutoff = new Date(Date.now() - 60 * 60 * 1000);
+    const found = await repo.findAllGlobalSince(cutoff);
+
+    assertEquals(found.length, 1);
+    assertEquals(found[0].data.name, "fresh-data");
+  });
+});
+
+Deno.test(
+  "findAllGlobalSince: file deleted mid-iteration is skipped, not fatal",
+  async () => {
+    await withDataRepo(async (repo) => {
+      const keep = makeData("keep-data");
+      await repo.save(testType, "model-1", keep, new TextEncoder().encode("x"));
+
+      const doomed = makeData("doomed-data");
+      await repo.save(
+        testType,
+        "model-1",
+        doomed,
+        new TextEncoder().encode("y"),
+      );
+
+      // Concurrent deletion of the doomed item's metadata file. The data
+      // repo already wraps stat in per-file try/catch; this test pins
+      // that behavior so future refactors don't lose it.
+      await Deno.remove(
+        repo.getMetadataPath(testType, "model-1", "doomed-data", 1),
+      );
+
+      const cutoff = new Date(Date.now() - 60 * 60 * 1000);
+      const found = await repo.findAllGlobalSince(cutoff);
+
+      assertEquals(found.length, 1);
+      assertEquals(found[0].data.name, "keep-data");
+    });
+  },
+);

src/infrastructure/persistence/yaml_output_repository.ts

@@ -122,24 +122,34 @@ export class YamlOutputRepository implements OutputRepository {
     try {
       // Iterate over method directories
       for await (const methodEntry of Deno.readDir(typeDir)) {
-        if (methodEntry.isDirectory) {
-          const methodDir = join(typeDir, methodEntry.name);
-          // Iterate over output files in method directory
-          for await (const entry of Deno.readDir(methodDir)) {
-            if (entry.isFile && entry.name.endsWith(".yaml")) {
-              const path = join(methodDir, entry.name);
-              const content = await Deno.readTextFile(path);
-              const data = parseYaml(content) as ModelOutputData;
-              // Convert logFile back to absolute path
-              if (data.logFile) {
-                data.logFile = toAbsolutePath(this.repoDir, data.logFile);
-              }
-              outputs.push(ModelOutput.fromData(data));
+        if (!methodEntry.isDirectory) continue;
+        const methodDir = join(typeDir, methodEntry.name);
+        // Iterate over output files in method directory
+        for await (const entry of Deno.readDir(methodDir)) {
+          if (!entry.isFile || !entry.name.endsWith(".yaml")) continue;
+          const path = join(methodDir, entry.name);
+
+          // Per-file try/catch closes the TOCTOU window: a concurrent
+          // delete (e.g. GC, output cleanup) can remove the file
+          // between readDir and readTextFile. NotFound on a single
+          // file means "skip it" — never "abandon the rest of the
+          // current method directory."
+          try {
+            const content = await Deno.readTextFile(path);
+            const data = parseYaml(content) as ModelOutputData;
+            if (data.logFile) {
+              data.logFile = toAbsolutePath(this.repoDir, data.logFile);
             }
+            outputs.push(ModelOutput.fromData(data));
+          } catch (error) {
+            if (error instanceof Deno.errors.NotFound) continue;
+            throw error;
           }
         }
       }
     } catch (error) {
+      // Outer catch handles "type/method directory itself doesn't
+      // exist." Per-file NotFound is handled above.
       if (error instanceof Deno.errors.NotFound) {
         return [];
       }
@@ -197,28 +207,40 @@ export class YamlOutputRepository implements OutputRepository {
             if (!entry.isFile || !entry.name.endsWith(".yaml")) continue;
             const path = join(methodDir, entry.name);
 
-            // Stage A: mtime pre-filter
-            const stat = await Deno.stat(path);
-            const mtimeMs = stat.mtime?.getTime();
-            if (mtimeMs !== undefined && mtimeMs < cutoffMs) continue;
+            // Per-file try/catch closes the TOCTOU window: a concurrent
+            // delete can remove the file between readDir and stat or
+            // between stat and readTextFile. NotFound on a single file
+            // means "skip it" — never "abandon the rest of the current
+            // method directory or model type."
+            try {
+              // Stage A: mtime pre-filter
+              const stat = await Deno.stat(path);
+              const mtimeMs = stat.mtime?.getTime();
+              if (mtimeMs !== undefined && mtimeMs < cutoffMs) continue;
 
-            // Stage B: parse and verify
-            const content = await Deno.readTextFile(path);
-            const data = parseYaml(content) as ModelOutputData;
-            if (data.logFile) {
-              data.logFile = toAbsolutePath(this.repoDir, data.logFile);
-            }
-            const output = ModelOutput.fromData(data);
-            if (output.startedAt.getTime() < cutoffMs) continue;
+              // Stage B: parse and verify
+              const content = await Deno.readTextFile(path);
+              const data = parseYaml(content) as ModelOutputData;
+              if (data.logFile) {
+                data.logFile = toAbsolutePath(this.repoDir, data.logFile);
+              }
+              const output = ModelOutput.fromData(data);
+              if (output.startedAt.getTime() < cutoffMs) continue;
 
-            results.push({
-              output,
-              type: modelType,
-              method: output.methodName,
-            });
+              results.push({
+                output,
+                type: modelType,
+                method: output.methodName,
+              });
+            } catch (error) {
+              if (error instanceof Deno.errors.NotFound) continue;
+              throw error;
+            }
           }
         }
       } catch (error) {
+        // Outer catch handles "type/method directory itself doesn't
+        // exist." Per-file NotFound is handled above.
         if (error instanceof Deno.errors.NotFound) continue;
         throw error;
       }

src/infrastructure/persistence/yaml_output_repository_test.ts

@@ -29,6 +29,14 @@ import { createDefinitionId } from "../../domain/definitions/definition.ts";
 import { ModelType } from "../../domain/models/model_type.ts";
 import { YamlOutputRepository } from "./yaml_output_repository.ts";
 
+// Import the models barrel so `modelRegistry.types()` returns real entries —
+// `findAllGlobalSince` and `findAll` both walk the registry, so tests that
+// exercise them need at least one registered type. The `command/shell` model
+// is the simplest entry the barrel registers.
+import "../../domain/models/models.ts";
+
+const registeredType = ModelType.create("command/shell");
+
 async function withTempDir(fn: (dir: string) => Promise<void>): Promise<void> {
   const dir = await Deno.makeTempDir({ prefix: "swamp-test-" });
   try {
@@ -402,3 +410,86 @@ Deno.test("YamlOutputRepository invokes markDirty with relPath on mutations", as
     assertEquals(calls.length, 2);
   });
 });
+
+// findAllGlobalSince tests use a type that's actually in the model registry
+// because the implementation iterates `modelRegistry.types()` to know where
+// to look on disk.
+
+async function makeOutput(
+  repo: YamlOutputRepository,
+  startedAt: Date,
+): Promise<ModelOutput> {
+  const output = ModelOutput.create({
+    definitionId: createDefinitionId(crypto.randomUUID()),
+    methodName: "run",
+    status: "running",
+    startedAt,
+    provenance: defaultProvenance,
+  });
+  output.markSucceeded();
+  await repo.save(registeredType, "run", output);
+  return output;
+}
+
+Deno.test("findAllGlobalSince: returns only in-window outputs", async () => {
+  await withTempDir(async (dir) => {
+    const repo = new YamlOutputRepository(dir);
+
+    const oldDate = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000);
+    const old = await makeOutput(repo, oldDate);
+
+    // Backdate the file so its mtime falls before the cutoff.
+    const oldPath = repo.getPath(registeredType, "run", old);
+    await Deno.utime(oldPath, oldDate, oldDate);
+
+    const fresh = await makeOutput(repo, new Date());
+
+    const cutoff = new Date(Date.now() - 60 * 60 * 1000);
+    const found = await repo.findAllGlobalSince(cutoff);
+
+    assertEquals(found.length, 1);
+    assertEquals(found[0].output.id, fresh.id);
+  });
+});
+
+Deno.test(
+  "findAllGlobalSince: file deleted mid-iteration is skipped, not fatal",
+  async () => {
+    await withTempDir(async (dir) => {
+      const repo = new YamlOutputRepository(dir);
+
+      const keep = await makeOutput(repo, new Date());
+      const doomed = await makeOutput(repo, new Date());
+
+      // Simulate a concurrent deletion. Pre-fix behavior: NotFound from
+      // Deno.stat propagates to the model-type catch and continues past
+      // the entire current type, dropping `keep`.
+      await Deno.remove(repo.getPath(registeredType, "run", doomed));
+
+      const cutoff = new Date(Date.now() - 60 * 60 * 1000);
+      const found = await repo.findAllGlobalSince(cutoff);
+
+      assertEquals(found.length, 1);
+      assertEquals(found[0].output.id, keep.id);
+    });
+  },
+);
+
+Deno.test(
+  "findAll: file deleted mid-iteration is skipped, not fatal",
+  async () => {
+    await withTempDir(async (dir) => {
+      const repo = new YamlOutputRepository(dir);
+
+      const keep = await makeOutput(repo, new Date());
+      const doomed = await makeOutput(repo, new Date());
+
+      await Deno.remove(repo.getPath(registeredType, "run", doomed));
+
+      const found = await repo.findAll(registeredType);
+
+      assertEquals(found.length, 1);
+      assertEquals(found[0].id, keep.id);
+    });
+  },
+);