From 1e11d5dfcad594d55f86177656d8c3453a03690d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20DUCOM?= Date: Mon, 17 Aug 2026 00:51:58 +0200 Subject: [PATCH] test(flaky): silence the FSWatcher echo and stop the sizing waits from giving up quietly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unrelated flakes in the same CI run (run 31976426203). denseBurstCoalescesToItsClusterAncestor asserts exactly which reconcile decision one forged FSEvents batch produces. But the suite's premise — "no FSEvents latency" — was never true: scan(url:) arms a real FSWatcher on the fixture, and it echoes the test's own writes back a second later. The extra batch re-marks paths the cycle already reconciled, landing as an extra re-stat. Idempotent for the tree, fatal for a counter. On a loaded runner a reconcile cycle outlives the watcher's latency, so the echo arrives while the test is still waiting. The suite half-knew this: preciseEventLeadsToRestatOnRefresh carried a NB dropping a dirtyPaths assert for exactly this reason. Generalise it instead — ScanController gains a suspend flag (not a bare stopWatching, which a rescan would undo) and the scanned() helper pins the watcher off, so the forged batches are the only input. The dropped assert comes back. Reproduced under CPU load: 10 failures in 20 runs before, 0 in 20 after. toggleAllCyclesTriState is the second, and the fix is diagnostic rather than reproduced — it never fell locally in 40 loaded runs. Its five failures are consistent with one cause: a row still .pending reads exactly like a row that was never there, and the whole tri-state cycle shifts by one. waitForReady polls RunLoop.main, which every parallel @MainActor suite pumps too, so a round costs far more than its 20 ms slice and 5 s went to mutual blocking rather than to work. The deadline only exists to stop a hang, so it is now generous — but the real defect was expiring in silence, which let a timeout masquerade as five bogus assertions downstream. Both waits now report it. --- .../ViewModel/ScanController.swift | 17 ++++++++++++++++- Tests/SpaceMattersTests/CleanupTests.swift | 18 ++++++++++++++++-- .../IncrementalRefreshTests.swift | 13 +++++++++++-- 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/Sources/SpaceMatters/ViewModel/ScanController.swift b/Sources/SpaceMatters/ViewModel/ScanController.swift index 7945773..b29ae17 100644 --- a/Sources/SpaceMatters/ViewModel/ScanController.swift +++ b/Sources/SpaceMatters/ViewModel/ScanController.swift @@ -1370,9 +1370,24 @@ final class ScanController { // MARK: Live disk watching (SPEC-04) + /// Live watching pinned off. The incremental-refresh tests forge FSEvents + /// batches straight into `handleDiskChanges` to assert *exactly* which + /// reconcile decision a batch produces; a real stream on the same fixture + /// echoes their own writes back one `FSWatcher` latency later and adds + /// re-stats nobody asked for. A flag rather than a bare `stopWatching()` + /// because a rescan re-arms the stream. + @ObservationIgnored private(set) var liveWatchingSuspended = false + + /// Detach from the disk for good — tests that drive `handleDiskChanges` + /// themselves need the forged batches to be the only input. + func suspendLiveWatching() { + liveWatchingSuspended = true + stopWatching() + } + private func startWatching() { stopWatching() - guard isHostScan, !watchPaths.isEmpty else { return } + guard !liveWatchingSuspended, isHostScan, !watchPaths.isEmpty else { return } let w = FSWatcher(paths: watchPaths) { [weak self] changes in Task { @MainActor in self?.handleDiskChanges(changes) } } diff --git a/Tests/SpaceMattersTests/CleanupTests.swift b/Tests/SpaceMattersTests/CleanupTests.swift index 35c1ddf..685f5cf 100644 --- a/Tests/SpaceMattersTests/CleanupTests.swift +++ b/Tests/SpaceMattersTests/CleanupTests.swift @@ -25,19 +25,33 @@ import Foundation icon: "shippingbox", note: "fixture", paths: paths) } - static func waitForReady(_ c: CleanupController, timeout: TimeInterval = 5) async { + /// Sizing is a real scanner walk, and the poll below pumps the *shared* main + /// run loop — every other `@MainActor` suite running in parallel pumps it + /// too, so each round costs far more than its 20 ms slice on a loaded + /// runner. The deadline is only there to stop a hang, so it is generous; + /// what matters is that expiring is *reported*. Giving up silently is what + /// turned one slow CI sizing into five bogus failures further down + /// `toggleAllCyclesTriState` — a row that was merely still `.pending` reads + /// exactly like a row that was never there. + static func waitForReady(_ c: CleanupController, timeout: TimeInterval = 60, + sourceLocation: SourceLocation = #_sourceLocation) async { let deadline = Date().addingTimeInterval(timeout) while c.state != .ready && Date() < deadline { RunLoop.main.run(until: Date().addingTimeInterval(0.02)) await Task.yield() } + #expect(c.state == .ready, "timed out waiting for .ready, still \(c.state)", + sourceLocation: sourceLocation) } - static func waitForCleaning(_ c: CleanupController, timeout: TimeInterval = 5) async { + static func waitForCleaning(_ c: CleanupController, timeout: TimeInterval = 60, + sourceLocation: SourceLocation = #_sourceLocation) async { let deadline = Date().addingTimeInterval(timeout) while c.state != .cleaning && Date() < deadline { await Task.yield() } + #expect(c.state == .cleaning, "timed out waiting for .cleaning, still \(c.state)", + sourceLocation: sourceLocation) } // MARK: Catalog diff --git a/Tests/SpaceMattersTests/IncrementalRefreshTests.swift b/Tests/SpaceMattersTests/IncrementalRefreshTests.swift index 5814aca..9b6b891 100644 --- a/Tests/SpaceMattersTests/IncrementalRefreshTests.swift +++ b/Tests/SpaceMattersTests/IncrementalRefreshTests.swift @@ -24,6 +24,14 @@ import Foundation let c = ScanController() c.scan(url: root) await NavigationTests.waitForScan(c) + // The forged batches must be the *only* input. A real FSWatcher is armed + // on this same fixture, and it echoes the writes these tests make back + // one second later — a second batch for paths already reconciled, which + // lands as an extra re-stat or subtree pass. Harmless to the tree (both + // are idempotent), fatal to any assertion that counts them: it flaked + // `autoRestatCount == 1` on a loaded runner, where a cycle outlives the + // watcher's latency. + c.suspendLiveWatching() return c } @@ -125,8 +133,9 @@ import Foundation #expect(c.dirtyNeeds[alphaPath] == .restat) await c.refreshDirty() - // NB: no `dirtyPaths.isEmpty` assert — the *real* FSWatcher also runs on - // this temp dir and its echo of our write may legitimately re-mark it. + // The watcher is suspended, so nothing re-marks the path behind us: the + // dirty set must drain completely. + #expect(c.dirtyPaths.isEmpty) #expect(alpha.directFileCount == 2) // Identity preserved — the precise path never went through invalidate. #expect(NavigationTests.child(c.root!, "alpha") === alpha)