fix(ios): release database file locks before suspension instead of closing every session - #2082
Merged
Merged
Conversation
…osing every session
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a
RUNNINGBOARD 0xdead10ccSIGKILL in TablePro Mobile, reproduced in three TestFlight crash reports (1.0 build 19, iOS 27.0, iPhone 14,5).Root cause
Xcode names the crash point
SwiftUI: specialized UnsafeMutablePointer<>.withMemoryRebound, but that is just the main thread parked inUIApplicationMain's runloop. The real signal is the termination reason: the app was killed for holding a file lock across suspension. Every report has a thread doing exactly that:Two defects compounded.
Wrong policy.
scenePhase == .backgroundrandisconnectAll(), closing every session. For the six network drivers that is pure cost: iOS drops sockets at suspension anyway, the resume path already pings and reconnects, and closing a live session deliberately destroys server-side state (temp tables,search_path,SETvariables, open transactions). For DuckDB it meant a multi-secondduckdb_close()holding the database file lock. Unlike SQLite, which the picker copies into the app container, DuckDB keeps a security-scoped bookmark and opens the user's file in place, so that file can live in iCloud Drive or another Files provider.Wrong mechanics. No background task assertion existed anywhere in the repo. The work started at
.background, which is the point Apple's documentation explicitly warns against because the assertion is granted asynchronously, and it ran a blocking C call on the Swift cooperative thread pool.The background disconnect landed in
dbaeea3d2, when every iOS driver was a network socket where close is instant. DuckDB arrived later inc01361f45and invalidated that premise.The change
Backgrounding is no longer "close everything". It is "release the OS resources that block suspension", which in practice is DuckDB's file lock and nothing else. This also matches every comparable client: no database client on any platform drops connections on backgrounding, and Postico shipped this exact behaviour in 1.0 and removed it by 1.2.
DatabaseDrivergainsholdsSuspensionBlockingResource, defaultedfalsein the protocol extension. OnlyDuckDBDriveroverrides it, and only when file-backed.ConnectionManagergainshasSuspensionBlockingResourcesandreleaseSuspensionBlockingResources(), which releases just those sessions and does so concurrently.disconnectAll()is removed; it had one caller.ConnectionManager.disconnect(_:)tracks in-flight teardowns, soconnect()for the same id awaits one instead of racing it. Previously a resumed app could start a secondduckdb_open_exton the same file while the first close was still running.DuckDBActorruns on its ownDispatchSerialQueueexecutor (SE-0392), takingduckdb_closeand every blockingduckdb_queryoff the cooperative pool.Task.detachedwould not have done this.BackgroundReleaseCoordinatorowns theUIApplicationassertion: taken at.inactive, consumed at.background, released on.active. Splitting prepare from release is the point, since the grant is asynchronous and.inactiveis where Apple's Preparing your UI to run in the background puts "close any open files". Doing no work at.inactiveavoids tearing down connections on every notification banner.Behaviour change
Remote connections now survive an app switch instead of being dropped and rebuilt. Returning to the app no longer reconnects and reloads everything.
Tests
ConnectionManagerTests: release touches only blocking sessions, releases run concurrently (proved by a rendezvous, not by timing), an in-flight teardown still counts as a blocking resource, andconnect()waits for an in-flight teardown.BackgroundReleaseCoordinatorTests: no assertion when nothing needs releasing, assertion taken once and ended on cancel, release ends it, release takes one when preparation was skipped, expiry does not double-end, and an in-flight release keeps the assertion alive across a foreground bounce.DuckDBDriverSuspensionTests: in-memory opts out, file-backed opts in.Each load-bearing test was checked against the pre-fix code and confirmed to fail there.
TEST SUCCEEDED, 0 failures--stricton changed app sourcesVerifying by hand
0xdead10ccterminations do not occur in the Simulator or on device with the debugger attached, so this needs a detached device build: open a file-backed DuckDB connection from Files, background the app, and confirm no crash report appears.