Tell a Mailroom sidecar apart from a damaged graph (#315)

GraphcodeKit/Sources/GraphWriter.swift

@@ -49,6 +49,27 @@ public final class GraphWriter: @unchecked Sendable {
     return persistence.loadGraph(path: path)
   }
 
+  /// Drops any save still queued for a project, for a caller that is about to delete it.
+  ///
+  /// The queue exists to let a write land after the actor has moved on, which is exactly
+  /// wrong once the graph is being thrown away: a drain that ran after `deleteGraph`
+  /// would put the file back, and `load` would keep answering from the queue for a
+  /// project that no longer exists. The delete is the one operation that has to reach
+  /// into the queue rather than trail it.
+  ///
+  /// Synced against the drain, not just the pending table: a drain that has already
+  /// popped a save still has the write ahead of it — a write made outside the lock, and
+  /// one that would land after `deleteGraph` removed the file. Only the drain's
+  /// completion makes "nothing queued" true, and the serial queue is where that
+  /// ordering lives.
+  public func forget(path: String) {
+    queue.sync {
+      lock.lock()
+      pending.removeValue(forKey: path)
+      lock.unlock()
+    }
+  }
+
   /// Returns once everything queued so far is on disk.
   public func flush() {
     queue.sync { drain() }

GraphcodeKit/Sources/ProjectPersistence.swift

@@ -52,9 +52,9 @@ public struct ProjectPersistence: Sendable {
     slim.mailroom = []
     guard let data = try? JSONEncoder().encode(slim) else { return }
     try? data.write(to: fileURL(forProjectPath: graph.project.path), options: .atomic)
-    let digest = MailroomDigest(of: graph.mailroom)
-    guard Self.roomDigests.changed(to: digest, for: graph.project.path) else { return }
     let roomURL = mailroomURL(forProjectPath: graph.project.path)
+    let digest = MailroomDigest(of: graph.mailroom)
+    guard Self.roomDigests.changed(to: digest, for: roomURL.path) else { return }
     if graph.mailroom.isEmpty {
       try? FileManager.default.removeItem(at: roomURL)
     } else if let room = try? JSONEncoder().encode(graph.mailroom) {
@@ -65,6 +65,11 @@ public struct ProjectPersistence: Sendable {
   /// What the room last written for each project looked like, so an unchanged room is
   /// not rewritten. Process-wide because this type is a value: every copy writes the
   /// same files. A miss (first save after launch) writes once and is then remembered.
+  ///
+  /// Keyed by the room *file*, not the project path: one path is the same project in
+  /// every workspace but a different file in each, and sharing an entry across them
+  /// would judge a room unchanged against a digest taken from someone else's file and
+  /// never write it.
   private static let roomDigests = RoomDigests()
 
   private final class RoomDigests: @unchecked Sendable {
@@ -78,6 +83,12 @@ public struct ProjectPersistence: Sendable {
       digests[path] = digest
       return true
     }
+
+    func forget(_ path: String) {
+      lock.lock()
+      defer { lock.unlock() }
+      digests.removeValue(forKey: path)
+    }
   }
 
   /// Throws away a project's loops for good — the "Delete Loops…" half of the sidebar's
@@ -87,6 +98,10 @@ public struct ProjectPersistence: Sendable {
   public func deleteGraph(path: String) {
     try? FileManager.default.removeItem(at: fileURL(forProjectPath: path))
     try? FileManager.default.removeItem(at: mailroomURL(forProjectPath: path))
+    // The digest cache is keyed by path and outlives the file. Left behind, a project
+    // re-created at the same path whose room happens to match the deleted one would be
+    // judged unchanged and never written.
+    Self.roomDigests.forget(mailroomURL(forProjectPath: path).path)
   }
 
   /// Filenames are the canonical path with `/` replaced by `_` — simple, deterministic,
@@ -100,7 +115,29 @@ public struct ProjectPersistence: Sendable {
   /// The room beside its graph: `<name>.mailroom.json`.
   private func mailroomURL(forProjectPath path: String) -> URL {
     let safeName = path.replacingOccurrences(of: "/", with: "_")
-    return projectsDirectory.appendingPathComponent("\(safeName).mailroom.json")
+    return projectsDirectory.appendingPathComponent("\(safeName)\(Self.roomFileSuffix)")
+  }
+
+  /// Every suffix this type writes into `projects/` *beside* a graph rather than as one.
+  ///
+  /// `projects/` held nothing but graphs until #307 moved the room out of the graph file,
+  /// so readers scanning it — `OrphanedSessionReaper`, `Workspace.contents` — took every
+  /// `.json` in it for a graph. That assumption is now false, and it failed loudly in the
+  /// worst place: `reap` treats an undecodable file as state it cannot account for and
+  /// aborts, so a room file disabled the tool people reach for when they are out of PTYs.
+  ///
+  /// **Adding a sidecar means adding its suffix here**, in the same type that mints the
+  /// name, so a reader never has to be taught about it separately. Anything not listed
+  /// still fails closed, which is the safe direction but also a silently broken `reap`.
+  static let roomFileSuffix = ".mailroom.json"
+  static let sidecarFileSuffixes = [roomFileSuffix]
+
+  /// Whether a file in `projects/` is a sidecar rather than a graph. Answered from the
+  /// name alone and deliberately not from the contents: a *corrupt* sidecar is still a
+  /// sidecar, and it never owned a session, so it must not be mistaken for a damaged
+  /// graph and stop a reap.
+  public static func isSidecarFileName(_ name: String) -> Bool {
+    sidecarFileSuffixes.contains { name.hasSuffix($0) }
   }
 
   // MARK: - Recent projects

GraphcodeKit/Sources/ProjectRegistry.swift

@@ -360,6 +360,9 @@ public actor ProjectRegistry {
       // Drop the in-memory store too, or a later reopen would resurrect the graph we
       // just deleted from the one still sitting in `stores`.
       stores.removeValue(forKey: canonicalPath)
+      // Before the file goes, so a save still in the writer's queue cannot land after the
+      // delete and put the graph back.
+      writer.forget(path: canonicalPath)
       persistence.deleteGraph(path: canonicalPath)
 
     case .graphCommand(let path, let inner):

GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift

@@ -65,14 +65,26 @@ public enum OrphanedSessionReaper {
     for directory in workspaceDirectories {
       var workspaceLive: Set<UUID> = []
       let projects = directory.appendingPathComponent("projects", isDirectory: true)
-      let graphFiles =
+      let projectFiles =
         (try? FileManager.default.contentsOfDirectory(
           at: projects, includingPropertiesForKeys: nil))?
         .filter { $0.pathExtension == "json" } ?? []
-      for file in graphFiles {
+      for file in projectFiles {
+        let sidecarNamed = ProjectPersistence.isSidecarFileName(file.lastPathComponent)
         guard let data = try? Data(contentsOf: file),
           let graph = try? JSONDecoder().decode(LoopGraph.self, from: data)
-        else { return nil }
+        else {
+          // A sidecar is skipped before it can look corrupt: it is not a graph, it never
+          // owned a session, and refusing to guess about it aborts a reap that has nothing
+          // to be uncertain about. Only a file that claims to be a graph and cannot be read
+          // as one is the "state this build cannot account for" the bail-out is for. The
+          // name is answered first because a *corrupt* room is still a room.
+          if sidecarNamed { continue }
+          return nil
+        }
+        // Where the name can lie, decode wins: a project path ending in `.mailroom` mints
+        // a graph file that carries the sidecar suffix, and skipping one by name would
+        // drop live sessions out of this set for a reap to kill.
         workspaceLive.formUnion(graph.nodesAtAnyDepth.map(\.id))
       }
       workspaceLive.formUnion(QuickChatStore(baseDirectory: directory).load().map(\.id))

GraphcodeKit/Sources/Workspace.swift

@@ -234,6 +234,10 @@ extension Workspace {
     let projectsDirectory = url.appendingPathComponent("projects", isDirectory: true)
     for name in (try? fileManager.contentsOfDirectory(atPath: projectsDirectory.path)) ?? []
     where name.hasSuffix(".json") {
+      // A sidecar is not a project, but the suffix alone must not be trusted here either:
+      // a project path ending in `.mailroom` mints a graph file that carries it, and a
+      // room — valid or corrupt — never decodes as a graph, so the decode below already
+      // skips every sidecar without also skipping a sidecar-named graph.
       guard let data = try? Data(contentsOf: projectsDirectory.appendingPathComponent(name)),
         let graph = try? JSONDecoder().decode(LoopGraph.self, from: data)
       else { continue }

graphcode/Tests/CondemnedSessionsTests.swift

@@ -1,4 +1,5 @@
 import Foundation
+import MailroomKit
 import Testing
 
 @testable import GraphcodeKit
@@ -151,6 +152,136 @@ struct OrphanedSessionReaperTests {
     #expect(live.isEmpty)
   }
 
+  /// #307 moved the Mailroom out of the graph file and into `<name>.mailroom.json`
+  /// beside it. This scan took every `.json` under `projects/` for a graph, so the room
+  /// looked like a graph it could not decode and `reap` aborted — on every workspace, for
+  /// good, and with a message ("refusing to guess") that reads as caution rather than
+  /// breakage. `reap` is the recovery tool for a machine out of PTYs, so it was broken
+  /// exactly when it is needed. Saved through `ProjectPersistence` rather than by writing
+  /// a hand-picked filename, so the test tracks the name the app actually mints.
+  @Test
+  func aRoomFileBesideItsGraphDoesNotStopAReap() throws {
+    let workspace = FileManager.default.temporaryDirectory
+      .appendingPathComponent("reap-ws-\(UUID().uuidString)")
+    defer { try? FileManager.default.removeItem(at: workspace) }
+    var graph = LoopGraph(
+      project: ProjectRef(path: "/tmp/p", name: "p"), nodes: [LoopNode(id: id1, title: "loop")])
+    graph.mailroom = [
+      MailroomPost(
+        id: 1, at: Date(timeIntervalSince1970: 1), authorID: nil, author: "a peer",
+        topic: nil, body: "a notice, so the room gets a file of its own")
+    ]
+    ProjectPersistence(baseDirectory: workspace).saveGraph(graph)
+
+    let projects = workspace.appendingPathComponent("projects", isDirectory: true)
+    let written = try FileManager.default.contentsOfDirectory(atPath: projects.path).sorted()
+    #expect(written.count == 2, "the graph and its room, or this no longer reproduces")
+
+    let live = try #require(
+      OrphanedSessionReaper.liveSessionIDs(workspaceDirectories: [workspace]),
+      "a room file beside a graph must not read as state the reap cannot account for")
+    #expect(live == [id1])
+  }
+
+  /// The other half of the same rule, and the reason this is not simply "skip what will
+  /// not decode": a *graph* that cannot be read still owns sessions nobody can enumerate,
+  /// so the reap must keep refusing rather than sweep them.
+  @Test
+  func aCorruptGraphStillStopsAReap() throws {
+    let workspace = FileManager.default.temporaryDirectory
+      .appendingPathComponent("reap-ws-\(UUID().uuidString)")
+    defer { try? FileManager.default.removeItem(at: workspace) }
+    let projects = workspace.appendingPathComponent("projects", isDirectory: true)
+    try FileManager.default.createDirectory(at: projects, withIntermediateDirectories: true)
+    try Data("{ not a graph".utf8)
+      .write(to: projects.appendingPathComponent("_tmp_p.json"))
+
+    #expect(OrphanedSessionReaper.liveSessionIDs(workspaceDirectories: [workspace]) == nil)
+  }
+
+  /// The suffix is a claim about who wrote the file, and a project can make that claim
+  /// against the reaper: a path ending in `.mailroom` mints a graph file that carries the
+  /// sidecar suffix. One that decodes as a graph is a graph — skipping it by name would
+  /// drop its sessions out of the live set for a reap to kill.
+  @Test
+  func aGraphNamedLikeASidecarStillOwnsItsSessions() throws {
+    let workspace = FileManager.default.temporaryDirectory
+      .appendingPathComponent("reap-ws-\(UUID().uuidString)")
+    defer { try? FileManager.default.removeItem(at: workspace) }
+    var graph = LoopGraph(
+      project: ProjectRef(path: "/tmp/x.mailroom", name: "p"),
+      nodes: [LoopNode(id: id1, title: "loop")])
+    graph.mailroom = [
+      MailroomPost(
+        id: 1, at: Date(timeIntervalSince1970: 1), authorID: nil, author: "a peer",
+        topic: nil, body: "a notice, so the room gets a file of its own")
+    ]
+    ProjectPersistence(baseDirectory: workspace).saveGraph(graph)
+
+    let projects = workspace.appendingPathComponent("projects", isDirectory: true)
+    let written = try FileManager.default.contentsOfDirectory(atPath: projects.path).sorted()
+    #expect(
+      written == ["_tmp_x.mailroom.json", "_tmp_x.mailroom.mailroom.json"],
+      "the graph file itself must carry the sidecar suffix, or this no longer reproduces")
+
+    let live = try #require(
+      OrphanedSessionReaper.liveSessionIDs(workspaceDirectories: [workspace]))
+    #expect(live == [id1])
+  }
+
+  /// The claim that makes the name worth reading at all — a *corrupt* room is still a
+  /// room, and never owned a session — pinned: a sidecar-named file that decodes as
+  /// nothing must not stop a reap either.
+  @Test
+  func aCorruptRoomFileStillDoesNotStopAReap() throws {
+    let workspace = FileManager.default.temporaryDirectory
+      .appendingPathComponent("reap-ws-\(UUID().uuidString)")
+    defer { try? FileManager.default.removeItem(at: workspace) }
+    var graph = LoopGraph(
+      project: ProjectRef(path: "/tmp/p", name: "p"), nodes: [LoopNode(id: id1, title: "loop")])
+    graph.mailroom = [
+      MailroomPost(
+        id: 1, at: Date(timeIntervalSince1970: 1), authorID: nil, author: "a peer",
+        topic: nil, body: "a notice")
+    ]
+    ProjectPersistence(baseDirectory: workspace).saveGraph(graph)
+    try Data("{ not a room".utf8)
+      .write(to: workspace.appendingPathComponent("projects/_tmp_p.mailroom.json"))
+
+    let live = try #require(
+      OrphanedSessionReaper.liveSessionIDs(workspaceDirectories: [workspace]))
+    #expect(live == [id1])
+  }
+
+  /// What keeps the rule from rotting into a stale list of names: whatever
+  /// `ProjectPersistence` writes beside a graph has to be something this scan recognises
+  /// as a sidecar. Add a sidecar without registering its suffix and this fails here,
+  /// rather than silently disabling `reap` again months later.
+  @Test
+  func everyFileWrittenBesideAGraphIsRecognisedAsASidecar() throws {
+    let workspace = FileManager.default.temporaryDirectory
+      .appendingPathComponent("reap-ws-\(UUID().uuidString)")
+    defer { try? FileManager.default.removeItem(at: workspace) }
+    var graph = LoopGraph(
+      project: ProjectRef(path: "/tmp/p", name: "p"), nodes: [LoopNode(id: id1, title: "loop")])
+    graph.mailroom = [
+      MailroomPost(
+        id: 1, at: Date(timeIntervalSince1970: 1), authorID: nil, author: "a peer",
+        topic: nil, body: "a notice")
+    ]
+    ProjectPersistence(baseDirectory: workspace).saveGraph(graph)
+
+    let projects = workspace.appendingPathComponent("projects", isDirectory: true)
+    let beside = try FileManager.default.contentsOfDirectory(atPath: projects.path)
+      .filter { $0 != "_tmp_p.json" }
+    #expect(!beside.isEmpty)
+    for name in beside {
+      #expect(
+        ProjectPersistence.isSidecarFileName(name),
+        "\(name) is written beside a graph but no sidecar suffix claims it")
+    }
+  }
+
   @Test
   func currentOverrideDirectoryJoinsDiscoveredWorkspacesOnce() {
     let defaultWorkspace = URL(fileURLWithPath: "/tmp/.graphcode")

graphcode/Tests/ProjectPersistenceTests.swift

@@ -208,6 +208,69 @@ struct MailroomPersistenceTests {
     #expect(writer.load(path: "/tmp/never-saved") == nil)
   }
 
+  /// The digest cache is process-wide while one project path is a different room file in
+  /// every workspace. Keyed by the path, two workspaces would share an entry and a save
+  /// be judged unchanged against a digest taken from someone else's file — the room never
+  /// written, the file left stale on disk.
+  @Test
+  func aRoomsDigestIsJudgedAgainstItsOwnFileNotTheProjectPath() throws {
+    let workspaceA = FileManager.default.temporaryDirectory
+      .appendingPathComponent("digest-ws-\(UUID().uuidString)")
+    let workspaceB = FileManager.default.temporaryDirectory
+      .appendingPathComponent("digest-ws-\(UUID().uuidString)")
+    defer {
+      try? FileManager.default.removeItem(at: workspaceA)
+      try? FileManager.default.removeItem(at: workspaceB)
+    }
+    func post(_ body: String) -> MailroomPost {
+      MailroomPost(
+        id: 1, at: Date(timeIntervalSince1970: 1), authorID: nil, author: "a peer",
+        topic: nil, body: body)
+    }
+    func graph(_ posts: [MailroomPost]) -> LoopGraph {
+      var graph = LoopGraph(
+        project: ProjectRef(path: "/tmp/p", name: "p"), nodes: [LoopNode(title: "loop")])
+      graph.mailroom = posts
+      return graph
+    }
+    let a = ProjectPersistence(baseDirectory: workspaceA)
+    let b = ProjectPersistence(baseDirectory: workspaceB)
+    a.saveGraph(graph([post("one")]))
+    b.saveGraph(graph([post("one"), post("two")]))
+
+    a.saveGraph(graph([post("one"), post("two")]))
+
+    let roomA = try String(
+      decoding: Data(
+        contentsOf: workspaceA.appendingPathComponent("projects/_tmp_p.mailroom.json")),
+      as: UTF8.self)
+    #expect(roomA.contains("two"), "workspace A's room was judged unchanged and never written")
+  }
+
+  /// The other side of `aLoadReturnsTheQueuedSnapshotBeforeTheDiskHasIt`: a queued save
+  /// must not outlive the graph it belongs to. Deleting a project evicts its store and
+  /// removes the file, and a `load` still answering from the queue would hand the
+  /// deleted loops back to the next reader.
+  @Test
+  func aDeletedProjectIsNotHandedBackFromTheQueue() throws {
+    let directory = makeDirectory()
+    defer { try? FileManager.default.removeItem(at: directory) }
+    let persistence = ProjectPersistence(baseDirectory: directory)
+    let writer = GraphWriter(persistence: persistence)
+    let path = "/tmp/deleted-\(UUID().uuidString.prefix(6))"
+    var graph = LoopGraph(project: ProjectRef(path: path, name: "deleted"))
+    graph.nodes.append(LoopNode(title: "Loop", loopType: .turnBased, firstInstruction: "Work"))
+
+    for _ in 0..<50 {
+      writer.save(graph)
+      writer.forget(path: path)
+      persistence.deleteGraph(path: path)
+      #expect(writer.load(path: path) == nil)
+      writer.flush()
+      #expect(persistence.loadGraph(path: path) == nil, "a queued save rewrote a deleted graph")
+    }
+  }
+
   /// The writer takes a burst and lands the newest snapshot once; `flush` returns with
   /// it on disk.
   @Test