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

fleetd: make lead-rollover outcomes readable after confirm() #600

Closed
agent wants to merge 0 commits from worker/rollover-outcome-291483-2 into main
pull from: worker/rollover-outcome-291483-2
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-09-19 11:17:55 +02:00
Member
Copy Link
Copy Source

LeadRollover previously logged every post-confirm() failure only, invisible to the lead that triggered the roll. Adds a bounded (cap=200) token->outcome record written at each of the three exits in runRollover (ROLLED, TURN_NEVER_SETTLED, CLEAR_NEVER_SETTLED), a read-only LeadRollover#status(token) accessor, and a 'status' action on the fleet_handover MCP tool. confirm() remains the only path that can ever cause a /clear; status() makes no herdr call. Extends LeadRolloverTest (33->39 tests) covering the six acceptance properties from the ticket, and FleetMcpHandoverTest (8->12) for the new tool action. Full mvn clean install: Tests run 1815, Failures 0, Errors 0.

LeadRollover previously logged every post-confirm() failure only, invisible to the lead that triggered the roll. Adds a bounded (cap=200) token->outcome record written at each of the three exits in runRollover (ROLLED, TURN_NEVER_SETTLED, CLEAR_NEVER_SETTLED), a read-only LeadRollover#status(token) accessor, and a 'status' action on the fleet_handover MCP tool. confirm() remains the only path that can ever cause a /clear; status() makes no herdr call. Extends LeadRolloverTest (33->39 tests) covering the six acceptance properties from the ticket, and FleetMcpHandoverTest (8->12) for the new tool action. Full mvn clean install: Tests run 1815, Failures 0, Errors 0.
agent added 1 commit 2026-09-19 11:17:55 +02:00
fleetd: make lead-rollover outcomes readable after confirm()
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 1m31s
Details
CI / build (pull_request) Successful in 2m14s
Details
b874afb0af 
LeadRollover previously logged every post-confirm() failure only — a lead
has no way to read the daemon log, so a roll that timed out because its
own turn never settled (or /clear never re-settled) was invisible; the
lead would carry on believing a fresh session was coming.

Add a bounded (cap=200) token -> outcome record, written at each of the
three exits in runRollover (ROLLED, TURN_NEVER_SETTLED, CLEAR_NEVER_SETTLED),
and a read-only LeadRollover#status(token) accessor. The TURN_NEVER_SETTLED
detail names turnSettleSeconds explicitly so a reader knows what to raise.

Wire a "status" action onto the fleet_handover MCP tool (handler + schema);
it never schedules, cancels, or retries anything — confirm() remains the
only path that can ever cause a /clear.

Extends LeadRolloverTest (33 -> 39 tests) covering the six acceptance
properties, and FleetMcpHandoverTest (8 -> 12) for the new tool action.
ltms commented 2026-09-19 11:20:38 +02:00
Owner
Copy Link
Copy Source

Lead review — one change needed before merge

I verified your build myself in a scratch worktree: mvn -o clean install exit 0, 1815 tests, 0 failures, 0 errors, 0 skipped, 143 surefire reports. LeadRolloverTest 39, FleetMcpHandoverTest 12. Every number you reported holds. 0 banned test shapes in either touched test file. I read the whole LeadRollover.java diff.

The work is good. status() makes no agents.* call, no existing gate or control-flow line is altered, and the null-guard ahead of the ConcurrentHashMap lookup is right. The reasoning about removeEldestEntry needing external synchronization even under a thread-safe wrapper is correct and I would not have caught it.

The defect: UNKNOWN conflates two states that need opposite handling

confirm() does pending.remove(token) before continuationRunner.accept(() -> runRollover(p, cfg)). outcomes.put(...) only runs once runRollover reaches one of its three exits.

So for the whole duration of the roll, the token is in neither map, and status(token) returns UNKNOWN — documented as "never issued, cancelled, or aged out of the bounded history".

That window is the remainder of the confirming lead's own turn. It is the single most likely moment for a lead to call status: right after confirm returns accepted, to check the request registered. It is told the roll was never requested.

This matters more here than it normally would, because this PR exists to stop a lead being left with a false belief about its own roll. A caller cannot distinguish "your roll is running" from "nothing was ever requested", and the two need opposite responses: wait, versus call open again. Calling open again mid-roll is the harmful one.

grep -cE "IN_PROGRESS|inProgress|midRoll|duringRoll" on your test file returns 0, so nothing covers this window.

What to change

Add a fifth state for "approved, continuation running, not yet finished" — name it as you see fit. Record it at the moment confirm hands off, not when the roll ends, so there is no gap between pending.remove and the first outcomes.put.

Keep UNKNOWN meaning only what its javadoc says today.

Acceptance, as properties under a change

  1. Between an approved confirm and the continuation finishing, status(token) reports the in-progress state — not UNKNOWN, and not PENDING. Drive this with a continuation runner that does not run the roll immediately, so the window is observable.
  2. Once the continuation finishes, the same token reports its terminal state. The in-progress state is not sticky.
  3. All three existing terminal states still report exactly as they do now. Your six current tests must stay green unchanged.
  4. The in-progress state is distinct from all four existing states — assert it against each.
  5. Eviction still holds with the new state in play: the cap counts in-progress entries too, or, if you decide it should not, say why and test that decision.

Also, please, in the same PR

Your out-of-scope note is right and I am pulling it into scope: the class javadoc around lines 28-30 still says "Nothing in this ticket wires an MCP tool onto open/confirm/cancel — that is a separate, later unit; until it lands, nothing calls this class at all." That is false, and this PR makes it more false. Correct it.

Not a problem

Extending FleetMcpHandoverTest was not unwanted scope — you touched FleetMcp.java, so testing the wiring there was right. Keep it.

Re-read this comment before you commit. It is newer than your brief and it wins.

## Lead review — one change needed before merge I verified your build myself in a scratch worktree: `mvn -o clean install` exit 0, **1815 tests, 0 failures, 0 errors, 0 skipped, 143 surefire reports**. `LeadRolloverTest` 39, `FleetMcpHandoverTest` 12. Every number you reported holds. 0 banned test shapes in either touched test file. I read the whole `LeadRollover.java` diff. The work is good. `status()` makes no `agents.*` call, no existing gate or control-flow line is altered, and the null-guard ahead of the `ConcurrentHashMap` lookup is right. The reasoning about `removeEldestEntry` needing external synchronization even under a thread-safe wrapper is correct and I would not have caught it. ### The defect: `UNKNOWN` conflates two states that need opposite handling `confirm()` does `pending.remove(token)` **before** `continuationRunner.accept(() -> runRollover(p, cfg))`. `outcomes.put(...)` only runs once `runRollover` reaches one of its three exits. So for the whole duration of the roll, the token is in **neither** map, and `status(token)` returns `UNKNOWN` — documented as *"never issued, cancelled, or aged out of the bounded history"*. That window is the remainder of the confirming lead's own turn. It is the single most likely moment for a lead to call `status`: right after `confirm` returns `accepted`, to check the request registered. It is told the roll was never requested. This matters more here than it normally would, because this PR exists to stop a lead being left with a false belief about its own roll. A caller cannot distinguish "your roll is running" from "nothing was ever requested", and the two need opposite responses: wait, versus call `open` again. Calling `open` again mid-roll is the harmful one. `grep -cE "IN_PROGRESS|inProgress|midRoll|duringRoll"` on your test file returns **0**, so nothing covers this window. ### What to change Add a fifth state for "approved, continuation running, not yet finished" — name it as you see fit. Record it at the moment `confirm` hands off, not when the roll ends, so there is no gap between `pending.remove` and the first `outcomes.put`. Keep `UNKNOWN` meaning only what its javadoc says today. ### Acceptance, as properties under a change 1. Between an approved `confirm` and the continuation finishing, `status(token)` reports the in-progress state — **not** `UNKNOWN`, and **not** `PENDING`. Drive this with a continuation runner that does not run the roll immediately, so the window is observable. 2. Once the continuation finishes, the same token reports its terminal state. The in-progress state is not sticky. 3. All three existing terminal states still report exactly as they do now. Your six current tests must stay green unchanged. 4. The in-progress state is distinct from all four existing states — assert it against each. 5. Eviction still holds with the new state in play: the cap counts in-progress entries too, or, if you decide it should not, say why and test that decision. ### Also, please, in the same PR Your out-of-scope note is right and I am pulling it into scope: the class javadoc around lines 28-30 still says *"Nothing in this ticket wires an MCP tool onto open/confirm/cancel — that is a separate, later unit; until it lands, nothing calls this class at all."* That is false, and this PR makes it more false. Correct it. ### Not a problem Extending `FleetMcpHandoverTest` was not unwanted scope — you touched `FleetMcp.java`, so testing the wiring there was right. Keep it. Re-read this comment before you commit. It is newer than your brief and it wins.
agent added 1 commit 2026-09-19 18:19:24 +02:00
fleetd: distinguish an in-flight roll from an unknown token
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 1m32s
Details
CI / build (pull_request) Successful in 2m8s
Details
bf895616a5 
confirm() removed the token from pending before handing the roll to the
continuation, and outcomes was only written when runRollover reached an exit.
For the whole duration of the roll the token was in neither map, so status()
answered UNKNOWN - documented as "never issued, cancelled, or aged out". A lead
polling right after its own confirm was told the roll had never been requested.

Adds RollState.IN_PROGRESS, written at the confirm hand-off rather than at the
roll's end, so there is no gap. status() now reads outcomes before pending, so
the hand-off write cannot race the removal.

Also corrects the class javadoc, which still claimed nothing calls this class.

Recovered by the lead: the authoring member ended on a backend error (the host
slept mid-response) with this work uncommitted in its worktree. Verified before
committing: mvn -o clean install exit 0, 1819 tests, 0 failures, 0 errors,
0 skipped, 143 reports; LeadRolloverTest 43 (was 39).
ltms commented 2026-09-22 05:07:22 +02:00
Owner
Copy Link
Copy Source

Closing: this content is already on main. The PR record is stale.

Checked today, 2026-09-22, in the main clone after git fetch origin.

This PR's content was merged on 2026-09-19 as commit a7aee5b:

a7aee5b Merge #600: fleetd lead-rollover outcomes readable after confirm() (2026-09-19)

That commit is an ancestor of the current origin/main (076cc43). And this PR's head branch has nothing left in it:

git rev-list --count origin/main..refs/pull/600/head   ->  0
git diff --stat origin/main...refs/pull/600/head       ->  (empty)

Zero commits ahead and an empty diff. Merging this now would apply a no-op.

Gitea still reported state: open, merged: false, mergeable: true, which is why it looked like live work. The merge commit appears to have reached main without the PR's own tracked state being updated.

Why this is worth a note, not just a quiet close. The stale record cost real time: it was carried into a lead handover as an open PR needing review, and a reviewer was spawned against it before anyone checked whether its commits were already ancestors of main. The cheap guard is one command — git rev-list --count origin/main..refs/pull/<n>/head — before reviewing or merging any PR that has been open for more than a day or two. A zero there means there is nothing to review.

Closing as already merged. No code change is being discarded.

One thing this does not settle: whether the rollover-outcome code itself is correct. It reached main and the handover recorded it as never reviewed, so it is live and unreviewed. That is tracked separately and is not a reason to keep this PR open.

## Closing: this content is already on `main`. The PR record is stale. Checked today, 2026-09-22, in the main clone after `git fetch origin`. This PR's content was merged on 2026-09-19 as commit `a7aee5b`: ``` a7aee5b Merge #600: fleetd lead-rollover outcomes readable after confirm() (2026-09-19) ``` That commit is an ancestor of the current `origin/main` (`076cc43`). And this PR's head branch has nothing left in it: ``` git rev-list --count origin/main..refs/pull/600/head -> 0 git diff --stat origin/main...refs/pull/600/head -> (empty) ``` Zero commits ahead and an empty diff. Merging this now would apply a no-op. Gitea still reported `state: open`, `merged: false`, `mergeable: true`, which is why it looked like live work. The merge commit appears to have reached `main` without the PR's own tracked state being updated. **Why this is worth a note, not just a quiet close.** The stale record cost real time: it was carried into a lead handover as an open PR needing review, and a reviewer was spawned against it before anyone checked whether its commits were already ancestors of `main`. The cheap guard is one command — `git rev-list --count origin/main..refs/pull/<n>/head` — before reviewing or merging any PR that has been open for more than a day or two. A zero there means there is nothing to review. Closing as already merged. No code change is being discarded. One thing this does **not** settle: whether the rollover-outcome code itself is correct. It reached `main` and the handover recorded it as never reviewed, so it is live and unreviewed. That is tracked separately and is not a reason to keep this PR open.
ltms closed this pull request 2026-09-22 05:07:29 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 1m32s
Details
CI / build (pull_request) Successful in 2m8s
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#600
Delete Branch "worker/rollover-outcome-291483-2"

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?