From 2ffc935d62752352c2877c891ef4e5714978ef0f Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Sat, 5 Sep 2026 11:19:42 +0000 Subject: [PATCH] core: show connections prepared for failed link joins in chat list Connecting via a long link whose server is unreachable leaves a connection with conn_status = prepared and no chat entity. The row is kept on purpose, so that retrying the same link reuses the same keys (#5018), but the chat list query excluded prepared connections, so the user could not delete it, and it kept being subscribed on every app start. --- .../2026-09-04-orphan-prepared-connections.md | 270 ++++++++++++++++++ src/Simplex/Chat/Store/Messages.hs | 3 +- .../SQLite/Migrations/chat_query_plans.txt | 3 - tests/ChatTests/Profiles.hs | 29 ++ 4 files changed, 300 insertions(+), 5 deletions(-) create mode 100644 plans/2026-09-04-orphan-prepared-connections.md diff --git a/plans/2026-09-04-orphan-prepared-connections.md b/plans/2026-09-04-orphan-prepared-connections.md new file mode 100644 index 0000000000..eec03c8a00 --- /dev/null +++ b/plans/2026-09-04-orphan-prepared-connections.md @@ -0,0 +1,270 @@ +# Orphan `prepared` connections after a failed connect via a long link + +## Problem + +Connecting via an **old-format (long) link** whose server is unreachable — contact +address, one-time invitation, or group link — leaves a row in `connections` with +`conn_status = prepared`, `conn_type = contact`, `contact_id IS NULL`. The user +sees the connect error, but the connection stays in the database forever: + +- it is **not shown in the chat list**, so there is no way to delete it from the UI; +- for address/group links the receive queue was already created on the user's own + server, so it is counted in server stats and re-subscribed on every app start. + +A new orphan is produced per distinct link — a repeat attempt with the *same* link +reuses the existing prepared connection, found by `via_contact_uri_hash`. + +Why the queue exists for some kinds and not others: for a contact URI the agent's +`joinConnSrv` creates our queue with `newRcvConnSrv` *before* `sendInvitation` +reaches the dead server. For an invitation URI the queue is created by +`createReplyQueue` inside `mkAgentConfirmation`, which runs *after* +`agentSecureSndQueue`, so whether a queue leaks there depends on whether the peer's +queue supports sender-securing. + +In the apps, short links do not produce this orphan: `getShortLinkConnReq` fails +before anything is created, and a successful short-link flow creates a *prepared +contact/group* chat entity, which is visible in the chat list. See +"Effect on short links" for the one path where a short link behaves like a long one. + +## Cause + +`connectViaContact` → `connect'` (`src/Simplex/Chat/Library/Commands.hs`) commits +the connection row **before** the only step that can fail: + +```haskell +(connId, chatV) <- prepareContact user cReq pqSup -- agent record only, no network +conn <- withFastStore' $ \db -> createConnReqConnection ... -- row committed here +conn' <- joinContact user conn cReq ... -- network: throws on unreachable server +``` + +`createConnReqConnection` (`src/Simplex/Chat/Store/Direct.hs`) writes `ConnPrepared` +and, when `preparedEntity_` is `Nothing`, `Nothing -> (ConnContact, Nothing, Nothing, +Nothing)` — hence `conn_type = contact` even for a group link. `preparedEntity_` is +`Nothing` for every `APIConnect`, which is how a plain long link is connected. (A long +link is not *by itself* entity-less: `connectContactViaAddress`, reached by +`APIConnectContactViaAddress`, passes `Just (PCEContact ct)` with the contact's saved +full link, and that connection carries `contact_id`.) The invitation-link path +(`connectViaInvitation` → `joinNewConn`, with `createDirectConnection'`) has the same +shape. + +Keeping the row is **intentional**: it is the retry mechanism introduced in +`9a87f344b` (#5018, "do not regenerate key when accepting connection to avoid +invalidating invitation link on bad networks"). A retry with the same link finds it +by `cReqHash` and re-joins with the same keys and `xContactId`; +`testRetryConnectingViaContactLink` covers this. + +What makes it an orphan is the chat-list preview query +(`getContactConnectionChatPreviews_` in `src/Simplex/Chat/Store/Messages.hs`): + +```sql +WHERE user_id = ? AND conn_type = ? AND conn_status != ? -- ConnContact, ConnPrepared + AND contact_id IS NULL AND conn_level = 0 AND via_contact IS NULL +``` + +`conn_status != 'prepared'` came from the same #5018 commit ("exclude prepared +connections from API responses"). Because the query already requires +`conn_type = contact AND contact_id IS NULL`, the only rows that filter excludes are +this orphan shape and, for as long as it stays `prepared`, the chat relay test +connection (see below) — prepared connections belonging to a prepared contact or group +have `contact_id` / `group_member_id` set and are shown as their own chats. On success the +same row becomes `ConnJoined` and *is* listed, so the connection is invisible only +while it is prepared, i.e. exactly when the join failed. + +## Fix + +Show entity-less prepared connections in the chat list so they can be deleted, and +keep the retry behaviour of #5018 unchanged. + +```diff + WHERE user_id = ? + AND conn_type = ? +- AND conn_status != ? ++ AND relay_test = 0 + AND contact_id IS NULL + AND conn_level = 0 + AND via_contact IS NULL + AND (via_group_link = 0 OR (via_group_link = 1 AND group_link_id IS NOT NULL)) + AND LOWER(local_alias) LIKE '%' || LOWER(?) || '%' +- params search = (userId, ConnContact, ConnPrepared, search) ++ params search = (userId, ConnContact, search) +``` + +Nothing else is needed: + +- **Deletion already works.** `apiDeleteChat CTContactConnection` uses + `getPendingContactConnection` / `deletePendingContactConnection`, neither of which + filters on status, and it calls `deleteAgentConnectionAsync` first — so the agent + connection and the queue on the user's own server go too, which is what stops the + per-restart traffic and the stats count. The chat-list context menu already offers + Delete (`ChatListNavLinkView.kt`), as does the iOS swipe action + (`ChatListNavLink.swift`). +- **Both clients render `prepared` pending connections correctly.** + `ConnStatus.prepared → initiated = false` (`ChatModel.kt`, `ChatTypes.swift`), so + the item reads "requested to connect" / "via contact address link" (or "accepted + invitation" / "via one-time link" for an invitation link), and the link/QR section — + which would otherwise show the *peer's* link, since a joining connection stores it + in `conn_req_inv` — is gated on `initiated` in both apps + (`ContactConnectionInfoView.kt`, `ContactConnectionInfo.swift`). No client change is + required. +- **The remaining conditions already hold for these rows.** `createConnReqConnection` + writes `via_group_link = isJust groupLinkId` and `group_link_id = groupLinkId` + together, so `via_group_link = 0 OR group_link_id IS NOT NULL` holds for old group + links (which carry `CRDataGroup`) and for addresses/business links (which do not). +- **No migration.** Orphans already in users' databases become visible and deletable + immediately. + +### Why `relay_test = 0` is required (separate commit) + +`APITestChatRelay` creates a connection via `createRelayTestConnection` with +`ConnPrepared`, `conn_type = ConnContact`, `contact_id NULL`, `conn_level = 0`, +`via_contact NULL`, and the schema defaults `via_group_link = 0`, `local_alias = ''` +— i.e. it satisfies every condition of this query except the status filter. + +The status filter does not actually hold it out. `processAgentMessageConn` runs +`updateConnStatus` on the entity **before** dispatching to `processDirectMessage`, +and `agentMsgConnStatus` maps `CONF{}` to `ConnRequested`; the relay-test +interception happens inside `processDirectMessage`, i.e. after the status has already +been written. A relay-test connection resolves to `RcvDirectMsgConnection c Nothing` +(`getConnectionEntity`, `ConnContact` with no entity id), so this applies to it. So as +soon as the relay responds, the row is `requested`, not `prepared`, and **today it +already appears in the chat list** as a phantom "connecting" entry — for the duration +of the test (up to 40s), and permanently if the app is killed before +`deleteConnectionRecord` runs, until `cleanupStaleRelayTestConns` removes it 5 minutes +later. + +That is a pre-existing bug unrelated to prepared connections, so it is a **separate +commit**. It has to come first: without it, dropping the status filter would also make +the prepared phase of every relay test visible, turning a transient leak into a +permanent one. + +Checked against **every** `INSERT INTO connections` site; only these can produce +`conn_type = contact` with `contact_id IS NULL`: + +| site | shape | in the list? | +|---|---|---| +| `createConnReqConnection`, `preparedEntity_ = Nothing` | ConnContact, contact_id NULL | **yes** — the orphan being fixed | +| `createConnReqConnection`, `PCEContact` / `PCEGroup` | contact_id set / ConnMember | no | +| `createDirectConnection_`, `contactId_ = Nothing` | ConnContact, contact_id NULL | **yes** — the orphan being fixed | +| `createRelayTestConnection` | ConnContact, contact_id NULL, `relay_test = 1` | no — excluded by the new guard | +| `createRelayConnection`, `createRelayMemberConnectionAsync` | ConnMember | no | +| `createMemberContactConn`, `createMemberContact` | contact_id set | no | +| `createConnection_` | sets `contact_id` whenever connType is ConnContact | no | + +No code path updates an existing row *to* `prepared`: `ConnPrepared` appears only at +row creation and as the *from* status of `updateConnectionStatusFromTo`, and +`agentMsgConnStatus` never returns it. + +## Effect on short links + +None on the normal short-link flows, and no new orphan shape. + +- The apps route on **presence of short link data, not link format** + (`ConnectPlan.kt`): when the link server returned link data they call + `apiPrepareContact` / `apiPrepareGroup`, which create a *prepared contact or group*; + the connection then carries `contact_id` (or is `ConnMember`) and is excluded by the + query's existing conditions, showing as its own chat as today. +- If the link server is unreachable, `connectPlan` → `getShortLinkConnReq` fails + before anything is written, so a broken short link creates nothing at all. +- The one path where a short link can still reach `connectViaContact` with no prepared + entity is `APIConnect`, which passes `Nothing` regardless of link format — used by + the terminal `/c ` and by the app's fallback branch when the plan returns `Ok` + with no link data. There the row is written only because the user attempted a + connect, and if the join then fails it is the same orphan; showing it is the intended + fix, not a regression. + +## Rejected alternative: delete the connection when the join fails + +- It reintroduces #5018 for one-time links: `agentSecureSndQueue` may already have + secured the peer's queue with our key, so regenerating keys on retry breaks the link + permanently — the scenario covered by `testRetryConnectingClientTimeout`. +- For addresses and group links it discards the stable `xContactId`, so a retry after a + *timeout* (where the invitation may in fact have been delivered) creates a duplicate + contact request on the peer. +- It does nothing for orphans already in users' databases without a migration. + +## Cost of the fix + +Every failed long-link connect now leaves a visible "connecting" entry that the user +deletes or retries, instead of an invisible row. + +## Verification + +A SQLite database built from `chat_schema.sql` with one row per affected shape, run +against the old and new predicates: + +| row | shape | old query | new query | new query *without* `relay_test = 0` | +|---|---|---|---|---| +| 1 | failed long **contact address** join | hidden | **shown** | shown | +| 2 | failed long **group link** join (`via_group_link=1`, `group_link_id` set) | hidden | **shown** | shown | +| 3 | failed long **one-time invitation** join (`conn_req_inv` set) | hidden | **shown** | shown | +| 4 | **relay test** connection (`relay_test=1`), still `prepared` | hidden | hidden | **leaks** | +| 4b | same row once the relay has responded (`requested`) | **leaks today** | hidden | leaks | +| 5 | healthy `joined` pending connection | shown | shown | shown | +| 6 | prepared **contact** (short link, `contact_id` set) | hidden | hidden | hidden | +| 7 | prepared **group** member (short link, `conn_type=member`) | hidden | hidden | hidden | + +`EXPLAIN QUERY PLAN` is identical before and after for all three pagination variants +and matches what is already recorded in `chat_query_plans.txt`, so only the three +predicate lines change there: +`SEARCH connections USING INDEX idx_connections_contact_id (contact_id=?)` + +`USE TEMP B-TREE FOR ORDER BY` for `PTLast`, and +`SEARCH connections USING INDEX idx_connections_updated_at (user_id=? AND updated_at>?/ DB.query db (query <> " ORDER BY updated_at DESC LIMIT ?") (params search :. Only count) PTAfter ts count -> DB.query db (query <> " AND updated_at > ? ORDER BY updated_at ASC LIMIT ?") (params search :. (ts, count)) diff --git a/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt b/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt index 15d57d5096..5dca335a16 100644 --- a/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt +++ b/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt @@ -2037,7 +2037,6 @@ Query: FROM connections WHERE user_id = ? AND conn_type = ? - AND conn_status != ? AND relay_test = 0 AND contact_id IS NULL AND conn_level = 0 @@ -2055,7 +2054,6 @@ Query: FROM connections WHERE user_id = ? AND conn_type = ? - AND conn_status != ? AND relay_test = 0 AND contact_id IS NULL AND conn_level = 0 @@ -2073,7 +2071,6 @@ Query: FROM connections WHERE user_id = ? AND conn_type = ? - AND conn_status != ? AND relay_test = 0 AND contact_id IS NULL AND conn_level = 0 diff --git a/tests/ChatTests/Profiles.hs b/tests/ChatTests/Profiles.hs index efe2097504..e9e71c8b4f 100644 --- a/tests/ChatTests/Profiles.hs +++ b/tests/ChatTests/Profiles.hs @@ -67,6 +67,7 @@ chatProfileTests = do it "rotate address ratchet keys" testRotateAddressRatchetKeys it "create address on specified server" testCreateAddressOnServer it "retry connecting via contact link" testRetryConnectingViaContactLink + it "connection prepared for failed join is shown in chat list" testFailedConnectViaContactLinkInChatList it "add contact link to profile" testProfileLink it "auto accept contact requests" testUserContactLinkAutoAccept it "deduplicate contact requests" testDeduplicateContactRequests @@ -819,6 +820,34 @@ testRetryConnectingViaContactLink ps = testChatCfgOpts2 cfg' opts' aliceProfile } } +testFailedConnectViaContactLinkInChatList :: HasCallStack => TestParams -> IO () +testFailedConnectViaContactLinkInChatList ps = testChatOpts2 opts' aliceProfile bobProfile test ps + where + tmp = tmpPath ps + test alice bob = do + cLink <- withSmpServer' serverCfg' $ do + alice ##> "/ad" + getContactLink alice True + alice <## "disconnected 1 connections on server localhost" + bob ##> ("/_connect 1 " <> cLink) + bob <##. "smp agent error: BROKER" + bob @@@ [(":1","")] + bob ##> "/_delete :1" + bob <## "connection :1 deleted" + bob @@@ [] + serverCfg' = + smpServerCfg + { transports = [("7003", transport @TLS, False)], + serverStoreCfg = persistentServerStoreCfg tmp + } + opts' = + testOpts + { coreOptions = + testCoreOpts + { smpServers = ["smp://LcJUMfVhwD8yxjAiSaDzzGF3-kLG4Uh0Fl_ZIjrRwjI=:server_password@localhost:7003"] + } + } + testProfileLink :: HasCallStack => TestParams -> IO () testProfileLink = testChat3 aliceProfile bobProfile cathProfile $