Repository navigation
chore(stack): restore #753–#757 onto main - #758
Merged
Merged
Conversation
* fix(database): set the busy timeout in seconds, not milliseconds SQLite.swift's `Connection.busyTimeout` is a `Double` of seconds that gets multiplied by 1000 on the way to `sqlite3_busy_timeout`, so `busyTimeout = 2000` asked both connections to block for 2000 seconds — 33 minutes — where the comment beside it said two. Nothing has hit that ceiling while one process owns the store. It stops being academic once the notification service extension opens the same file: a wait that long is indistinguishable from a hang, and the extension has about thirty seconds to live. * feat(database): open the store on demand and close it on request `Database` opened a reader and a writer in `init` and held both for the life of the process, with no way to give them back. `close()` supplies the other half: checkpoint the write-ahead log, drop both connections, and let the next `reader` or `writer` access reopen the store and reapply the pragmas. Dropping the references is what closes the store. SQLite.swift's `Connection` releases its handle from `deinit` and exposes no `close()` of its own, so a connection another caller is still holding closes when that caller returns rather than here. That is also why nothing pairs with `close()`: a call that lands at an awkward moment costs a reopen instead of leaving a dead object behind. `reader` and `writer` become throwing computed properties. All 121 uses in `Database+*.swift` and `Schema.swift` already sat inside a `try` expression and did not change; the eight in tests reading `writer.totalChanges` did not, and now do. `journal_mode` lives in the database header, but `cache_size`, `foreign_keys` and the busy timeout are per-connection and are gone after a close, so `pragmasAreReappliedOnReopen` and `busyTimeoutIsTwoSeconds` assert them through a cycle rather than trusting the open path to have run. * feat(app): close the database when the app enters the background `scenePhaseChanged(.background)` now checkpoints and closes the store under a background-task assertion. `.active` has no counterpart on purpose: the connections reopen on the first read, so an app that never comes back costs nothing and a close that lands at an awkward moment repairs itself. The assertion covers the checkpoint, which is file I/O proportional to the write-ahead log. Being suspended partway through it leaves the log on disk for the next launch to replay rather than damaging the store, so the assertion buys a faster next launch, not correctness.
) * feat(core): add StoreLocation for the App Group store paths The store's four file names are currently built inline in four `URL` extensions on `applicationSupportDirectory`, which the notification extensions cannot read. StoreLocation names the same files against an injectable directory so the move to `group.com.flipcash.shared` becomes a change of directory rather than a rewrite of every path. It resolves the container itself and reports, via `isShared`, when that lookup failed and it fell back to the legacy directory. FlipcashCore has no reporting channel, so the flag exists for the caller to act on. The container lookup is injected. On iOS it returns nil for an unentitled group, but on macOS — where this package's tests run — it constructs a path for any identifier, so the fallback branch is unreachable from a test that just passes a bogus group name. * fix(database): throw instead of trapping when the version file cannot be written `setUserVersion` is declared `throws` but used `try!`, so a failed write terminated the process instead of reaching the caller. The one caller, `SessionAuthenticator.initializeDatabase`, already propagates. It has not fired because the destination is inside the app's own container and is created moments earlier by `createApplicationSupportIfNeeded`. The next commit moves that destination into the App Group container, where the write depends on a container the app does not create. * feat(database): move the store into the App Group container The notification service extension prefetches five messages on every push and writes them to a JSON side-car, because the SQLite store sits in the app's private Application Support directory and the extension is a separate process with a separate container. Moving the store into `group.com.flipcash.shared` is what gives that prefetch somewhere the app will look. `Database` no longer builds its own paths. The three static file operations take a `StoreLocation.Files`, and `SessionAuthenticator` resolves the location once per login and passes it down, so a migration cannot read one directory while the store opens from another. `StoreMigration` handles an install that already has a store in the old location. It checkpoints first, so what moves is a single file rather than a database and a log that can end up split across directories. It moves the version file before the database, which is the ordering that survives being interrupted: a next launch that finds no database at the destination retries and treats the already-moved version file as done, where the reverse order leaves a store with no recorded version, which reads as 0 and triggers a full rebuild. A move that fails removes only what it created, so a destination store that already held data is never cleared. When the container does not resolve, `StoreLocation.resolved()` falls back to Application Support and reports it. The app keeps working with an extension that cannot see the store, which is a provisioning problem rather than a reason to refuse login. * test(database): cover the move into the App Group container Twelve cases over `StoreMigration.migrateIfNeeded`, against seeded stores in temporary directories rather than a real container, so the App Group entitlement is not a precondition for running them. The interesting ones are the interrupted cases. `interruptedMoveResumes` seeds the state a launch that died between the version file and the database leaves behind, and asserts the next launch finishes the job. `destinationStoreWins` and `destinationVersionFileIsAuthoritative` cover the store that is already in the container, where the legacy files are leftovers to sweep, not data to adopt. `walContentsAreFoldedInBeforeTheMove` is what justifies checkpointing first: it puts a row in the log and nowhere else, and reads it back at the destination. Two of them go through `Database` rather than a raw `Connection`, which is what covers the ordering in `Database.init`. A migrated store arrives as a lone `.sqlite` — the log was folded in and the `-shm` swept — and a read-only connection to a WAL-mode database cannot create the `-shm` it needs, so it fails with `unable to open database file`. Opening the writer first is what creates it.
…re package (#755) The notification service extension cannot reach `Database` while it lives in the app target. Xcode's synchronised file groups do not offer a way in: all three `PBXFileSystemSynchronizedBuildFileExceptionSet` entries in this project list `Info.plist` as an exclusion, which is the only thing the mechanism does. A file cannot join a second target that way, so sharing code with the extension means a package. `Flipcash/Core/Controllers/Database/` becomes `FlipcashStore`, a new target in `FlipcashCore` linked into both `Flipcash` and `NotificationService`. It is separate from `FlipcashCore` rather than part of it so that everything depending on the models does not also pull in SQLite. The SQLite.swift dependency is branch-pinned to match the app project's reference to the same fork, because SPM resolves one version of it for the whole graph. The move is mechanical: types the app target already used become `public`, and `import FlipcashStore` is added to the 28 app files and 38 test files that read through the database. Two things did not move cleanly: - `Updateable` stayed in the app target. It is a SwiftUI `@Observable` wrapper that re-queries on `.databaseDidChange`, not persistence. The app target defaults new types to `@MainActor` and a package target does not, so in the package its `init` could not send `self` into a `@MainActor` task. - `Database` is `open` rather than `public`, so the test bundle's `TempDatabase` can keep tying temp-file cleanup to the database's lifetime. Its members stay `public`: a subclass can add state but cannot override behaviour. No behaviour change. The database tests stay in the app test bundle so they keep running on the simulator — the read-only-WAL and same-inode failures this store hits are iOS-specific and do not reproduce on macOS.
…756) * feat(notifications): write prefetched messages into the shared store The extension already fetches five messages on every chat push. Until now they went only into `NotificationPreviewCache`, a JSON side-car the content extension reads and the app never opens, so the app refetched the same messages on launch. Now the extension also writes them through `Database.persistMessages`. The side-car stays: it is the content extension's only source, and nothing about this change replaces it. `ExtensionStore.perform` wraps the write in the cycle the extension has to use — open, write, checkpoint, close — inside `performExpiringActivity`, so the process is not suspended holding the App Group store's lock. It refuses to run in two cases: - No store file. `Database.init` would create one, and an empty store at the shared path reads to `StoreMigration` as a finished migration, which would make the app delete the legacy store. - A recorded schema version that is not this build's. Rebuilding a store belongs to the app, not to a 30-second extension. The schema version moves from the app's `SQLiteVersion` Info.plist key to `Database.schemaVersion`. An extension cannot read the app's Info.plist, and both targets link `FlipcashStore`, so they cannot disagree about the number. The write runs before `deliver()`. It costs the banner a few milliseconds plus roughly 100 ms of checkpoint, but after delivery nothing keeps the process alive. Two deliberate limits: the catch-up cursor is not advanced, because the extension fetched a bounded preview rather than a delta and advancing it would make the next sync skip the gap; and no conversation row is synthesized, so a conversation the app has never seen stays absent from the feed until sync introduces it. * chore(deps): bump flipcash2-client-protocol to 0.5.0 0.5.0 adds `push.v1.ChatMetadata.message`, the chat message a push is notifying about, carried inline. It also marks `ChatMetadata.sending_user_id` deprecated in favour of reading the sender off that message. The bump on its own changes no behaviour. `Flipcash_Push_V1_Payload` moves to heap storage and so becomes `@unchecked Sendable` rather than `Sendable`, which is generated-code bookkeeping, not a contract change. * feat(notifications): persist the message embedded in the push Every store row the extension wrote came from a network fetch, so a push that arrived with no usable connection wrote nothing — the case where a warm store matters most, because the app is about to cold-start too. `ChatMetadata.message` carries the message the push is notifying about, so the newest message needs no transport to become a store row. `NotificationPayload.chatMessage` decodes it through the existing `ConversationMessage.init?(_:)`, which means unrepresentable content is dropped the same way it is everywhere else rather than becoming an empty row. It is merged with the fetched transcript rather than written separately: both sources carry the same `eventSequence` for the message they share, and one store cycle per push keeps the cost at one checkpoint. The fetched copy wins a tie, being the one the server rendered most recently. The two paths that used to write nothing — an empty fetch, and a transport failure — now write the embedded message if the push carried one. `persist` returns early on an empty array so neither path opens the store to write no rows. * test(database): cover the migration against the real containers Every existing migration test builds its own `StoreLocation` from two temporary directories. That leaves the first step of a real upgrade untested: resolving the App Group container. With the entitlement inactive at runtime, `resolved` falls back to Application Support, both sides name the same file, the migration reports `.notNeeded`, and the extension never sees the store — a silent failure that an injected location cannot produce. This suite uses `StoreLocation.resolved()` and the real directories: it seeds a WAL-mode store and a version file where a shipped build leaves them, migrates, then reads the rows back through `Database` and again through `ExtensionStore`, which resolves the container itself. Files are named from a random owner key, and store paths are owner-scoped, so nothing here can name a real account's store. 16 tests pass on an iPhone 16 Pro, including the three new ones.
`ChatUpdate.new_messages` is marked `[deprecated = true]` in the contract, superseded by the sequenced, gap-detectable `events` batch. The decode path kept it as an additive overlay so a message arriving only there was never dropped; the backend now sends `events` and leaves `new_messages` empty, so the overlay only costs a second pass over an empty batch and an enum case every switch has to carry. `.chatEvents` already covers what the `.newMessages` arm did. The store's last-activity bump is the same, restricted to `.sent` mutations so an edit or delete can't move a feed row on its original low id; the controller's persist arm matches, plus the cursor write that makes messages and the advanced frontier land atomically. Ported the ordering and messages-plus-typing decode tests to `events`, and the receipt-wiring and backlog tests to `.chatEvents` via a new `ConversationStreamEvent.sent` helper. Deleted the tests that only covered the overlay itself, including the store's last-activity test — `chatEventsBumpsActivity` already asserts it.
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
1 participant
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.
#753 through #757 were a stack, each based on the one below it. They were merged bottom-up, but the repo has
delete_branch_on_mergeoff, so merging a parent left its branch in place and GitHub never retargeted the children ontomain. Each child then squashed into its original base branch.maingot #751 and nothing after it.This branch is
mainwith those five squash commits cherry-picked back in order:8b700b422b924e5d788051bd3e63bffefe6fef50All five applied without conflict. The branch differs from
feat/nse-store-writesin 27 files, and all 27 are commits that landed onmainafter the stack forked at22dabe4a— the stack never had them.Merge this with rebase, not squash: the five commits already carry their
(#NNN)suffixes and should land individually, matching the rest of the history. A squash would collapse five PRs into one commit.Once this lands, the five stale branches (
feat/push-preload-groundwork,feat/database-connection-lifecycle,feat/database-app-group-store,refactor/shared-store-package,feat/nse-store-writes) can be deleted.