agent: fast queue rotation not requiring the current server to be online - #1847
Conversation
92f05e6 to
5355221
Compare
2003790 to
80b32ae
Compare
There was a problem hiding this comment.
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.
securingSndQueuemakessubmitPendingMsg/resumeMsgDeliveryno-ops while R' isNew+ replacing, so no worker delivers to R' untilICQSndSecuresends the confirmation synchronously and only then activates the queue. Restart-safe:ICQSndSecureis a persisted command and delivery resume is guarded by the same predicate. - Redundant delivery + dedup absorbs the races.
copyPendingSndDeliveriesruns 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. QENDhandling is idempotent. Delivered on both queues;removeQP/L.partition+withConnLockNotifymean the second copy is a no-op, so no duplicateSPCompletedand no double-remove. Self-deletion of the old queue's worker fromsmpDeliveryWorkersmirrors the existingAM_QTEST_pattern.- Version gating is correct. Fast path only at agreed agent version ≥ 8 (
rpcAddressSMPAgentVersion),QENDis v8-only, andcanAbortRcvSwitchnow refuses to abort a committedRSSendingQADDat v8+. BothcanAbortRcvSwitchcall 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 (
ackreturningNothingon a missing queue) and the bounded-retryICDeleteRcvQueuefor 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).
There was a problem hiding this comment.
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 onsecuringSndQueue(status == New && isJust dbReplaceQueueId). So the accumulated redundant rows for R' cannot be sent untilICQSndSecuresends the empty confirmation and then callsresumeMsgDelivery. This also holds across restart, since startup resume goes through the same gated callers andICQSndSecureis a persisted command. -
opsInProgress stays balanced. The rows copied by
copyPendingSndDeliveries(and any added while R' is securing, sinceisActiveSndQnow counts a securing queue on v8) bypasssubmitPendingMsg, so they are not counted at enqueue time.ICQSndSecurecounts them once withcountSndQueueDeliveriesand adds exactly that toopsInProgressunder the connection lock, and each delivered message callsendAgentOperationonce. No double count, no leak. -
ICQSndSecurereplay is idempotent.setSndQueuePrimarynullsreplace_snd_queue_id, so a re-run after success reaches theNothingbranch 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 harmlesslyprohibited. -
QENDis idempotent on both sides. Send side (AM_QEND_) usesremoveQP (sndSwchStatus == Just SSSendingQEND); once the old queue is gone the secondQENDsend finds nothing and produces no event. Receive side (qEndMsg) partitions onsndAddressand no-ops when the removed set is empty.QENDon the old queue arriving before the confirmation on R' is safe becauseqEndMsgdoes not touch R''s replace reference, so the later confirmation still secures R'. -
ICDeleteRcvQueuebounding mirrors the existingdeleteQueueRecpattern exactly (temporaryOrHostError e && deleteErrors rq + 1 < maxErrs→incRcvDeleteErrors+ retry, elsedeleteConnRcvQueue), so a dead old server no longer retries forever and the bound survives restarts. -
canAbortRcvSwitch/ack. Treating a sentQADDas committed at v8 is consistent with the peer always choosing fast rotation. Theackchange (swallowSEConnNotFound, 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;QENDis v8-only and only sent during fast rotation, so pre-v8 peers never parse it. No schema migration is needed (statuses reuseswitch_status, R''s secret reusesrcv_queues.e2e_dh_secret).
Minor, non-blocking
-
Fast rotation is gated on
rpcAddressSMPAgentVersion(v8) atAgent.hs:2363,Agent.hs:3939, andStore.hs:216. This reuse is deliberate and documented (plan §"New definitions", and the comment inStore.hs), but the constant is named for an unrelated feature, so at the twoAgent.hssites 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.
No description provided.