fix: prevent SQLite crash from concurrent database access in iOS (Sentry YND+YNE, 36K users)#9610
Merged
Merged
Conversation
mm-cloud-bot
added
kind/bug
Coverage Comparison Report |
Contributor
Author
|
Note: This PR should be rebased after #9616 (flaky ChecklistItem test fix) is merged. The cherry-picked commit |
pavelzeman
marked this pull request as ready for review
March 20, 2026 15:32
Contributor
📝 WalkthroughWalkthroughThis pull request refactors SQLite database query execution patterns across the iOS storage layer. Changes introduce a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift (1)
178-217: The test relies on timing-dependent behaviour.The 0.1-second delay before
COMMITcombined with the 5-secondbusyTimeoutshould generally work, but this pattern can be flaky under heavy CI load. Consider adding a small sleep before the reader starts, or using a synchronisation primitive (e.g.,DispatchSemaphore) to ensure the lock is held when the reader begins.That said, the current implementation is reasonable for demonstrating the
busyTimeoutretry mechanism and should pass reliably in most environments.💡 Optional: More deterministic synchronisation
// Hold an exclusive lock briefly, then release from another thread try writer.execute("BEGIN EXCLUSIVE TRANSACTION") try writer.execute("INSERT INTO data VALUES (2, 'world')") + let lockHeld = DispatchSemaphore(value: 0) + let readerStarted = DispatchSemaphore(value: 0) + // Release the lock after a short delay so the reader's busyTimeout can retry DispatchQueue.global().asyncAfter(deadline: .now() + 0.1) { + _ = readerStarted.wait(timeout: .now() + 1.0) try? writer.execute("COMMIT") + lockHeld.signal() } + readerStarted.signal() // Reader should eventually succeed thanks to busyTimeout retry let iterator = try reader.prepareRowIterator(table)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift` around lines 178 - 217, The test testBusyTimeoutHandlesConcurrentAccess is timing-dependent; replace the fragile DispatchQueue global asyncAfter delay with a synchronization primitive so the reader starts while the writer still holds the exclusive lock: acquire a DispatchSemaphore (or DispatchGroup) before beginning the reader, have the writer perform BEGIN EXCLUSIVE TRANSACTION and then signal the semaphore only after the lock is held, start the reader after waiting on the semaphore so it attempts to read while locked, and finally have the writer COMMIT to release the lock; update references to writer, reader, and the COMMIT sequence in the test to use this synchronization to make retries deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift`:
- Around line 178-217: The test testBusyTimeoutHandlesConcurrentAccess is
timing-dependent; replace the fragile DispatchQueue global asyncAfter delay with
a synchronization primitive so the reader starts while the writer still holds
the exclusive lock: acquire a DispatchSemaphore (or DispatchGroup) before
beginning the reader, have the writer perform BEGIN EXCLUSIVE TRANSACTION and
then signal the semaphore only after the lock is held, start the reader after
waiting on the semaphore so it attempts to read while locked, and finally have
the writer COMMIT to release the lock; update references to writer, reader, and
the COMMIT sequence in the test to use this synchronization to make retries
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 362c7468-dd78-4cf3-8327-401a3236cd47
📒 Files selected for processing (9)
app/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item.test.tsxios/Gekidou/Sources/Gekidou/Storage/Database+Channels.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Mentions.swiftios/Gekidou/Sources/Gekidou/Storage/Database+System.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Team.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Thread.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Users.swiftios/Gekidou/Sources/Gekidou/Storage/Database.swiftios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift
larkox
approved these changes
Mar 20, 2026
larkox
left a comment
larkox
left a comment
Contributor
There was a problem hiding this comment.
Looks like the changes from this PR got into #9611 (review) . Probably you want to remove them from that one.
Replace all unsafe db.prepare() iteration patterns with safe
prepareRowIterator + failableNext() in the Gekidou Storage layer.
SQLite.swift 0.15.4 uses `try!` in FailableIterator.next() (Statement.swift:218),
which means any SQLite error during row iteration crashes the app instantly.
This is triggered when the main app and NotificationService extension access
the shared database concurrently, causing SQLITE_BUSY ("database is locked").
Two-part fix:
1. Add busyTimeout (5s) to all SQLite connections via new openConnection()
helper, so transient locks are retried instead of failing immediately.
2. Replace db.prepare(query) iteration with prepareRowIterator() +
failableNext(), which returns throwable errors instead of crashing
via try!. Also replace db.prepare(string).scalar() with db.scalar().
Files changed:
- Database.swift: Add openConnection() helper, fix 4 methods
- Database+Users.swift: Fix queryUsers(byIds:), queryUsers(byUsernames:),
getUserLastPictureAt (YND/YNE crash sites)
- Database+Thread.swift: Fix handleThreads team ID iteration
- Database+Team.swift: Fix queryAllMyTeamIds
- Database+Channels.swift: Fix serverHasChannels scalar query
- Database+Mentions.swift: Fix getChannelMentions, getThreadMentions
- Database+System.swift: Fix getDeviceToken connection creation
Sentry: https://mattermost-mr.sentry.io/issues/7117724547/ (YND, 20K users)
Sentry: https://mattermost-mr.sentry.io/issues/7118237218/ (YNE, 15K users)
Co-authored-by: Claude <claude@anthropic.com>
- Replace Array(iterator).map with explicit failableNext() loops in Database.swift and Database+Users.swift. Array(iterator) still goes through SQLite.swift's non-throwing next() which uses try! internally, defeating the purpose of the safe iteration fix. - Remove unsafe sqlite3_close_v2() test that violated Connection ownership (double-free risk). Replace with proper lock contention test using DELETE journal mode + EXCLUSIVE lock. - Fix concurrent access test: WAL mode allows readers during writes without exercising busyTimeout. Switched to DELETE journal mode with EXCLUSIVE lock + async release to actually test the retry path. Co-authored-by: Claude <claude@anthropic.com>
pavelzeman
force-pushed
the
fix/sentry-ios-sqlite-crash-ynd-yne
branch
from
March 20, 2026 20:03
2b2f180 to
d4ba59f
Compare
Contributor
Author
Yes, you're right. Now that the other fix (flaky test) is merged, I'm rebasing here to clean that up and will merge this one. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift (1)
196-214: Potential test flakiness due to timing-dependent assertions.The test relies on the async commit occurring within the reader's busyTimeout window, but has no explicit synchronization to ensure the commit completes before assertions. While the 5.0s busyTimeout vs 0.1s delay provides a reasonable margin, this could be flaky under heavy CI load.
Consider using an
XCTestExpectationwith a semaphore or barrier to ensure the commit completes deterministically before asserting the read result.♻️ Suggested improvement for deterministic test
// Hold an exclusive lock briefly, then release from another thread try writer.execute("BEGIN EXCLUSIVE TRANSACTION") try writer.execute("INSERT INTO data VALUES (2, 'world')") + let commitExpectation = expectation(description: "Writer committed") + // Release the lock after a short delay so the reader's busyTimeout can retry DispatchQueue.global().asyncAfter(deadline: .now() + 0.1) { try? writer.execute("COMMIT") + commitExpectation.fulfill() } // Reader should eventually succeed thanks to busyTimeout retry let iterator = try reader.prepareRowIterator(table) var values = [String]() while let row = try iterator.failableNext() { values.append(try row.get(valueCol)) } + + // Ensure commit completed + wait(for: [commitExpectation], timeout: 5.0) // Reader should see data (either pre-commit or post-commit state) XCTAssertFalse(values.isEmpty, "Reader with busyTimeout should eventually read data after lock is released")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift` around lines 196 - 214, The test is timing-dependent: the async commit started with DispatchQueue.global().asyncAfter may not complete before the reader's assertion; replace the ad-hoc delay with explicit synchronization (e.g., an XCTestExpectation or DispatchSemaphore) so the commit from writer.execute("COMMIT") is signaled when done, then wait for that signal (with a short timeout) before iterating/ asserting; update the block that releases the lock (currently using DispatchQueue.global().asyncAfter) to signal the expectation/semaphore after writer.execute("COMMIT") and wait in the test prior to calling reader.prepareRowIterator / while let row = try iterator.failableNext() to make the assertion deterministic while still honoring reader busyTimeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift`:
- Around line 196-214: The test is timing-dependent: the async commit started
with DispatchQueue.global().asyncAfter may not complete before the reader's
assertion; replace the ad-hoc delay with explicit synchronization (e.g., an
XCTestExpectation or DispatchSemaphore) so the commit from
writer.execute("COMMIT") is signaled when done, then wait for that signal (with
a short timeout) before iterating/ asserting; update the block that releases the
lock (currently using DispatchQueue.global().asyncAfter) to signal the
expectation/semaphore after writer.execute("COMMIT") and wait in the test prior
to calling reader.prepareRowIterator / while let row = try
iterator.failableNext() to make the assertion deterministic while still honoring
reader busyTimeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7828bbf6-dc3e-431c-a033-585daa33d5fa
📒 Files selected for processing (8)
ios/Gekidou/Sources/Gekidou/Storage/Database+Channels.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Mentions.swiftios/Gekidou/Sources/Gekidou/Storage/Database+System.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Team.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Thread.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Users.swiftios/Gekidou/Sources/Gekidou/Storage/Database.swiftios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift
✅ Files skipped from review due to trivial changes (3)
- ios/Gekidou/Sources/Gekidou/Storage/Database+Channels.swift
- ios/Gekidou/Sources/Gekidou/Storage/Database+Mentions.swift
- ios/Gekidou/Sources/Gekidou/Storage/Database+System.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- ios/Gekidou/Sources/Gekidou/Storage/Database+Thread.swift
- ios/Gekidou/Sources/Gekidou/Storage/Database+Users.swift
- ios/Gekidou/Sources/Gekidou/Storage/Database.swift
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
4 participants
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.
Summary
Fix fatal crashes in the iOS Gekidou (native Swift) database layer caused by concurrent SQLite access between the main app and the NotificationService extension.
Impact: ~46,000 crash events affecting 36,000+ unique users across iOS.
Root Cause
SQLite.swift 0.15.4 uses
try!inFailableIterator.next()(Statement.swift:218). Since Swift'sIteratorProtocol.next()cannot throw, the library force-unwraps withtry!— meaning any SQLite error during row iteration crashes the app instantly.The trigger is SQLITE_BUSY ("database is locked", error code 5): the main app and NotificationService extension share a database via the app group container and access it concurrently. With no busy timeout configured, SQLite immediately returns SQLITE_BUSY instead of retrying, and the
try!makes it fatal.Two compounding problems:
busyTimeout— Gekidou never configuredConnection.busyTimeout. SQLite defaults to 0ms (no retry).try!in iteration — Even though Gekidou usestry?ondb.prepare(), that only catches errors during statement preparation. Row-by-row iteration callsstatement.next()→try! failableNext()→ crash.Crash Flow
Fix (Two-Part)
Part 1: busyTimeout (prevents the error)
openConnection()helper setsbusyTimeout = 5.0on every SQLite connectionConnection(path)calls now go through this helperPart 2: Safe iteration (makes errors recoverable)
db.prepare(query)iteration (which usestry!) withdb.prepareRowIterator(query)+failableNext()(which throws catchable errors)db.prepare(string).scalar()withdb.scalar(string)(avoids Statement iteration entirely)Affected Call Sites
Database.swiftgetOnlyServerUrlfor result in try db.prepare(query)Database.swiftgetAllActiveDatabasestry db.prepare(query).map { }Database.swiftgetAllActiveServerUrlstry db.prepare(query).map { }Database+Users.swiftgetUserLastPictureAt.prepare(query).mapDatabase+Users.swiftqueryUsers(byIds:)for user in users+user[idCol](alsotry!)Database+Users.swiftqueryUsers(byUsernames:)Database+Thread.swifthandleThreadstry? db.prepare(...)+.mapDatabase+Team.swiftqueryAllMyTeamIdstry? db.prepare(...)+.compactMapDatabase+Channels.swiftserverHasChannelsdb.prepare(string).scalar()Database+Mentions.swiftgetChannelMentionsdb.prepare(string).scalar()Database+Mentions.swiftgetThreadMentionsdb.prepare(string).scalar()Database+System.swiftgetDeviceTokenConnection(path)without busyTimeoutTests
7 new tests in
DatabaseSafeIterationTests.swift:testOpenConnectionSetsBusyTimeout— verifies busyTimeout > 0testSafeIterationReturnsCorrectResults— prepareRowIterator + failableNext workstestSafeIterationWithFilter— filtered queries work correctlytestArrayFromRowIterator— Array(RowIterator) bulk pattern workstestFailableNextThrowsInsteadOfCrashing— errors are catchable, not fataltestBusyTimeoutHandlesConcurrentAccess— concurrent read/write with WAL modetestScalarQueryDoesNotUseDangerousIteration— scalar queries bypass try! pathTicket Link
Sentry YND: https://mattermost-mr.sentry.io/issues/7117724547/ (26,135 events, 20,685 users)
Sentry YNE: https://mattermost-mr.sentry.io/issues/7118237218/ (20,450 events, 15,324 users)
Release Note