Fix sync issues: auth token expiry, logout/login race, collection creation errors - #15
Open
sara-gnucoop wants to merge 51 commits into
Open
Fix sync issues: auth token expiry, logout/login race, collection creation errors#15sara-gnucoop wants to merge 51 commits into
sara-gnucoop wants to merge 51 commits into
Conversation
…istic
Idle sessions froze after a few minutes of inactivity: empty lists, endless
spinners and stuck sync indicators. Three defects combined.
- The guard and the interceptor tested a truthy object ({token, evt}) instead of
its `token` property, so an expired token passed every check.
- checkToken() computed the expiry once into a const, so the interceptor reused
the value captured at bootstrap for the whole session.
- intercept() answered a 401 with obsOf(null) and delegated the retry to an
EventEmitter whose result was discarded: the caller got an empty response and
the replayed request went nowhere. This is what emptied the lists.
Expiry detection:
- add decodeJwt/tokenExpiresAt/isTokenExpired to auth-utils, with a 10s skew.
Decoding never throws, so a malformed token no longer breaks bootstrap.
- recognise Hasura's `invalid-jwt` in a 200 response, both in the interceptor
and on the replication error, matching extensions.code as well as the
message: JWSInvalidSignature never matched the configured JWTExpired text.
Store order and concurrent refresh:
- persist the tokens before notifying, and emit `authenticated` before
`authToken`, since the sync setup samples the former when the latter emits.
- make refreshToken() single-flight, so interceptor, guard and timer share one
call instead of racing over a refresh token that rotates server side.
- reset the interceptor retry counter on every successful refresh, so a later
idle cycle does not exhaust it and log the user out.
- replay the failed request with the refreshed token, not the expired one.
Pre-emptive renewal and offline behaviour:
- schedule a refresh at 75% of the token lifetime, re-armed on every new token.
- offline, the guard neither blocks navigation nor redirects to the login.
- drop the debounceTime delays that sat before the refresh request.
Bounded retries:
- add boundedRetry() and apply it to the five unbounded retryWhen loops in the
data context service, the managers and createCollection.
Replication cost of a now periodic refresh:
- hand the new token to the running replications with setHeaders() instead of
tearing them down and recreating them.
- keep the pull checkpoint when a response is empty instead of rewinding it to
the epoch, which made the following pull re-download the whole collection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Creating the rxdb database while the logout teardown was still running threw DB8 on the still reserved name, and the cached error on the shared _db observable made every later query fail for the rest of the session. The creation now waits for the pending teardown, the teardown removes the database even with no collection registered, and the replications are cancelled before the storage goes away.
A collection that failed to register was never synced again for the rest of the session, and failed in complete silence: the log sat behind isDevMode(), and the exhaustion of the bounded retries was swallowed by a bare catchError(() => obsOf(false)). In production nothing said the collection was missing - the only symptom was a sync that "sometimes gets stuck", typically after a release that changed a schema without bumping its version, which makes rxdb throw DB6 for every user who does not log out.
…stop Cap replication cancellation at 5s during teardown so the local database is always removed on logout. Previously, a pending write could leave replication cancellation hung indefinitely, quietly skipping database removal and causing schema conflicts (rxdb DB6) on subsequent logins Timeouts are now logged and reported to the error handler.
triggering build on vercel
Offline the refresh returns without storing a new token, so nothing re-armed the pre-emptive timer that had just been consumed: the session ran to expiry with no timer at all and the refresh was left to the reactive paths only - a frequent state on a backgrounded PWA that loses connectivity. The offline branch re-arms the timer with a longer floor, so the retry does not turn into a poll for as long as the connection is missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The auth service reports the same negative result for a revoked refresh token and for any transient failure - a 5xx from the auth server, a timeout, or a request failing while `navigator.onLine` is still true, which is common on mobile. `runSync` logged out on the first one, and the logout destroys the local database along with the data not yet pushed: a network blip cost the user every unsynced write. A failed refresh now only skips the sync cycle. The session is torn down after three consecutive failures, with the budget given back by any refresh that goes through. Skipped cycles are reported as warnings, so that a sync doing nothing is visible instead of silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`runSync()` with no collection name only refreshed the auth token: the cycle came for free from the sync setup, which tore down and recreated every replication whenever the token changed. Handing the renewed token to the running replication instead - needed to stop the full re-pulls and the mass push - left the full sync doing nothing in live mode, where the websocket client is always renewed and the per collection run waits for the subscription to emit. The full sync now triggers the cycle for each active sync explicitly, through the per collection branch: the replication state and its checkpoint are reused, only a run is asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Written against the deployment constraint that matters: connectivity absent for days and offline collected data that must never be lost. Maps every path that can destroy the local database, compares the branch with what runs on dev today, and lists the residual risks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… refresh Both paths that reached `_logoutEvt` could be triggered by a refresh that proves nothing about the session being dead, and the logout destroys the local database along with the data collected offline and not pushed yet. On reconnection the handler logged out on the first negative result, with no budget at all. The `online` event fires on link-up, not on working connectivity, so the first refresh after a long offline stretch is a prime candidate for a transient failure - and the refresh reports the same `false` for a 5xx, a timeout and a revoked refresh token. The logout is gone from that path; `authenticated` still goes false, and the retry is left to the guard on the next navigation or to the next sync cycle. `_handleAuthFailure` spent an attempt per failing request rather than per refresh. With `retryAttemptsMax: 1` in every environment, two requests failing inside one refresh round trip - a few parallel uploads with an expired token - were enough to exhaust the budget and log out. The refresh is single-flight, so a request that joins one already in flight now spends nothing: `AuthService.isRefreshing` exposes that state. Both regressions are covered by tests verified to fail against the previous code. SYNC.md is updated accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…start The reconnection handler filtered the online status before skipping the first emission, so what it dropped was the first *online* status rather than the status the interceptor was built on. A session started offline - the normal outcome for a tablet left in background for days, whose PWA is discarded and relaunched without connectivity - had its reconnection swallowed: the expired token was never refreshed and no replication started, so the data collected offline stayed on the device until the user happened to navigate or press Sync. On a device left on one screen that may never happen, and a connectivity window missed that way is rare where these deployments run. Ordering the operators the other way round is correct for every sequence: started online, started offline, repeated transitions, no transition, and an interceptor built late on the replayed status. Safe only now that a failed refresh on this path no longer logs out, which would have wiped the local database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three taps on the sync button shared one refresh: its single failure was counted three times and reached the logout threshold on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This backend does not treat refresh tokens as single-use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An automatic logout destroyed the local database, unpushed data included, and nothing told "the user asked to log out" apart from "the token could not be renewed". AuthService.endSession() now ends a session locally - tokens dropped, no http call, works offline - and the two paths that give up by themselves use it, navigating unconditionally instead of only when a logout request succeeded. Destroying the data is left to the two deliberate acts: an explicit logout, and a different user logging in, which _removeDataOfPreviousUser detects through the owner recorded next to the database. The collection registrations had to survive a session end for this to be worth anything: the takeUntil in createCollection moved from the outer stream to the attempt in flight, so the next login re-registers instead of finding the collection gone for the rest of the page session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeping the data when a session ends only helps if the person at the device knows it is there: locked out, the natural move is to try another account, and that is now the one action that wipes it. The owner recorded next to the database is exposed by localDataOwners(), a plain function so the login page needs no data service injected, and Login shows a persistent notice naming the account when it matches the user info the session end left behind. Not tied to the expired or sync_error routes: the record only outlives a session that ended without a logout, so its presence is the signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A refresh that failed reported `authenticated: false` from three places: the auth service's catchError, the interceptor's reconnection handler and `_initAuthentication`, whose subscription outlives the service and re-runs on every network transition. Any of them dismantled the session - the permission context reset, so the permissions retried and gave up on an empty list, the replications stopped, the menu and the user name went - over one 401 that the next attempt may well recover from. Coming back online after days offline hit the third one every time, since the access token is always expired by then. None of them reports it any more. The session ends when it is over, which is endSession()'s job, and an expired access token with a refresh token in hand is a session to renew: the replications stay up and their next cycle asks for the refresh that resumes everything. What the user gets instead is the sync badge: a refresh that fails adds an `authentication` entry to problemSyncing, from the replications as much as from a sync, and the first one that succeeds removes it. It was fed only by an exhausted push retry and by a collection that failed to register, while a failed refresh went to Sentry - which nobody in the field reads. Found by running the app, not by the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The notice on the login page could never name anybody. It read the user info through the auth service, but the core LoginComponent constructor calls resetAuth() - which clears the tokens, the user info and the auth config - and it runs first, from super(). So the notice always fell back to its generic wording, telling the user to log in with the right account without saying which one. The account name now travels with the owner record the data service keeps next to the database: it describes the data on the device, so it carries what is needed to talk about it, and it survives everything but the removal of that data. A record written by the previous version, a bare user id, still reads as an owner with no label, and the next login rewrites it in place. Found by running the app, not by the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A session the app gives up on no longer tears the database down, and it was leaving two things behind that belong to the session, not to the data. `dbToken` kept the token of the database being abandoned. The app registers its collections with a `take(1)` - see SyncManager.initializeMainCollections - so asked right after the next login, while the new database is still being created, the registration found that stale token, registered against the database being left and reported itself done. Nothing was then registered on the database in use and the first query threw `Cannot read properties of undefined`. The registered collections kept the `RxCollection` handles of that same database, and a re-registration is dropped as a duplicate by name, so the next session set its replications up on collections of a closed database: they never reached in-sync and the sync spinner never stopped. Both are cleared when the session ends, as they already were on a logout. The owner record moves the other way, from the session reset to the teardown: it describes the data, so it goes when the data goes, not when a session that keeps it ends. Also, five of the six `Invalid collection` guards in the query methods built their error observable and dropped it instead of returning it, which is why a missing collection surfaced as a TypeError rather than the error the docstrings promise. Found by running the app, not by the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With a token that will not renew, the app said two opposite things at once: the badge reported a problem while the spinner kept turning, and underneath rxdb retried the push every 5s for as long as the app stayed open, each failure asking for a refresh that could not succeed. The spinner is off when there is nothing to wait for - blocked on renewing the token, or with no replication active at all, where combineLatest of an empty array completes without emitting and left whoever rendered it showing the last value it had ever seen, true. The replications stop after three consecutive failed renewals. Nothing can replicate without a token, so the retry loop was pure cost on a device in the field. The session and the data are untouched: a sync request, or a renewal that goes through, brings them back. And the badge follows whether the token is usable, not whether the refresh call reported success: offline it reports success without even trying, so a brief offline moment switched the badge off while nothing had been renewed - dark badge, turning spinner, sync dead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The badge said the session had expired, and the only route to a login page a user knows is the logout button - the one action that destroys the data collected on this device. The message was pointing at the damage. The sync icon now says what is wrong and is the way out: tapping it while the session needs renewing ends the session and goes to the login page, instead of starting a cycle that cannot succeed. Ending it first is what makes that page reachable at all, since LoginGuard closes it while the app still reports itself authenticated, and after R5 ending it keeps the data. From there the login page names the account whose data is on the device, and the backlog goes out on the next login. No step asks the user to guess. The three AuthService substitutes get endSession(): the path is unreachable in backendless mode, where the sync icon is hidden, but a missing method there is a crash waiting for that to change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Organised around the investigation, it had grown into a list of fixed risks. What it has to explain is the mechanism: what the pieces are, what happens online and offline, what a token that will not renew does, and what - deliberately - destroys local data. Two diagrams for the normal operation, one for the destructive paths. The differences from dev stay, as a table. The manual test script stays too: it is what found the defects the suites did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The badge of a collection given up on was cleared every time the token renewal set it up again, and a renewal that went through did not clear it unless the sync itself had asked for it.
Both ways out of a session were taken without a question: the logout icon always destroyed the local database, and the sync icon with the error badge went straight to the login page. On a device that collects data offline for days the first is the one action that can lose it. Both now ask, through SessionDialog. The logout offers to delete the data or to only end the session and keep it. The sync icon tries a refresh first - a tap means "the connection is back", and nothing else asks for a token once the replications are stopped - and only then asks whether to go to the login page. A collection the server refuses comes before everything else: its own message, one button, because a logout there would destroy the very data still to be exported. The sync also stops ending the session by itself after three failed pre-sync refreshes. Sync cycles are started by the replication error handlers too, so three background retries with a dead token sent the user to a login page with no gesture of theirs and no question asked. What is left of that count is the severity of the Sentry report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard redirected to the login page when a refresh failed, and that redirect could not be followed: a failed refresh deliberately leaves the session alone, so LoginGuard found the app still authenticated and returned false, which cancels the navigation outright. Every guarded navigation was silently dropped and the user stayed locked on whatever page they were on. It now grants access as long as a session is reported, exactly as it already did offline; the refresh is still attempted on every activation, which is also how the session recovers once the auth server answers again. With no session at all the redirect stays, and there LoginGuard agrees there is nothing to protect. The dev logs on the pre-emptive refresh come from the same investigation: nothing said whether a timer was armed, and the difference decides whether the app comes back on its own or waits for a navigation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What a dev needs from this document is how the sync behaves in each situation, so the cases lead: normal operation, a token that will not renew, data the server refuses, and what destroys local data. The differences from dev stay, as a table, and it now opens with the rows that describe behaviour rather than internal budgets. Out: the test coverage section and every spec count, and the internal names that do not help understand a behaviour. In: the case of a push the server refuses, which was buried among the known limits, and the limit found while testing this branch by hand - when only the server was unreachable, recovery waits for a tap, a navigation or a reload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rxdb cancels a `live: false` replication after its first cycle, so the tap renewed the token and asked a cancelled state to resync: nothing left. It rebuilds the replication now, and a renewal alone no longer runs a cycle - with live off the tap is the only trigger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plus two passages that packed too much into one sentence: the reconnection handler that was dead code on dev, and what a token renewal costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md points every session at SYNC.md before it touches sync or auth, and carries the repo conventions. TEST-SYNC.md is the same cases as a checklist for the PMs, DevTools in hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The login page greets `login/expired` with an authentication error, true when the interceptor gives up, wrong when the user has just chosen to end the session from the dialog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It goes grey while a sync runs or the app is offline, and was clickable all the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
700px instead of the 80vw a dialog defaults to, and the buttons all of one width: Material's margin between adjacent buttons misaligned them once they were stacked in a column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What a session end leaves behind is the database instance, not the data. A constraint violation is usually a push order between collections, which the retries fix; the massive import is the case that stops a collection. Plus the retry rounds, which read as if nothing tried again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A login opens a new RxDatabase instance even when the previous session only ended and its data was kept, and the collections are re-added to that instance asynchronously. Every read and write indexed db.collections directly, so one landing in that window was told the collection did not exist when it was only not there yet: for a write, data the user believed saved and lost without a word. The console shows seven such requests for user_data at every single login, so this was systematic, not an edge case of ending a session. The wait is bounded by the same budget createCollection gives its own retries, and a collection that never arrives is now reported instead of being indistinguishable from a wrong name. The hand-rolled retryWhen on get and insert existed for this and would have multiplied the wait by ten, so they go. destroyCollection keeps failing fast: a teardown gains nothing from waiting for what it is about to remove. isSyncing comes along because it was found broken while looking into this: it was a cold observable with seven subscribers, each running its own copy of the replicationCycleComplete side effect, and a rejected awaitInSync - routine with live false, where every cycle ends cancelled - errored the whole chain and froze the sync spinner for the rest of the page session.
…eded A live:false cycle pulls and then pushes, so what it pushed needs one more pull to come back as the backend resolved it. That second pass was asked for by the main nav, reacting to the "a cycle finished" event: so any cycle dragged a full sync of every collection behind it, the one every login runs included. A login therefore cost sixteen cycles and then sixteen rebuilt replications that pushed nothing at all - measured, not estimated: every collection reported zero documents sent, on both passes. The pass now belongs to the sync that was asked for, and to the collection that actually pushed: state.sent$ is silent when the documents came from the pull, and fires only for the one the user changed. A login is one pass with no rebuild; a sync after editing one case is sixteen cycles plus a single rebuild of that case, where it used to be thirty-two. The marker cannot live on the ActiveSync entry, because asking for the second pass goes through _rebuildCollectionSync, which replaces it - the two passes would then keep asking for each other. sentSub is unsubscribed optionally: the teardown runs before the database is removed, and must not be aborted by a bookkeeping subscription.
…ment update With live false, a collection reported its first cycle complete only when something called update() on it. Nothing does at login, so no collection ever reported, firstReplicationComplete never turned true, and the initialization screen was left to initializationScreenMaxDuration to time out: 25 seconds of spinner on every login, whatever the data actually did. The check dates from 2023, when a non-live cycle emitted nothing on completion and a document update was the only signal available. It got a completion event of its own two years later and the check was never moved onto it, so the workaround outlived its reason by two years.
SYNC.md gains what a login now costs in non-live mode and who asks for the second pass, the wait a data request gets while a session starts, and what the initialization screen waits for. TEST-SYNC.md gains the case that started all of this - close the session, log back in, edit something at once - and says that one Rebuilding per collection is the expected cost of a cycle, so a tester does not report it.
sara-gnucoop
force-pushed
the
sync-problems
branch
from
September 8, 2026 07:58
6b8684b to
1d79e11
Compare
The context was read from disk before the pull had landed, and addToContext refuses a second write on a key it holds, so a permission changed since the last session stayed invisible until a logout destroyed the database. It is built after the first pull of user_data, user_group and user_role now, and the ids granted since the last recorded session are asked for by id, once.
The total came from the raw query while the rows went through canView, so a metric revoked from a group left the list and not the count. Both go through the same filter now.
SYNC.md gains the context section: where it lives, why the first writer wins, what the backfill asks for. TEST-SYNC.md gains the two-account check, and the note that a reload does as much as ending the session.
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
2 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.
Bundles several fixes to sync and auth reliability, plus some cleanup:
sessions.