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

fleetd #726 unit 3: make LeadRollover.confirm() single-flight per lead terminal #733

Closed
agent wants to merge 0 commits from worker/726-ea34a0-2 into main
pull from: worker/726-ea34a0-2
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-04 18:54:24 +02:00
Member
Copy Link
Copy Source

What

Two open() calls for the same lead terminal mint two tokens, both of which pass confirm()'s ownership check (p.leadTerminal().equals(callerTerminal)), so both could reach continuationRunner.accept(...) and roll the same lead twice. Today that costs two /clears; after fleetd #726 unit 2 turns the roll into "end the process and launch a fresh one", it becomes a race to kill and rebuild the same lead twice.

Change

  • Added rollingByTerminal: Map<String, String> (lead terminal -> the token currently claiming it).
  • confirm() claims a slot with an atomic putIfAbsent(p.leadTerminal(), token) after every other gate (NOT_CONFIGURED, UNKNOWN_TOKEN, NOT_YOUR_ROLLOVER, OPERATOR_NOT_CONFIRMED, checkHandover) has passed, and before outcomes.put(...) / pending.remove(...) / continuationRunner.accept(...). A non-null previous value refuses with the new RefusalReason.ROLL_ALREADY_RUNNING, naming the lead terminal and the token holding the claim.
  • runRollover releases the claim in a finally (rollingByTerminal.remove(p.leadTerminal(), p.token())), covering both the normal return and the fleetd #615 thrown-exception path, so one failed roll does not leave the lead permanently unrollable.
  • cancel(token) is unchanged (removes from pending only) — covered by a new test proving it cannot release a claim already taken by a confirmed roll.

Tests (LeadRolloverTest, 5 new, all existing kept)

  • secondConfirmForSameTerminalIsRefusedWhileFirstRollIsStillInFlight — two confirms on one terminal: first approved, second ROLL_ALREADY_RUNNING, continuationRunner ran exactly once; after the held roll finishes, a fresh confirm on the same terminal is approved.
  • differentLeadTerminalsConfirmIndependently — two different terminals both succeed.
  • refusedConfirmDoesNotTakeTheClaim — an OPERATOR_NOT_CONFIRMED refusal never claims the terminal.
  • throwingContinuationStillReleasesTheClaim — a RuntimeException out of the continuation still releases the claim (fleetd #615 interaction).
  • cancelAfterConfirmDoesNotReleaseTheClaim — cancel() on an already-confirmed token returns false and does not free the claim.

One existing test, evictionCountsInProgressEntriesTowardTheCap, used a HoldingRunner to put OUTCOME_HISTORY_CAP+50 simultaneous in-flight rolls on the SAME lead terminal — incompatible with single-flight by construction. Changed it to use a distinct terminal per iteration so it still exercises the outcomes bounded-eviction behaviour it was written for, without tripping the new per-terminal claim.

Build

mvn -q clean install green. LeadRolloverTest: 50/50 passing. Project-wide (summed from target/surefire-reports): 2059 run, 0 failures, 0 errors.

Mutation evidence (apply / RED / revert / GREEN, each verified individually)

  1. putIfAbsent -> put: secondConfirmForSameTerminalIsRefusedWhileFirstRollIsStillInFlight fails on the final re-claim assertion (the overwritten map entry is never released by the first roll's finally, since it checks the two-arg remove(key, value)).
  2. finally release moved to the success-path only: throwingContinuationStillReleasesTheClaim fails — a fresh confirm after the throw is refused ROLL_ALREADY_RUNNING instead of approved.
  3. Claim taken before the OPERATOR_NOT_CONFIRMED check: refusedConfirmDoesNotTakeTheClaim fails — the retry confirm is refused instead of approved.
  4. Claim keyed on token instead of p.leadTerminal(): secondConfirmForSameTerminalIsRefusedWhileFirstRollIsStillInFlight fails — the second confirm (different token) is wrongly approved.

All four reverted cleanly back to the committed source (diffed identical) with the full LeadRolloverTest class green (50/50) afterward.

Scope note

Only LeadRollover.java and LeadRolloverTest.java touched, per the ticket's unit-3 scope. Did not touch LeadLauncher.java, FleetdAssembly.java, Fleetd.java, or runRolloverUnguarded's body (reserved for unit 2).

Ref: fleetd #726 (comment #726 (comment) names this exact unit: "LeadRollover.confirm(): single-flight claim keyed on p.leadTerminal(), released in runRollover's finally").

## What Two `open()` calls for the same lead terminal mint two tokens, both of which pass `confirm()`'s ownership check (`p.leadTerminal().equals(callerTerminal)`), so both could reach `continuationRunner.accept(...)` and roll the same lead twice. Today that costs two `/clear`s; after fleetd #726 unit 2 turns the roll into "end the process and launch a fresh one", it becomes a race to kill and rebuild the same lead twice. ## Change - Added `rollingByTerminal: Map<String, String>` (lead terminal -> the token currently claiming it). - `confirm()` claims a slot with an atomic `putIfAbsent(p.leadTerminal(), token)` **after** every other gate (`NOT_CONFIGURED`, `UNKNOWN_TOKEN`, `NOT_YOUR_ROLLOVER`, `OPERATOR_NOT_CONFIRMED`, `checkHandover`) has passed, and **before** `outcomes.put(...)` / `pending.remove(...)` / `continuationRunner.accept(...)`. A non-null previous value refuses with the new `RefusalReason.ROLL_ALREADY_RUNNING`, naming the lead terminal and the token holding the claim. - `runRollover` releases the claim in a `finally` (`rollingByTerminal.remove(p.leadTerminal(), p.token())`), covering both the normal return and the fleetd #615 thrown-exception path, so one failed roll does not leave the lead permanently unrollable. - `cancel(token)` is unchanged (removes from `pending` only) — covered by a new test proving it cannot release a claim already taken by a confirmed roll. ## Tests (LeadRolloverTest, 5 new, all existing kept) - `secondConfirmForSameTerminalIsRefusedWhileFirstRollIsStillInFlight` — two confirms on one terminal: first approved, second `ROLL_ALREADY_RUNNING`, continuationRunner ran exactly once; after the held roll finishes, a fresh confirm on the same terminal is approved. - `differentLeadTerminalsConfirmIndependently` — two different terminals both succeed. - `refusedConfirmDoesNotTakeTheClaim` — an `OPERATOR_NOT_CONFIRMED` refusal never claims the terminal. - `throwingContinuationStillReleasesTheClaim` — a `RuntimeException` out of the continuation still releases the claim (fleetd #615 interaction). - `cancelAfterConfirmDoesNotReleaseTheClaim` — `cancel()` on an already-confirmed token returns false and does not free the claim. One existing test, `evictionCountsInProgressEntriesTowardTheCap`, used a `HoldingRunner` to put `OUTCOME_HISTORY_CAP+50` simultaneous in-flight rolls on the SAME lead terminal — incompatible with single-flight by construction. Changed it to use a distinct terminal per iteration so it still exercises the `outcomes` bounded-eviction behaviour it was written for, without tripping the new per-terminal claim. ## Build `mvn -q clean install` green. `LeadRolloverTest`: 50/50 passing. Project-wide (summed from `target/surefire-reports`): 2059 run, 0 failures, 0 errors. ## Mutation evidence (apply / RED / revert / GREEN, each verified individually) 1. `putIfAbsent` -> `put`: `secondConfirmForSameTerminalIsRefusedWhileFirstRollIsStillInFlight` fails on the final re-claim assertion (the overwritten map entry is never released by the first roll's `finally`, since it checks the two-arg `remove(key, value)`). 2. `finally` release moved to the success-path only: `throwingContinuationStillReleasesTheClaim` fails — a fresh confirm after the throw is refused `ROLL_ALREADY_RUNNING` instead of approved. 3. Claim taken before the `OPERATOR_NOT_CONFIRMED` check: `refusedConfirmDoesNotTakeTheClaim` fails — the retry confirm is refused instead of approved. 4. Claim keyed on `token` instead of `p.leadTerminal()`: `secondConfirmForSameTerminalIsRefusedWhileFirstRollIsStillInFlight` fails — the second confirm (different token) is wrongly approved. All four reverted cleanly back to the committed source (diffed identical) with the full `LeadRolloverTest` class green (50/50) afterward. ## Scope note Only `LeadRollover.java` and `LeadRolloverTest.java` touched, per the ticket's unit-3 scope. Did not touch `LeadLauncher.java`, `FleetdAssembly.java`, `Fleetd.java`, or `runRolloverUnguarded`'s body (reserved for unit 2). Ref: fleetd #726 (comment https://git.ltms.dev/fleet/fleetd/issues/726#issuecomment-18865 names this exact unit: "`LeadRollover.confirm()`: single-flight claim keyed on `p.leadTerminal()`, released in `runRollover`'s `finally`").
agent added 1 commit 2026-10-04 18:54:24 +02:00
fleetd #726 unit 3: make LeadRollover.confirm() single-flight per lead terminal
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m48s
Details
4ce3149bfd 
Two open() calls for the same lead terminal minted two tokens that both
passed confirm()'s ownership check, so both could reach the deferred
continuation and roll the same lead twice. confirm() now claims a
per-lead-terminal slot (an atomic put-if-absent) once every other gate has
passed, refusing a concurrent confirm with the new ROLL_ALREADY_RUNNING
reason; runRollover releases the claim in a finally, on both the success
and the thrown-exception path.
ltms commented 2026-10-04 18:58:49 +02:00
Owner
Copy Link
Copy Source

Review — verified myself, one fix wanted

My own reading of the pushed tree. Throwaway worktree on origin/worker/726-ea34a0-2, tree hash
confirmed equal to the branch's, then mvn clean install unpiped:

BUILD SUCCESS
Tests run: 2059, Failures: 0, Errors: 0, Skipped: 0
dev.ltms.fleet.lead.LeadRolloverTest — Tests run: 50, Failures: 0, Errors: 0

Matches the report. The claim is placed correctly — after every gate including checkHandover, before
outcomes.put/pending.remove/continuationRunner.accept — and released in runRollover's
finally, so it covers the fleetd #615 catch as well as the normal return.

Two things done better than the brief asked, worth naming so they are not lost:

  • The release uses the two-argument remove(key, value). A plain remove(key) would let a roll
    clear a claim it does not hold. Nothing in the brief said to do that.
  • You reported the existing test you had to change instead of quietly changing it.
    evictionCountsInProgressEntriesTowardTheCap piled up OUTCOME_HISTORY_CAP + 50 simultaneous rolls
    on one terminal, which single-flight makes impossible by construction. I read the change: it swaps
    LEAD for "term_cap_" + i and nothing else, so the eviction assertions and the test's purpose are
    intact, and the inline comment states the constraint rather than the history. That is the right
    resolution and the right way to surface it. The acceptance criterion was mine and it was wrong to
    say "unchanged" — a behaviour change is allowed to invalidate a test that encoded the old behaviour.

Your mutation-1 reading is also sharper than my brief expected, and correct: put returns the previous
value too, so the refusal still works; what breaks is that put overwrites the entry with the second
token, so the first roll's remove(terminal, firstToken) no longer matches and the claim leaks. Good
that the suite catches the consequence rather than only the obvious branch.

The one fix: a throw from continuationRunner.accept leaks the claim permanently

String holder = rollingByTerminal.putIfAbsent(p.leadTerminal(), token);
if (holder != null) { return RollDecision.refused(...); }
outcomes.put(token, new RollStatus(RollState.IN_PROGRESS, ...));
pending.remove(token);
log.info(...);
continuationRunner.accept(() -> runRollover(p, cfg));   // <- if this throws
return RollDecision.approved();

If accept throws, runRollover never runs, so the finally that releases never runs either. The
claim is held with no owner, and there is no way to clear it — cancel() only touches pending,
the token is already out of pending, and nothing else writes rollingByTerminal. That lead terminal
can never be rolled again until the daemon restarts.

Reachability, stated honestly. With today's runner — r -> Thread.ofVirtual().name(...).start(r) —
I do not think a RuntimeException is reachable: a virtual thread start does not allocate an OS
thread, and the failure it could throw is an Error, not a RuntimeException. So I am not claiming a
live defect. What makes it worth four lines now is the combination: the hole is invisible, the
consequence is unrecoverable, and it opens the moment the runner changes — a bounded executor would
throw RejectedExecutionException and every rejected roll would permanently brick its lead.

This is in scope for this unit because unit 3 is what makes it unrecoverable. Before the claim, the
same throw left a stuck IN_PROGRESS — bad, but visible in status() and harmless to the next roll.

Wrap the hand-off so a failure to start the continuation is treated like a failure inside it:
release the claim, and write a terminal FAILED outcome whose detail says the continuation never
started, so status() does not report IN_PROGRESS forever. Note the asymmetry with fleetd #615 in
the commit message, not in a comment.

Add one test: a continuationRunner that throws, asserting the outcome is terminal (not
IN_PROGRESS) and that a fresh open() + confirm() on the same terminal is then approved. Mutation:
remove the release from the new catch and show that test die.

Then re-run mvn clean install and report the count. Nothing else in this PR needs to change.

## Review — verified myself, one fix wanted **My own reading of the pushed tree.** Throwaway worktree on `origin/worker/726-ea34a0-2`, tree hash confirmed equal to the branch's, then `mvn clean install` unpiped: ``` BUILD SUCCESS Tests run: 2059, Failures: 0, Errors: 0, Skipped: 0 dev.ltms.fleet.lead.LeadRolloverTest — Tests run: 50, Failures: 0, Errors: 0 ``` Matches the report. The claim is placed correctly — after every gate including `checkHandover`, before `outcomes.put`/`pending.remove`/`continuationRunner.accept` — and released in `runRollover`'s `finally`, so it covers the fleetd #615 catch as well as the normal return. Two things done better than the brief asked, worth naming so they are not lost: - **The release uses the two-argument `remove(key, value)`.** A plain `remove(key)` would let a roll clear a claim it does not hold. Nothing in the brief said to do that. - **You reported the existing test you had to change instead of quietly changing it.** `evictionCountsInProgressEntriesTowardTheCap` piled up `OUTCOME_HISTORY_CAP + 50` simultaneous rolls on one terminal, which single-flight makes impossible by construction. I read the change: it swaps `LEAD` for `"term_cap_" + i` and nothing else, so the eviction assertions and the test's purpose are intact, and the inline comment states the constraint rather than the history. That is the right resolution and the right way to surface it. The acceptance criterion was mine and it was wrong to say "unchanged" — a behaviour change is allowed to invalidate a test that encoded the old behaviour. Your mutation-1 reading is also sharper than my brief expected, and correct: `put` returns the previous value too, so the *refusal* still works; what breaks is that `put` overwrites the entry with the second token, so the first roll's `remove(terminal, firstToken)` no longer matches and the claim leaks. Good that the suite catches the consequence rather than only the obvious branch. ### The one fix: a throw from `continuationRunner.accept` leaks the claim permanently ```java String holder = rollingByTerminal.putIfAbsent(p.leadTerminal(), token); if (holder != null) { return RollDecision.refused(...); } outcomes.put(token, new RollStatus(RollState.IN_PROGRESS, ...)); pending.remove(token); log.info(...); continuationRunner.accept(() -> runRollover(p, cfg)); // <- if this throws return RollDecision.approved(); ``` If `accept` throws, `runRollover` never runs, so the `finally` that releases never runs either. The claim is held with no owner, and **there is no way to clear it** — `cancel()` only touches `pending`, the token is already out of `pending`, and nothing else writes `rollingByTerminal`. That lead terminal can never be rolled again until the daemon restarts. **Reachability, stated honestly.** With today's runner — `r -> Thread.ofVirtual().name(...).start(r)` — I do not think a `RuntimeException` is reachable: a virtual thread start does not allocate an OS thread, and the failure it could throw is an `Error`, not a `RuntimeException`. So I am not claiming a live defect. What makes it worth four lines now is the combination: the hole is invisible, the consequence is unrecoverable, and it opens the moment the runner changes — a bounded executor would throw `RejectedExecutionException` and every rejected roll would permanently brick its lead. This is in scope for this unit because unit 3 is what makes it unrecoverable. Before the claim, the same throw left a stuck `IN_PROGRESS` — bad, but visible in `status()` and harmless to the next roll. Wrap the hand-off so a failure to *start* the continuation is treated like a failure *inside* it: release the claim, and write a terminal `FAILED` outcome whose detail says the continuation never started, so `status()` does not report `IN_PROGRESS` forever. Note the asymmetry with fleetd #615 in the commit message, not in a comment. Add one test: a `continuationRunner` that throws, asserting the outcome is terminal (not `IN_PROGRESS`) and that a fresh `open()` + `confirm()` on the same terminal is then approved. Mutation: remove the release from the new catch and show that test die. Then re-run `mvn clean install` and report the count. Nothing else in this PR needs to change.
agent added 1 commit 2026-10-04 19:07:04 +02:00
fleetd #726 unit 3 fix: release the single-flight claim when continuationRunner rejects the hand-off
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 1m1s
Details
CI / build (pull_request) Failing after 1m51s
Details
d0f4ae057b 
confirm() takes the per-lead-terminal claim before handing the roll to
continuationRunner, and the only release path was runRollover's own
finally. If continuationRunner.accept itself throws, runRollover never
starts, so that finally never runs, and nothing else ever writes
rollingByTerminal — the claim is held forever and the terminal can never
be rolled again. This differs from fleetd #615, which covers a throw
INSIDE the continuation (runRollover already catches that and still
releases the claim) — this is a throw from the hand-off itself, which
is not reachable with today's virtual-thread runner but would be with
a bounded executor's RejectedExecutionException.

confirm() now catches that throw, releases the claim, and overwrites
the IN_PROGRESS outcome with a terminal FAILED one, matching how a
throw inside the continuation is already surfaced.
ltms commented 2026-10-04 19:09:25 +02:00
Owner
Copy Link
Copy Source

Merged locally as a332dfd, pushed to main. Closing by hand — we merge locally, so Gitea does not close it itself.

The finding is fixed where it had to be fixed, and the fix is right in the detail that mattered.

  • The release uses the two-argument remove(key, value), so it clears only this roll's own claim. A one-argument remove here would have been a new defect: confirm() can throw on a claim it does not hold once the map is shared.
  • confirm() still returns approved(), and the failure surfaces through status(). That matches how #615 already handles a throw inside the continuation, so the two rejection paths now report the same way rather than one refusing and one not.
  • The outcomes.put overwrites the IN_PROGRESS entry written a few lines above. Without it status() would say IN_PROGRESS forever for a roll that never began — the worse half of the original finding, and you fixed both halves.

On reachability, stated plainly. This path is not reachable with today's virtual-thread runner, which does not reject. I asked for it anyway because the leak is unrecoverable once it happens, and it opens the moment the runner changes. The new test uses a lambda that always throws, standing in for RejectedExecutionException, which is the honest way to cover a path no production wiring reaches yet.

My own verification, not yours. I merged origin/main into your branch in a throwaway worktree and built the merge result. main had moved twice under you — #729, then unit 1 (#731) — so your branch's own build was not the merge. Merge tree 6639327, byte-identical to the tree I pushed.

BUILD SUCCESS
Tests run: 2070, Failures: 0, Errors: 0, Skipped: 0
LeadRolloverTest   51, Failures: 0
LeadLauncherTest   37, Failures: 0   (unit 1, unaffected by this merge)

2070 rather than your 2060, and the arithmetic agrees: 2054 base + 3 (#729) + 7 (unit 1) + 6 (this unit) = 2070.

One check I could not run. ide_diagnostics is unavailable right now — the fleetd project is not open in IntelliJ, which answers project_not_found. I am reporting the Maven build only; I have not run the IDE inspections on LeadRollover.java.

On my own wrong acceptance criterion, for the record. I wrote "every existing test still passes unchanged" in the brief, and that was unsatisfiable: evictionCountsInProgressEntriesTowardTheCap piles up OUTCOME_HISTORY_CAP + 50 simultaneous rolls on one terminal, which single-flight makes impossible by construction. You flagged the tension instead of hiding it and resolved it correctly with a distinct terminal per iteration. The criterion was wrong, not your change.

Unit 2 now goes on this base.

Merged locally as `a332dfd`, pushed to `main`. Closing by hand — we merge locally, so Gitea does not close it itself. The finding is fixed where it had to be fixed, and the fix is right in the detail that mattered. - The release uses the **two-argument** `remove(key, value)`, so it clears only this roll's own claim. A one-argument remove here would have been a new defect: `confirm()` can throw on a claim it does not hold once the map is shared. - `confirm()` still returns `approved()`, and the failure surfaces through `status()`. That matches how #615 already handles a throw *inside* the continuation, so the two rejection paths now report the same way rather than one refusing and one not. - The `outcomes.put` overwrites the `IN_PROGRESS` entry written a few lines above. Without it `status()` would say `IN_PROGRESS` forever for a roll that never began — the worse half of the original finding, and you fixed both halves. **On reachability, stated plainly.** This path is not reachable with today's virtual-thread runner, which does not reject. I asked for it anyway because the leak is *unrecoverable* once it happens, and it opens the moment the runner changes. The new test uses a lambda that always throws, standing in for `RejectedExecutionException`, which is the honest way to cover a path no production wiring reaches yet. **My own verification, not yours.** I merged `origin/main` into your branch in a throwaway worktree and built the *merge result*. `main` had moved twice under you — #729, then unit 1 (#731) — so your branch's own build was not the merge. Merge tree `6639327`, byte-identical to the tree I pushed. ``` BUILD SUCCESS Tests run: 2070, Failures: 0, Errors: 0, Skipped: 0 LeadRolloverTest 51, Failures: 0 LeadLauncherTest 37, Failures: 0 (unit 1, unaffected by this merge) ``` 2070 rather than your 2060, and the arithmetic agrees: 2054 base + 3 (#729) + 7 (unit 1) + 6 (this unit) = 2070. **One check I could not run.** `ide_diagnostics` is unavailable right now — the `fleetd` project is not open in IntelliJ, which answers `project_not_found`. I am reporting the Maven build only; I have not run the IDE inspections on `LeadRollover.java`. **On my own wrong acceptance criterion, for the record.** I wrote "every existing test still passes unchanged" in the brief, and that was unsatisfiable: `evictionCountsInProgressEntriesTowardTheCap` piles up `OUTCOME_HISTORY_CAP + 50` simultaneous rolls on one terminal, which single-flight makes impossible by construction. You flagged the tension instead of hiding it and resolved it correctly with a distinct terminal per iteration. The criterion was wrong, not your change. Unit 2 now goes on this base.
ltms closed this pull request 2026-10-04 19:09:28 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 1m1s
Details
CI / build (pull_request) Failing after 1m51s
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#733
Delete Branch "worker/726-ea34a0-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?