From 2ba69894819c5653dde23f8dc059e15bda5c5006 Mon Sep 17 00:00:00 2001 From: rzen Date: Sun, 26 Jul 2026 19:24:11 -0400 Subject: [PATCH] Align FolderWatcher with the newly ratified design decisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parallel design session resolved the m3 watcher findings; two changed behavior: .git internals are now filtered at any depth (a nested clone or submodule is a stray whose internals never render), and a root change landing mid-bracket is owned by the root-change path — endBracket() skips its mandatory reload once a .rootChanged tore the stream down, cleared when a fresh stream attaches. The skip is scoped by an explicit flag, not by stream absence: a watcher whose stream never came up still delivers post-bracket reloads, since distrust of FSEvents delivery is the mandatory reload's whole reason. Two new watcher tests; full suite 281 tests in 54 suites green. Claude-Session: https://claude.ai/code/session_018BjQRYBR6jQja3jCRi5S3A --- Kanban/LiveStore/FolderWatcher.swift | 54 ++++++++++++++++++++++----- KanbanTests/FolderWatcherTests.swift | 55 ++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/Kanban/LiveStore/FolderWatcher.swift b/Kanban/LiveStore/FolderWatcher.swift index a41864f..da65461 100644 --- a/Kanban/LiveStore/FolderWatcher.swift +++ b/Kanban/LiveStore/FolderWatcher.swift @@ -136,6 +136,14 @@ public final class FolderWatcher { /// that use them do: an undo restore inside a branch switch is one bracketed span, not two. private var bracketDepth = 0 + /// True from a delivered `.rootChanged` until a fresh stream attaches (`start()` or + /// `reattach(to:)`). While set, `endBracket()` skips its mandatory reload — the root-change + /// path owns recovery (settled, 02-architecture.md § Write-failure surfacing). Deliberately + /// **not** the same test as `handle == nil`: a watcher that never got a stream (or whose + /// creation failed on some degraded volume) must still deliver post-bracket reloads — + /// distrust of FSEvents delivery is the reason the mandatory reload exists at all. + private var rootChangeOwnsRecovery = false + /// The origin a pending delivery will carry, merged per `WatchOrigin.merged(_:_:)`. Non-nil /// means "a delivery is owed" — which is not the same as "a timer is running": while a /// bracket is open the origin is remembered with no timer armed, and `endBracket()` merges @@ -264,6 +272,9 @@ public final class FolderWatcher { } handle = StreamHandle(stream: stream) + // A fresh stream at whatever root we now watch: if a root change had handed recovery to + // the root-change path, that handoff is complete. + rootChangeOwnsRecovery = false return true } @@ -295,6 +306,11 @@ public final class FolderWatcher { stop() watchedPath = Self.canonicalPath(of: newRoot) _ = start() + // Reattaching completes the root-change handoff even if the fresh stream failed to come + // up (`start()` clears the flag only on success): the reconciling reload below is + // promised unconditionally, and mid-bracket it must fold into the post-bracket delivery + // rather than be stranded behind `endBracket()`'s root-change skip. + rootChangeOwnsRecovery = false schedule(.reconciling) } @@ -326,16 +342,24 @@ public final class FolderWatcher { cancelPendingDelivery() } - /// Closes a bracket. At depth 0 this **always** schedules the debounced - /// `.treeChanged(.appMediated)` — the mandatory single post-bracket reload — even if not one - /// filesystem event was seen inside the bracket. The bracket's contract is "finish with one - /// full reload"; making that conditional on having observed events would make correctness - /// depend on FSEvents delivery, which is precisely the thing this design refuses to trust. + /// Closes a bracket. At depth 0 this schedules the debounced `.treeChanged(.appMediated)` — + /// the mandatory single post-bracket reload — even if not one filesystem event was seen + /// inside the bracket. The bracket's contract is "finish with one full reload"; making that + /// conditional on having observed events would make correctness depend on FSEvents delivery, + /// which is precisely the thing this design refuses to trust. /// /// FSEvents produced by the bracketed operation and still in flight when it closes (kernel /// latency does not respect our brackets) simply coalesce into that pending delivery via the /// origin merge, arriving as `.appMediated` rather than as a second `.foreign` reload. /// + /// **The one exception: a root change that landed mid-bracket** (settled, 02-architecture.md + /// § Write-failure surfacing — "owned by the root-change path, not the bracket"). A + /// `.rootChanged` tears the stream down, so with no live stream at close the mandatory + /// reload is skipped as a no-op: a rename the consumer absorbed mid-bracket has already + /// re-attached by now (its banked reconciling reload delivers here instead, at the + /// re-resolved root), and a true vanish raised the read-only lock whose clearance runs a + /// reconciling reload of its own — nothing is lost by not failing redundantly. + /// /// An unbalanced call — `endBracket()` at depth 0 — is ignored rather than trapping: the /// consumer's brackets wrap `do`/`catch` spans over git operations, and a bug there should /// not take the app down. @@ -343,6 +367,7 @@ public final class FolderWatcher { guard bracketDepth > 0 else { return } bracketDepth -= 1 guard bracketDepth == 0 else { return } + guard !rootChangeOwnsRecovery else { return } schedule(.appMediated) } @@ -466,9 +491,11 @@ public final class FolderWatcher { /// /// Bypassing the bracket is deliberate and is the one place the "nothing fires mid-bracket" /// rule yields: a bracketed operation whose root disappeared underneath it cannot finish, and - /// its post-bracket reload would resolve against a path that is gone. The consumer must know - /// now, so it can re-resolve its bookmark and either `reattach(to:)` or enter the - /// vanished-root read-only lock (02-architecture.md § Write-failure surfacing). + /// its post-bracket reload would resolve against a path that is gone (with the stream torn + /// down here, `endBracket()` skips that reload — the root-change path owns recovery from this + /// point, settled). The consumer must know now, so it can re-resolve its bookmark and either + /// `reattach(to:)` or enter the vanished-root read-only lock (02-architecture.md + /// § Write-failure surfacing). /// /// Any pending debounced delivery is dropped: it was going to report on a tree that no longer /// exists at that path, and whatever the consumer does next — `reattach(to:)` or the lock — @@ -477,12 +504,14 @@ public final class FolderWatcher { teardownStream() cancelPendingDelivery() pendingOrigin = nil + rootChangeOwnsRecovery = true handler(.rootChanged) } // MARK: - .git filtering - /// Whether `path` lives inside the board root's `.git` directory. + /// Whether `path` lives under any `.git` component inside the board tree (settled, + /// 02-architecture.md § Components). /// /// **Why filter at all**: the app auto-commits a couple of seconds after every change /// (06-history-undo.md), so every single edit the user makes is followed by a burst of writes @@ -491,6 +520,11 @@ public final class FolderWatcher { /// redundant full tree walk. None of that churn can alter the rendered tree: the loader walks /// UUID-shaped folders and `index.md` files, and `.git` contains neither. /// + /// **Any depth, not just the root's own repo**: a repo nested deeper — a card folder + /// containing a clone, a submodule — is a stray (01-storage-format.md) whose internals never + /// render either, so its churn is filtered on the same grounds. The nested repo's *working + /// files* are ordinary stray paths and still fire like anything else. + /// /// **Why it is safe**: this filters `.git`'s *internals*, not git's effects. An external /// `git checkout`, `git pull`, or `git stash` rewrites working-tree files, and those events /// arrive unfiltered in the same batch as the `.git` writes — the batch has a relevant path, @@ -503,7 +537,7 @@ public final class FolderWatcher { guard path.hasPrefix(watchedPath) else { return false } let relative = path.dropFirst(watchedPath.count) guard relative.hasPrefix("/") else { return false } - return relative.dropFirst().hasPrefix(".git/") || relative.dropFirst() == ".git" + return relative.split(separator: "/").contains(".git") } // MARK: - Helpers diff --git a/KanbanTests/FolderWatcherTests.swift b/KanbanTests/FolderWatcherTests.swift index 6efc1c5..447bb56 100644 --- a/KanbanTests/FolderWatcherTests.swift +++ b/KanbanTests/FolderWatcherTests.swift @@ -344,6 +344,34 @@ struct FolderWatcherTests { #expect(log.events == [.treeChanged(.foreign)]) } + @Test("A repo nested inside a card folder is filtered like the root's own") + func nestedGitChurnIsFiltered() async throws { + let fixture = try WatchFixture() + defer { fixture.tearDown() } + let cardFolder = "1e9a7c5e-0000-4000-8000-000000000000" + fixture.makeDirectory("\(cardFolder)/.git") + let log = EventLog() + let watcher = FolderWatcher(root: fixture.root, debounce: testDebounce, latency: testLatency) { + log.record($0) + } + #expect(watcher.start()) + defer { watcher.stop() } + await drainStartupChurn(log) + + // A clone living inside a card folder is a stray (01-storage-format.md) whose internals + // never render — filtered at any depth, not just the board root's own repo (02, settled). + fixture.write("\(cardFolder)/.git/index", "fake index") + fixture.write("\(cardFolder)/.git/refs/heads/main", "deadbeef") + await quiet(.milliseconds(700)) + #expect(log.events.isEmpty) + + // The nested repo's *working files* are ordinary stray paths and still fire. + fixture.write("\(cardFolder)/README.md") + await waitUntil { log.count >= 1 } + await quiet() + #expect(log.events == [.treeChanged(.foreign)]) + } + // MARK: Root identity @Test("Deleting the watched root delivers rootChanged and tears the stream down") @@ -367,6 +395,33 @@ struct FolderWatcherTests { #expect(!watcher.isWatching) } + @Test("A root vanish mid-bracket is owned by the root-change path; endBracket() is a no-op") + func rootVanishMidBracketSkipsThePostBracketReload() async throws { + let fixture = try WatchFixture() + defer { fixture.tearDown() } + let log = EventLog() + let watcher = FolderWatcher(root: fixture.root, debounce: testDebounce, latency: testLatency) { + log.record($0) + } + #expect(watcher.start()) + defer { watcher.stop() } + await drainStartupChurn(log) + + watcher.beginBracket() + fixture.remove() + // `.rootChanged` bypasses the bracket — the consumer must know *now*. + await waitUntil(.seconds(10)) { log.events.contains(.rootChanged) } + #expect(log.events == [.rootChanged]) + #expect(!watcher.isWatching) + + // The bracket's mandatory final reload is skipped: the stream is gone, and the + // root-change path owns recovery from here (02, settled) — `reattach(to:)` or the + // vanished-root lock, each ending in a reload of its own. + watcher.endBracket() + await quiet(.milliseconds(700)) + #expect(log.events == [.rootChanged]) + } + // MARK: Stop @Test("Nothing is delivered after stop()")