agent: fix three races that closing clients on a network change makes reachable

Closing the clients on a network change makes a client that is no longer the one
in the session map a routine occurrence rather than a rare one, which makes these
pre-existing races reachable in normal use.

- setSubsPending removes the subscriptions of a transport session only when the
  session id matches. A client that is no longer the one in the map never set
  its session id, and when the session mode stopped matching the transport
  session its disconnection removed the subscriptions of the client that
  replaced it without making them pending, so they were neither subscribed nor
  scheduled and nothing corrected it until the app restarted.
- smpClientDisconnected removes the sessions to the proxied relays only when no
  other client replaced this one, as those sessions are then the sessions of the
  client that replaced it.
- updateClientService reads the stored service id in the transaction that writes
  it. The id read before the handshake can be stale, as the transaction of a
  client that is no longer current can commit after the one of the client that
  replaced it, and removeRcvServiceAssocs would then run against a service id
  that changed since, removing the associations of the current client.
This commit is contained in:
Narasimha-sc
2026-10-05 20:57:41 +00:00
parent 8e3f9a47b9
commit 2c4ea1f0a4
2 changed files with 11 additions and 5 deletions
+8 -4
View File
@@ -781,7 +781,9 @@ smpConnectClient c@AgentClient {smpClients, msgQ, proxySessTs, presetDomains} nm
when current $ updateClientService service smp
pure SMPConnectedClient {connectedClient = smp, proxiedRelays = prs}
updateClientService service smp = case (service, smpClientServiceId smp) of
(Just (_, serviceId_), Just serviceId) -> withStore' c $ \db -> do
(Just _, Just serviceId) -> withStore' c $ \db -> do
-- the id read before the handshake can be stale - this transaction can commit after another client's
serviceId_ <- (>>= snd) <$> getClientServiceCredentials db userId srv
setClientServiceId db userId srv serviceId
forM_ serviceId_ $ \sId -> when (sId /= serviceId) $ removeRcvServiceAssocs db userId srv
(Just _, Nothing) -> withStore' c $ \db -> deleteClientService db userId srv -- e.g., server version downgrade
@@ -807,9 +809,11 @@ smpClientDisconnected c@AgentClient {active, smpClients, smpProxiedRelays} tSess
(subs, serviceSub_) <- SS.setSubsPending mode tSess sessId $ currentSubs c
let qs = M.elems subs
cs = nubOrd $ map qConnId qs
-- this removes proxied relays that this client created sessions to
destSrvs <- M.keys <$> readTVar prs
forM_ destSrvs $ \destSrv -> TM.delete (userId, destSrv, cId) smpProxiedRelays
-- this removes proxied relays that this client created sessions to, unless another client
-- replaced this one, e.g. when the network changed - those sessions are now its sessions
unlessM (TM.member tSess smpClients) $ do
destSrvs <- M.keys <$> readTVar prs
forM_ destSrvs $ \destSrv -> TM.delete (userId, destSrv, cId) smpProxiedRelays
pure (qs, cs, serviceSub_)
serverDown :: ([RcvQueueSub], [ConnId], Maybe ServiceSub) -> IO ()
+3 -1
View File
@@ -216,7 +216,9 @@ setSubsPending mode tSess@(uId, srv, connId_) sessId tss@(TSessionSubs ss)
| entitySession == isJust connId_ =
TM.lookup tSess ss >>= withSessSubs (`setSubsPending_` Nothing)
| otherwise =
TM.lookupDelete tSess ss >>= withSessSubs setPendingChangeMode
-- the subscriptions are removed only when the session matches - a client that is no longer
-- the one in the map never set its session id, and its disconnection must not remove them
TM.lookup tSess ss >>= withSessSubs (\s -> TM.delete tSess ss >> setPendingChangeMode s)
where
entitySession = mode == TSMEntity
sessEntId = if entitySession then Just else const Nothing