fix: MAJOR — send() race between concurrent transport switch and an in-flight send
Two issues: (1) directoryExists(at:) only checked existence + isDirectory, so an existing-but-unwritable OneDrive folder skipped the oneDriveFolderUnavailable branch and surfaced as a generic pdfCompositionFailed message instead — now uses OneDriveLocator.isWritableDirectory(at:). (2) send() read `self.outboxURL` again inside composePDFForSend() after at least one await had already run, so a concurrent chooseTransport() call (SettingsView.swift) could flip transport/outboxURL/recordUncommented mid-send, landing the PDF under one transport's folder while the archive/status branch ran the other's. Fix: send() now snapshots BOTH transport and the destination folder into local `let`s once, before any await, and passes the folder explicitly into the renamed composePDFForSend(outbox:) — which no longer reads self.outboxURL at all. The archive/status switch already used the frozen `transport` local. (chooseTransport/chooseOneDriveFolder additionally refuse outright while isSending is true — see the SettingsView.swift commit — so in practice this race can no longer even be triggered, but the snapshot is the actual structural fix regardless.) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
This commit is contained in:
@@ -16,17 +16,28 @@ extension AppModel: SendCapable {
|
|||||||
guard !session.isEmpty, !isSending else { return }
|
guard !session.isEmpty, !isSending else { return }
|
||||||
setSending(true)
|
setSending(true)
|
||||||
|
|
||||||
|
// Snapshot BOTH the transport AND the destination folder into local `let`s
|
||||||
|
// ONCE, before any `await` in this function. chooseTransport/chooseOneDriveFolder
|
||||||
|
// now refuse (status "Finish the current send first.") while isSending is true,
|
||||||
|
// but this snapshot is the actual fix for the race: even without that guard,
|
||||||
|
// everything below operates on these frozen values — composePDFForSend(outbox:)
|
||||||
|
// takes the folder as a parameter and never re-reads `self.outboxURL` after a
|
||||||
|
// suspension point, so a concurrent transport switch mid-send can no longer land
|
||||||
|
// the PDF under one transport's folder while the archive/status branch (which
|
||||||
|
// switches on the same frozen `transport` local) runs the other's.
|
||||||
let transport = TransportSettings.transport()
|
let transport = TransportSettings.transport()
|
||||||
|
let destinationFolder: URL
|
||||||
|
|
||||||
// OneDrive mode: verify the real destination exists RIGHT NOW, before composing
|
// OneDrive mode: verify the real destination exists AND is writable RIGHT NOW,
|
||||||
// anything. `outboxURL` is kept in sync with the resolved OneDrive folder by
|
// before composing anything. `outboxURL` is kept in sync with the resolved
|
||||||
// bootstrap/chooseTransport/chooseOneDriveFolder, but this is re-resolved fresh
|
// OneDrive folder by bootstrap/chooseTransport/chooseOneDriveFolder, but this is
|
||||||
// here (never trusted stale) so a folder that vanished since then (OneDrive
|
// re-resolved fresh here (never trusted stale) so a folder that vanished or lost
|
||||||
// signed out, external volume unmounted, folder deleted) is caught instead of
|
// its permissions since then (OneDrive signed out, external volume unmounted,
|
||||||
// silently writing into whatever `outboxURL` happens to hold.
|
// folder deleted, chmod'd unwritable) is caught instead of silently attempted
|
||||||
|
// and surfacing as a generic PDF-composition failure.
|
||||||
if transport == .oneDrive {
|
if transport == .oneDrive {
|
||||||
guard let folder = OneDriveLocator.resolveOneDriveFolder(),
|
guard let folder = OneDriveLocator.resolveOneDriveFolder(),
|
||||||
Self.directoryExists(at: folder)
|
OneDriveLocator.isWritableDirectory(at: folder)
|
||||||
else {
|
else {
|
||||||
let path = OneDriveLocator.resolveOneDriveFolder()?.path
|
let path = OneDriveLocator.resolveOneDriveFolder()?.path
|
||||||
?? TransportSettings.storedOneDriveFolderPath()
|
?? TransportSettings.storedOneDriveFolderPath()
|
||||||
@@ -36,16 +47,19 @@ extension AppModel: SendCapable {
|
|||||||
setSending(false)
|
setSending(false)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
destinationFolder = folder
|
||||||
setResolvedOneDriveFolder(folder)
|
setResolvedOneDriveFolder(folder)
|
||||||
if outboxURL != folder || watchFolderURL != folder {
|
if outboxURL != folder || watchFolderURL != folder {
|
||||||
setFolderURLs(outbox: folder, watch: folder)
|
setFolderURLs(outbox: folder, watch: folder)
|
||||||
try? await watcher.updateWatchFolder(folder)
|
try? await watcher.updateWatchFolder(folder)
|
||||||
}
|
}
|
||||||
|
} else {
|
||||||
|
destinationFolder = outboxURL
|
||||||
}
|
}
|
||||||
|
|
||||||
let pending: ComposedSend
|
let pending: ComposedSend
|
||||||
do {
|
do {
|
||||||
pending = try await composePDFForSend()
|
pending = try await composePDFForSend(outbox: destinationFolder)
|
||||||
} catch {
|
} catch {
|
||||||
// Never unlink the published PDF, and never unlink the temp file either:
|
// Never unlink the published PDF, and never unlink the temp file either:
|
||||||
// a rename failure would leave the complete document at the temp name.
|
// a rename failure would leave the complete document at the temp name.
|
||||||
@@ -93,19 +107,14 @@ extension AppModel: SendCapable {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private static func directoryExists(at url: URL) -> Bool {
|
/// Writes the PDF to `outboxDir` and records its path. Does not archive the session
|
||||||
var isDirectory: ObjCBool = false
|
|
||||||
let exists = FileManager.default.fileExists(atPath: url.path, isDirectory: &isDirectory)
|
|
||||||
return exists && isDirectory.boolValue
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Writes the PDF to the outbox and records its path. Does not archive the session
|
|
||||||
/// and does not present AirDrop — that happens only after the share completes.
|
/// and does not present AirDrop — that happens only after the share completes.
|
||||||
func composePDFForSend() async throws -> ComposedSend {
|
/// `outboxDir` is passed in (a value `send(anchor:)` snapshotted before any await)
|
||||||
|
/// rather than read from `self.outboxURL` here, so a concurrent transport switch
|
||||||
|
/// mid-send can never redirect an in-flight compose to a different folder.
|
||||||
|
func composePDFForSend(outbox outboxDir: URL) async throws -> ComposedSend {
|
||||||
let workingSession = session
|
let workingSession = session
|
||||||
let composer = self.composer
|
let composer = self.composer
|
||||||
// Live outbox (FolderSettings), not `paths.outbox` — Settings changes take effect.
|
|
||||||
let outboxDir = outboxURL
|
|
||||||
let sourceDir = paths.sessionDirectory(workingSession.id)
|
let sourceDir = paths.sessionDirectory(workingSession.id)
|
||||||
let fileName = PDFComposer.fileName(for: workingSession)
|
let fileName = PDFComposer.fileName(for: workingSession)
|
||||||
let finalURL = outboxDir.appendingPathComponent(fileName)
|
let finalURL = outboxDir.appendingPathComponent(fileName)
|
||||||
|
|||||||
Reference in New Issue
Block a user