From 9ed9748049fa5390b102d12b35ebf8d66fd567ae Mon Sep 17 00:00:00 2001 From: scgopi Date: Sun, 6 Sep 2026 19:04:33 -0700 Subject: [PATCH 1/3] Start fix/reap-mailroom-sidecar From 6bf3a69ed768f7ce8fc9f6766c4698e3a19e8f63 Mon Sep 17 00:00:00 2001 From: scgopi Date: Sun, 6 Sep 2026 19:30:59 -0700 Subject: [PATCH 2/3] Tell a Mailroom sidecar apart from a damaged graph (#315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `graphcode reap` has aborted on every workspace since #308 shipped in 0.1.64-beta3. `OrphanedSessionReaper.liveSessionIDs` took every `.json` under `projects/` for a `LoopGraph` and refused to reap if any one of them failed to decode. That held until #307 moved the Mailroom into `.mailroom.json` beside the graph: the room is a JSON array, so it can never decode as a graph, and every reap bailed with "refusing to guess which sessions it owns" — a message that reads as caution rather than breakage, on the tool people reach for when they are out of PTYs. Refusing to guess stays right for a graph that is genuinely damaged: it still owns sessions nobody can enumerate, and reaping them would be a sweep. What was wrong is that a sidecar is not a damaged graph. `ProjectPersistence` now answers which files it writes beside a graph, next to where it mints their names, and both readers of `projects/` ask it — the reaper and `Workspace.contents`, which had the same assumption and only escaped notice by skipping rather than bailing. The rule is answered from the name alone: a *corrupt* room is still a room, it never owned a session, and treating it as a damaged graph would disable reap for exactly the reason being fixed here. Two things the same file turned up, both from state that outlives a delete: - A save still queued when a project is deleted could drain afterwards and put the graph file back, and `GraphWriter.load` would keep answering from the queue for a project that no longer exists. `GraphWriter.forget(path:)` drops it, and the delete calls it before removing the file. - The room digest cache was keyed by project path and outlived the file. One path is the same project in every workspace but a different file in each, so two workspaces shared an entry and a room could be judged unchanged against a digest taken from someone else's file. Keyed by the room file now, and cleared on delete. Two registry tests only passed because `/tmp/project-f` and `/tmp/project-g` happened to exist on the machine that wrote them: `routing` refuses a path with no folder at it, so on a clean checkout no loop was created and `deletingAClosedProjectsLoopsStillEndsTheirSessions` passed for having found nothing. Both now make the folder, and that test asserts it created something before asserting what was killed. Closes #315. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Gvj8H68LzmfgKg4cjEz1nx --- GraphcodeKit/Sources/GraphWriter.swift | 13 ++++ GraphcodeKit/Sources/ProjectPersistence.swift | 43 ++++++++++- GraphcodeKit/Sources/ProjectRegistry.swift | 3 + .../Sessions/OrphanedSessionReaper.swift | 9 ++- GraphcodeKit/Sources/Workspace.swift | 2 +- graphcode/Tests/CondemnedSessionsTests.swift | 77 +++++++++++++++++++ graphcode/Tests/ProjectPersistenceTests.swift | 24 ++++++ graphcode/Tests/ProjectRegistryTests.swift | 65 ++++++++++++++-- 8 files changed, 225 insertions(+), 11 deletions(-) diff --git a/GraphcodeKit/Sources/GraphWriter.swift b/GraphcodeKit/Sources/GraphWriter.swift index f89d41f0..fb5d680f 100644 --- a/GraphcodeKit/Sources/GraphWriter.swift +++ b/GraphcodeKit/Sources/GraphWriter.swift @@ -49,6 +49,19 @@ 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. + public func forget(path: String) { + lock.lock() + pending.removeValue(forKey: path) + lock.unlock() + } + /// Returns once everything queued so far is on disk. public func flush() { queue.sync { drain() } diff --git a/GraphcodeKit/Sources/ProjectPersistence.swift b/GraphcodeKit/Sources/ProjectPersistence.swift index a8765d5d..1c4501fd 100644 --- a/GraphcodeKit/Sources/ProjectPersistence.swift +++ b/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: `.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 diff --git a/GraphcodeKit/Sources/ProjectRegistry.swift b/GraphcodeKit/Sources/ProjectRegistry.swift index 6ee1a2b1..ff30334f 100644 --- a/GraphcodeKit/Sources/ProjectRegistry.swift +++ b/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): diff --git a/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift b/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift index a3785ad8..42dcced8 100644 --- a/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift +++ b/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift @@ -65,11 +65,16 @@ public enum OrphanedSessionReaper { for directory in workspaceDirectories { var workspaceLive: Set = [] 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 { + // 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. + if ProjectPersistence.isSidecarFileName(file.lastPathComponent) { continue } guard let data = try? Data(contentsOf: file), let graph = try? JSONDecoder().decode(LoopGraph.self, from: data) else { return nil } diff --git a/GraphcodeKit/Sources/Workspace.swift b/GraphcodeKit/Sources/Workspace.swift index 7ec47ff9..761b0163 100644 --- a/GraphcodeKit/Sources/Workspace.swift +++ b/GraphcodeKit/Sources/Workspace.swift @@ -233,7 +233,7 @@ extension Workspace { let projectsDirectory = url.appendingPathComponent("projects", isDirectory: true) for name in (try? fileManager.contentsOfDirectory(atPath: projectsDirectory.path)) ?? [] - where name.hasSuffix(".json") { + where name.hasSuffix(".json") && !ProjectPersistence.isSidecarFileName(name) { guard let data = try? Data(contentsOf: projectsDirectory.appendingPathComponent(name)), let graph = try? JSONDecoder().decode(LoopGraph.self, from: data) else { continue } diff --git a/graphcode/Tests/CondemnedSessionsTests.swift b/graphcode/Tests/CondemnedSessionsTests.swift index 00980ba2..7aa875c2 100644 --- a/graphcode/Tests/CondemnedSessionsTests.swift +++ b/graphcode/Tests/CondemnedSessionsTests.swift @@ -1,4 +1,5 @@ import Foundation +import MailroomKit import Testing @testable import GraphcodeKit @@ -151,6 +152,82 @@ struct OrphanedSessionReaperTests { #expect(live.isEmpty) } + /// #307 moved the Mailroom out of the graph file and into `.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) + } + + /// 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") diff --git a/graphcode/Tests/ProjectPersistenceTests.swift b/graphcode/Tests/ProjectPersistenceTests.swift index 56a308da..4fabe9c8 100644 --- a/graphcode/Tests/ProjectPersistenceTests.swift +++ b/graphcode/Tests/ProjectPersistenceTests.swift @@ -208,6 +208,30 @@ struct MailroomPersistenceTests { #expect(writer.load(path: "/tmp/never-saved") == nil) } + /// 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 diff --git a/graphcode/Tests/ProjectRegistryTests.swift b/graphcode/Tests/ProjectRegistryTests.swift index 694a1dab..42896762 100644 --- a/graphcode/Tests/ProjectRegistryTests.swift +++ b/graphcode/Tests/ProjectRegistryTests.swift @@ -317,14 +317,21 @@ struct ProjectRegistryTests { // killing them. let directory = FileManager.default.temporaryDirectory .appendingPathComponent("graphcode-tests-\(UUID().uuidString)", isDirectory: true) + // A real folder, or `routing` refuses the open, no loop is ever created, and every + // expectation below holds for having found nothing. + let project = FileManager.default.temporaryDirectory + .appendingPathComponent("project-closed-\(UUID().uuidString)", isDirectory: true) + try? FileManager.default.createDirectory(at: project, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: project) } + let projectPath = project.resolvingSymlinksInPath().path let firstRun = ProjectRegistry( persistenceDirectory: directory, ensureSession: { _, _ in }, terminateSession: { _, _ in }) let connectionID = UUID() await firstRun.addConnection(id: connectionID, fileDescriptor: -1) - await firstRun.handle(.openProject(path: "/tmp/project-g"), connectionID: connectionID) + await firstRun.handle(.openProject(path: projectPath), connectionID: connectionID) await firstRun.handle( .graphCommand( - projectPath: "/tmp/project-g", + projectPath: projectPath, command: .createNode( NodeDraft(title: "Watcher", loopType: .timeBased, triggerPrompt: "/loop 1h Check"))), connectionID: connectionID) @@ -332,7 +339,8 @@ struct ProjectRegistryTests { // daemon below only ever exists after that. firstRun.flushPersistence() let persistence = ProjectPersistence(baseDirectory: directory) - let nodeID = persistence.loadGraph(path: "/tmp/project-g")?.nodes.first?.id + let nodeID = persistence.loadGraph(path: projectPath)?.nodes.first?.id + #expect(nodeID != nil, "nothing was created, so what follows would pass for finding none") let killed = LockIsolated>([]) let started = LockIsolated>([]) @@ -343,13 +351,17 @@ struct ProjectRegistryTests { let freshConnection = UUID() await secondRun.addConnection(id: freshConnection, fileDescriptor: -1) await secondRun.handle( - .deleteProjectGraph(path: "/tmp/project-g"), connectionID: freshConnection) + .deleteProjectGraph(path: projectPath), connectionID: freshConnection) #expect(killed.value == Set([nodeID].compactMap { $0 })) #expect(started.value.isEmpty) - #expect(persistence.loadGraph(path: "/tmp/project-g") == nil) + #expect(persistence.loadGraph(path: projectPath) == nil) } + /// The same delete with the writer running as it does in production — asynchronously, + /// with a save for this very project still queued. The graph must go and stay gone: a + /// drain landing after `deleteGraph` would rewrite the file, and a `load` still + /// answering from the queue would hand the deleted loops to the next reader. /// The bug this guards: a folder added from outside the app — `graphcode status /// `, which is how an editor plugin adds one — was persisted into the open set /// but reached no *running* app, so it appeared to have been ignored and only showed up @@ -453,3 +465,46 @@ private func readFrameOffThread(_ fileDescriptor: Int32) async throws -> Data { } } } + +/// Kept out of the suite's body only to stay inside swiftlint's `type_body_length`. +extension ProjectRegistryTests { + @Test + func deletingAProjectWithASaveStillQueuedDoesNotBringItBack() async throws { + let directory = FileManager.default.temporaryDirectory + .appendingPathComponent("graphcode-tests-\(UUID().uuidString)", isDirectory: true) + // A real folder: `routing` refuses a path with nothing at it, and a test that skips + // this creates no loops at all and then passes for having found none. + let project = FileManager.default.temporaryDirectory + .appendingPathComponent("project-queued-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: project, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: project) } + let path = project.resolvingSymlinksInPath().path + + let killed = LockIsolated>([]) + let registry = ProjectRegistry( + persistenceDirectory: directory, + ensureSession: { _, _ in }, + terminateSession: { node, _ in _ = killed.withValue { $0.insert(node.id) } }) + let persistence = ProjectPersistence(baseDirectory: directory) + let connectionID = UUID() + await registry.addConnection(id: connectionID, fileDescriptor: -1) + await registry.handle(.openProject(path: path), connectionID: connectionID) + for index in 0..<8 { + await registry.handle( + .graphCommand( + projectPath: path, + command: .createNode( + NodeDraft( + title: "Watcher\(index)", loopType: .timeBased, triggerPrompt: "/loop 1h Check"))), + connectionID: connectionID) + } + + // Deliberately no flush: the delete races the writer, the way it does in a daemon + // that is still running. + await registry.handle(.deleteProjectGraph(path: path), connectionID: connectionID) + #expect(killed.value.count == 8) + + registry.flushPersistence() + #expect(persistence.loadGraph(path: path) == nil, "a queued save rewrote a deleted graph") + } +} From 19028d205b5b5b6413a184bf44d52d0297bf3916 Mon Sep 17 00:00:00 2001 From: scgopi Date: Sun, 6 Sep 2026 20:25:28 -0700 Subject: [PATCH 3/3] Decode decides where the sidecar name can lie; forget serialises with the drain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A project path ending in `.mailroom` mints a graph file that carries the sidecar suffix, so skipping by name alone would drop its live sessions out of a reap's set — the fail-open direction, the opposite of the one this fix trades in. Both readers now decode first and let the name decide only the failure path: a sidecar, corrupt or not, never decodes as a graph and is skipped; a sidecar-named file that decodes as a graph is a graph. Workspace.contents drops its pre-decode filter — a room never decodes as a graph, so the decode already skipped it, and the filter is what dropped a sidecar-named project from the listing. `GraphWriter.forget` syncs against the drain queue: a drain that had already popped a save still had the write ahead of it, and that write — made outside the lock — could land after `deleteGraph` removed the file and resurrect it. Observed once under full-suite load with the lock-only version; the queue is the one place that ordering lives. Tests: the sidecar-named graph at the reaper and in the workspace listing, a corrupt room still not stopping a reap, and the room digest judged against its own file rather than the project path — the re-keying fix had no test of its own. --- GraphcodeKit/Sources/GraphWriter.swift | 14 +++-- .../Sessions/OrphanedSessionReaper.swift | 19 ++++--- GraphcodeKit/Sources/Workspace.swift | 6 ++- graphcode/Tests/CondemnedSessionsTests.swift | 54 +++++++++++++++++++ graphcode/Tests/ProjectPersistenceTests.swift | 39 ++++++++++++++ graphcode/Tests/WorkspaceTests.swift | 25 +++++++++ 6 files changed, 147 insertions(+), 10 deletions(-) diff --git a/GraphcodeKit/Sources/GraphWriter.swift b/GraphcodeKit/Sources/GraphWriter.swift index fb5d680f..429413c2 100644 --- a/GraphcodeKit/Sources/GraphWriter.swift +++ b/GraphcodeKit/Sources/GraphWriter.swift @@ -56,10 +56,18 @@ public final class GraphWriter: @unchecked Sendable { /// 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) { - lock.lock() - pending.removeValue(forKey: path) - lock.unlock() + queue.sync { + lock.lock() + pending.removeValue(forKey: path) + lock.unlock() + } } /// Returns once everything queued so far is on disk. diff --git a/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift b/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift index 42dcced8..421ee1b9 100644 --- a/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift +++ b/GraphcodeKit/Sources/Sessions/OrphanedSessionReaper.swift @@ -70,14 +70,21 @@ public enum OrphanedSessionReaper { at: projects, includingPropertiesForKeys: nil))? .filter { $0.pathExtension == "json" } ?? [] for file in projectFiles { - // 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. - if ProjectPersistence.isSidecarFileName(file.lastPathComponent) { continue } + 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)) diff --git a/GraphcodeKit/Sources/Workspace.swift b/GraphcodeKit/Sources/Workspace.swift index 761b0163..c8e6b570 100644 --- a/GraphcodeKit/Sources/Workspace.swift +++ b/GraphcodeKit/Sources/Workspace.swift @@ -233,7 +233,11 @@ extension Workspace { let projectsDirectory = url.appendingPathComponent("projects", isDirectory: true) for name in (try? fileManager.contentsOfDirectory(atPath: projectsDirectory.path)) ?? [] - where name.hasSuffix(".json") && !ProjectPersistence.isSidecarFileName(name) { + 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 } diff --git a/graphcode/Tests/CondemnedSessionsTests.swift b/graphcode/Tests/CondemnedSessionsTests.swift index 7aa875c2..4df16e98 100644 --- a/graphcode/Tests/CondemnedSessionsTests.swift +++ b/graphcode/Tests/CondemnedSessionsTests.swift @@ -199,6 +199,60 @@ struct OrphanedSessionReaperTests { #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, diff --git a/graphcode/Tests/ProjectPersistenceTests.swift b/graphcode/Tests/ProjectPersistenceTests.swift index 4fabe9c8..b9ad83a7 100644 --- a/graphcode/Tests/ProjectPersistenceTests.swift +++ b/graphcode/Tests/ProjectPersistenceTests.swift @@ -208,6 +208,45 @@ 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 diff --git a/graphcode/Tests/WorkspaceTests.swift b/graphcode/Tests/WorkspaceTests.swift index e5497a8a..fb41cbb5 100644 --- a/graphcode/Tests/WorkspaceTests.swift +++ b/graphcode/Tests/WorkspaceTests.swift @@ -311,6 +311,31 @@ struct WorkspaceTests { #expect(contents.sessionNames.count == 2) } + @Test + func aGraphNamedLikeASidecarIsCountedAndItsRoomIsNot() throws { + // A project path ending in `.mailroom` mints a graph file that carries the sidecar + // suffix. A room — valid or corrupt — never decodes as a graph, so a room is skipped + // by the decode and a sidecar-named graph is not. + let home = makeHome() + defer { try? FileManager.default.removeItem(at: home) } + let workspace = try Workspace.create(name: "work", home: home) + + let node = LoopNode(title: "a loop") + var graph = LoopGraph(project: ProjectRef(path: "/tmp/x.mailroom", name: "project")) + graph.nodes.append(node) + let projects = workspace.url.appendingPathComponent("projects", isDirectory: true) + try FileManager.default.createDirectory(at: projects, withIntermediateDirectories: true) + try JSONEncoder().encode(graph) + .write(to: projects.appendingPathComponent("_tmp_x.mailroom.json")) + try Data("[]".utf8) + .write(to: projects.appendingPathComponent("_tmp_x.mailroom.mailroom.json")) + + let contents = workspace.contents() + #expect(contents.projects == 1) + #expect(contents.loops == 1) + #expect(contents.sessionNames == ["graphcode-\(node.id.uuidString)"]) + } + @Test func aLiveClaimIsNotStolenByASecondProcess() { // The test host launches the app, which claims the default workspace on the way up.