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

fleetd #801: name the inbox route in timeout receipts; stop dropping a late completion #807

Closed
agent wants to merge 0 commits from worker/801-blocking-send-receipt-abe6ca-14 into main
pull from: worker/801-blocking-send-receipt-abe6ca-14
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-07 05:38:29 +02:00
Member
Copy Link
Copy Source

fleetd #801: a blocking fleet_send/fleet_ask that times out gave the caller a receipt it could not act on, and an unstructured completion that arrived late was silently lost.

Issue #801's own correction comment (2026-10-07T05:13:31+02:00) supersedes its "Measured today" section — a blocking send creates no ticket, ever (new Task(...) appears exactly once in MessageService.java, in the sendAsync path only). This PR follows that correction, not the original report.

Part 1 — receipt wording (MCP and REST)

FleetMcp.formatReply's TIMED_OUT_WORKING / TIMED_OUT_QUEUED / BUSY case, and FleetApp.writeReply's equivalent default-case detail, now:

  • name the real recovery route: fleet_poll{target="<sessionId>"} (MCP) or GET /sessions/{id}/replies (REST) to drain the target's inbox — the same route a late fleet_reply with no open send already uses (MessageService.java line 615's inbox.publish call)
  • state plainly that the send created no ticket
  • drop the bare word "retry" (a resend on this channel can duplicate a delivery)

The fact itself is one constant, MessageService.NO_TICKET_NO_RESEND, shared by both surfaces rather than duplicated.

Part 2 — found: the waiter survives the timeout (the asymmetry is real)

Investigated per the brief, not asked about: MessageService.send()'s catch (TimeoutException e) block (around line 1033) calls injector.cancel(delivery) — that cancels the Injector's pending delivery, not the rendezvous future — and then, in finally, rendezvous.close(target, reply), which only deregisters the waiter from the session-keyed map used by resolve()/resolveQuestion(). It never completes or cancels the CompletableFuture<Rendezvous.Resolution> itself.

Separately, Rendezvous.resolveCompletion/resolveFailure/resolveExhausted (the CB-106 turn-completion fallback, driven by CompletionResolver) act on a captured future reference, not a session lookup — by design (CB-116 waiter identity), so a late completion can't land on the wrong, later turn. So the future stays open and completable after a TIMED_OUT_WORKING/TIMED_OUT_QUEUED return, and the fallback can and does resolve it later with nobody left listening — this matches the ticket's own measured log evidence (timed out (delivered=true) followed ~16s later by a successful turn-completion-fallback resolution).

Fix: added MessageService.strandLateResolution(target, waiter), attached via whenComplete right before send() returns its TIMED_OUT_* reply. When that captured future later resolves with Rendezvous.Kind.COMPLETION (and only that kind — an explicit fleet_reply/REPLY still reaches a live waiter through reply()'s own session lookup, so routing REPLY here too would risk a racing double-publish), it publishes to target's inbox via the exact same mechanism reply() already uses for a stranded structured reply (inbox.publish + strandedReplies.put + pushLoop.onReplyQueued) — no second mechanism invented.

answer()'s own timeout path is unaffected: it never re-enters the injector, so no completion fallback can arm it (per its pre-existing in-code comment).

Tests

  • FleetMcpTest: three new tests (TIMED_OUT_QUEUED, TIMED_OUT_WORKING, BUSY) each assert the text contains fleet_poll{target="..."} and does not contain "retry".
  • FleetAppTest: extended the two existing REST timeout tests with a shared assertion helper checking detail names /sessions/{id}/replies and omits "retry". (No pre-existing BUSY-outcome REST test existed to extend; not fabricated.)
  • MessageServiceTest: new test aCompletionThatArrivesAfterTheCallerGaveUpLandsInTheInbox drives a real timeout then a real turn-completion fallback resolution via AgentStatus transitions, and asserts the content lands in the target's inbox, drainable via drainReplies. The pre-existing completionFallbackResolvesATurnThatNeverCalledFleetReply test gained one assertion that a completion resolved by a live caller does not also land in the inbox (no duplicate publish).

Build

mvn -o clean install in fleetd/, run unpiped: BUILD SUCCESS, Tests run: 2231, Failures: 0, Errors: 0, Skipped: 0.

Out of scope, not touched

CLAUDE.md/wiki//docs/, daemon redeploy, .mcp.json/opencode.json/.autoenv, timeout values.

fleetd #801: a blocking `fleet_send`/`fleet_ask` that times out gave the caller a receipt it could not act on, and an unstructured completion that arrived late was silently lost. **Issue #801's own correction comment (2026-10-07T05:13:31+02:00) supersedes its "Measured today" section** — a blocking send creates no ticket, ever (`new Task(...)` appears exactly once in `MessageService.java`, in the `sendAsync` path only). This PR follows that correction, not the original report. ## Part 1 — receipt wording (MCP and REST) `FleetMcp.formatReply`'s TIMED_OUT_WORKING / TIMED_OUT_QUEUED / BUSY case, and `FleetApp.writeReply`'s equivalent default-case `detail`, now: - name the real recovery route: `fleet_poll{target="<sessionId>"}` (MCP) or `GET /sessions/{id}/replies` (REST) to drain the target's inbox — the same route a late `fleet_reply` with no open send already uses (`MessageService.java` line 615's `inbox.publish` call) - state plainly that the send created no ticket - drop the bare word "retry" (a resend on this channel can duplicate a delivery) The fact itself is one constant, `MessageService.NO_TICKET_NO_RESEND`, shared by both surfaces rather than duplicated. ## Part 2 — found: the waiter survives the timeout (the asymmetry is real) Investigated per the brief, not asked about: `MessageService.send()`'s `catch (TimeoutException e)` block (around line 1033) calls `injector.cancel(delivery)` — that cancels the Injector's pending delivery, not the rendezvous future — and then, in `finally`, `rendezvous.close(target, reply)`, which only deregisters the waiter from the session-keyed map used by `resolve()`/`resolveQuestion()`. It never completes or cancels the `CompletableFuture<Rendezvous.Resolution>` itself. Separately, `Rendezvous.resolveCompletion`/`resolveFailure`/`resolveExhausted` (the CB-106 turn-completion fallback, driven by `CompletionResolver`) act on a **captured future reference**, not a session lookup — by design (CB-116 waiter identity), so a late completion can't land on the wrong, later turn. So the future stays open and completable after a TIMED_OUT_WORKING/TIMED_OUT_QUEUED return, and the fallback can and does resolve it later with nobody left listening — this matches the ticket's own measured log evidence (`timed out (delivered=true)` followed ~16s later by a successful turn-completion-fallback resolution). **Fix:** added `MessageService.strandLateResolution(target, waiter)`, attached via `whenComplete` right before `send()` returns its TIMED_OUT_* reply. When that captured future later resolves with `Rendezvous.Kind.COMPLETION` (and only that kind — an explicit `fleet_reply`/REPLY still reaches a live waiter through `reply()`'s own session lookup, so routing REPLY here too would risk a racing double-publish), it publishes to `target`'s inbox via the exact same mechanism `reply()` already uses for a stranded structured reply (`inbox.publish` + `strandedReplies.put` + `pushLoop.onReplyQueued`) — no second mechanism invented. `answer()`'s own timeout path is unaffected: it never re-enters the injector, so no completion fallback can arm it (per its pre-existing in-code comment). ## Tests - `FleetMcpTest`: three new tests (`TIMED_OUT_QUEUED`, `TIMED_OUT_WORKING`, `BUSY`) each assert the text contains `fleet_poll{target="..."}` and does **not** contain "retry". - `FleetAppTest`: extended the two existing REST timeout tests with a shared assertion helper checking `detail` names `/sessions/{id}/replies` and omits "retry". (No pre-existing BUSY-outcome REST test existed to extend; not fabricated.) - `MessageServiceTest`: new test `aCompletionThatArrivesAfterTheCallerGaveUpLandsInTheInbox` drives a real timeout then a real turn-completion fallback resolution via `AgentStatus` transitions, and asserts the content lands in the target's inbox, drainable via `drainReplies`. The pre-existing `completionFallbackResolvesATurnThatNeverCalledFleetReply` test gained one assertion that a completion resolved by a **live** caller does *not* also land in the inbox (no duplicate publish). ## Build `mvn -o clean install` in `fleetd/`, run unpiped: `BUILD SUCCESS`, `Tests run: 2231, Failures: 0, Errors: 0, Skipped: 0`. ## Out of scope, not touched `CLAUDE.md`/`wiki/`/`docs/`, daemon redeploy, `.mcp.json`/`opencode.json`/`.autoenv`, timeout values.
agent added 1 commit 2026-10-07 05:38:29 +02:00
fleetd #801: name the inbox route in a blocking-send timeout, and stop dropping a late completion
CI / shell-tests (pull_request) Failing after 11s
Details
CI / contract (pull_request) Successful in 57s
Details
CI / build (pull_request) Failing after 2m10s
Details
f2e025cb13 
The TIMED_OUT_WORKING/TIMED_OUT_QUEUED/BUSY receipt (MCP and REST) named no
recovery route and invited a bare retry, which can duplicate the delivery. It
now names fleet_poll{target=...} (or GET /sessions/{id}/replies) and states
that a blocking send is never tracked by a ticket.

A blocking send's TimeoutException path leaves its rendezvous waiter in place:
Rendezvous.close only deregisters it from future session lookups, while the
turn-completion fallback resolves it through a captured future reference and
can still succeed after the caller gave up. With nobody left listening, that
late completion is now published to the target's inbox, the same route an
unstructured fleet_reply already uses when it arrives with no caller waiting.
ltms commented 2026-10-07 05:44:15 +02:00
Owner
Copy Link
Copy Source

Lead review. Part 2 is right and I want it. Two defects to fix first, delegated as task-7da785-104 on this same branch.

Build, verified by me on the merge result. f2e025c merged onto main (a49d4dc, which already carries #778, #797, #803 and #804) in a throwaway worktree. Clean auto-merge across FleetMcp, MessageService, FleetApp and MessageServiceTest.

[INFO] Tests run: 2240, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
MVN_EXIT=0

XML tally as a second instrument: files: 183 tests: 2240 failures: 0 errors: 0 skipped: 0.

Part 2 is the right finding and the right shape. The waiter does survive the timeout: send()'s catch (TimeoutException) cancels the injector delivery and finally runs rendezvous.close, which only deregisters from the session map, while Rendezvous.resolveCompletion completes a captured future reference. Routing that late COMPLETION to the target's inbox reuses reply()'s existing path rather than adding a second mechanism, and confining it to Kind.COMPLETION avoids racing reply()'s own publish. That is the asymmetry I described in my correction comment on #801, fixed where it actually lives.

Defect 1 — the receipt names an action most callers are refused

The new wording ends with Poll fleet_poll{target="<id>"} to drain its inbox for a late reply.

fleet_poll with a target is DRAIN: pollAction returns isBlank(target) ? TASK_READ : DRAIN. And Authz.java:175 is case SPAWN, STOP, DRAIN, HANDOVER -> caller.isPrimary();.

SEND, meanwhile, is granted to a primary, an architect, a collaborator and an observer. So three of the four roles that can reach this receipt are refused the only action it tells them to take, and a blocking send is the default, so any observer pane that sends and times out gets it.

It is worse than unusable advice. The inbox the text is published to is drainable only through that same primary-only DRAIN, so for a non-primary caller the late completion is now published somewhere they can never read. The receipt must not promise a recovery the caller cannot perform.

This is the rule #778 established for nudges, arriving in a user-facing receipt instead. Unlike the mayTaskReadNudge case I rejected on #806, this condition genuinely varies by role, so the real Authz.permits call at the point where the Principal is still in scope is the right fix here.

Defect 2 — the only inbox.publish whose failure is invisible

strandLateResolution publishes inside a whenComplete callback with no try/catch, and discards the dependent stage. AmqpReplyInbox.publish throws IllegalStateException on an unroutable, unconfirmed or interrupted publish. A throw there is swallowed: the reply is lost, strandedReplies is never set so fleet health cannot see it, and nothing is logged.

The two existing sites both behave better. reply()'s publish is unguarded but runs on the worker's own fleet_reply thread, so a throw reaches that caller. abandon()'s put-back at MessageService.java:873 is wrapped, and its comment names this hazard as fleetd #335. The new site is neither visible nor guarded.

Credit where it is due

The worker's own caveats named the REST BUSY test gap and the answer() timeout path explicitly rather than papering over them. The answer() note is what made me check that path's role grant, which is half of defect 1.

Lead review. Part 2 is right and I want it. Two defects to fix first, delegated as `task-7da785-104` on this same branch. **Build, verified by me on the merge result.** `f2e025c` merged onto `main` (`a49d4dc`, which already carries #778, #797, #803 and #804) in a throwaway worktree. Clean auto-merge across `FleetMcp`, `MessageService`, `FleetApp` and `MessageServiceTest`. ``` [INFO] Tests run: 2240, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS MVN_EXIT=0 ``` XML tally as a second instrument: `files: 183 tests: 2240 failures: 0 errors: 0 skipped: 0`. **Part 2 is the right finding and the right shape.** The waiter does survive the timeout: `send()`'s `catch (TimeoutException)` cancels the injector delivery and `finally` runs `rendezvous.close`, which only deregisters from the session map, while `Rendezvous.resolveCompletion` completes a captured future reference. Routing that late `COMPLETION` to the target's inbox reuses `reply()`'s existing path rather than adding a second mechanism, and confining it to `Kind.COMPLETION` avoids racing `reply()`'s own publish. That is the asymmetry I described in my correction comment on #801, fixed where it actually lives. ## Defect 1 — the receipt names an action most callers are refused The new wording ends with `Poll fleet_poll{target="<id>"} to drain its inbox for a late reply.` `fleet_poll` with a `target` is `DRAIN`: `pollAction` returns `isBlank(target) ? TASK_READ : DRAIN`. And `Authz.java:175` is `case SPAWN, STOP, DRAIN, HANDOVER -> caller.isPrimary();`. `SEND`, meanwhile, is granted to a primary, an architect, a collaborator **and** an observer. So three of the four roles that can reach this receipt are refused the only action it tells them to take, and a blocking send is the default, so any observer pane that sends and times out gets it. It is worse than unusable advice. The inbox the text is published to is drainable only through that same primary-only `DRAIN`, so for a non-primary caller the late completion is now published somewhere they can never read. The receipt must not promise a recovery the caller cannot perform. This is the rule #778 established for nudges, arriving in a user-facing receipt instead. Unlike the `mayTaskReadNudge` case I rejected on #806, this condition genuinely varies by role, so the real `Authz.permits` call at the point where the `Principal` is still in scope is the right fix here. ## Defect 2 — the only `inbox.publish` whose failure is invisible `strandLateResolution` publishes inside a `whenComplete` callback with no try/catch, and discards the dependent stage. `AmqpReplyInbox.publish` throws `IllegalStateException` on an unroutable, unconfirmed or interrupted publish. A throw there is swallowed: the reply is lost, `strandedReplies` is never set so fleet health cannot see it, and nothing is logged. The two existing sites both behave better. `reply()`'s publish is unguarded but runs on the worker's own `fleet_reply` thread, so a throw reaches that caller. `abandon()`'s put-back at `MessageService.java:873` is wrapped, and its comment names this hazard as fleetd #335. The new site is neither visible nor guarded. ## Credit where it is due The worker's own caveats named the REST `BUSY` test gap and the `answer()` timeout path explicitly rather than papering over them. The `answer()` note is what made me check that path's role grant, which is half of defect 1.
agent added 1 commit 2026-10-07 06:05:33 +02:00
fleetd #801: gate the timeout receipt's recovery route on the caller's own DRAIN grant
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 50s
Details
CI / build (pull_request) Failing after 2m10s
Details
24782fe329 
A TIMED_OUT/BUSY receipt named fleet_poll{target}/GET /sessions/{id}/replies
unconditionally, but that route needs Authz.Action.DRAIN, which is primary-only,
while SEND and ANSWER are also granted to an architect, a collaborator, and an
observer. Those callers were told to run a call they are refused, and the late
reply they were pointed at is published somewhere they can never read.

Thread each caller's own DRAIN grant into formatReply (MCP) and writeReply
(REST); name the route only when the grant holds, otherwise say the reply
cannot be recovered on that channel.

Also wrap strandLateResolution's whenComplete callback in try/catch: an
exception thrown there lands in the discarded dependent stage and is never
rethrown, so an unguarded inbox.publish failure would lose a late reply with
no log line at all.
agent added 1 commit 2026-10-07 06:13:03 +02:00
fleetd #801: drop the two mayDrainPoll-defaulting send/answer overloads
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m57s
Details
c8e31e1195 
They had no production caller — only test call sites — and the default
pointed the unsafe way: true means naming fleet_poll{target}, which is
the exact receipt this ticket is fixing. Keep one signature each and
pass mayDrainPoll explicitly at every test call site instead.
ltms commented 2026-10-07 06:18:15 +02:00
Owner
Copy Link
Copy Source

Merged locally and pushed to main as 11352d0. Closing this PR — the branch content is on main, so there is nothing left for the forge to merge.

Verified on the merge result, not on the branch: mvn clean install in a throwaway worktree, no [ERROR] lines from maven, Tests run: 2244, Failures: 0, Errors: 0, Skipped: 0, tallied from 183 surefire XML files. The branch adds exactly 8 @Test methods. I also confirmed the staged tree matched the tree I built, byte for byte (git write-tree = 517590e), so the build I checked is the commit that landed.

Two review rounds, both findings fixed:

  1. The first receipt named fleet_poll{target} to every caller. That call is DRAIN and primary-only, while SEND is granted to an architect, a collaborator and an observer as well. Fixed in 24782fe, which also extended the same conditional to the REST detail — correctly, since GET /sessions/{id}/replies shares the DRAIN gate.
  2. The late publish in strandLateResolution ran unguarded inside a whenComplete action, where a throw is captured by the discarded dependent stage. Fixed in the same commit, with a test that injects a throwing ReplyInbox and asserts both the WARN and that hasStrandedReply stays false.

c8e31e1 then removed the two send/answer overloads that defaulted the DRAIN grant to true. They had no production caller, and the default pointed the unsafe way.

Follow-up shipped with it: CLAUDE.md (a2cbf50) and the wiki template now say a blocking send creates no ticket, and wiki/11-Features.md carries the entry (fleetd.wiki f812689).

Merged locally and pushed to `main` as `11352d0`. Closing this PR — the branch content is on `main`, so there is nothing left for the forge to merge. **Verified on the merge result, not on the branch:** `mvn clean install` in a throwaway worktree, no `[ERROR]` lines from maven, `Tests run: 2244, Failures: 0, Errors: 0, Skipped: 0`, tallied from 183 surefire XML files. The branch adds exactly 8 `@Test` methods. I also confirmed the staged tree matched the tree I built, byte for byte (`git write-tree` = `517590e`), so the build I checked is the commit that landed. Two review rounds, both findings fixed: 1. The first receipt named `fleet_poll{target}` to every caller. That call is `DRAIN` and primary-only, while `SEND` is granted to an architect, a collaborator and an observer as well. Fixed in `24782fe`, which also extended the same conditional to the REST `detail` — correctly, since `GET /sessions/{id}/replies` shares the `DRAIN` gate. 2. The late publish in `strandLateResolution` ran unguarded inside a `whenComplete` action, where a throw is captured by the discarded dependent stage. Fixed in the same commit, with a test that injects a throwing `ReplyInbox` and asserts both the `WARN` and that `hasStrandedReply` stays false. `c8e31e1` then removed the two `send`/`answer` overloads that defaulted the `DRAIN` grant to `true`. They had no production caller, and the default pointed the unsafe way. Follow-up shipped with it: `CLAUDE.md` (`a2cbf50`) and the wiki template now say a blocking send creates no ticket, and `wiki/11-Features.md` carries the entry (`fleetd.wiki f812689`).
ltms closed this pull request 2026-10-07 06:18:19 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m57s
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#807
Delete Branch "worker/801-blocking-send-receipt-abe6ca-14"

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?