Skip to content

agent: fast queue rotation not requiring the current server to be online - #1847

Merged
epoberezkin merged 19 commits into
masterfrom
ep/fast-rotation
Aug 20, 2026
Merged

agent: fast queue rotation not requiring the current server to be online#1847
epoberezkin merged 19 commits into
masterfrom
ep/fast-rotation

Conversation

@epoberezkin

Copy link
Copy Markdown
Member

No description provided.

@epoberezkin
epoberezkin requested a review from spaced4ndy as a code owner August 9, 2026 19:44
@epoberezkin epoberezkin changed the title agent: fast queue rotation does not requiring the current server to be online agent: fast queue rotation not requiring the current server to be online Aug 16, 2026
Base automatically changed from ep/drop-agent-versions to master August 17, 2026 11:51

@simplex-chat-agent simplex-chat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adds v8 "fast" queue rotation: the peer secures the new receive queue R' over R' itself (an empty confirmation carrying a fresh queue-box DH) instead of over the current queue, so a rotation completes even when the current server is down. A new QEND agent message removes the replaced queue on both sides, and pending deliveries are copied to R' so the backlog survives the move.

I traced the whole flow — QADD (qAddMsg) → copyPendingSndDeliveries + ICQSndSecure → confirmation on R' + QEND to both queues → recipient qEndMsg / sender AM_QEND_ — plus the protocol/store additions and the test changes. The design is careful and the invariants hold:

  • Confirmation-first ordering is structural, not timed. securingSndQueue makes submitPendingMsg/resumeMsgDelivery no-ops while R' is New + replacing, so no worker delivers to R' until ICQSndSecure sends the confirmation synchronously and only then activates the queue. Restart-safe: ICQSndSecure is a persisted command and delivery resume is guarded by the same predicate.
  • Redundant delivery + dedup absorbs the races. copyPendingSndDeliveries runs under the conn lock while the old worker deletes rows without it, but a doubly-copied or missed row is either deduplicated or already delivered on the other queue — no loss either way.
  • QEND handling is idempotent. Delivered on both queues; removeQP/L.partition + withConnLockNotify mean the second copy is a no-op, so no duplicate SPCompleted and no double-remove. Self-deletion of the old queue's worker from smpDeliveryWorkers mirrors the existing AM_QTEST_ pattern.
  • Version gating is correct. Fast path only at agreed agent version ≥ 8 (rpcAddressSMPAgentVersion), QEND is v8-only, and canAbortRcvSwitch now refuses to abort a committed RSSendingQADD at v8+. Both canAbortRcvSwitch call sites were updated. The recipient reacts to whichever message arrives, and repeated confirmations fall into the existing (Just e2eDh, Just _) idempotent branch.
  • Tolerant ack after queue removal (ack returning Nothing on a missing queue) and the bounded-retry ICDeleteRcvQueue for a dead old server are both sound.

Tests: existing switch tests are correctly pinned to v7 (agentCfgV7) to keep exercising the QKEY/QUSE slow path and abort semantics, and new tests cover fast rotation, a dead old server with an in-flight message, and notification re-subscription on both paths. Good coverage.

One minor, non-blocking note (no fix needed): during securing, deliveries copied to R' and new deliveries scheduled to R' skip the submitPendingMsg increment of msgDeliveryOp.opsInProgress, but the worker started by ICQSndSecure decrements once per drained message. This over-decrements (clamped at 0 by endOperation), so the counter can reach 0 while R' still has queued deliveries. Impact is limited to graceful-suspend accounting; deliveries are persisted and redelivered on resume, so no message is lost. Worth a comment if you want to keep the accounting exact.

One small suggestion inline: reuse securingSndQueue inside sndDeliverTo instead of re-spelling status == New && isJust (dbReplaceQId sq).

Comment thread src/Simplex/Messaging/Agent.hs Outdated

@simplex-chat-agent simplex-chat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fast queue rotation (agent v8)

Reviewed the whole branch, not just the last commit: Agent.hs, Agent/Protocol.hs, Store.hs, Store/AgentStore.hs, the two test modules, and the RFC/plan/diagram. I traced the callers/callees of every changed function (isActiveSndQ, securingSndQueue, submitPendingMsg, resumeMsgDelivery, canAbortRcvSwitch, the new ICQSndSecure/QEND paths, copyPendingSndDeliveries, countSndQueueDeliveries, setSndQueuePrimary, the ICDeleteRcvQueue retry).

This is a careful, well-structured change and it matches the RFC and the implementation plan closely. I found no correctness, concurrency, security, or accounting bug worth a code change. Notes below are for the record.

What holds up under scrutiny

  • Confirmation-first invariant is structural, not disciplinary. R''s delivery worker is started only through submitPendingMsg/resumeMsgDelivery, and both now short-circuit on securingSndQueue (status == New && isJust dbReplaceQueueId). So the accumulated redundant rows for R' cannot be sent until ICQSndSecure sends the empty confirmation and then calls resumeMsgDelivery. This also holds across restart, since startup resume goes through the same gated callers and ICQSndSecure is a persisted command.

  • opsInProgress stays balanced. The rows copied by copyPendingSndDeliveries (and any added while R' is securing, since isActiveSndQ now counts a securing queue on v8) bypass submitPendingMsg, so they are not counted at enqueue time. ICQSndSecure counts them once with countSndQueueDeliveries and adds exactly that to opsInProgress under the connection lock, and each delivered message calls endAgentOperation once. No double count, no leak.

  • ICQSndSecure replay is idempotent. setSndQueuePrimary nulls replace_snd_queue_id, so a re-run after success reaches the Nothing branch and is a safe no-op; a partial-failure retry re-secures (SKEY idempotent for the same key) and re-sends the confirmation, which lands on an already-Confirmed R' and is harmlessly prohibited.

  • QEND is idempotent on both sides. Send side (AM_QEND_) uses removeQP (sndSwchStatus == Just SSSendingQEND); once the old queue is gone the second QEND send finds nothing and produces no event. Receive side (qEndMsg) partitions on sndAddress and no-ops when the removed set is empty. QEND on the old queue arriving before the confirmation on R' is safe because qEndMsg does not touch R''s replace reference, so the later confirmation still secures R'.

  • ICDeleteRcvQueue bounding mirrors the existing deleteQueueRec pattern exactly (temporaryOrHostError e && deleteErrors rq + 1 < maxErrsincRcvDeleteErrors + retry, else deleteConnRcvQueue), so a dead old server no longer retries forever and the bound survives restarts.

  • canAbortRcvSwitch / ack. Treating a sent QADD as committed at v8 is consistent with the peer always choosing fast rotation. The ack change (swallow SEConnNotFound, rethrow other store errors) is a reasonable robustness fix for a queue removed mid-rotation.

  • Protocol additions (QEND/QE, AM_QEND_, SSSecuringQueue/SSSendingQEND, ICQSndSecure) are wired through every encode/decode/tag site; QEND is v8-only and only sent during fast rotation, so pre-v8 peers never parse it. No schema migration is needed (statuses reuse switch_status, R''s secret reuses rcv_queues.e2e_dh_secret).

Minor, non-blocking

  • Fast rotation is gated on rpcAddressSMPAgentVersion (v8) at Agent.hs:2363, Agent.hs:3939, and Store.hs:216. This reuse is deliberate and documented (plan §"New definitions", and the comment in Store.hs), but the constant is named for an unrelated feature, so at the two Agent.hs sites the intent isn't self-evident. A dedicated alias (fastRotationSMPAgentVersion = rpcAddressSMPAgentVersion) or a one-line comment would make the gate read as what it is. Purely a readability point.

  • Tests cover the new/new happy path, the dead-old-server path, notification re-subscription on both slow and fast rotation, and the v7 pinning that keeps the slow-path/abort tests exercising QKEY/QUSE. Good coverage for the feature.

Approving.

@epoberezkin
epoberezkin merged commit 0d3cf39 into master Aug 20, 2026
4 of 6 checks passed
@epoberezkin
epoberezkin deleted the ep/fast-rotation branch August 20, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants