[I] [DISCUSS] QoS1 send-buffer overflow drops newest (hardcoded dropOldest=false) — is this the intended design? [bifromq]

["ImDanXie (via GitHub)" <gi...@apache.org>]

ImDanXie opened a new issue, #312:
URL: https://github.com/apache/bifromq/issues/312

   ### Environment
   
   - BifroMQ `4.0.0` / `main @ eef5e3af`
   - Module: `bifromq-inbox-store` (`InboxStoreCoProc.insertInbox`)
   
   ### What we observed
   
   During a burst load test (shared subscription, 3 bridge replicas consuming ~1.15k msg/s into a persistent session with `SessionInboxSize=10000`), we measured **~40% QoS1 message loss**. The loss was completely silent end-to-end until we fixed both sides of the notification chain (our event plugin was also dropping the `Overflowed` events — our bug, separately fixed).
   
   Tracing the loss to the source, we found that the QoS1 send-buffer overflow path in `insertInbox` passes `false` for `dropOldest`:
   
   ```java
   // QoS0 path — respects the per-session flag from BatchAttachRequest:
   insertToInbox(..., metaBuilder.getDropOldest(), ...);
   
   // QoS1/buffer path — hardcoded:
   insertToInbox(..., false, ...);
   ```
   
   This means: when the send buffer is full, **newly arriving QoS1 messages are the ones dropped** (never buffered, never redelivered), while older buffered messages survive and are eventually drained.
   
   ### Our question
   
   Is this asymmetry a deliberate design choice? We can think of arguments for both sides and would like to understand the intent before proposing any change:
   
   **Possible reasons for drop-newest (current behavior) on QoS1:**
   - The oldest messages are closest to being delivered — dropping them wastes the queuing work already done
   - QoS1 send buffers might carry ordered command streams where processing the head first matters more than receiving the tail
   - It preserves the natural FIFO contract: what was accepted first gets delivered first; the loss is at the ingress edge
   
   **Why drop-oldest might serve some workloads better:**
   - In mixed traffic dominated by high-volume, self-superseding status streams (heartbeats, metrics) with a small fraction of unique, high-value events (alerts, session records), drop-newest during a burst means **the events arriving in the burst window are exactly the ones lost** — stale heartbeats survive while fresh alerts do not
   - Post-burst convergence: with drop-oldest, the buffer holds the newest window and the consumer catches up to current state immediately; with drop-newest, the consumer must first drain the full stale backlog
   
   ### One observation about the existing plumbing
   
   The per-session `dropOldest` flag already exists in the protocol (`BatchAttachRequest`, field 5) and is faithfully wired from the tenant setting through `MQTTConnectHandler` (`settings.inboxDropOldest`, sourced from `QoS0DropOldest`). The QoS1 path in `InboxStoreCoProc` is the only place that ignores it — which is why we wondered whether this was intentional (the setting name `QoS0DropOldest` does suggest it was scoped to QoS0 deliberately).
   
   If the current behavior is intentional, we'd appreciate a pointer to the design rationale — it would help us configure our deployment correctly (e.g., size `SessionInboxSize` and scale consumers so the buffer never fills, and treat `Overflowed` drop counts as a hard SLO violation).
   
   If the asymmetry is not intentional, honoring the flag on the QoS1 path would be a one-line change (`metaBuilder.getDropOldest()` instead of `false`), and the existing parameterized test (`InboxInsertTest.insertDropOldest(QoS)`) already covers both QoS levels — we'd be happy to submit a PR.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@bifromq.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org