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

CB-528: enforce publish — publisher confirms + mandatory flag on a separate channel #11

New Issue
Closed
opened 2026-08-10 17:35:24 +02:00 by kevin · 1 comment
kevin commented 2026-08-10 17:35:24 +02:00
Owner
Copy Link
Copy Source

Found by the 2026-08-10 adversarial design review of CB-308 / wiki chapter 10. The fix is a prerequisite for CB-308 (its §7.9 decision) but stands on its own for v1.0.x: today's durability claim is weaker than documented.

Problem

AmqpReplyInbox.publish is fire-and-forget: default exchange, no mandatory flag, no publisher confirms, no return listener. Consequences today:

  • a publish to a queue that was never declared is a silent black hole — no error, no DLQ, nothing;
  • a publish accepted by a broker that crashes before persisting is lost despite deliveryMode(2) — "durable" is only true after a confirm;
  • (cross-host later) the CB-308 "remote fails fast" promise is unenforceable without this.

Fix

  • Publish on a channel separate from the consume/ack channel — synchronous confirms on the single shared channelLock-guarded channel would hold the lock across a broker round trip and serialize acks.
  • Enable publisher confirms (async listener acceptable), set mandatory=true, register a return listener.
  • Ordering caveat to encode, not just document: a return (unroutable) arrives before its confirm — "confirmed" ≠ "routed"; check the returned-set at confirm time.
  • Surface an unroutable/unconfirmed publish as an error to the caller path (today that is MessageService.reply holding a stranded reply — it must not report success for a black-holed publish).

Acceptance

  • Contract test: publish to a nonexistent queue with mandatory → return listener fires, the call reports failure (not silent success).
  • Contract test: normal publish → confirm received, delivery unchanged.
  • No throughput regression on the ack path (acks never wait on a publish confirm).
  • mvn clean install green.

🤖 Generated with Claude Code

https://claude.ai/code/session_013ZGgxLQ2VpwZhEYoru8rkf

Found by the 2026-08-10 adversarial design review of CB-308 / wiki chapter 10. The fix is a prerequisite for CB-308 (its §7.9 decision) but stands on its own for v1.0.x: today's durability claim is weaker than documented. ## Problem `AmqpReplyInbox.publish` is fire-and-forget: default exchange, no `mandatory` flag, no publisher confirms, no return listener. Consequences today: - a publish to a queue that was never declared is a **silent black hole** — no error, no DLQ, nothing; - a publish accepted by a broker that crashes before persisting is lost despite `deliveryMode(2)` — "durable" is only true after a confirm; - (cross-host later) the CB-308 "remote fails fast" promise is unenforceable without this. ## Fix - Publish on a **channel separate from the consume/ack channel** — synchronous confirms on the single shared `channelLock`-guarded channel would hold the lock across a broker round trip and serialize acks. - Enable publisher confirms (async listener acceptable), set `mandatory=true`, register a return listener. - **Ordering caveat to encode, not just document:** a *return* (unroutable) arrives **before** its confirm — "confirmed" ≠ "routed"; check the returned-set at confirm time. - Surface an unroutable/unconfirmed publish as an error to the caller path (today that is `MessageService.reply` holding a stranded reply — it must not report success for a black-holed publish). ## Acceptance - Contract test: publish to a nonexistent queue with `mandatory` → return listener fires, the call reports failure (not silent success). - Contract test: normal publish → confirm received, delivery unchanged. - No throughput regression on the ack path (acks never wait on a publish confirm). - `mvn clean install` green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013ZGgxLQ2VpwZhEYoru8rkf
ltms added this to the 1.1 — single-host close-out milestone 2026-08-16 16:49:37 +02:00
ltms commented 2026-08-16 17:31:20 +02:00
Owner
Copy Link
Copy Source

Delivered and merged to main as part of #88 (which carried #83), and it took two rounds.

Round one (#83). Publishing moved to a dedicated confirm-mode channel, separate from the consume/ack channel, with mandatory=true and a return listener. An unroutable or unconfirmed publish now throws instead of silently vanishing. Because a broker Return always arrives before its matching Confirm, the confirm callback checks a per-message returned flag rather than trusting an ack alone. The multiple flag is handled by failing the whole covered range via headMap(seq, true).

Round two (#88), from the review. failPendingPublishesOnRecovery swept the pending maps without holding publishChannelLock, while publish() holds that lock across the sequence number, the map insert, and basicPublish. So a publish issued on the already-recovered channel could be caught by the still-running sweep and completed exceptionally — a successful publish reported as failed.

That is the exact inversion of what this ticket exists to prevent, and it is worse than a stray error, because MessageService.reply (MessageService.java:276) mints a fresh UUID.randomUUID() per call. A retry therefore carries a new msgId, so dedup-by-msgId cannot catch it and the reply lands twice.

Fixed by taking the same lock. No deadlock: publish() awaits its confirm outside the lock, so the sweep can only ever wait for an in-flight basicPublish to return, never for a broker round trip. close() now also fails in-flight publishes promptly instead of letting them wait out the full 10s CONFIRM_TIMEOUT_MS.

The implementer proved the regression test by reverting the guard and running it 8 times — it failed 7/8, with the surviving run simply missing the timing window. That is the right way to show a race test is not vacuous.

Verified by the lead: mvn -f bridged/pom.xml clean install unpiped → Tests run: 809 — BUILD SUCCESS; and mvn test -Pcontract -Dtest=AmqpReplyInboxContractTest against a real broker → Tests run: 8 — BUILD SUCCESS.

One thing this round did not fix, deliberately: if the same msgId were published twice while the first is still in flight, the second insert into pendingByMsgId would overwrite the first and misdirect the onReturn lookup. Not reachable today, because the only production caller generates a fresh UUID per publish. The assumption is now recorded as a comment on the field rather than guarded — a guard for an unreachable case is unearned complexity.

Delivered and merged to `main` as part of #88 (which carried #83), and it took two rounds. **Round one (#83).** Publishing moved to a dedicated confirm-mode channel, separate from the consume/ack channel, with `mandatory=true` and a return listener. An unroutable or unconfirmed publish now throws instead of silently vanishing. Because a broker `Return` always arrives before its matching `Confirm`, the confirm callback checks a per-message `returned` flag rather than trusting an ack alone. The `multiple` flag is handled by failing the whole covered range via `headMap(seq, true)`. **Round two (#88), from the review.** `failPendingPublishesOnRecovery` swept the pending maps without holding `publishChannelLock`, while `publish()` holds that lock across the sequence number, the map insert, and `basicPublish`. So a publish issued on the *already-recovered* channel could be caught by the still-running sweep and completed exceptionally — a successful publish reported as failed. That is the exact inversion of what this ticket exists to prevent, and it is worse than a stray error, because `MessageService.reply` (`MessageService.java:276`) mints a fresh `UUID.randomUUID()` per call. A retry therefore carries a **new `msgId`**, so dedup-by-`msgId` cannot catch it and the reply lands twice. Fixed by taking the same lock. No deadlock: `publish()` awaits its confirm *outside* the lock, so the sweep can only ever wait for an in-flight `basicPublish` to return, never for a broker round trip. `close()` now also fails in-flight publishes promptly instead of letting them wait out the full 10s `CONFIRM_TIMEOUT_MS`. The implementer proved the regression test by reverting the guard and running it 8 times — it failed 7/8, with the surviving run simply missing the timing window. That is the right way to show a race test is not vacuous. Verified by the lead: `mvn -f bridged/pom.xml clean install` unpiped → Tests run: 809 — BUILD SUCCESS; and `mvn test -Pcontract -Dtest=AmqpReplyInboxContractTest` against a real broker → Tests run: 8 — BUILD SUCCESS. One thing this round did **not** fix, deliberately: if the same `msgId` were published twice while the first is still in flight, the second insert into `pendingByMsgId` would overwrite the first and misdirect the `onReturn` lookup. Not reachable today, because the only production caller generates a fresh UUID per publish. The assumption is now recorded as a comment on the field rather than guarded — a guard for an unreachable case is unearned complexity.
ltms closed this issue 2026-08-16 17:31:24 +02:00
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 1.1 — single-host close-out
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#11
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?