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

CB-590: the two ReplyPushLoop schedules do not gate each other, so one lead pane can get two injections #75

New Issue
Closed
opened 2026-08-15 16:45:29 +02:00 by ltms · 1 comment
ltms commented 2026-08-15 16:45:29 +02:00
Owner
Copy Link
Copy Source

Found by a reviewer during the CB-588 review (PR #73). Split out deliberately — fixing it means
restructuring both loops, which does not belong in that PR.

What

ReplyPushLoop now has two independent reminder schedules:

Loop Keyed by Started from Tracked in
CB-307 reply nudge worker target onReplyQueued (durable-inbox, no-waiter path) activeTargets
CB-588 ticket nudge lead terminal onTicketTerminal (async ticket went terminal) activeLeads

Both end in agents.send(<lead terminal>, nudge) — the same pane. Neither checks the other's active
set. isActive() ORs them, so the CB-551 idle-lead heartbeat correctly stands aside for both, but the
two loops never stand aside for each other.

This is the hazard isActive()'s own javadoc names: "a concurrent heartbeat injection would start a
second competing turn in the same pane — racing loops multiply turns and context burn." CB-551 fixed
that for the heartbeat. CB-588 then added a second injector without extending the same protection.

How bad, honestly

Narrower than it first looks. Both loops gate on status.injectable() before injecting. Once the
first loop injects, the pane goes BUSY, so the second sees BUSY and returns WAIT_BUSY. The double
injection needs both loops to read the status in the same window. So this is a race, not routine
behaviour — but it is a race whose cost is a lead's turn being interrupted, which is the expensive
kind.

Acceptance criteria

  1. Two nudge injections into the same lead pane cannot overlap, whichever loops they come from.
  2. Whatever gating is added, a nudge must never be dropped because the other loop held the lead —
    deferred is fine, lost is not. CB-588's whole point was a lost nudge.
  3. A test drives both entry points at the same lead and proves criterion 1, and a second test proves
    criterion 2 (the deferred nudge still arrives).
  4. isActive() keeps its CB-551 contract: the heartbeat still stands aside for either loop.
  5. No spin: the two loops must not be able to hand the lead back and forth without progress.

Suggested shape

One schedule per lead that drains both kinds of pending work, rather than two schedules that must
avoid each other. That collapses the problem instead of adding a lock, and it makes the coalescing
that CB-588 already does for tickets apply across both sources.

Found by a reviewer during the CB-588 review (PR #73). Split out deliberately — fixing it means restructuring both loops, which does not belong in that PR. ## What `ReplyPushLoop` now has two independent reminder schedules: | Loop | Keyed by | Started from | Tracked in | |---|---|---|---| | CB-307 reply nudge | worker target | `onReplyQueued` (durable-inbox, no-waiter path) | `activeTargets` | | CB-588 ticket nudge | lead terminal | `onTicketTerminal` (async ticket went terminal) | `activeLeads` | Both end in `agents.send(<lead terminal>, nudge)` — the same pane. Neither checks the other's active set. `isActive()` ORs them, so the CB-551 idle-lead heartbeat correctly stands aside for both, but the two loops never stand aside for **each other**. This is the hazard `isActive()`'s own javadoc names: *"a concurrent heartbeat injection would start a second competing turn in the same pane — racing loops multiply turns and context burn."* CB-551 fixed that for the heartbeat. CB-588 then added a second injector without extending the same protection. ## How bad, honestly Narrower than it first looks. Both loops gate on `status.injectable()` before injecting. Once the first loop injects, the pane goes BUSY, so the second sees BUSY and returns `WAIT_BUSY`. The double injection needs both loops to read the status in the same window. So this is a race, not routine behaviour — but it is a race whose cost is a lead's turn being interrupted, which is the expensive kind. ## Acceptance criteria 1. Two nudge injections into the same lead pane cannot overlap, whichever loops they come from. 2. Whatever gating is added, a nudge must never be **dropped** because the other loop held the lead — deferred is fine, lost is not. CB-588's whole point was a lost nudge. 3. A test drives both entry points at the same lead and proves criterion 1, and a second test proves criterion 2 (the deferred nudge still arrives). 4. `isActive()` keeps its CB-551 contract: the heartbeat still stands aside for either loop. 5. No spin: the two loops must not be able to hand the lead back and forth without progress. ## Suggested shape One schedule per lead that drains both kinds of pending work, rather than two schedules that must avoid each other. That collapses the problem instead of adding a lock, and it makes the coalescing that CB-588 already does for tickets apply across both sources.
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:39 +02:00
Author
Owner
Copy Link
Copy Source

Delivered and merged to main as part of #90 (which carried #84). Two rounds.

Round one (#84). ReplyPushLoop ran two independent reminder schedules that both ended in agents.send(<lead terminal>, nudge) on the same pane: the CB-307 reply schedule, keyed by worker target, and the CB-588 ticket schedule, keyed by lead terminal. Neither checked the other's active set, so both could inject into the same pane in the same window. They are now one schedule per lead. Each tick reads everything pending for that lead and sends at most one combined nudge. stopOrRestartTicketLoop generalised to stopOrRestart, now diffing both pending sets, so work landing in the decide→release window is not stranded — and the reply arm gained that protection, which it never had.

Round two (#90), from the review. Collapsing the schedules also collapsed the two reminder caps onto one shared counter. Before this change the sources could not touch each other's budget: replies were counted per target, tickets per lead. After it, a busy reply stream could spend all 5 reminders, and a ticket arriving afterwards would be nudged zero times — it did not exist when those five nudges were sent, so it was never named in any of them. Acceptance criterion 2 allows deferred; that is lost.

The fix keeps the single schedule and gives each source its own budget. decide() returns INJECT while either source has pending work under its own cap, and STOP only when neither does. A source riding along in the combined message while already at its own cap is not charged again. stopOrRestart and its snapshot-diff logic are byte-identical — the split lives entirely in decide() and tick().

Verified by the lead: mvn -f bridged/pom.xml clean install unpiped, exit code captured — Tests run: 814, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. ReplyPushLoopTest: 41 tests.

Two notes for the record.

The regression test was proved by simulating the old shared counter (Math.max of the two) and watching it fail with expected: <INJECT> but was: <STOP>. A cap test that passes with and without the fix proves nothing, and this class has a history of exactly that.

Known and out of scope: an item arriving during the 15s backoff is already inside the tick's "before" snapshot, so at-cap work can be abandoned rather than nudged once. That shape predates all of this in the ticket-only loop and is filed as #87 (CB-598).

Delivered and merged to `main` as part of #90 (which carried #84). Two rounds. **Round one (#84).** `ReplyPushLoop` ran two independent reminder schedules that both ended in `agents.send(<lead terminal>, nudge)` on the same pane: the CB-307 reply schedule, keyed by worker target, and the CB-588 ticket schedule, keyed by lead terminal. Neither checked the other's active set, so both could inject into the same pane in the same window. They are now one schedule per lead. Each tick reads everything pending for that lead and sends at most one combined nudge. `stopOrRestartTicketLoop` generalised to `stopOrRestart`, now diffing both pending sets, so work landing in the decide→release window is not stranded — and the reply arm gained that protection, which it never had. **Round two (#90), from the review.** Collapsing the schedules also collapsed the two reminder caps onto one shared counter. Before this change the sources could not touch each other's budget: replies were counted per target, tickets per lead. After it, a busy reply stream could spend all 5 reminders, and a ticket arriving afterwards would be nudged zero times — it did not exist when those five nudges were sent, so it was never named in any of them. Acceptance criterion 2 allows *deferred*; that is *lost*. The fix keeps the single schedule and gives each source its own budget. `decide()` returns INJECT while either source has pending work under its own cap, and STOP only when neither does. A source riding along in the combined message while already at its own cap is not charged again. `stopOrRestart` and its snapshot-diff logic are byte-identical — the split lives entirely in `decide()` and `tick()`. Verified by the lead: `mvn -f bridged/pom.xml clean install` unpiped, exit code captured — Tests run: 814, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. `ReplyPushLoopTest`: 41 tests. Two notes for the record. The regression test was proved by simulating the old shared counter (`Math.max` of the two) and watching it fail with `expected: <INJECT> but was: <STOP>`. A cap test that passes with and without the fix proves nothing, and this class has a history of exactly that. Known and out of scope: an item arriving during the 15s backoff is already inside the tick's "before" snapshot, so at-cap work can be abandoned rather than nudged once. That shape predates all of this in the ticket-only loop and is filed as #87 (CB-598).
ltms closed this issue 2026-08-16 17:31:44 +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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

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