Skip to content

fix: reconnecting to same remote archive server fails after clearing data#110

Open
luandro wants to merge 3 commits into
mainfrom
agent/comapeo-cloud-app/issue-73
Open

fix: reconnecting to same remote archive server fails after clearing data#110
luandro wants to merge 3 commits into
mainfrom
agent/comapeo-cloud-app/issue-73

Conversation

@luandro

@luandro luandro commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #73

Root Causes & Fixes

Four root causes were identified and fixed:

1. clearAllData() preserved remoteServers table

Stale records blocked re-adding the same server via duplicate check.

  • Now clears remoteServers and iconCache tables alongside all other tables.

2. In-memory auth store not cleared after IndexedDB wipe

StorageSettings cleared IndexedDB but left the auth store in memory, creating inconsistency.

  • Added useAuthStore.getState().clearAll() call after clearAllData().

3. Lock contention in syncRemoteArchive()

Concurrent sync calls returned confusing errors instead of waiting.

  • Now returns the existing in-flight Promise when a sync is already running.

4. No connection validation before adding a server

Invalid/unreachable servers were added silently and failed during sync.

  • Added healthCheck + getProjects validation with specific error messages.

Testing

  • 124/124 tests pass in directly affected files
  • 1105/1105 tests pass in broader suite
  • TypeScript: clean
  • ESLint: clean
  • i18n: messages extracted successfully

Closes #73

Greptile Summary

This PR fixes four root causes behind reconnecting to the same remote archive server failing after "Clear All Cached Data": clearAllData now also wipes remoteServers and iconCache, the in-memory auth store is cleared alongside IndexedDB, concurrent syncRemoteArchive calls for the same server share a single in-flight Promise instead of colliding, and a healthCheck + getProjects validation step guards against silently adding unreachable or unauthorized servers.

  • clearAllData / StorageSettings: remoteServers and iconCache tables are now cleared in the same transaction, and useAuthStore.clearAll() is called immediately after to drain the in-memory mirror.
  • sync.ts concurrency guard: a module-level activeSyncs Map returns the existing Promise to any concurrent caller for the same serverDbId, preventing duplicate sync attempts.
  • AddArchiveServerDialog pre-validation: validateConnection runs healthCheck then getProjects (checking for 401) before writing the server to the store, surfacing "Cannot connect" or "Invalid token" errors before the server is persisted.

Confidence Score: 5/5

Safe to merge — all four targeted bugs are correctly fixed and 1105 tests pass.

The core fixes are well-scoped and consistent: clearAllData wipes the two previously-missed tables inside an existing transaction, the auth-store wipe pairs correctly with the DB wipe, the concurrency guard correctly deduplicates in-flight Promises with a guaranteed .finally() cleanup, and the pre-validation guards both the happy and error paths. The only notes are a minor edge case where an in-flight sync could re-populate a just-cleared database, and a floating addServer promise where an onAdded throw would leave the dialog in a loading state — neither is a regression and both are uncommon paths.

No files require special attention; the two notes above are low-probability edge cases that do not affect the primary fix.

Important Files Changed

Filename Overview
src/lib/sync.ts Adds a module-level concurrency guard (activeSyncs Map) that deduplicates in-flight syncs by returning the existing Promise; works correctly, but the Map is never cleared on clearAllData, which could let an in-flight sync re-populate a just-wiped database.
src/lib/storage.ts Adds db.remoteServers.clear() and db.iconCache.clear() to clearAllData(); both tables exist in the schema and are inside the existing db.transaction('rw', db.tables, ...) scope — change is correct.
src/components/shared/StorageSettings.tsx Adds useAuthStore.getState().clearAll() after clearAllData() to synchronise the in-memory auth store with the newly-emptied IndexedDB; consistent with how clearAllStorage works elsewhere.
src/screens/Home/AddArchiveServerDialog.tsx Adds validateConnection (healthCheck + getProjects) before committing a server; logic is sound. The addServer promise is not awaited in either finalizeAddServer or handleAdvancedSubmit, leaving a gap where an onAdded throw would produce an unhandled rejection and a permanently loading button.
tests/unit/lib/sync.test.ts Good coverage of concurrency guard, non-critical data-type failures, and all new sync paths; tests are clean and well-structured.
tests/unit/lib/storage.test.ts Covers clearAllData, clearServerData, and getStorageStats; existing structure unchanged.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant D as AddArchiveServerDialog
    participant V as validateConnection
    participant A as authStore
    participant DB as IndexedDB
    participant S as syncRemoteArchive

    U->>D: Submit (URL + token)
    D->>V: healthCheck(url)
    V-->>D: true/false
    alt unreachable
        D-->>U: Could not connect
    end
    D->>V: getProjects(url, token)
    V-->>D: ok / 401
    alt 401
        D-->>U: Invalid token
    end
    D->>A: addServer(label, url, token)
    A-->>D: serverId
    D-->>U: onAdded(serverId)
    U->>D: Clear All Data
    D->>DB: clearAllData()
    D->>A: clearAll()
    note over A,DB: In-memory and IndexedDB both empty
    U->>D: Re-add same server
    D->>S: syncRemoteArchive(serverId, options)
    S->>S: activeSyncs.get(serverId)?
    alt already syncing
        S-->>D: return existing Promise
    else new sync
        S->>DB: pullProjects / pullObservations
        DB-->>S: stored
        S->>A: updateServerStatus connected
    end
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/lib/sync.ts:28
**In-flight sync can re-populate data after `clearAllData`**

`activeSyncs` is a module-level singleton that is never cleared when the user wipes the database. If `clearAllData()` + `clearAll()` fires while a sync is in flight, the sync will complete and write records (projects, observations, etc.) back into the freshly-emptied tables before the `.finally()` cleanup removes the entry. The user sees their data partially reappear after an explicit "Clear All" action, and the stale sync result also calls `updateServerStatus` on a server ID that no longer exists in the auth store.

### Issue 2 of 2
src/screens/Home/AddArchiveServerDialog.tsx:267-291
**Floating `addServer` promise — dialog stays loading if `onAdded` throws**

`addServer({...}).then(onFulfilled, onRejected)` is not returned or awaited. The `.then(onFulfilled, onRejected)` form means `onRejected` only guards against `addServer`'s rejection; it does NOT catch an exception thrown inside `onFulfilled`. If `onAdded(serverId)` throws for any reason, the resulting rejection is silently swallowed and the dialog stays in the loading state permanently. The same pattern appears in `handleAdvancedSubmit` on line 413.

Reviews (2): Last reviewed commit: "fix(storybook): add missing ApiError and..." | Re-trigger Greptile

github-actions Bot added 2 commits June 9, 2026 08:13
…data

Four root causes identified and fixed:

1. clearAllData() preserved remoteServers table, leaving stale records
   that blocked re-adding the same server via duplicate check.
   - Now clears remoteServers and iconCache tables too.

2. StorageSettings didn't clear the in-memory auth store after clearing
   IndexedDB, creating inconsistency between memory and DB.
   - Added useAuthStore.getState().clearAll() call after clearAllData().

3. syncRemoteArchive() returned error on lock contention, causing
   confusing failures when auto-sync and manual add competed.
   - Now returns the existing in-flight Promise, waiting for it to
     complete instead of rejecting.

4. No connection validation before adding a server — invalid/unreachable
   servers were added then failed during sync.
   - Added healthCheck + getProjects validation with specific error
     messages for unreachable server and invalid token.

Closes #73
Comment thread src/lib/sync.ts
Comment on lines 257 to 260
const existingSync = activeSyncs.get(serverDbId);
if (existingSync) {
return {
success: false,
error: 'Sync already in progress',
};
return existingSync;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Second caller's options silently dropped on concurrent sync

When a second syncRemoteArchive call for the same serverDbId arrives while a sync is in-flight, the second caller's options (including a potentially different/refreshed token) are discarded without ensureServerInStore being called for them. In the scenario where a token is rotated between two overlapping sync cycles, the new credentials are never written to the auth store — any sync that starts after the in-flight one completes will still use the stale token from the first call. The auth store will only get the updated token when a fresh (non-deduplicated) syncRemoteArchive call is made after the in-flight one settles.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/sync.ts
Line: 257-260

Comment:
**Second caller's options silently dropped on concurrent sync**

When a second `syncRemoteArchive` call for the same `serverDbId` arrives while a sync is in-flight, the second caller's `options` (including a potentially different/refreshed token) are discarded without `ensureServerInStore` being called for them. In the scenario where a token is rotated between two overlapping sync cycles, the new credentials are never written to the auth store — any sync that starts after the in-flight one completes will still use the stale token from the first call. The auth store will only get the updated token when a fresh (non-deduplicated) `syncRemoteArchive` call is made after the in-flight one settles.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 339 to 361
@@ -307,7 +361,7 @@ function AddArchiveServerDialog({
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unhandled rejection from finalizeAddServer in encrypted invite path

The async (redeemed) => { await finalizeAddServer(...) } callback runs inside .then(onFulfilled, onRejected). If finalizeAddServer throws for any reason (e.g. an unexpected error from checkDuplicate, dispatch, or a future caller that changes the function), the rejection from the async callback is not caught by the onRejected handler — that handler only covers redeemEncryptedInvite failures. The result is an unhandled promise rejection and a dialog permanently stuck in the submitting state. Adding a .catch() at the end of the chain (or wrapping finalizeAddServer in a try/catch that dispatches an error) would close the gap.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/screens/Home/AddArchiveServerDialog.tsx
Line: 339-361

Comment:
**Unhandled rejection from `finalizeAddServer` in encrypted invite path**

The `async (redeemed) => { await finalizeAddServer(...) }` callback runs inside `.then(onFulfilled, onRejected)`. If `finalizeAddServer` throws for any reason (e.g. an unexpected error from `checkDuplicate`, `dispatch`, or a future caller that changes the function), the rejection from the async callback is not caught by the `onRejected` handler — that handler only covers `redeemEncryptedInvite` failures. The result is an unhandled promise rejection and a dialog permanently stuck in the submitting state. Adding a `.catch()` at the end of the chain (or wrapping `finalizeAddServer` in a try/catch that dispatches an error) would close the gap.

How can I resolve this? If you propose a fix, please make it concise.

@luandro luandro added agent:pr-needs-ci-fix PR lifecycle label agent:pr-readiness-in-progress PR lifecycle label labels Jun 10, 2026
The AddArchiveServerDialog component (added in this PR) imports ApiError,
apiClient, and redeemEncryptedInvite from @/lib/api-client. The Storybook
mock for this module was missing the ApiError class and apiClient object,
causing all three Storybook-related CI checks to fail:

- storybook-tests: import error on HomeScreen.stories.tsx
- storybook-test-runner: build-storybook failed with MISSING_EXPORT
- visual-regression-check: same build-storybook failure

Add the missing exports to the Storybook api-client mock.
@luandro luandro added the agent:pr-blocked PR lifecycle label label Jun 10, 2026
@luandro

luandro commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

PR Readiness Worker — Run 2026-06-11T18:30:22Z

  • Actions taken: Rework attempt cap reached (7/3)
  • Remaining blockers: - Maximum rework attempts (3) exceeded — manual intervention required
  • Review tier completed: none
  • Reviewer confidence: low
  • Next recommended step: Resolve review comments manually and remove agent:pr-blocked

@github-actions

Copy link
Copy Markdown
Contributor

Preview deployment ready: https://agent-comapeo-cloud-app-issu-kaq7.comapeo-cloud-app.pages.dev

Commit: f4a0eda

@luandro luandro added agent:pr-needs-review PR lifecycle label agent:pr-needs-rework PR lifecycle label agent:pr-readiness-in-progress PR lifecycle label agent:pr-blocked PR lifecycle label and removed agent:pr-needs-ci-fix PR lifecycle label agent:pr-blocked PR lifecycle label agent:pr-needs-review PR lifecycle label agent:pr-needs-rework PR lifecycle label agent:pr-readiness-in-progress PR lifecycle label labels Jun 11, 2026
@luandro luandro added agent:pr-blocked PR lifecycle label agent:pr-needs-rework PR lifecycle label agent:pr-readiness-in-progress PR lifecycle label and removed agent:pr-blocked PR lifecycle label agent:pr-needs-rework PR lifecycle label agent:pr-readiness-in-progress PR lifecycle label labels Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:pr-blocked PR lifecycle label agent:pr-needs-rework PR lifecycle label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Reconnecting to same remote archive server fails after clearing data

1 participant