fix(core): probeWritable(at:) always cleans up the probe file, even on partial failure
Two leak paths: (a) AtomicFile.write renames the temp file onto the probe path and THEN fsyncs the containing directory — if that last fsync throws, the probe file already exists on disk but the old code returned false without ever attempting removal; (b) if the explicit removeItem call itself threw, there was no retry, so a transient File Provider removal failure left the file behind permanently. Fix: an unconditional `defer` now checks fileExists and retries removeItem regardless of which branch returned early. The function only reports true when the explicit write, fsync (inside AtomicFile.write), AND removal all succeeded AND the file is confirmed gone afterward. New test: a FileManager subclass whose removeItem(at:) throws on its first call (AtomicFile.write itself never touches this injected FileManager — it uses raw Darwin/POSIX calls, not FileManager, so this only intercepts the explicit removal + the defer's retry) asserts the function returns false AND no probe file remains — verified this actually needs the defer by temporarily removing it and confirming the same test then fails with a leftover ".redline-probe-<uuid>" file (see this branch's history for the discarded revert). The directory-fsync failure path has no injectable seam (raw fsync(2) on an already-open fd, not parameterized by any FileManager or other substitutable dependency, and not reproducible via chmod or other standard test techniques) — documented in the test rather than simulated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
This commit is contained in:
@@ -189,16 +189,33 @@ public enum OneDriveLocator {
|
||||
fileManager: FileManager = .default
|
||||
) -> Bool {
|
||||
let probeURL = folder.appendingPathComponent(".redline-probe-\(UUID().uuidString)")
|
||||
// Belt-and-suspenders cleanup, unconditional: AtomicFile.write renames the temp
|
||||
// file onto probeURL and THEN fsyncs the containing directory — if that last
|
||||
// fsync throws, the probe file already exists on disk but the catch below
|
||||
// returns false before ever reaching the explicit removeItem call. And if the
|
||||
// explicit removeItem itself throws, this is the only retry it gets. Either
|
||||
// way, never leave the probe file behind just because we're about to return.
|
||||
defer {
|
||||
if fileManager.fileExists(atPath: probeURL.path) {
|
||||
try? fileManager.removeItem(at: probeURL)
|
||||
}
|
||||
}
|
||||
|
||||
do {
|
||||
try AtomicFile.write(Data(), to: probeURL)
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
|
||||
do {
|
||||
try fileManager.removeItem(at: probeURL)
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
return true
|
||||
|
||||
// Only true when the explicit removal above actually succeeded AND the file is
|
||||
// confirmed gone — never trust a removeItem call that returned without throwing
|
||||
// as proof of anything on a File Provider domain.
|
||||
return !fileManager.fileExists(atPath: probeURL.path)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user