diff --git a/Mounty/Services/MountService.swift b/Mounty/Services/MountService.swift index 9ab571a..e5f9c34 100644 --- a/Mounty/Services/MountService.swift +++ b/Mounty/Services/MountService.swift @@ -49,21 +49,34 @@ struct MountService { SystemMountService.findMountPath(forURL: url) } ).value { - if await ReachabilityService.isMountPointAlive(path: existing) { + switch await ReachabilityService.probeMountPoint(path: existing) { + case .alive: AppLogger.log( "Mount skipped; already mounted and responsive: \(mountTarget(for: url)) -> \(existing)", source: .mountService ) return .success(path: existing) - } - AppLogger.log( - "Existing mount is unresponsive: \(existing); unmounting before retry", - level: .warning, - source: .mountService - ) - guard await unmount(path: existing) else { - return .failed(code: EBUSY) + case .indeterminate: + // A share saturated with I/O answers statfs slowly. Recovering it here + // would force-unmount a healthy mount and destroy other processes' + // open file descriptors, so a busy mount is always left alone. + AppLogger.log( + "Mount skipped; already mounted and busy: \(mountTarget(for: url)) -> \(existing)", + source: .mountService + ) + return .success(path: existing) + + case .dead(let code): + AppLogger.log( + "Existing mount is dead: \(existing); errno=\(code): " + + "\(String(cString: strerror(code))); unmounting before retry", + level: .warning, + source: .mountService + ) + guard await unmount(path: existing) else { + return .failed(code: EBUSY) + } } } diff --git a/Mounty/Services/ReachabilityService.swift b/Mounty/Services/ReachabilityService.swift index b84315f..100f857 100644 --- a/Mounty/Services/ReachabilityService.swift +++ b/Mounty/Services/ReachabilityService.swift @@ -3,52 +3,89 @@ import Foundation import Network import Synchronization +/// Outcome of a mount-point liveness probe. +enum MountProbe: Sendable, Equatable { + /// statfs(2) answered: the mount is responsive. + case alive + /// statfs(2) answered with an error: the mount is gone or unusable. + case dead(code: Int32) + /// The deadline elapsed while statfs(2) was still outstanding. A share saturated + /// with I/O answers slowly — it is busy, not dead — so this outcome must never + /// be treated as a failure or trigger recovery. + case indeterminate + + nonisolated static func == (lhs: MountProbe, rhs: MountProbe) -> Bool { + switch (lhs, rhs) { + case (.alive, .alive), (.indeterminate, .indeterminate): true + case (.dead(let lhsCode), .dead(let rhsCode)): lhsCode == rhsCode + default: false + } + } +} + /// Verifies server and mount point responsiveness. struct ReachabilityService { nonisolated private static let mountProbes = MountProbeRegistry() - /// Validates filesystem responsiveness by calling statfs(2) on the mount path. + /// Probes filesystem responsiveness by calling statfs(2) on the mount path. /// /// statfs() queries kernel-level filesystem metadata without reading file content, /// so it never triggers the macOS TCC "access files on a network volume" prompt. - /// It will block (and thus timeout) on a hung/dead mount, which is exactly the - /// behaviour we need to detect silently dead kernel mounts. + /// It will block on a hung/dead mount, which is exactly the behaviour we need to + /// detect silently dead kernel mounts. + /// + /// A busy share also answers slowly, so an elapsed deadline reports `.indeterminate` + /// rather than a failure: only an errno from statfs(2) proves the mount is `.dead`. /// - /// Async: dispatches statfs to a background thread so the Swift cooperative - /// thread pool is never blocked waiting for a hung mount. - nonisolated static func isMountPointAlive(path: String) async -> Bool { + /// Async: dispatches statfs to a background thread so the Swift cooperative thread + /// pool is never blocked waiting for a hung mount. Concurrent probes of one path + /// share a single syscall, and every caller applies its own deadline. + nonisolated static func probeMountPoint( + path: String, + timeout: TimeInterval = 2.0, + hangGrace: Duration = .seconds(60) + ) async -> MountProbe { + let token = UUID() return await withCheckedContinuation { continuation in - guard mountProbes.register(path: path, continuation: continuation) else { return } - - DispatchQueue.global(qos: .utility).async { - defer { mountProbes.finish(path: path) } - // Allocate uninitialized memory instead of calling statfs.init(), - // which is @MainActor under SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor. - // The C statfs(2) syscall writes the struct entirely so zero-init - // is unnecessary and the @MainActor init can be bypassed safely. - let buf = UnsafeMutablePointer.allocate(capacity: 1) - defer { buf.deallocate() } - let status = statfs(path, buf) - let errorCode = errno - let alive = status == 0 - if mountProbes.resolve(path: path, result: alive) { - if !alive { + if mountProbes.register(path: path, token: token, continuation: continuation) { + DispatchQueue.global(qos: .utility).async { + // Allocate uninitialized memory instead of calling statfs.init(), + // which is @MainActor under SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor. + // The C statfs(2) syscall writes the struct entirely so zero-init + // is unnecessary and the @MainActor init can be bypassed safely. + let buf = UnsafeMutablePointer.allocate(capacity: 1) + defer { buf.deallocate() } + let status = statfs(path, buf) + let errorCode = errno + if status == 0 { + mountProbes.complete(path: path, result: .alive) + } else { AppLogger.log( "Mount probe failed: \(path); errno=\(errorCode): \(String(cString: strerror(errorCode)))", level: .warning, source: .reachability ) + mountProbes.complete(path: path, result: .dead(code: errorCode)) } } } - DispatchQueue.global().asyncAfter(deadline: .now() + 1.0) { - if mountProbes.resolve(path: path, result: false) { + DispatchQueue.global().asyncAfter(deadline: .now() + timeout) { + switch mountProbes.timeOut(path: path, token: token, hangGrace: hangGrace) { + case .indeterminate: + AppLogger.log( + "Mount probe still pending after \(timeout) s: \(path); the mount is busy, not dead", + level: .debug, + source: .reachability + ) + case .dead: AppLogger.log( - "Mount probe timed out after 1 s: \(path)", + "Mount probe has been stuck for over \(hangGrace): \(path); treating the mount as dead", level: .warning, source: .reachability ) + default: + break } } } @@ -108,10 +145,16 @@ struct ReachabilityService { } } -private final class MountProbeRegistry: Sendable { +/// Coalesces concurrent statfs(2) probes of the same path onto a single syscall. +/// +/// A probe entry exists only while its syscall is outstanding, and no verdict is ever +/// stored: a caller that gives up on its own deadline is simply dropped from the waiters. +/// A later caller therefore joins the still-running syscall and waits for the real +/// answer instead of inheriting an earlier caller's timeout as if it were a result. +final class MountProbeRegistry: Sendable { private struct ProbeState { - var result: Bool? - var waiters: [CheckedContinuation] + let startedAt = ContinuousClock.now + var waiters: [UUID: CheckedContinuation] } private let probes = Mutex([String: ProbeState]()) @@ -119,48 +162,50 @@ private final class MountProbeRegistry: Sendable { /// Registers a caller and returns true only when it must start the underlying syscall. nonisolated func register( path: String, - continuation: CheckedContinuation + token: UUID, + continuation: CheckedContinuation ) -> Bool { - var immediateResult: Bool? - let shouldStart = probes.withLock { probes in + probes.withLock { probes in guard var state = probes[path] else { - probes[path] = ProbeState(result: nil, waiters: [continuation]) + probes[path] = ProbeState(waiters: [token: continuation]) return true } - if let result = state.result { - immediateResult = result - } else { - state.waiters.append(continuation) - probes[path] = state - } + state.waiters[token] = continuation + probes[path] = state return false } - if let immediateResult { - continuation.resume(returning: immediateResult) - } - return shouldStart } - /// Resolves all current and future waiters while the non-cancellable syscall remains active. - @discardableResult - nonisolated func resolve(path: String, result: Bool) -> Bool { - let waiters: [CheckedContinuation]? = probes.withLock { probes in - guard var state = probes[path], state.result == nil else { return nil } - state.result = result - let waiters = state.waiters - state.waiters.removeAll() - probes[path] = state - return waiters - } - guard let waiters else { return false } - for continuation in waiters { + /// Delivers the syscall's verdict to every remaining waiter and clears the probe. + nonisolated func complete(path: String, result: MountProbe) { + guard let state = probes.withLock({ $0.removeValue(forKey: path) }) else { return } + for continuation in state.waiters.values { continuation.resume(returning: result) } - return true } - nonisolated func finish(path: String) { - probes.withLock { _ = $0.removeValue(forKey: path) } + /// Releases a single caller whose deadline elapsed while the syscall is still + /// outstanding, reporting the mount as busy rather than failed — unless the syscall + /// itself has been stuck past `hangGrace`, which a live filesystem never is. + /// + /// Returns the verdict delivered, or `nil` when that caller was no longer waiting. + @discardableResult + nonisolated func timeOut(path: String, token: UUID, hangGrace: Duration) -> MountProbe? { + typealias Resolution = ( + continuation: CheckedContinuation, verdict: MountProbe + ) + + let resolution = probes.withLock { probes -> Resolution? in + guard var state = probes[path], + let continuation = state.waiters.removeValue(forKey: token) + else { return nil } + probes[path] = state + let isHung = state.startedAt.duration(to: .now) > hangGrace + return (continuation, isHung ? .dead(code: ETIMEDOUT) : .indeterminate) + } + guard let resolution else { return nil } + resolution.continuation.resume(returning: resolution.verdict) + return resolution.verdict } } diff --git a/Mounty/ViewModels/VolumeManager.swift b/Mounty/ViewModels/VolumeManager.swift index 8ac1cc8..e9d3dda 100644 --- a/Mounty/ViewModels/VolumeManager.swift +++ b/Mounty/ViewModels/VolumeManager.swift @@ -385,7 +385,10 @@ final class VolumeManager { address: volume.serverAddress ) else { return (volume.id, nil) } - guard await ReachabilityService.isMountPointAlive(path: path) else { + // Only a definitive statfs error means the mount is gone. A busy + // share answers slowly, and dropping it here would make automount + // recover — and thereby tear down — a perfectly healthy mount. + if case .dead = await ReachabilityService.probeMountPoint(path: path) { return (volume.id, nil) } return (volume.id, path) diff --git a/MountyTests/ReachabilityServiceTests.swift b/MountyTests/ReachabilityServiceTests.swift new file mode 100644 index 0000000..9898418 --- /dev/null +++ b/MountyTests/ReachabilityServiceTests.swift @@ -0,0 +1,99 @@ +import Foundation +import Testing + +@testable import Mounty + +private typealias ProbeContinuation = CheckedContinuation + +/// Tests the mount-point liveness probe — the logic that decides whether a kernel mount +/// is healthy, gone, or merely busy. A wrong verdict here makes automount recover (and +/// thus unmount) a share that other processes are actively reading and writing. +struct ReachabilityServiceTests { + + @Test func respondingMountProbesAlive() async { + #expect(await ReachabilityService.probeMountPoint(path: "/") == .alive) + } + + @Test func missingPathProbesDead() async { + let result = await ReachabilityService.probeMountPoint( + path: "/nonexistent-\(UUID().uuidString)" + ) + #expect(result == .dead(code: ENOENT)) + } + + @Test func concurrentProbesOfOnePathAllResolve() async { + let results = await withTaskGroup(of: MountProbe.self) { group in + for _ in 0..<10 { + group.addTask { await ReachabilityService.probeMountPoint(path: "/") } + } + var results: [MountProbe] = [] + for await result in group { results.append(result) } + return results + } + #expect(results.count == 10) + #expect(results.allSatisfy { $0 == .alive }) + } + + /// The regression: a caller that hits its deadline must be released as `.indeterminate` + /// without leaving a verdict behind. Caching it made the next caller — the guard that + /// decides whether to unmount — see a dead mount that was only slow to answer. + @Test func timedOutCallerLeavesNoVerdictForLaterCallers() async { + let registry = MountProbeRegistry() + let path = "/probe" + let first = UUID() + let second = UUID() + + let firstResult = await withCheckedContinuation { (continuation: ProbeContinuation) in + #expect(registry.register(path: path, token: first, continuation: continuation)) + // The syscall is still outstanding when this caller's deadline elapses. + let verdict = registry.timeOut(path: path, token: first, hangGrace: .seconds(60)) + #expect(verdict == .indeterminate) + } + #expect(firstResult == .indeterminate) + + // The next caller joins the still-running syscall and waits for its real answer. + let secondResult = await withCheckedContinuation { (continuation: ProbeContinuation) in + let startsSyscall = registry.register( + path: path, + token: second, + continuation: continuation + ) + #expect(startsSyscall == false) + registry.complete(path: path, result: .alive) + } + #expect(secondResult == .alive) + } + + /// A syscall that stays stuck far past any plausible metadata round trip is a dead + /// mount, not a busy one — the silent-death case automount must still recover. + @Test func syscallStuckPastTheGraceIntervalProbesDead() async { + let registry = MountProbeRegistry() + let path = "/probe" + let token = UUID() + + let result = await withCheckedContinuation { (continuation: ProbeContinuation) in + #expect(registry.register(path: path, token: token, continuation: continuation)) + #expect(registry.timeOut(path: path, token: token, hangGrace: .zero) != nil) + } + #expect(result == .dead(code: ETIMEDOUT)) + } + + @Test func completedProbeIsNotReusedByTheNextCaller() async { + let registry = MountProbeRegistry() + let path = "/probe" + + let result = await withCheckedContinuation { (continuation: ProbeContinuation) in + #expect(registry.register(path: path, token: UUID(), continuation: continuation)) + registry.complete(path: path, result: .dead(code: ENOTCONN)) + } + #expect(result == .dead(code: ENOTCONN)) + + // A finished probe leaves no entry behind, so the next caller — asserted by the + // `true` return — starts a fresh syscall instead of reusing the old verdict. + let laterResult = await withCheckedContinuation { (continuation: ProbeContinuation) in + #expect(registry.register(path: path, token: UUID(), continuation: continuation)) + registry.complete(path: path, result: .alive) + } + #expect(laterResult == .alive) + } +} diff --git a/docs/stories/026-automount-must-not-disturb-busy-mounts.md b/docs/stories/026-automount-must-not-disturb-busy-mounts.md new file mode 100644 index 0000000..398e8b5 --- /dev/null +++ b/docs/stories/026-automount-must-not-disturb-busy-mounts.md @@ -0,0 +1,93 @@ +# STORY-026: Automount must not disturb busy mounts + +- Status: CLOSED +- Type: fix +- Date: 2026-08-13 +- Commit: _none_ + +## Intent + +A slow liveness probe on a healthy but busy share currently makes automount force-unmount and +remount it, which tears down in-flight file I/O in other applications every few minutes. Automount +must only recover mounts that are provably dead, never mounts that are merely slow to answer. + +Reported failure: with automount enabled, the SMB connection drops briefly every few minutes and +Java file I/O on the share aborts; disabling automount makes the problem disappear. + +Chain (heartbeat, every 5 s): + +1. `VolumeManager.detectMounts` probes each mount with `ReachabilityService.isMountPointAlive`, + whose `statfs(2)` is abandoned after a 1 s deadline. Heavy SMB traffic — exactly what a Java + workload produces — pushes `statfs` past that deadline, so the probe reports the mount as dead. +2. The volume drops out of `mountPaths` and therefore becomes an automount candidate. +3. `runAutomount` calls `MountService.mount`, which finds the existing mount and re-probes it before + deciding to recover it. +4. That re-probe returns the **cached** `false` from step 1: `MountProbeRegistry` keeps the timed-out + verdict as the probe result until the still-blocked `statfs` thread calls `finish(path:)`, so the + guard protecting the destructive branch cannot see a healthy mount. +5. `MountService.unmount` runs. The polite `unmountAndEjectDevice` fails with `EBUSY` because the + other application holds open descriptors, so the fallback `unmount(path, MNT_FORCE)` succeeds and + the kernel tears down a healthy share — killing those descriptors. +6. The next heartbeat remounts it, which is why the outage looks brief and recurring. + +## Acceptance criteria + +- [x] A mount-point probe distinguishes *alive*, *dead* (a real `statfs` errno), and *indeterminate* + (deadline reached while the syscall is still outstanding). +- [x] An indeterminate probe never removes a volume from the detected mount state and never triggers + the recovery unmount; only a definitive `statfs` error does. +- [x] Concurrent probes of the same path still share one syscall, and a timed-out verdict is never + served to a later caller as if it were a completed result. +- [x] A volume that is present in the kernel mount table and answering is left untouched by + automount, with no unmount and no remount. +- [x] Recovery of a genuinely dead mount (including the `MNT_FORCE` fallback) keeps working. +- [x] Probe timeouts are logged at a level that does not flood the in-app log on a busy share. + +## Outcome + +`ReachabilityService.isMountPointAlive` became `probeMountPoint`, returning the three-state +`MountProbe`. `MountProbeRegistry` no longer stores a verdict at all: an entry lives only while its +syscall is outstanding, a caller that hits its own deadline is simply dropped from the waiters, and a +later caller joins the running syscall instead of inheriting the earlier timeout. Probes of one path +still share a single syscall, so repeated checks cannot pile up blocked workers (STORY-008). + +`MountService.mountExclusively` now switches on the verdict: `.alive` and `.indeterminate` both +return the existing mount untouched, and only `.dead` reaches the unmount-and-retry branch — so the +`MNT_FORCE` fallback can no longer be aimed at a healthy share. `VolumeManager.detectMounts` keeps a +volume mounted unless the probe is definitively `.dead`, which also stops the "Lost connection" log +churn on a busy share. + +Silent-death detection is preserved by `hangGrace` (60 s): a deadline that elapses while the syscall +itself has been stuck past that interval reports `.dead(ETIMEDOUT)` instead of `.indeterminate`, so a +mount whose `statfs` never returns is still recovered. The per-caller deadline rose from 1 s to 2 s +and its log dropped to `debug`, since a busy share crossing it is now expected and harmless. + +## Validation + +`ReachabilityServiceTests` covers the three verdicts, ten concurrent probes of one path, the +timed-out caller leaving no verdict behind, the completed probe not being reused, and the +hang-grace escalation. `xcrun swift-format lint --strict -r Mounty MountyTests` is clean, and +`xcodebuild -scheme Mounty -destination 'platform=macOS' test CODE_SIGNING_ALLOWED=NO` succeeds; a +`clean build` reports zero source warnings. + +Live run against a corporate SMB share with automount enabled: three minutes of +sustained load (four parallel read/write workers plus one long-lived descriptor doing +`write`/`fsync`, 0.32 GB written and 0.10 GB read) while a watchdog sampled the kernel mount table +twice a second. + +- `statfs(2)` on the share peaked at **3291 ms**, so the probe deadline really was exceeded — the + exact trigger the old 1 s deadline turned into a "dead" verdict. +- Eleven probe timeouts were logged across the two mounted shares, every one of them as + `the mount is busy, not dead`. +- **Zero unmounts and zero remounts**: 400 of 400 watchdog samples found the mount present, with a + constant device id, and no `Lost connection`, `unmounting before retry`, or `Automount` entry + appeared for the duration. +- The long-lived descriptor finished with **no I/O errors** — the failure the report describes. + +A manual connect/disconnect during the same session mounted the share in 1.97 s and unmounted it +politely, so the user-initiated paths are unaffected. + +Not verified on hardware: recovery of a genuinely dead mount. The share could not be unmounted +cleanly (other processes held files open) and force-unmounting a live share was out of scope for +the test, so that branch rests on `syscallStuckPastTheGraceIntervalProbesDead` and the `.dead` +switch case. diff --git a/docs/stories/INDEX.md b/docs/stories/INDEX.md index ea326c8..4d18200 100644 --- a/docs/stories/INDEX.md +++ b/docs/stories/INDEX.md @@ -4,6 +4,7 @@ Newest stories first. Statuses: `OPEN`, `IN_PROGRESS`, `CLOSED`. | ID | Type | Story | Status | Date | | --- | --- | --- | --- | --- | +| [026](./026-automount-must-not-disturb-busy-mounts.md) | fix | Automount must not disturb busy mounts | CLOSED | 2026-08-13 | | [025](./025-gatekeeper-install-instructions.md) | docs | Gatekeeper install instructions | CLOSED | 2026-08-10 | | [024](./024-homebrew-tap-distribution.md) | ci | Homebrew tap distribution | CLOSED | 2026-08-10 | | [023](./023-readme-logo-visual-alignment.md) | fix | README logo visual alignment | CLOSED | 2026-08-10 |