Skip to content

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

Description

@ImDanXie

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:

// 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions