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

fleetd #737 unit 4: key the rollover single-flight claim on the lead's name #747

Closed
agent wants to merge 0 commits from worker/737-a263f3-3 into main
pull from: worker/737-a263f3-3
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-05 05:58:17 +02:00
Member
Copy Link
Copy Source

Fixes the rollover single-flight lock so it stays a per-lead lock across a roll.

Defect: rollingByTerminal was keyed on p.leadTerminal(), the pane address. Since a roll replaces the pane, a second roll of the same lead opened from the NEW terminal landed on a different map key and could run concurrently with the first roll's still-in-flight continuation.

Fix (per ticket #737 comment 19060, 'Unit 4 addendum'): PendingRollover now carries a rolloverKey, resolved ONCE in open() from leadNameForTerminal while the lead is certainly still live, falling back to the terminal itself when the name resolves null/blank. confirm()'s claim and both release sites (the continuationRunner-rejection catch, and runRollover's finally) use this carried key instead of recomputing it — recomputing at release time would silently read a different value, because leadNameForTerminal no longer resolves the OLD terminal by then, making the lead unrollable forever. NOT_YOUR_ROLLOVER and the pane-teardown calls stay keyed on p.leadTerminal(), unchanged. Field renamed rollingByTerminal -> rollingByLead.

Tests: 3 new tests added covering acceptance criteria 1-2 (cross-terminal collision refused; claim released on both success and throw paths even with the old terminal already evicted from the live roster). One pre-existing test's assertion updated to match the new name-based refusal wording; one pre-existing test's shared helper fixed to preserve per-terminal independence where no lead name resolves.

Mutation testing (criterion 5): (a) making the release recompute the key via leadNameForTerminal instead of using the carried key turned RED (2 failures — exactly the two new release-survives-roster-churn tests); (b) reverting the claim key to p.leadTerminal() turned RED (5 failures, including the new cross-terminal-collision test). Both reverted back to GREEN before this PR.

Build: mvn clean install from fleetd/ — BUILD SUCCESS, Tests run: 2095, Failures: 0, Errors: 0, Skipped: 0.

Scope: LeadRollover.java and LeadRolloverTest.java only, per the ticket's addendum.

Fixes the rollover single-flight lock so it stays a per-lead lock across a roll. **Defect**: `rollingByTerminal` was keyed on `p.leadTerminal()`, the pane address. Since a roll replaces the pane, a second roll of the same lead opened from the NEW terminal landed on a different map key and could run concurrently with the first roll's still-in-flight continuation. **Fix** (per ticket #737 comment 19060, 'Unit 4 addendum'): `PendingRollover` now carries a `rolloverKey`, resolved ONCE in `open()` from `leadNameForTerminal` while the lead is certainly still live, falling back to the terminal itself when the name resolves null/blank. `confirm()`'s claim and both release sites (the continuationRunner-rejection catch, and `runRollover`'s `finally`) use this carried key instead of recomputing it — recomputing at release time would silently read a different value, because `leadNameForTerminal` no longer resolves the OLD terminal by then, making the lead unrollable forever. `NOT_YOUR_ROLLOVER` and the pane-teardown calls stay keyed on `p.leadTerminal()`, unchanged. Field renamed `rollingByTerminal` -> `rollingByLead`. **Tests**: 3 new tests added covering acceptance criteria 1-2 (cross-terminal collision refused; claim released on both success and throw paths even with the old terminal already evicted from the live roster). One pre-existing test's assertion updated to match the new name-based refusal wording; one pre-existing test's shared helper fixed to preserve per-terminal independence where no lead name resolves. **Mutation testing** (criterion 5): (a) making the release recompute the key via `leadNameForTerminal` instead of using the carried key turned RED (2 failures — exactly the two new release-survives-roster-churn tests); (b) reverting the claim key to `p.leadTerminal()` turned RED (5 failures, including the new cross-terminal-collision test). Both reverted back to GREEN before this PR. **Build**: `mvn clean install` from `fleetd/` — BUILD SUCCESS, Tests run: 2095, Failures: 0, Errors: 0, Skipped: 0. Scope: `LeadRollover.java` and `LeadRolloverTest.java` only, per the ticket's addendum.
agent added 1 commit 2026-10-05 05:58:17 +02:00
fleetd #737 unit 4: key the rollover single-flight claim on the lead's name
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 52s
Details
CI / build (pull_request) Failing after 1m59s
Details
8a1d73b39e 
rollingByTerminal keyed the single-flight lock on p.leadTerminal(), the pane
address. A roll replaces the pane, so a second roll of the same lead opened
from the new terminal landed on a different map key and could run concurrent
with the first roll's still-in-flight continuation.

PendingRollover now carries rolloverKey, resolved once in open() from
leadNameForTerminal while the lead is certainly still live, falling back to
the terminal itself when the name resolves null or blank. confirm()'s claim
and both release sites (the continuationRunner-rejection catch and
runRollover's finally) use the carried key instead of recomputing it, since
leadNameForTerminal no longer resolves the old terminal by release time.
NOT_YOUR_ROLLOVER and the pane-teardown calls stay keyed on p.leadTerminal(),
unchanged.

rollingByTerminal is renamed rollingByLead to match.
ltms commented 2026-10-05 06:02:18 +02:00
Owner
Copy Link
Copy Source

Merged locally into main as 8a1d73b (merge commit abe617c). Closing this PR as the branch is in main. No lead follow-up commit was needed on this one.

What I checked myself

The test that mattered most: does criterion 1 actually build the collision? A test asserting "the second confirm is refused" passes trivially if the two requests never share a key. This one is genuine — it supplies its own leadNameForTerminal = _ -> LEAD_NAME, so LEAD and OTHER_LEAD really do resolve to one lead, and it carries three controls:

  • assertEquals(1, runner.heldCount(), "sanity: the first roll is held, not run yet") — the first roll is actually in flight, so the claim is really held;
  • the same assertion again after the refusal — the refused confirm() never reached continuationRunner;
  • a third confirm() after runner.runNext() is approved — so the refusal was a live claim, not a permanent block.

That third assertion is what separates this from a test that would pass against a claim which never releases.

Criterion 2 is the one I was most worried about, and it is correct. The brief warned that leadNameForTerminal reads the live roster, so recomputing the key at release time returns null for a dead pane and leaks the claim forever. The test drives exactly that: a mutable roster, roster.remove(LEAD) before the continuation runs, and a synchronous Runnable::run runner so the continuation executes inside confirm() — strictly after open() resolved and carried the key.

Its control is the part I want to highlight:

assertEquals(LeadRollover.RollState.RELAUNCH_FAILED, status.state(), "sanity: leadName "
        + "resolved to null (the roster had already dropped the old terminal) ...");

That assertion proves the hazard condition was reached. Without it, the retry could be approved for an unrelated reason and the test would still look green. This is the difference between a test that pins the fix and one that merely agrees with it.

Build on the merged result. This unit's base predated both the unit 6 and unit 3 merges — it reported 2095 tests against main's 2108 — so its own green build proved nothing about main. Merged onto main and built from fleetd/: MVN_EXIT=0, BUILD SUCCESS, Tests run: 2111, Failures: 0. Confirmed independently by summing 177 surefire XML files. 2108 + this unit's 3 new tests = 2111, so nothing was dropped in the merge.

I also confirmed my own earlier LeadRollover.java:96 javadoc fix survived the auto-merge, and that the claim and both release sites now all read p.rolloverKey() (:583, :614, :660) with the field renamed rollingByLead (:383).

On the shared test-helper change

The unit changed newRolloverWithHoldingRunner's name supplier from _ -> LEAD_NAME to t -> LEAD.equals(t) ? LEAD_NAME : null. That touches tests the unit did not write, so it deserves a note rather than silence.

It was necessary, and for a reason worth recording: before this change the name supplier was not used by the claim at all — leadNameForTerminal only fed LeadLauncher#relaunch. So _ -> LEAD_NAME was harmless. Once the claim keys on the name, that supplier would collapse the 250 distinct terminals in evictionCountsInProgressEntriesTowardTheCap onto a single key and the 2nd through 250th confirms would all be refused. The replacement models "a terminal the live roster does not recognise", which is the documented fallback path, and those rolls are held by HoldingRunner so they never reach relaunch anyway.

This is the general shape: a value that was inert under the old code becomes load-bearing under the new, and the test fixture that supplied a convenient constant becomes wrong without any test failing for the right reason.

Scope and API

NOT_YOUR_ROLLOVER still compares p.leadTerminal() to callerTerminal, and the pane teardown still uses p.leadTerminal() — both as required. The brief asked for the pane address to stay where it is genuinely about the pane, and it did.

PendingRollover gained a public rolloverKey component. The unit checked that FleetMcp.handoverOpen maps the record's fields by hand and so does not expose it. I will re-verify that after unit 5 merges, since unit 5 is adding fields to that same response right now — not a concern with this diff, just the one interaction worth re-checking at the end.

Merged locally into `main` as `8a1d73b` (merge commit `abe617c`). Closing this PR as the branch is in `main`. No lead follow-up commit was needed on this one. ## What I checked myself **The test that mattered most: does criterion 1 actually build the collision?** A test asserting "the second confirm is refused" passes trivially if the two requests never share a key. This one is genuine — it supplies its own `leadNameForTerminal = _ -> LEAD_NAME`, so `LEAD` and `OTHER_LEAD` really do resolve to one lead, and it carries three controls: - `assertEquals(1, runner.heldCount(), "sanity: the first roll is held, not run yet")` — the first roll is actually in flight, so the claim is really held; - the same assertion again after the refusal — the refused `confirm()` never reached `continuationRunner`; - a third `confirm()` after `runner.runNext()` is **approved** — so the refusal was a live claim, not a permanent block. That third assertion is what separates this from a test that would pass against a claim which never releases. **Criterion 2 is the one I was most worried about, and it is correct.** The brief warned that `leadNameForTerminal` reads the live roster, so recomputing the key at release time returns `null` for a dead pane and leaks the claim forever. The test drives exactly that: a mutable `roster`, `roster.remove(LEAD)` before the continuation runs, and a synchronous `Runnable::run` runner so the continuation executes inside `confirm()` — strictly after `open()` resolved and carried the key. Its control is the part I want to highlight: ```java assertEquals(LeadRollover.RollState.RELAUNCH_FAILED, status.state(), "sanity: leadName " + "resolved to null (the roster had already dropped the old terminal) ..."); ``` That assertion proves the hazard condition was **reached**. Without it, the retry could be approved for an unrelated reason and the test would still look green. This is the difference between a test that pins the fix and one that merely agrees with it. **Build on the merged result.** This unit's base predated both the unit 6 and unit 3 merges — it reported 2095 tests against `main`'s 2108 — so its own green build proved nothing about `main`. Merged onto `main` and built from `fleetd/`: `MVN_EXIT=0`, `BUILD SUCCESS`, `Tests run: 2111, Failures: 0`. Confirmed independently by summing 177 surefire XML files. 2108 + this unit's 3 new tests = 2111, so nothing was dropped in the merge. I also confirmed my own earlier `LeadRollover.java:96` javadoc fix survived the auto-merge, and that the claim and both release sites now all read `p.rolloverKey()` (`:583`, `:614`, `:660`) with the field renamed `rollingByLead` (`:383`). ## On the shared test-helper change The unit changed `newRolloverWithHoldingRunner`'s name supplier from `_ -> LEAD_NAME` to `t -> LEAD.equals(t) ? LEAD_NAME : null`. That touches tests the unit did not write, so it deserves a note rather than silence. It was necessary, and for a reason worth recording: **before this change the name supplier was not used by the claim at all** — `leadNameForTerminal` only fed `LeadLauncher#relaunch`. So `_ -> LEAD_NAME` was harmless. Once the claim keys on the name, that supplier would collapse the 250 distinct terminals in `evictionCountsInProgressEntriesTowardTheCap` onto a single key and the 2nd through 250th confirms would all be refused. The replacement models "a terminal the live roster does not recognise", which is the documented fallback path, and those rolls are held by `HoldingRunner` so they never reach `relaunch` anyway. This is the general shape: a value that was inert under the old code becomes load-bearing under the new, and the test fixture that supplied a convenient constant becomes wrong without any test failing for the right reason. ## Scope and API `NOT_YOUR_ROLLOVER` still compares `p.leadTerminal()` to `callerTerminal`, and the pane teardown still uses `p.leadTerminal()` — both as required. The brief asked for the pane address to stay where it is genuinely about the pane, and it did. `PendingRollover` gained a public `rolloverKey` component. The unit checked that `FleetMcp.handoverOpen` maps the record's fields by hand and so does not expose it. I will re-verify that after unit 5 merges, since unit 5 is adding fields to that same response right now — not a concern with this diff, just the one interaction worth re-checking at the end.
ltms closed this pull request 2026-10-05 06:02:21 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 52s
Details
CI / build (pull_request) Failing after 1m59s
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#747
Delete Branch "worker/737-a263f3-3"

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?