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

CB-598: work that arrives during a backoff is treated as stale backlog, so at-cap work is abandoned without ever being nudged once #87

New Issue
Closed
opened 2026-08-16 17:18:51 +02:00 by ltms · 1 comment
ltms commented 2026-08-16 17:18:51 +02:00
Owner
Copy Link
Copy Source

Found by a reviewer on PR #84 (CB-590), verified by the lead against the code. Pre-existing — this shape is already on main in the ticket-only loop (ReplyPushLoop.stopOrRestartTicketLoop / ticketTick), so it is not a defect in that PR and was deliberately kept out of its fix round. Filed here so it is not lost by being out of scope.

The mechanism

tick() takes its repliesBefore / ticketsBefore snapshots at the start of the tick, before decide() runs:

private void tick(String lead, int reminderCount) {
    Set<String> repliesBefore = pendingReplyTargetsFor(lead);
    Set<String> ticketsBefore = pendingTicketIdsFor(lead);
    var action = decide(lead, reminderCount);
    ...
    case STOP -> stopOrRestart(lead, repliesBefore, ticketsBefore);
}

stopOrRestart restarts only for work absent from those snapshots. The reasoning is sound and its javadoc explains it well: at-cap work is expected to still be sitting there, and restarting on any non-empty pending set would nudge forever and defeat the bound.

But the window it treats as "the race" is only decide() → activeLeads.remove, a matter of microseconds. The snapshot is taken much earlier than that. Anything that arrives during the preceding backoff — 15 seconds by default (push_backoff_ms) — is already in the snapshot by the time the tick fires, and is therefore classified as stale backlog rather than new work.

Why that is wrong

Stale backlog has been named in a nudge already. Work that arrived during the backoff has not been named in anything — it did not exist when the earlier nudges were sent. The code cannot currently tell those two apart, and it treats the second as the first.

So when a tick both (a) finds the reminder cap exhausted and (b) has work that arrived during the last backoff, the schedule stops and that work is orphaned in pendingReplies / pendingTickets with nothing live to tick it. It is resurrected only by an unrelated later trigger for the same lead. There is no bound on when that happens, or whether it does.

Knock-on effect

isActive() reads the active-schedule map, so once the schedule dies with work orphaned it returns false while real work is still pending — contradicting its own javadoc. LeadHeartbeatLoop.tick() feeds that straight into its decision, so CB-551's heartbeat stops standing aside for that lead and sends its own unrelated idle ping. That does not deliver the orphaned nudge; it just confirms the "no schedule" state is real.

The shape of a fix

The cap should bound reminders about a given item, not ticks for a lead. An item that has never been named in any nudge should get at least one, whatever the schedule's tick count.

That suggests tracking, per pending item, whether it has been named yet — and letting STOP retire the schedule only once every pending item has been nudged at least once. The bound stays finite: each item can extend the schedule only until its own first nudge.

Do not simply restart on any non-empty pending set. That is the failure mode the current javadoc correctly warns about, and two existing tests (successfulTicketNudgeIncrementsDelivered, ticketNudgesSendUpToCapThenStop) will catch it.

Acceptance criteria

  1. Work that arrives during a backoff and finds the cap exhausted is still nudged at least once.
  2. Work that has already been nudged and remains undrained is still subject to the cap — it must not be nudged forever. Both existing cap tests stay green unmodified.
  3. isActive() is true whenever work is pending for that lead.
  4. A test drives the ~15s-window case from the reviewer's trace and fails without the fix. Say what that failure looked like.
  5. No spin: the schedule must still terminate when there is nothing new to say.

Dependencies

Land after the CB-590 fix round (per-source reminder budgets), which is in flight against PR #84. That round deliberately does not touch this. Rebase onto it rather than working around it.

Credit

Both this and the CB-590 regression came from a reviewer briefed only from the diff and this class's ticket history, with no access to the implementer's reasoning. The history in the brief is what made it look hard enough to find these.

Found by a reviewer on PR #84 (CB-590), verified by the lead against the code. **Pre-existing** — this shape is already on `main` in the ticket-only loop (`ReplyPushLoop.stopOrRestartTicketLoop` / `ticketTick`), so it is not a defect in that PR and was deliberately kept out of its fix round. Filed here so it is not lost by being out of scope. ## The mechanism `tick()` takes its `repliesBefore` / `ticketsBefore` snapshots at the **start of the tick**, before `decide()` runs: ```java private void tick(String lead, int reminderCount) { Set<String> repliesBefore = pendingReplyTargetsFor(lead); Set<String> ticketsBefore = pendingTicketIdsFor(lead); var action = decide(lead, reminderCount); ... case STOP -> stopOrRestart(lead, repliesBefore, ticketsBefore); } ``` `stopOrRestart` restarts only for work **absent** from those snapshots. The reasoning is sound and its javadoc explains it well: at-cap work is *expected* to still be sitting there, and restarting on any non-empty pending set would nudge forever and defeat the bound. But the window it treats as "the race" is only `decide() → activeLeads.remove`, a matter of microseconds. The snapshot is taken much earlier than that. **Anything that arrives during the preceding backoff — 15 seconds by default (`push_backoff_ms`) — is already in the snapshot by the time the tick fires**, and is therefore classified as stale backlog rather than new work. ## Why that is wrong Stale backlog has been named in a nudge already. Work that arrived during the backoff **has not been named in anything** — it did not exist when the earlier nudges were sent. The code cannot currently tell those two apart, and it treats the second as the first. So when a tick both (a) finds the reminder cap exhausted and (b) has work that arrived during the last backoff, the schedule stops and that work is orphaned in `pendingReplies` / `pendingTickets` with nothing live to tick it. It is resurrected only by an **unrelated** later trigger for the same lead. There is no bound on when that happens, or whether it does. ## Knock-on effect `isActive()` reads the active-schedule map, so once the schedule dies with work orphaned it returns **false while real work is still pending** — contradicting its own javadoc. `LeadHeartbeatLoop.tick()` feeds that straight into its decision, so CB-551's heartbeat stops standing aside for that lead and sends its own unrelated idle ping. That does not deliver the orphaned nudge; it just confirms the "no schedule" state is real. ## The shape of a fix The cap should bound **reminders about a given item**, not ticks for a lead. An item that has never been named in any nudge should get at least one, whatever the schedule's tick count. That suggests tracking, per pending item, whether it has been named yet — and letting `STOP` retire the schedule only once every pending item has been nudged at least once. The bound stays finite: each item can extend the schedule only until its own first nudge. Do **not** simply restart on any non-empty pending set. That is the failure mode the current javadoc correctly warns about, and two existing tests (`successfulTicketNudgeIncrementsDelivered`, `ticketNudgesSendUpToCapThenStop`) will catch it. ## Acceptance criteria 1. Work that arrives during a backoff and finds the cap exhausted is still nudged at least once. 2. Work that has already been nudged and remains undrained is still subject to the cap — it must not be nudged forever. Both existing cap tests stay green **unmodified**. 3. `isActive()` is true whenever work is pending for that lead. 4. A test drives the ~15s-window case from the reviewer's trace and **fails without the fix**. Say what that failure looked like. 5. No spin: the schedule must still terminate when there is nothing new to say. ## Dependencies Land **after** the CB-590 fix round (per-source reminder budgets), which is in flight against PR #84. That round deliberately does not touch this. Rebase onto it rather than working around it. ## Credit Both this and the CB-590 regression came from a reviewer briefed only from the diff and this class's ticket history, with no access to the implementer's reasoning. The history in the brief is what made it look hard enough to find these.
ltms added this to the 1.1 — single-host close-out milestone 2026-08-16 17:18:51 +02:00
ltms closed this issue 2026-08-16 18:13:30 +02:00
ltms commented 2026-08-16 18:13:43 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 7a120b3 (PR #94). I held this until CB-601 (#95) landed so main was not merged onto while red; that is now done.

What I checked myself:

  • Read the diff, not just the build. The set bumped in bumpNudgeCounts is exactly the set formatNudge names, and the "everything drained" early return returns before the bump, so nothing is charged for a nudge that was never built. decide() is untouched, so the existing cap tests still guard the cap.
  • Trial-merged onto the current main and built: Tests run: 826, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, unpiped.

One thing worth writing down about the design. With two targets — an old one at the cap and a fresh one at 0 — the minimum keeps the source eligible, so the old target is named again and its count climbs past the cap. That is deliberate and I think correct: the lead is being told about work that really is still outstanding, and the extra bumps are inert because decide() only reads the minimum. It does mean the cap bounds nudges per item, not per lead. That is the behaviour we want, but it is not what the config key's name suggests, so it should be said out loud somewhere an operator reads.

Found while verifying this, not caused by it: the first trial merge failed with a ConcurrentModificationException out of FakeHerdr.called(). FakeHerdr.calls was a plain ArrayList, written by the push-loop scheduler thread while a test streams it from the test thread. The race is on main today; CB-598 only nudges more often and so exposes it. Fixed separately as CB-603 (863d477) by making the list a CopyOnWriteArrayList.

Merged as 7a120b3 (PR #94). I held this until CB-601 (#95) landed so `main` was not merged onto while red; that is now done. What I checked myself: - **Read the diff, not just the build.** The set bumped in `bumpNudgeCounts` is exactly the set `formatNudge` names, and the "everything drained" early return returns before the bump, so nothing is charged for a nudge that was never built. `decide()` is untouched, so the existing cap tests still guard the cap. - **Trial-merged onto the current `main` and built**: `Tests run: 826, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, unpiped. One thing worth writing down about the design. With two targets — an old one at the cap and a fresh one at 0 — the minimum keeps the source eligible, so the old target is named again and its count climbs past the cap. That is deliberate and I think correct: the lead is being told about work that really is still outstanding, and the extra bumps are inert because `decide()` only reads the minimum. It does mean the cap bounds nudges *per item*, not *per lead*. That is the behaviour we want, but it is not what the config key's name suggests, so it should be said out loud somewhere an operator reads. **Found while verifying this, not caused by it:** the first trial merge failed with a `ConcurrentModificationException` out of `FakeHerdr.called()`. `FakeHerdr.calls` was a plain `ArrayList`, written by the push-loop scheduler thread while a test streams it from the test thread. The race is on `main` today; CB-598 only nudges more often and so exposes it. Fixed separately as CB-603 (863d477) by making the list a `CopyOnWriteArrayList`.
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#87
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?