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

Second instance of the #399 shape: a test uses Phase.ASKING as a barrier for push-loop state published later #418

New Issue
Closed
opened 2026-09-10 05:03:27 +02:00 by ltms · 0 comments
ltms commented 2026-09-10 05:03:27 +02:00
Owner
Copy Link
Copy Source

Found by the fleet01 lead while verifying #399 under load. Verified here line by line before filing. Test-only defect; production is correct and must not change.

The failure

MessageServiceTest.anAskThatLeavesByThrowingStillClosesItsQuestion, the first assertion (around line 1806):

expected: <INJECT> but was: <STOP>

Note this is the assertion before the interrupt, so it has nothing to do with the throwing path the test is named for. The name misdirects.

With decide(LEAD, 0, 0, 0) and maxReminders = 5, the reminder-cap branch is unreachable, so STOP can only come from the "nothing pending for lead" branch at ReplyPushLoop.java:359-361. Confirmed by reading it: that branch fires only when all five pending-work sets are empty.

Mechanism — the same shape as #399

MessageService.ask() does three things in this order:

MessageService.java:991   markAsyncQuestion(...)         <- poll() now reports Phase.ASKING
MessageService.java:992   rendezvous.resolveQuestion(...)
MessageService.java:1006  pushLoop.onQuestionOpened(...) <- decide() now sees question work

The test's barrier is awaitTicketPhaseOn(service, ticket, Phase.ASKING), which returns after the first step. The assertion depends on the third. Under load the asker thread is descheduled in between, the barrier releases early, and decide correctly reports STOP because the question has not been published to the push loop yet.

This is exactly #399 restated: wait on the visible thing, assert on a thing published later. Phase.ASKING is not a barrier for push-loop state, because phase does not publish push-loop state.

Evidence — a discriminating control, not an argument

fleet01 ran the single method unloaded, three times:

control (unmodified)                            PASS
300 ms sleep inserted BEFORE onQuestionOpened   FAIL
the same 300 ms sleep inserted AFTER it         PASS

The third line is what makes this proof rather than correlation: the same delay on the other side of the same call is harmless, so the cause is the window, not the sleep. Their worktree was restored clean.

Production must not change

The ordering in ask() is deliberate and the existing comment says why — register the reverse waiter first, then surface the question. A real push-loop tick re-reads the state, so a live daemon self-heals within one backoff. Only a test that treats phase as a barrier for something phase does not publish can observe this. Do not reorder ask() to make a test pass.

The fix, and the snag in it

The right shape is a barrier on the push loop's own view: poll until the lead's pending question turnIds contain this turnId, then assert decide(...) == INJECT. That keeps the assertion about policy while the barrier waits for the state the policy reads.

Do not instead wait on decide(...) == INJECT. A barrier that waits for the asserted condition asserts nothing — it converts the test into a tautology that passes even if the state never becomes correct for the right reason.

The snag fleet01's proposal did not hit: pendingQuestionTurnIdsFor is private (ReplyPushLoop.java:228), not package-private. MessageServiceTest is in the same package dev.ltms.fleet.msg, but private still blocks it. So this needs a small test-visibility seam on ReplyPushLoop, in the style of MessageService.isCompletionStampedForTest added by #399 — a package-private accessor answering "is this turnId open for this lead", documented as a test seam.

That is a candidate, not an instruction. Any barrier that waits on state the push loop actually reads is acceptable, as long as it is not the asserted condition itself.

Acceptance

  1. The test's barrier waits on push-loop state, not on Phase.ASKING.
  2. Bounded: it fails with a clear message rather than hanging.
  3. Prove the barrier is load-bearing. Reproduce the original failure deterministically by inserting a bounded delay before onQuestionOpened, exactly as fleet01 did, and show the test red without your barrier and green with it. Then remove the delay and restore the source. Paste both results.
  4. Check every other use of awaitTicketPhaseOn(..., Phase.ASKING) and any other phase-as-barrier in this file for the same defect. Report what you find; fix only what is in scope here.
  5. No production behaviour change. A package-private test seam is allowed; a reordering is not.
  6. mvn clean install green, real unpiped Tests run: line.

Credit: fleet01 lead, who found it under the load regime built for #399 and isolated it with a before/after control.

Found by the fleet01 lead while verifying #399 under load. **Verified here line by line before filing.** Test-only defect; production is correct and must not change. ## The failure `MessageServiceTest.anAskThatLeavesByThrowingStillClosesItsQuestion`, the **first** assertion (around line 1806): ``` expected: <INJECT> but was: <STOP> ``` Note this is the assertion *before* the interrupt, so it has nothing to do with the throwing path the test is named for. The name misdirects. With `decide(LEAD, 0, 0, 0)` and `maxReminders = 5`, the reminder-cap branch is unreachable, so `STOP` can only come from the "nothing pending for lead" branch at `ReplyPushLoop.java:359-361`. Confirmed by reading it: that branch fires only when all five pending-work sets are empty. ## Mechanism — the same shape as #399 `MessageService.ask()` does three things in this order: ``` MessageService.java:991 markAsyncQuestion(...) <- poll() now reports Phase.ASKING MessageService.java:992 rendezvous.resolveQuestion(...) MessageService.java:1006 pushLoop.onQuestionOpened(...) <- decide() now sees question work ``` The test's barrier is `awaitTicketPhaseOn(service, ticket, Phase.ASKING)`, which returns after the **first** step. The assertion depends on the **third**. Under load the asker thread is descheduled in between, the barrier releases early, and `decide` correctly reports `STOP` because the question has not been published to the push loop yet. This is exactly #399 restated: **wait on the visible thing, assert on a thing published later.** `Phase.ASKING` is not a barrier for push-loop state, because phase does not publish push-loop state. ## Evidence — a discriminating control, not an argument fleet01 ran the single method unloaded, three times: ``` control (unmodified) PASS 300 ms sleep inserted BEFORE onQuestionOpened FAIL the same 300 ms sleep inserted AFTER it PASS ``` The third line is what makes this proof rather than correlation: the same delay on the other side of the same call is harmless, so the cause is the window, not the sleep. Their worktree was restored clean. ## Production must not change The ordering in `ask()` is deliberate and the existing comment says why — register the reverse waiter first, then surface the question. A real push-loop tick re-reads the state, so a live daemon self-heals within one backoff. Only a test that treats phase as a barrier for something phase does not publish can observe this. **Do not reorder `ask()` to make a test pass.** ## The fix, and the snag in it The right shape is a barrier on the push loop's own view: poll until the lead's pending question turnIds contain this `turnId`, then assert `decide(...) == INJECT`. That keeps the assertion about policy while the barrier waits for the state the policy reads. **Do not instead wait on `decide(...) == INJECT`.** A barrier that waits for the asserted condition asserts nothing — it converts the test into a tautology that passes even if the state never becomes correct for the right reason. **The snag fleet01's proposal did not hit:** `pendingQuestionTurnIdsFor` is `private` (`ReplyPushLoop.java:228`), not package-private. `MessageServiceTest` is in the same package `dev.ltms.fleet.msg`, but `private` still blocks it. So this needs a small test-visibility seam on `ReplyPushLoop`, in the style of `MessageService.isCompletionStampedForTest` added by #399 — a package-private accessor answering "is this turnId open for this lead", documented as a test seam. That is a candidate, not an instruction. Any barrier that waits on state the push loop actually reads is acceptable, as long as it is not the asserted condition itself. ## Acceptance 1. The test's barrier waits on push-loop state, not on `Phase.ASKING`. 2. Bounded: it fails with a clear message rather than hanging. 3. **Prove the barrier is load-bearing.** Reproduce the original failure deterministically by inserting a bounded delay before `onQuestionOpened`, exactly as fleet01 did, and show the test red without your barrier and green with it. Then remove the delay and restore the source. Paste both results. 4. Check every other use of `awaitTicketPhaseOn(..., Phase.ASKING)` and any other phase-as-barrier in this file for the same defect. **Report what you find; fix only what is in scope here.** 5. No production behaviour change. A package-private test seam is allowed; a reordering is not. 6. `mvn clean install` green, real unpiped `Tests run:` line. Credit: fleet01 lead, who found it under the load regime built for #399 and isolated it with a before/after control.
ltms closed this issue 2026-09-10 06:29:43 +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
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#418
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?