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

CB-588: nudge the lead when an async ticket reaches a terminal phase #73

Closed
agent wants to merge 0 commits from worker/cb588-async-ticket-nudge-3218f7-5 into main
pull from: worker/cb588-async-ticket-nudge-3218f7-5
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-08-15 16:10:40 +02:00
Member
Copy Link
Copy Source

Closes gitea issue #72.

The gap: MessageService.reply returns on the rendezvous fast path before ReplyPushLoop is ever told anything, and sendAsync's send always registers a rendezvous waiter — so a wait:false ticket never nudges, the exact mode the charter tells leads to prefer.

Fix: a new ReplyPushLoop.onTicketTerminal(ticket, target, failed) entry point, reusing the loop's status gating, bounded/backoff reminders, and metrics. Keyed by the nudge-receiving lead (not the worker target) so several tickets finishing together coalesce into one nudge naming the count. MessageService wires it via task.future.whenComplete inside sendAsync — this fires on any path that completes the future (a worker reply, the CB-106 completion fallback, a CB-109 wedge, or a CB-516 abandon()), not just the common one. poll() calls the new ticketCollected(ticket) once it hands back a terminal view, so a ticket the lead already polled is never nudged again.

The CB-307 inbox path (onReplyQueued/decide/NUDGE_FORMAT) is untouched — same trigger, same text, same bound; its regression tests pass unmodified.

Follow-up fix (same PR, review caught it): tasks is the sole authority on whether a ticket exists, and pruneTerminalTickets (10-minute TICKET_TTL_NANOS) dropped entries from it directly. ticketCollected was only ever called from poll's terminal branch, which the prune short-circuits past — poll returns null at the top once a ticket is gone from tasks. So a ticket the lead never polled, or one the reminder cap already gave up on, was pruned from tasks but never collected from ReplyPushLoop.pendingTickets: it rode along on every later nudge to the same lead forever, naming a ticket bridge_poll could no longer find, and the map itself never shrank. Fixed by having pruneTerminalTickets call pushLoop.ticketCollected for every ticket it actually removes (guarded on pushLoop != null) — tasks stays the only removal trigger, no second source of truth added. Added an injectable clock (LongSupplier nowNanos, defaulting to System::nanoTime) to MessageService, mirroring the existing SessionManager/SessionReaper nowNanos seam, so the new regression test crosses the 10-minute TTL deterministically instead of sleeping for real. I confirmed the new test (aPrunedTicketIsReclaimedFromThePushLoopNotLeakedForever) fails against the prior pruneTerminalTickets — a stale ticket rides along on a later ticket's coalesced nudge (... task-2, task-1) — before restoring the fix and re-running green.

This also bounds the narrow whenComplete-vs-poll race flagged in an earlier version of this PR body: any pendingTickets entry that race could leave stale is now reclaimed by the same prune within 10 minutes, same as every other unpolled ticket. I did not add a live query from the push loop back into MessageService to close the race itself (that would need ReplyPushLoop and MessageService to depend on each other, and they're constructed in a fixed order in Bridged.java — pushLoop first, MessageService second) — the bound from the prune is judged sufficient, and no test exercises the race directly.

Tests: mvn clean install, unpiped — Tests run: 800, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS. New coverage beyond the original PR: MessageServiceTest.aPrunedTicketIsReclaimedFromThePushLoopNotLeakedForever, using an injectable clock to cross the TTL without a real 10-minute wait, and driving the real pruneTerminalTickets path (not a private-method reflection call, no assertion on internal map sizes) — it asserts on the actual nudge text sent to the lead's fake pane and on poll(staleTicket) returning null.

Round 3 fix (same PR, two independent reviewers caught it): ReplyPushLoop.onTicketTerminal raced ticketTick's STOP branch. Sequence: (1) the scheduler thread's decideTickets(lead, n) finds nothing pending (or the cap reached) and returns STOP, before activeLeads.remove(lead) has run; (2) a ticket lands for the same lead — onTicketTerminal adds it to pendingTickets, then sees activeLeads still occupied and coalesces onto the dying schedule instead of starting a new one; (3) the STOP branch then removes the lead from activeLeads. The ticket is left in pendingTickets with no schedule ever going to tick again — a lost nudge, the exact failure this ticket exists to remove.

Fixed by having the STOP branch (stopOrRestartTicketLoop) release the slot and then recheck, restarting only for a ticket that a pendingBefore snapshot — taken just before the tick's decision — did not already account for. A first version of this fix restarted on any non-empty pendingFor(lead) after release; that is wrong and I caught it only because it broke two existing tests (successfulTicketNudgeIncrementsDelivered, ticketNudgesSendUpToCapThenStop): when STOP is reached because the reminder cap was hit, the same never-collected ticket is expected to still be sitting there — that's the cap doing its job — and restarting on it nudges forever, defeating the bound (acceptance criterion #7). Diffing against the pre-decision snapshot tells a genuine race arrival apart from stale cap-exhausted backlog.

No spin risk: stopOrRestartTicketLoop restarts the schedule at most once per call, and a fresh onTicketTerminal racing the recheck itself still terminates in one of two ways — either it sees the slot already vacated by activeLeads.remove (which happens-before the recheck in program order) and claims it itself, or it lands first and the recheck then sees its ticket and reclaims the slot instead. Exactly one side wins; neither can miss the other.

Tested by driving stopOrRestartTicketLoop directly (now package-private) rather than forcing the underlying thread interleaving, which isn't reliable to force deterministically: aTicketStillPendingWhenTheLoopStopsIsNotStranded (pendingBefore=empty, a ticket present after release must restart the loop) and aStaleUncollectedTicketAtCapDoesNotRestartTheLoop (pendingBefore already contains the only pending ticket, so it must NOT restart). Both were verified honestly: I reverted the fix (once to "no recheck at all", once to the naive "restart on any pending ticket" version) and confirmed each reverted version fails the specific test it's meant to catch, before restoring the real fix and rerunning green.

Also this round: MessageServiceTest's success-path nudge test now also asserts the nudge does NOT contain "FAILED" (previously only the failure-path test pinned that direction), and the whenComplete comment in MessageService.sendAsync now lists every path that can complete task.future — finishAsyncTask(task, result) on any non-QUESTION send() outcome (a worker reply, the CB-106 fallback, a CB-109 wedge, TIMED_OUT, BUSY, BACKEND_EXHAUSTED), the same finishAsyncTask reached via answer() once a QUESTION resolves, completeExceptionally when send() throws, and a CB-516 abandon() on teardown.

Tests (round 3): mvn clean install, unpiped — Tests run: 802, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Closes gitea issue #72. **The gap:** MessageService.reply returns on the rendezvous fast path before ReplyPushLoop is ever told anything, and sendAsync's send always registers a rendezvous waiter — so a wait:false ticket never nudges, the exact mode the charter tells leads to prefer. **Fix:** a new ReplyPushLoop.onTicketTerminal(ticket, target, failed) entry point, reusing the loop's status gating, bounded/backoff reminders, and metrics. Keyed by the nudge-receiving lead (not the worker target) so several tickets finishing together coalesce into one nudge naming the count. MessageService wires it via task.future.whenComplete inside sendAsync — this fires on any path that completes the future (a worker reply, the CB-106 completion fallback, a CB-109 wedge, or a CB-516 abandon()), not just the common one. poll() calls the new ticketCollected(ticket) once it hands back a terminal view, so a ticket the lead already polled is never nudged again. The CB-307 inbox path (onReplyQueued/decide/NUDGE_FORMAT) is untouched — same trigger, same text, same bound; its regression tests pass unmodified. **Follow-up fix (same PR, review caught it):** `tasks` is the sole authority on whether a ticket exists, and `pruneTerminalTickets` (10-minute TICKET_TTL_NANOS) dropped entries from it directly. `ticketCollected` was only ever called from `poll`'s terminal branch, which the prune short-circuits past — `poll` returns null at the top once a ticket is gone from `tasks`. So a ticket the lead never polled, or one the reminder cap already gave up on, was pruned from `tasks` but never collected from `ReplyPushLoop.pendingTickets`: it rode along on every later nudge to the same lead forever, naming a ticket `bridge_poll` could no longer find, and the map itself never shrank. Fixed by having `pruneTerminalTickets` call `pushLoop.ticketCollected` for every ticket it actually removes (guarded on `pushLoop != null`) — `tasks` stays the only removal trigger, no second source of truth added. Added an injectable clock (`LongSupplier nowNanos`, defaulting to `System::nanoTime`) to `MessageService`, mirroring the existing `SessionManager`/`SessionReaper` `nowNanos` seam, so the new regression test crosses the 10-minute TTL deterministically instead of sleeping for real. I confirmed the new test (`aPrunedTicketIsReclaimedFromThePushLoopNotLeakedForever`) fails against the prior `pruneTerminalTickets` — a stale ticket rides along on a later ticket's coalesced nudge (`... task-2, task-1`) — before restoring the fix and re-running green. This also bounds the narrow `whenComplete`-vs-`poll` race flagged in an earlier version of this PR body: any `pendingTickets` entry that race could leave stale is now reclaimed by the same prune within 10 minutes, same as every other unpolled ticket. I did not add a live query from the push loop back into MessageService to close the race itself (that would need `ReplyPushLoop` and `MessageService` to depend on each other, and they're constructed in a fixed order in `Bridged.java` — `pushLoop` first, `MessageService` second) — the bound from the prune is judged sufficient, and no test exercises the race directly. **Tests:** `mvn clean install`, unpiped — Tests run: 800, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS. New coverage beyond the original PR: `MessageServiceTest.aPrunedTicketIsReclaimedFromThePushLoopNotLeakedForever`, using an injectable clock to cross the TTL without a real 10-minute wait, and driving the real `pruneTerminalTickets` path (not a private-method reflection call, no assertion on internal map sizes) — it asserts on the actual nudge text sent to the lead's fake pane and on `poll(staleTicket)` returning null. **Round 3 fix (same PR, two independent reviewers caught it):** `ReplyPushLoop.onTicketTerminal` raced `ticketTick`'s STOP branch. Sequence: (1) the scheduler thread's `decideTickets(lead, n)` finds nothing pending (or the cap reached) and returns STOP, before `activeLeads.remove(lead)` has run; (2) a ticket lands for the same lead — `onTicketTerminal` adds it to `pendingTickets`, then sees `activeLeads` still occupied and coalesces onto the dying schedule instead of starting a new one; (3) the STOP branch then removes the lead from `activeLeads`. The ticket is left in `pendingTickets` with no schedule ever going to tick again — a lost nudge, the exact failure this ticket exists to remove. Fixed by having the STOP branch (`stopOrRestartTicketLoop`) release the slot and then recheck, restarting only for a ticket that a `pendingBefore` snapshot — taken just before the tick's decision — did not already account for. A first version of this fix restarted on *any* non-empty `pendingFor(lead)` after release; that is wrong and I caught it only because it broke two existing tests (`successfulTicketNudgeIncrementsDelivered`, `ticketNudgesSendUpToCapThenStop`): when STOP is reached because the reminder cap was hit, the same never-collected ticket is expected to still be sitting there — that's the cap doing its job — and restarting on it nudges forever, defeating the bound (acceptance criterion #7). Diffing against the pre-decision snapshot tells a genuine race arrival apart from stale cap-exhausted backlog. No spin risk: `stopOrRestartTicketLoop` restarts the schedule at most once per call, and a fresh `onTicketTerminal` racing the recheck itself still terminates in one of two ways — either it sees the slot already vacated by `activeLeads.remove` (which happens-before the recheck in program order) and claims it itself, or it lands first and the recheck then sees its ticket and reclaims the slot instead. Exactly one side wins; neither can miss the other. Tested by driving `stopOrRestartTicketLoop` directly (now package-private) rather than forcing the underlying thread interleaving, which isn't reliable to force deterministically: `aTicketStillPendingWhenTheLoopStopsIsNotStranded` (pendingBefore=empty, a ticket present after release must restart the loop) and `aStaleUncollectedTicketAtCapDoesNotRestartTheLoop` (pendingBefore already contains the only pending ticket, so it must NOT restart). Both were verified honestly: I reverted the fix (once to "no recheck at all", once to the naive "restart on any pending ticket" version) and confirmed each reverted version fails the specific test it's meant to catch, before restoring the real fix and rerunning green. Also this round: `MessageServiceTest`'s success-path nudge test now also asserts the nudge does NOT contain "FAILED" (previously only the failure-path test pinned that direction), and the `whenComplete` comment in `MessageService.sendAsync` now lists every path that can complete `task.future` — `finishAsyncTask(task, result)` on any non-QUESTION `send()` outcome (a worker reply, the CB-106 fallback, a CB-109 wedge, TIMED_OUT, BUSY, BACKEND_EXHAUSTED), the same `finishAsyncTask` reached via `answer()` once a QUESTION resolves, `completeExceptionally` when `send()` throws, and a CB-516 `abandon()` on teardown. **Tests (round 3):** `mvn clean install`, unpiped — Tests run: 802, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.
agent added 1 commit 2026-08-15 16:10:41 +02:00
CB-588: nudge the lead when an async ticket reaches a terminal phase
CI / contract (pull_request) Successful in 41s
Details
CI / build (pull_request) Successful in 1m46s
Details
3d10ed385c 
An async bridge_send(wait:false) registers a rendezvous waiter, so its
reply always takes MessageService.reply's fast path and returns before
ReplyPushLoop.onReplyQueued is ever called — the exact mode the charter
tells leads to prefer never nudged.

Add ReplyPushLoop.onTicketTerminal(ticket, target, failed), a second
entry point reusing the loop's status gating, bounded/backoff reminders
and metrics, keyed by the nudge-receiving lead so several tickets
finishing together coalesce into one nudge naming the count. MessageService
wires it via task.future.whenComplete in sendAsync (covers reply,
completion fallback, wedge, and abandon() alike) and calls the new
ticketCollected(ticket) from poll() once a terminal view is handed back,
so an already-collected ticket is never nudged again. The CB-307 inbox
path (onReplyQueued/decide/NUDGE_FORMAT) is untouched.
ltms added 1 commit 2026-08-15 16:41:30 +02:00
CB-588 follow-up: reclaim a pruned ticket's pendingTickets entry too
CI / contract (pull_request) Successful in 45s
Details
CI / build (pull_request) Successful in 1m0s
Details
6ebad2a91f 
tasks is the sole authority on whether a ticket exists, but
pruneTerminalTickets dropped entries from it without telling
ReplyPushLoop. ticketCollected(ticket) was only ever called from
MessageService.poll's terminal branch, which pruneTerminalTickets
short-circuits past once a ticket is gone (poll returns null at the
top). A ticket the lead never polled — or one the reminder cap already
gave up on — was pruned from tasks but never collected in
ReplyPushLoop.pendingTickets, so it rode along on every later nudge to
the same lead forever, naming a ticket bridge_poll could no longer
find, and the map itself never shrank.

pruneTerminalTickets now calls pushLoop.ticketCollected for every
ticket it actually removes (guarded on pushLoop != null), so a pending
nudge entry lives exactly as long as its ticket is pollable. Reused
tasks as the only removal trigger rather than adding a second live
query back into MessageService — no new source of truth.

Added an injectable clock (LongSupplier nowNanos, defaulting to
System::nanoTime) to MessageService, mirroring the SessionManager/
SessionReaper nowNanos seam, so a test can cross the 10-minute
TICKET_TTL_NANOS deterministically instead of sleeping for real.
Confirmed the new regression test fails against the prior
pruneTerminalTickets (a stale ticket rides along on a later coalesced
nudge) before restoring the fix.
ltms added 1 commit 2026-08-15 17:02:10 +02:00
CB-588 round 3: close the STOP-vs-onTicketTerminal race, pin nudge polarity, fix comment
CI / build (pull_request) Successful in 1m19s
Details
CI / contract (pull_request) Successful in 1m31s
Details
ac044e7573 
- ReplyPushLoop.stopOrRestartTicketLoop: after releasing a lead's active-schedule
  slot on STOP, restart only if a ticket landed that the pre-decision snapshot
  did not already account for. A naive "restart on any pending ticket" version
  was tried first and reverted: it defeated the reminder cap by restarting
  forever on a stale, never-collected ticket (broke
  successfulTicketNudgeIncrementsDelivered and ticketNudgesSendUpToCapThenStop).
  Diffing against a pendingBefore snapshot distinguishes a genuine race arrival
  from stale cap-exhausted backlog.
- Two new ReplyPushLoopTest cases exercise stopOrRestartTicketLoop directly
  (now package-private) rather than forcing the underlying thread race:
  aTicketStillPendingWhenTheLoopStopsIsNotStranded (the race must restart) and
  aStaleUncollectedTicketAtCapDoesNotRestartTheLoop (the cap must still hold).
  Both were verified to fail against deliberately-reverted versions of the fix
  before being restored to green.
- MessageServiceTest: pin the success-path nudge test's negative direction too
  (must not contain "FAILED"), not just the failure-path test.
- MessageService: complete the whenComplete comment's list of completion paths
  (TIMED_OUT/BUSY/BACKEND_EXHAUSTED via finishAsyncTask, completeExceptionally
  on throw, and answer() -> finishAsyncTask(turnId, result)).
ltms commented 2026-08-16 16:48:33 +02:00
Owner
Copy Link
Copy Source

Closing — this is already on main, merged locally by the lead rather than through the forge, so Gitea never noticed.

5206679 Merge CB-588: nudge the lead when an async ticket goes terminal
ac044e7 CB-588 round 3: close the STOP-vs-onTicketTerminal race, pin nudge polarity, fix comment
6ebad2a CB-588 follow-up: reclaim a pruned ticket's pendingTickets entry too
3d10ed3 CB-588: nudge the lead when an async ticket reaches a terminal phase

The head sha ac044e7 is contained in main, which is why this PR reports changed_files: 0 and an empty diff. It was also dogfooded live on 2026-08-15.

Follow-up #75 (CB-590) came out of this PR's review and is open in the 1.1 milestone: the two ReplyPushLoop schedules still do not gate each other.

Worth noting as a process point — a locally merged PR stays open and its diff silently empties out. Two of these had accumulated.

Closing — this is **already on `main`**, merged locally by the lead rather than through the forge, so Gitea never noticed. ``` 5206679 Merge CB-588: nudge the lead when an async ticket goes terminal ac044e7 CB-588 round 3: close the STOP-vs-onTicketTerminal race, pin nudge polarity, fix comment 6ebad2a CB-588 follow-up: reclaim a pruned ticket's pendingTickets entry too 3d10ed3 CB-588: nudge the lead when an async ticket reaches a terminal phase ``` The head sha `ac044e7` is contained in `main`, which is why this PR reports `changed_files: 0` and an empty diff. It was also dogfooded live on 2026-08-15. Follow-up **#75 (CB-590)** came out of this PR's review and is open in the 1.1 milestone: the two `ReplyPushLoop` schedules still do not gate each other. Worth noting as a process point — a locally merged PR stays open and its diff silently empties out. Two of these had accumulated.
ltms closed this pull request 2026-08-16 16:49:20 +02:00
Some checks are pending
CI / build (pull_request) Successful in 1m19s
Details
CI / contract (pull_request) Successful in 1m31s
Details

Pull request closed

Please reopen this pull request to perform a merge.
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#73
Delete Branch "worker/cb588-async-ticket-nudge-3218f7-5"

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?