fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

AmqpReplyInbox pulls a whole queue into memory on ownership, so x-max-length and per-message TTL never fire #154

New Issue
Closed
opened 2026-08-23 14:05:10 +02:00 by ltms · 1 comment
ltms commented 2026-08-23 14:05:10 +02:00
Owner
Copy Link
Copy Source

What

AmqpReplyInbox documents this itself, at the top of the class:

on ownership the broker pushes the whole queue into the in-memory held map

So the broker-side bounds we set are decorative. x-max-length and per-message TTL are enforced by the broker on messages sitting in the queue. Once a consumer takes ownership, the messages leave the queue and land in a plain in-heap map, where neither limit applies any more. held has no cap of its own.

prefetch (basicQos, default 32) bounds unacked messages per channel, which is a different thing and does not bound held after the messages are acked into it.

Why this is worth a ticket now

Two changes in the last day made this materially more likely to bite:

  1. The reply inbox is now genuinely durable and genuinely used. Before, most fleets ran on InMemoryReplyInbox and lost replies on restart, which incidentally kept the map small.
  2. Two fleets now share one LavinMQ instance (Mac on vhost /mac, fleet01 on /fleet01). One daemon that owns many targets and reads slowly now holds another fleet's broker memory as well as its own. The vhosts isolate visibility, not the box.

The failure shape is a daemon that grows without bound while every broker metric looks healthy — the queue is empty, because everything has already been pulled into our heap.

Not yet measured

I have not reproduced this or measured the growth rate; this is read from the code and its own comment. Worth doing before choosing a fix:

  • how many messages held actually accumulates for a busy target over a long session
  • whether ownership really drains the full queue or only up to prefetch at a time (the class comment says the former; I did not verify it against a running broker)

Please confirm that first — the fix depends on which it is.

Possible directions

  • Bound held explicitly and stop acking once it is full, so back-pressure returns to the broker and the broker-side limits regain their meaning.
  • Or drop the held map and read from the queue on demand, so x-max-length and TTL are the only bounds and they work.

Related: #152 (boot-time fallback) touched the same class's call site but not this behaviour.

## What `AmqpReplyInbox` documents this itself, at the top of the class: > on ownership the broker pushes the whole queue into the in-memory `held` map So the broker-side bounds we set are decorative. `x-max-length` and per-message TTL are enforced by the broker on messages **sitting in the queue**. Once a consumer takes ownership, the messages leave the queue and land in a plain in-heap map, where neither limit applies any more. `held` has no cap of its own. `prefetch` (`basicQos`, default 32) bounds *unacked* messages per channel, which is a different thing and does not bound `held` after the messages are acked into it. ## Why this is worth a ticket now Two changes in the last day made this materially more likely to bite: 1. **The reply inbox is now genuinely durable and genuinely used.** Before, most fleets ran on `InMemoryReplyInbox` and lost replies on restart, which incidentally kept the map small. 2. **Two fleets now share one LavinMQ instance** (Mac on vhost `/mac`, fleet01 on `/fleet01`). One daemon that owns many targets and reads slowly now holds another fleet's broker memory as well as its own. The vhosts isolate *visibility*, not the box. The failure shape is a daemon that grows without bound while every broker metric looks healthy — the queue is empty, because everything has already been pulled into our heap. ## Not yet measured I have **not** reproduced this or measured the growth rate; this is read from the code and its own comment. Worth doing before choosing a fix: - how many messages `held` actually accumulates for a busy target over a long session - whether ownership really drains the full queue or only up to `prefetch` at a time (the class comment says the former; I did not verify it against a running broker) Please confirm that first — the fix depends on which it is. ## Possible directions - Bound `held` explicitly and stop acking once it is full, so back-pressure returns to the broker and the broker-side limits regain their meaning. - Or drop the `held` map and read from the queue on demand, so `x-max-length` and TTL are the only bounds and they work. Related: #152 (boot-time fallback) touched the same class's call site but not this behaviour.
ltms closed this issue 2026-08-28 01:06:44 +02:00
ltms commented 2026-08-28 01:06:59 +02:00
Author
Owner
Copy Link
Copy Source

Closing: measured, and the bound already exists. CB-527 added it. Fixed by #181, which changes no behaviour and adds the test that pins it.

What was measured

held is bounded by prefetch. Ownership does not drain the queue.

  • own() calls basicConsume(queue, false, ...) — autoAck is false, so manual ack.
  • basicQos(prefetch) is set on the channel before any consumer starts. There is one shared channel, so the bound is on total unacked across all targets, not per target — a stronger bound than the ticket assumed.
  • deliverCallback acks only duplicate redeliveries, so it can drop them without holding them. A new message is put in held and left unacked.
  • So the broker stops delivering once prefetch messages are unacked, and back-pressure stays where the ticket wanted it: on the broker, where x-max-length and TTL still apply.

DEFAULT_PREFETCH is 32.

Where the ticket went wrong

The quoted sentence is real, but it is the second half of a conditional. The full text reads:

Without a bound, the broker pushes its entire queue into held the instant a target is owned, so an undrained primary grows the JVM heap without limit and any queue-level control (x-max-length, per-message TTL) never fires… Prefetch keeps the backlog where it is visible — on the broker — until the owner drains it.

That paragraph is titled "Prefetch bounds the held backlog (CB-527)". It describes the unbounded case as the thing prefetch prevents. Read as a standalone claim it says the opposite of what it means.

Worth noting as a documentation lesson, not just a filing error: a javadoc that explains a hazard and then its mitigation can be quoted into a bug report by anyone reading only the hazard half. The fix in #181 now names the prefetch window at the first mention of the held map, so the bound is visible before the hazard paragraph is reached.

What #181 adds

AmqpReplyInboxPrefetchTest drives the real own() path against a fake channel: 8 messages behind prefetch 3, then asserts 3 held, 5 still queued, 0 broker acks, and exactly one new delivery after one caller ack. It also asserts basicQos was called with manual acknowledgement. It fails if receipt ever starts acking and draining.

No live broker was contacted, per the brief. The tagged real-broker contract suite was not run.

Closing: measured, and the bound already exists. CB-527 added it. Fixed by #181, which changes no behaviour and adds the test that pins it. ## What was measured `held` is bounded by **prefetch**. Ownership does not drain the queue. - `own()` calls `basicConsume(queue, false, ...)` — `autoAck` is **false**, so manual ack. - `basicQos(prefetch)` is set on the channel *before* any consumer starts. There is one shared channel, so the bound is on total unacked across **all** targets, not per target — a stronger bound than the ticket assumed. - `deliverCallback` acks **only** duplicate redeliveries, so it can drop them without holding them. A new message is put in `held` and left unacked. - So the broker stops delivering once `prefetch` messages are unacked, and back-pressure stays where the ticket wanted it: on the broker, where `x-max-length` and TTL still apply. `DEFAULT_PREFETCH` is 32. ## Where the ticket went wrong The quoted sentence is real, but it is the second half of a conditional. The full text reads: > **Without a bound**, the broker pushes its entire queue into `held` the instant a target is owned, so an undrained primary grows the JVM heap without limit and any queue-level control (`x-max-length`, per-message TTL) never fires… **Prefetch keeps the backlog where it is visible — on the broker — until the owner drains it.** That paragraph is titled *"Prefetch bounds the held backlog (CB-527)"*. It describes the unbounded case as the thing prefetch **prevents**. Read as a standalone claim it says the opposite of what it means. Worth noting as a documentation lesson, not just a filing error: a javadoc that explains a hazard and then its mitigation can be quoted into a bug report by anyone reading only the hazard half. The fix in #181 now names the prefetch window at the *first* mention of the held map, so the bound is visible before the hazard paragraph is reached. ## What #181 adds `AmqpReplyInboxPrefetchTest` drives the real `own()` path against a fake channel: 8 messages behind prefetch 3, then asserts 3 held, 5 still queued, 0 broker acks, and exactly one new delivery after one caller ack. It also asserts `basicQos` was called with manual acknowledgement. It fails if receipt ever starts acking and draining. No live broker was contacted, per the brief. The tagged real-broker contract suite was not run.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#154
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?