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

A member is deregistered while still alive: release() removes the roster entry, then shells out to git before stopping the pane, so the resolver's spawned-member check goes blind for that window #702

New Issue
Closed
opened 2026-10-04 01:28:49 +02:00 by ltms · 4 comments
ltms commented 2026-10-04 01:28:49 +02:00
Owner
Copy Link
Copy Source

Found by a reviewer on PR #701 (#669 Unit D), dimension "resolver wiring and roster freshness". I confirmed the sequence and the intervening work myself in the main clone at b92a669. Filed separately rather than folded into Unit D, for the reasons at the end.

The sequence

SessionManager.release(String, ReleaseCause) removes the registry entry first:

private MemberSession release(String paneId, ReleaseCause cause) {
    MemberSession removed = registry.remove(paneId);          // SessionManager.java:306
    releaseRemoved(paneId, removed, handles.remove(paneId), cause);
    return removed;
}

The pane is stopped inside releaseRemoved, and only after this work:

  • worktrees.hasUncommitted(removed.worktree()) (:341), which shells out to git status. That is not my inference — the code says so at :363: "CB-581: hasUncommitted shells out to git status and can throw on a non-zero exit."
  • possibly trySnapshot(removed, cause) (:360), a second git operation that writes a WIP ref.

The comment at :387 confirms the stop is last: "the pane must always stop, even if the dirty check above threw."

So between "no longer in the roster" and "process gone" there are one or two git subprocesses. The member is alive and can issue requests for that whole window.

Why it matters after #669 Unit D

Unit D adds a step to CallerResolver.resolve(): a terminal belonging to a live spawned member resolves as that member's own role, consulting no tab map. That step reads a function built from SessionManager.roster().

A releasing-but-still-live member is absent from that roster. So during the window its terminal looks unknown, the resolver falls through to the lead and collaborator tab maps, and a pane whose tab carries a lead or collaborator label resolves as PRIMARY or COLLABORATOR instead of WORKER — which is exactly the tab-label override Unit D exists to stop.

It fails in the unsafe direction. "Not in the roster" is read as "not a member", when here it means "a member on its way out".

Severity, stated honestly

This is not a regression. Before Unit D the resolver had no roster step at all, so every member fell through to the tab maps on every request; the hole was total. Unit D narrows it to the release window. The residual window is a narrower instance of the old hole, not new damage.

The precondition does not hold on the mac host today. Reaching it needs a member pane carrying a configured lead or collaborator tab label. Startup validation refuses that combination (FleetConfig.validatePanePlacementAgainstLeadTabs, plus the exact/template collision check added by #669 Unit B), and I checked the live fleetd/fleetd.yaml myself: no profile uses placement: pane. So this is latent here.

It becomes live on any host that uses pane placement, and fleet01 is the direction where a second herdr daemon and different placement are plausible. I have not read fleet01's config and cannot.

What a fix looks like

Keep a terminal classifiable as a member until the pane is actually gone. Either hold a "releasing" marker that spawnedMemberRole also consults and that is cleared only after launcher.stop returns, or stop the pane before deregistering and leave the worktree work after it.

The ordering is not free either way: the current order exists so the handle and the registry entry are dropped together (:331, fleetd #209), and reordering teardown is how teardown bugs get made. Whichever is chosen, the property to pin is: for every instant between release() being entered and the pane being gone, resolving that terminal does not consult a tab map.

That property is testable without timing. Inject a hasUncommitted that resolves the terminal while it is being asked whether the worktree is dirty — that is a real call inside the window, so no sleep and no race are needed to land in it.

Not verified by me

  • I did not measure how long the window actually is. It is "one or two git subprocesses", which I read in the code; I did not time it, and a number here would be invented.
  • I did not check whether the reaper and the shutdown drain reach release by the same path, so the window may exist on more than one route.
  • I did not check whether MemberPresence has the same blind spot during release.
  • Whether any host in the fleet uses placement: pane. I verified only the mac host's config.
Found by a reviewer on PR #701 (#669 Unit D), dimension "resolver wiring and roster freshness". I confirmed the sequence and the intervening work myself in the main clone at `b92a669`. Filed separately rather than folded into Unit D, for the reasons at the end. ## The sequence `SessionManager.release(String, ReleaseCause)` removes the registry entry **first**: ```java private MemberSession release(String paneId, ReleaseCause cause) { MemberSession removed = registry.remove(paneId); // SessionManager.java:306 releaseRemoved(paneId, removed, handles.remove(paneId), cause); return removed; } ``` The pane is stopped inside `releaseRemoved`, and only after this work: - `worktrees.hasUncommitted(removed.worktree())` (`:341`), which **shells out to `git status`**. That is not my inference — the code says so at `:363`: *"CB-581: hasUncommitted shells out to `git status` and can throw on a non-zero exit."* - possibly `trySnapshot(removed, cause)` (`:360`), a second git operation that writes a WIP ref. The comment at `:387` confirms the stop is last: *"the pane must always stop, even if the dirty check above threw."* So between "no longer in the roster" and "process gone" there are one or two git subprocesses. The member is alive and can issue requests for that whole window. ## Why it matters after #669 Unit D Unit D adds a step to `CallerResolver.resolve()`: a terminal belonging to a live spawned member resolves as that member's own role, consulting no tab map. That step reads a function built from `SessionManager.roster()`. A releasing-but-still-live member is **absent** from that roster. So during the window its terminal looks unknown, the resolver falls through to the lead and collaborator tab maps, and a pane whose tab carries a lead or collaborator label resolves as `PRIMARY` or `COLLABORATOR` instead of `WORKER` — which is exactly the tab-label override Unit D exists to stop. It fails in the **unsafe** direction. "Not in the roster" is read as "not a member", when here it means "a member on its way out". ## Severity, stated honestly **This is not a regression.** Before Unit D the resolver had no roster step at all, so every member fell through to the tab maps on every request; the hole was total. Unit D narrows it to the release window. The residual window is a narrower instance of the old hole, not new damage. **The precondition does not hold on the mac host today.** Reaching it needs a member pane carrying a configured lead or collaborator tab label. Startup validation refuses that combination (`FleetConfig.validatePanePlacementAgainstLeadTabs`, plus the exact/template collision check added by #669 Unit B), and I checked the live `fleetd/fleetd.yaml` myself: no profile uses `placement: pane`. So this is latent here. It becomes live on any host that uses pane placement, and fleet01 is the direction where a second herdr daemon and different placement are plausible. I have not read fleet01's config and cannot. ## What a fix looks like Keep a terminal classifiable as a member until the pane is actually gone. Either hold a "releasing" marker that `spawnedMemberRole` also consults and that is cleared only after `launcher.stop` returns, or stop the pane before deregistering and leave the worktree work after it. The ordering is not free either way: the current order exists so the handle and the registry entry are dropped together (`:331`, fleetd #209), and reordering teardown is how teardown bugs get made. Whichever is chosen, the property to pin is: **for every instant between `release()` being entered and the pane being gone, resolving that terminal does not consult a tab map.** That property is testable without timing. Inject a `hasUncommitted` that resolves the terminal while it is being asked whether the worktree is dirty — that is a real call inside the window, so no sleep and no race are needed to land in it. ## Not verified by me - I did not measure how long the window actually is. It is "one or two git subprocesses", which I read in the code; I did not time it, and a number here would be invented. - I did not check whether the reaper and the shutdown drain reach `release` by the same path, so the window may exist on more than one route. - I did not check whether `MemberPresence` has the same blind spot during release. - Whether any host in the fleet uses `placement: pane`. I verified only the mac host's config.
ltms commented 2026-10-04 08:49:37 +02:00
Author
Owner
Copy Link
Copy Source

Decision — settled by an architect (profile opus) at 6e06058. I accept it, and it corrected a premise of mine. Ready to delegate; scheduled after #721 and before #705's OBSERVER floor.

What I had wrong

I went in believing #702 and #705's presence split were one mechanism, on the grounds that this ticket's own last bullet wonders whether MemberPresence has the same blind spot during release. It does not. Presence has no writer on the release path at all:

Step Line
registry entry removed SessionManager.java:306
architect slot unbound SessionManager.java:338
git status subprocess SessionManager.java:341
maybe a second git op SessionManager.java:360
pane stopped SessionManager.java:390
presence cleared nowhere

The only wiring of MemberPresence.forget in main is FleetdAssembly.java:376, as the Injector's forget consumer, firing at Injector.java:676 and :845. grep -n "presence" session/SessionManager.java returns a field, its construction, the asPresence() accessor and two javadoc lines — release never touches it. PresenceFleet overrides only markPresent, not forget.

So the defect runs the other way: a dead pane stays marked present until the Injector tries to deliver and fails. That self-heals, and it fails toward one wasted delivery attempt, not a lockout. That closes this ticket's open bullet: no, and it does not need to be fixed here.

Two mechanisms, one shared premise

The shared premise is real: the registry is not a reliable answer to "is this pane a live member?" But the two differ where it counts:

#702 #705's presence predicate
gate that fails authorization role (CallerResolver.resolve) deliverability (Fleetd.java:240)
why the registry is empty teardown in flight the daemon restarted; the pane outlived it
how long one or two git subprocesses for ever
needs persistence? no yes, or a different gate

That last row decides it. One construct cannot cover both unless it is persisted, so designing them together would force persistence into a fix that does not need any. Ship #702 alone.

Also worth recording: the OBSERVER floor does not fix this. The floor is the last rung, CallerResolver.java:326, while the lead map is checked at :302, the architect map at :310 and the collaborator map at :319 — all before it. This ticket's whole harm is a tab map winning, so the floor changes nothing here.

Two facts this ticket did not have

1. There are two registry-removal sites, not one. This answers another of the ticket's own open bullets — "I did not check whether the reaper and the shutdown drain reach release by the same path". They do not. The idle reaper has its own CAS remove at SessionManager.java:317 (releaseIfCurrent). A fix installed only at :306 leaves the reaper's window wide open. Put the write in one shared private helper and call it from both. Four entry routes reach the window: :288, :980 (the context cap inside completeTurn), :1031 (the reaper), :1167 (the shutdown drain).

2. A plain Set is the wrong data structure. Two threads can be tearing down one pane at once — the CAS at :317 exists because that race is real, and the loser then runs releaseRemoved with removed == null and still calls launcher.stop at :390. With Set.add/Set.remove, the loser's finally unmarks the terminal while the winner is still inside its git status, reopening the exact window the fix closes. Use a depth count: ConcurrentHashMap<String, Releasing> updated with compute, key dropped only at depth zero.

The mechanism

  • Writes: before the removal, at both :306 and :317. Before, not after — "for every instant" is false otherwise.
  • Reads: exactly one reader, the spawnedMemberRole function, today an inline lambda at FleetdAssembly.java:485 that streams sessions.roster(). It becomes a SessionManager method, so "in the registry OR being released" is computed in one place.
  • Clear: in a finally wrapping removal through the end of releaseRemoved — not a line after launcher.stop. PeerLauncher.stop (peer/PeerLauncher.java:338) returns void and declares nothing, so an unchecked throw is possible. With a finally the marker cannot outlive the method whatever stop does. Written as a trailing line instead, the terminal stays marked a live member for the daemon's life, and the resolver then returns Principal.worker(...) for it ignoring every tab map — a demotion, so a confusing lockout rather than a security hole, and cleared by any restart.

The alternative, rejected on a count. Keeping the session in the registry with a new RELEASING state is attractive — registry.replace's CAS preserves the atomic claim against double teardown that makes registry.remove-first load-bearing. But grep -rn "\.roster()\|sessions::roster" gives 23 hits, 18 distinct read sites: the metrics gauges (FleetMetrics.java:89, :102), FleetHealthMonitor, LeadHeartbeatLoop, spawn capacity (FleetdAssembly.java:223), fleet_list (FleetMcp.java:1417). A RELEASING session would hold a spawn seat and show up in fleet_list. That is a behaviour change at ~18 sites to fix a bug at one. The separate marker changes one reader.

SessionManager alone — CallerResolver does not change

Its field javadoc already describes exactly this contract (CallerResolver.java:78-83): "A function rather than the roster itself … the lookup strategy is the caller's to choose." The resolver asks; the manager answers. Teaching the auth layer about teardown is the wrong direction.

It also fixes a second thing for free: FleetdAssembly.java:485 is an inline lambda, the untested-wiring shape #589 swept for. A method reference can be pinned by a wiring test, and it drops a roster() list copy plus a stream from the per-request hot path. That is a second concern — keep it visible in the diff, do not fold it in silently.

The test property, and the trap in stating it

This ticket's test idea holds up and should be reused: inject a hasUncommitted that resolves the releasing terminal from inside the window. That is a real call inside the window, so no sleep and no race.

Two things the brief must say. The control is mandatory — the lead tab map must contain that exact terminal, and the same resolve outside the window must return the lead role. Without it the test passes on an empty map and proves nothing.

And state the property as "no tab map is consulted", never "the same role is returned". Inside the window an architect legitimately resolves WORKER, because memberLifecycle.released at :338 unbinds the slot before the git work (MemberRegistry.java:405-409) and CallerResolver.java:294 reads that same map. A test written the second way fails, and the tempting fix is to move :338, which would be wrong. That is the kind of acceptance criterion that turns into a defect, and I have caused that nine times, so it goes in the brief verbatim.

Ordering

#721 → #702 → the OBSERVER floor. #702 before the floor is a real argument, not tidiness: afterwards a releasing member never reaches the floor at all, so the floor applies only to genuinely unknown panes, which is the set it is for.

If the floor went first, during every release window an unlabelled member pane would resolve OBSERVER instead of WORKER and silently lose TASK_READ, and an architect would lose SEND, for one or two git subprocesses. REPLY/ASK survive — Authz.java:135 names no role — so the member can still end its turn. Low harm, but a narrowing nobody asked for and invisible.

Documentation

By the project's own table this touches no fleet_* tool, no Authz, no ConnectionIdentity, no launcher and no injector gating. So it is an internal contract change → wiki/9-Implementation.md, and no 11-Features.md entry. I will confirm that against the final diff rather than now.

What the architect did not do

It ran no build and no tests, so nothing it says about which tests go red is measured — I will re-run the mutation myself before merging. It did not time the window. It did not check whether herdr reuses a terminal_id; if it does, a marker from an old teardown could shadow a new spawn, which the depth count makes a narrow race rather than a leak, but that is unverified. It did not re-verify #721.

It did go one step further than this ticket on the latency question, and I am recording it because it closes a door: validateLeadTabPrefixes (FleetConfig.java:2722) and validatePanePlacementAgainstLeadTabs (:2891) have no direct call site and run through the reflective sweep invokeAllValidators (:3166) behind validateAll() (:3150), called at both Fleetd.java:201 (startup) and ConfigRef.java:416 (every hot reload). So a hot reload cannot sneak in a colliding tab label. It read the live fleetd/fleetd.yaml (39368 bytes, mtime Oct 3 21:54) and confirmed every profile is placement: tab with no collaborators: block — so the precondition stays latent on this host, and that is now true across reloads, not only at boot.

Still unmeasured, and the ticket already says so: whether any other host uses placement: pane. I have asked the fleet01 lead directly, since neither I nor an architect here can read their config.

**Decision — settled by an architect (profile `opus`) at `6e06058`. I accept it, and it corrected a premise of mine.** Ready to delegate; scheduled after #721 and before #705's `OBSERVER` floor. ## What I had wrong I went in believing #702 and #705's presence split were **one mechanism**, on the grounds that this ticket's own last bullet wonders whether `MemberPresence` has the same blind spot during release. **It does not.** Presence has **no writer on the release path at all**: | Step | Line | |---|---| | registry entry removed | `SessionManager.java:306` | | architect slot unbound | `SessionManager.java:338` | | `git status` subprocess | `SessionManager.java:341` | | maybe a second git op | `SessionManager.java:360` | | pane stopped | `SessionManager.java:390` | | **presence cleared** | **nowhere** | The only wiring of `MemberPresence.forget` in `main` is `FleetdAssembly.java:376`, as the **Injector's** `forget` consumer, firing at `Injector.java:676` and `:845`. `grep -n "presence" session/SessionManager.java` returns a field, its construction, the `asPresence()` accessor and two javadoc lines — `release` never touches it. `PresenceFleet` overrides only `markPresent`, not `forget`. So the defect runs the **other way**: a *dead* pane stays marked present until the Injector tries to deliver and fails. That self-heals, and it fails toward one wasted delivery attempt, not a lockout. **That closes this ticket's open bullet: no, and it does not need to be fixed here.** ## Two mechanisms, one shared premise The shared premise is real: *the registry is not a reliable answer to "is this pane a live member?"* But the two differ where it counts: | | #702 | #705's presence predicate | |---|---|---| | gate that fails | authorization role (`CallerResolver.resolve`) | deliverability (`Fleetd.java:240`) | | why the registry is empty | teardown in flight | the daemon restarted; the pane outlived it | | how long | one or two git subprocesses | for ever | | needs persistence? | **no** | **yes, or a different gate** | That last row decides it. One construct cannot cover both unless it is persisted, so **designing them together would force persistence into a fix that does not need any.** Ship #702 alone. Also worth recording: **the `OBSERVER` floor does not fix this.** The floor is the *last* rung, `CallerResolver.java:326`, while the lead map is checked at `:302`, the architect map at `:310` and the collaborator map at `:319` — all before it. This ticket's whole harm is a tab map winning, so the floor changes nothing here. ## Two facts this ticket did not have **1. There are two registry-removal sites, not one.** This answers another of the ticket's own open bullets — *"I did not check whether the reaper and the shutdown drain reach `release` by the same path"*. They do not. The idle reaper has its own CAS remove at `SessionManager.java:317` (`releaseIfCurrent`). **A fix installed only at `:306` leaves the reaper's window wide open.** Put the write in one shared private helper and call it from both. Four entry routes reach the window: `:288`, `:980` (the context cap inside `completeTurn`), `:1031` (the reaper), `:1167` (the shutdown drain). **2. A plain `Set` is the wrong data structure.** Two threads can be tearing down one pane at once — the CAS at `:317` exists because that race is real, and the loser then runs `releaseRemoved` with `removed == null` and still calls `launcher.stop` at `:390`. With `Set.add`/`Set.remove`, the loser's `finally` unmarks the terminal while the winner is still inside its `git status`, **reopening the exact window the fix closes.** Use a depth count: `ConcurrentHashMap<String, Releasing>` updated with `compute`, key dropped only at depth zero. ## The mechanism - **Writes:** before the removal, at both `:306` and `:317`. Before, not after — "for every instant" is false otherwise. - **Reads:** exactly one reader, the `spawnedMemberRole` function, today an inline lambda at `FleetdAssembly.java:485` that streams `sessions.roster()`. It becomes a `SessionManager` method, so "in the registry OR being released" is computed in one place. - **Clear:** in a `finally` wrapping removal through the end of `releaseRemoved` — **not** a line after `launcher.stop`. `PeerLauncher.stop` (`peer/PeerLauncher.java:338`) returns `void` and declares nothing, so an unchecked throw is possible. With a `finally` the marker cannot outlive the method whatever `stop` does. Written as a trailing line instead, the terminal stays marked a live member for the daemon's life, and the resolver then returns `Principal.worker(...)` for it ignoring every tab map — a **demotion**, so a confusing lockout rather than a security hole, and cleared by any restart. **The alternative, rejected on a count.** Keeping the session in the registry with a new `RELEASING` state is attractive — `registry.replace`'s CAS preserves the atomic claim against double teardown that makes `registry.remove`-first load-bearing. But `grep -rn "\.roster()\|sessions::roster"` gives **23 hits, 18 distinct read sites**: the metrics gauges (`FleetMetrics.java:89`, `:102`), `FleetHealthMonitor`, `LeadHeartbeatLoop`, spawn capacity (`FleetdAssembly.java:223`), `fleet_list` (`FleetMcp.java:1417`). A `RELEASING` session would hold a spawn seat and show up in `fleet_list`. That is a behaviour change at ~18 sites to fix a bug at one. The separate marker changes **one** reader. ## `SessionManager` alone — `CallerResolver` does not change Its field javadoc already describes exactly this contract (`CallerResolver.java:78-83`): *"A function rather than the roster itself … the lookup strategy is the caller's to choose."* The resolver asks; the manager answers. Teaching the auth layer about teardown is the wrong direction. It also fixes a second thing for free: `FleetdAssembly.java:485` is an inline lambda, the untested-wiring shape #589 swept for. A method reference can be pinned by a wiring test, and it drops a `roster()` list copy plus a stream from the per-request hot path. **That is a second concern — keep it visible in the diff, do not fold it in silently.** ## The test property, and the trap in stating it This ticket's test idea holds up and should be reused: inject a `hasUncommitted` that resolves the releasing terminal from inside the window. That is a real call inside the window, so no sleep and no race. **Two things the brief must say.** The control is mandatory — the lead tab map must contain that exact terminal, and the same resolve *outside* the window must return the lead role. Without it the test passes on an empty map and proves nothing. And state the property as **"no tab map is consulted"**, never "the same role is returned". Inside the window an architect legitimately resolves `WORKER`, because `memberLifecycle.released` at `:338` unbinds the slot before the git work (`MemberRegistry.java:405-409`) and `CallerResolver.java:294` reads that same map. **A test written the second way fails, and the tempting fix is to move `:338`, which would be wrong.** That is the kind of acceptance criterion that turns into a defect, and I have caused that nine times, so it goes in the brief verbatim. ## Ordering **#721 → #702 → the `OBSERVER` floor.** #702 before the floor is a real argument, not tidiness: afterwards a releasing member never reaches the floor at all, so the floor applies only to genuinely unknown panes, which is the set it is for. If the floor went first, during every release window an unlabelled member pane would resolve `OBSERVER` instead of `WORKER` and silently lose `TASK_READ`, and an architect would lose `SEND`, for one or two git subprocesses. `REPLY`/`ASK` survive — `Authz.java:135` names no role — so the member can still end its turn. Low harm, but a narrowing nobody asked for and invisible. ## Documentation By the project's own table this touches no `fleet_*` tool, no `Authz`, no `ConnectionIdentity`, no launcher and no injector gating. So it is an internal contract change → `wiki/9-Implementation.md`, and **no `11-Features.md` entry.** I will confirm that against the final diff rather than now. ## What the architect did not do It ran **no build and no tests**, so nothing it says about which tests go red is measured — I will re-run the mutation myself before merging. It did not time the window. It did not check whether herdr reuses a `terminal_id`; if it does, a marker from an old teardown could shadow a new spawn, which the depth count makes a narrow race rather than a leak, but that is unverified. It did not re-verify #721. It did go one step further than this ticket on the latency question, and I am recording it because it closes a door: `validateLeadTabPrefixes` (`FleetConfig.java:2722`) and `validatePanePlacementAgainstLeadTabs` (`:2891`) have no direct call site and run through the reflective sweep `invokeAllValidators` (`:3166`) behind `validateAll()` (`:3150`), called at both `Fleetd.java:201` (startup) **and** `ConfigRef.java:416` (every hot reload). **So a hot reload cannot sneak in a colliding tab label.** It read the live `fleetd/fleetd.yaml` (39368 bytes, mtime Oct 3 21:54) and confirmed every profile is `placement: tab` with no `collaborators:` block — so the precondition stays latent on this host, and that is now true across reloads, not only at boot. Still unmeasured, and the ticket already says so: whether any **other** host uses `placement: pane`. I have asked the `fleet01` lead directly, since neither I nor an architect here can read their config.
ltms commented 2026-10-04 09:34:30 +02:00
Author
Owner
Copy Link
Copy Source

Lead verification of PR #724, and one reviewer finding adjudicated

My own verification — on the merge, not the branch

main moved to ed4f4b0 after the branch was cut, so the merge tree (54feb485…) differs from the branch tree (6ea93fb0…). The implementer's green build therefore does not cover the merge, so I built the merge myself in a throwaway worktree.

  • mvn -o clean install: exit 0, Tests run: 2031, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.
  • Merge is clean, no conflict leftovers.

Three mutations of my own, aimed at the parts the implementer's tests claim to protect. Each compiled green first, so each was live rather than a compile error, and the file was restored byte-identical afterwards (git diff empty).

Mutation Tests killed
M1 — Releasing.leave() → always null, i.e. plain-Set behaviour 1: spawnedMemberRoleSurvivesAnOverlappingReleaseThatUnmarksEarly
M2 — drop enter()'s preservation of the prior terminal 1: the same test
M3 — neutralise the fix (the releasing map is scanned but never answers) 3, across both classes, including CallerResolverTest.aPaneMidTeardownResolvesAsItsOwnRoleConsultingNoTabMap

M3's kill goes through the production method reference, so it is behavioural evidence rather than a unit-level one. The depth count is genuinely load-bearing — M1 confirms a plain Set would be wrong.

One weakness worth naming: M1 and M2 are two distinct invariants and both are pinned by the same single test. That is a thin pin. If spawnedMemberRoleSurvivesAnOverlappingReleaseThatUnmarksEarly is ever weakened, both go unprotected at once and nothing else notices.

I also checked the claim that both removal sites are covered, rather than taking the shared helper's existence as proof. There are exactly two registry.remove sites, :320 and :333, both now inside releaseWindow. registry.replace at :1328 is a state transition and opens no window. The four entry routes are the public release, :1064 (the context cap in completeTurn), :1115 (the reaper) and :1251 (the shutdown drain). findByTerminal is null-safe at :1309, so the later null guard in spawnedMemberRole is not redundant — it is what protects the loop's equals.

Reviewer finding — rejected on the security claim, accepted as a test gap

A reviewer reported, at high severity, that the in-window test covers only the dev role and so misses "a regression where a releasing ARCHITECT stays ARCHITECT instead of becoming WORKER, which grants wrong permissions during teardown".

The escalation does not exist. I read the path:

  1. releaseRemoved:422 calls memberLifecycle.released(removed.terminalId()), which is MemberRegistry.released (auth/MemberRegistry.java:405-409) → unbind(slot, terminal).
  2. That is before worktrees.hasUncommitted(...) at :425, the git subprocess that creates the window.
  3. So inside the window CallerResolver:288-298 sees spawnedRole == ARCHITECT, looks the terminal up in architectTerminals, gets null, and returns Principal.worker(...).

The roster's ARCHITECT answer is confirmed against the live slot map (the fleetd #424 defence), and that map is already unbound. A releasing architect resolves WORKER. The new path cannot escalate, because the check it might have shadowed sits inside the same branch it added to.

But the interaction is untested, and that part is fair. Nothing currently pins that a releasing architect is demoted. That coupling is security-relevant and a later change could break it silently, so I want one test for it — stating the property as "the architect slot map is unbound, so the result is WORKER", never as role-equality.

Not a merge blocker: the behaviour is correct today. I am asking for the test before merge because it is cheap and the implementer still holds the context.

## Lead verification of PR #724, and one reviewer finding adjudicated ### My own verification — on the merge, not the branch `main` moved to `ed4f4b0` after the branch was cut, so the merge tree (`54feb485…`) differs from the branch tree (`6ea93fb0…`). The implementer's green build therefore does **not** cover the merge, so I built the merge myself in a throwaway worktree. - `mvn -o clean install`: exit **0**, **Tests run: 2031, Failures: 0, Errors: 0, Skipped: 0**, BUILD SUCCESS. - Merge is clean, no conflict leftovers. Three mutations of my own, aimed at the parts the implementer's tests claim to protect. Each compiled green **first**, so each was live rather than a compile error, and the file was restored byte-identical afterwards (`git diff` empty). | Mutation | Tests killed | |---|---| | M1 — `Releasing.leave()` → always `null`, i.e. plain-`Set` behaviour | 1: `spawnedMemberRoleSurvivesAnOverlappingReleaseThatUnmarksEarly` | | M2 — drop `enter()`'s preservation of the prior terminal | 1: the same test | | M3 — neutralise the fix (the releasing map is scanned but never answers) | 3, across both classes, including `CallerResolverTest.aPaneMidTeardownResolvesAsItsOwnRoleConsultingNoTabMap` | M3's kill goes through the production method reference, so it is behavioural evidence rather than a unit-level one. The depth count is genuinely load-bearing — M1 confirms a plain `Set` would be wrong. **One weakness worth naming:** M1 and M2 are two *distinct* invariants and both are pinned by the *same single* test. That is a thin pin. If `spawnedMemberRoleSurvivesAnOverlappingReleaseThatUnmarksEarly` is ever weakened, both go unprotected at once and nothing else notices. I also checked the claim that both removal sites are covered, rather than taking the shared helper's existence as proof. There are exactly **two** `registry.remove` sites, `:320` and `:333`, both now inside `releaseWindow`. `registry.replace` at `:1328` is a state transition and opens no window. The four entry routes are the public `release`, `:1064` (the context cap in `completeTurn`), `:1115` (the reaper) and `:1251` (the shutdown drain). `findByTerminal` is null-safe at `:1309`, so the later null guard in `spawnedMemberRole` is not redundant — it is what protects the loop's `equals`. ### Reviewer finding — rejected on the security claim, accepted as a test gap A reviewer reported, at high severity, that the in-window test covers only the `dev` role and so misses "a regression where a releasing ARCHITECT stays ARCHITECT instead of becoming WORKER, which grants wrong permissions during teardown". **The escalation does not exist.** I read the path: 1. `releaseRemoved:422` calls `memberLifecycle.released(removed.terminalId())`, which is `MemberRegistry.released` (`auth/MemberRegistry.java:405-409`) → `unbind(slot, terminal)`. 2. That is **before** `worktrees.hasUncommitted(...)` at `:425`, the git subprocess that creates the window. 3. So inside the window `CallerResolver:288-298` sees `spawnedRole == ARCHITECT`, looks the terminal up in `architectTerminals`, gets `null`, and returns `Principal.worker(...)`. The roster's ARCHITECT answer is confirmed against the live slot map (the fleetd #424 defence), and that map is already unbound. A releasing architect resolves `WORKER`. The new path cannot escalate, because the check it might have shadowed sits inside the same branch it added to. **But the interaction is untested, and that part is fair.** Nothing currently pins that a releasing architect is demoted. That coupling is security-relevant and a later change could break it silently, so I want one test for it — stating the property as "the architect slot map is unbound, so the result is `WORKER`", never as role-equality. Not a merge blocker: the behaviour is correct today. I am asking for the test before merge because it is cheap and the implementer still holds the context.
ltms commented 2026-10-04 09:35:40 +02:00
Author
Owner
Copy Link
Copy Source

Second reviewer: no defect in the production code. One javadoc line to fix, which I will do at merge.

A second reviewer took the production files on four concurrency points and reported no issue. It checked the depth count most closely, and its reasoning is the right reasoning: each enter and each leave is one atomic ConcurrentHashMap.compute on the same key, so each exit reads the live depth rather than a value captured when that thread entered. It walked three-thread interleavings with exits in a different order from entries (1→2→3→2→1→null) and they hold.

It also confirmed two things I had not checked myself, and I accept them because it named the mechanism rather than just the verdict:

  • a CAS loser in releaseIfCurrent still marks and unmarks even though it never reaches releaseRemoved, so it cannot clear the marker while a sibling is inside its own launcher.stop;
  • a redundant release() that finds removed == null still calls launcher.stop, and its marker stays up until its own finally.

The javadoc line

Both that reviewer and I, reading independently, found the same imprecision. The Releasing record's javadoc says:

a losing releaseIfCurrent CAS has no known session of its own, so it must not blank out what the winner already recorded

That attribution is wrong. releaseIfCurrent is called only from the reaper, which passes a non-null expected, so known is never null on that path. The known == null case happens in release(), when registry.get(paneId) already returns null.

The code's null handling is correct either way — only the comment names the wrong caller. It matters because this repo's rules say a comment describes the code as it is, and a comment that claims an invariant is a free test case. A future reader trying to find the null-known path would look in the wrong method.

I am fixing this myself at merge, not asking the implementer. I have already told it "test files only" for its current turn, and sending a contradicting instruction now would reach a busy member anyway — which this project has measured does not work. The edit is one sentence of prose in a file I am about to merge.

Where this leaves the PR

Production code: verified by me, no defect from either reviewer, three of my own mutations killed. The only outstanding work is the two test additions I asked for in the follow-up — the releasing-architect demotion test, and splitting the single test that currently pins two separate invariants.

## Second reviewer: no defect in the production code. One javadoc line to fix, which I will do at merge. A second reviewer took the production files on four concurrency points and reported **no issue**. It checked the depth count most closely, and its reasoning is the right reasoning: each `enter` and each `leave` is one atomic `ConcurrentHashMap.compute` on the same key, so each exit reads the **live** depth rather than a value captured when that thread entered. It walked three-thread interleavings with exits in a different order from entries (1→2→3→2→1→null) and they hold. It also confirmed two things I had not checked myself, and I accept them because it named the mechanism rather than just the verdict: - a CAS **loser** in `releaseIfCurrent` still marks and unmarks even though it never reaches `releaseRemoved`, so it cannot clear the marker while a sibling is inside its own `launcher.stop`; - a redundant `release()` that finds `removed == null` still calls `launcher.stop`, and its marker stays up until its own `finally`. ### The javadoc line Both that reviewer and I, reading independently, found the same imprecision. The `Releasing` record's javadoc says: > a losing `releaseIfCurrent` CAS has no `known` session of its own, so it must not blank out what the winner already recorded **That attribution is wrong.** `releaseIfCurrent` is called only from the reaper, which passes a non-null `expected`, so `known` is never null on that path. The `known == null` case happens in `release()`, when `registry.get(paneId)` already returns null. The code's null handling is correct either way — only the comment names the wrong caller. It matters because this repo's rules say a comment describes the code as it is, and a comment that claims an invariant is a free test case. A future reader trying to find the null-`known` path would look in the wrong method. **I am fixing this myself at merge, not asking the implementer.** I have already told it "test files only" for its current turn, and sending a contradicting instruction now would reach a busy member anyway — which this project has measured does not work. The edit is one sentence of prose in a file I am about to merge. ### Where this leaves the PR Production code: verified by me, no defect from either reviewer, three of my own mutations killed. The only outstanding work is the two test additions I asked for in the follow-up — the releasing-architect demotion test, and splitting the single test that currently pins two separate invariants.
ltms commented 2026-10-04 10:25:51 +02:00
Author
Owner
Copy Link
Copy Source

Lead verification and merge

Merged into main as 38f4fd6, with one review commit of my own on top, 8d3f10d. Pushed
(3fc39b9..8d3f10d).

The merge needed its own build. Merge tree 338f5c16… was not the branch tree
1412e4cc…, because main moved under this branch (#715 landed first). The worker's green build
did not cover the merge, so I built it.

Merge build: mvn -o clean install, Maven exit 0, BUILD SUCCESS,
Tests run: 2047, Failures: 0, Errors: 0, Skipped: 0 (2040 on main + 7 new).

Mutations, on the merge

# Mutation Result
M1 leave() → return null (a plain Set instead of a depth count) KILLED by releasingLeaveStepsDownADepthGreaterThanOneInsteadOfRemovingIt — 1 failure, that test only
M2 enter() drops the prior terminal when it has no session of its own KILLED by releasingEnterPreservesThePriorTerminalWhenTheOverlappingCallHasNoSessionOfItsOwn — 1 failure, that test only
M3 spawnedMemberRole stops consulting the releasing map KILLED by 2 tests: the SessionManagerTest unit pin and aPaneMidTeardownResolvesAsItsOwnRoleConsultingNoTabMap (expected: <WORKER> but was: <PRIMARY> — the lead tab won)
M4 MemberRegistry.released no longer unbinds the architect slot KILLED by aReleasingArchitectIsDemotedToWorkerInsideTheTeardownWindow (expected: <WORKER> but was: <ARCHITECT>), plus onlyArchitectsBindAndReleaseMakesTheirSlotReusable

M1 and M2 each fail exactly one test. That is the thing I asked for: the two invariants were
previously pinned by a single assertion, and they are now separable. M4 confirms the architect test
really depends on the unbind rather than on anything incidental, and the second red is extra
coverage I did not know was there.

My review commit, 8d3f10d

Two things, both in SessionManager and its test.

The Releasing javadoc named the wrong caller. It said a losing releaseIfCurrent CAS is the
call that arrives with no known session. releaseIfCurrent is only called from the reaper, always
with a non-null expected, so it always has one. The call that really passes null is an
overlapping release that finds the registry entry already gone. The same wrong claim had been
copied into a test's failure message, where whoever hits the failure would read it, so I fixed both.

Reflection replaced with a compile-time binding. The two split tests reached Releasing.enter
and leave through six setAccessible helpers. The worker's reasoning for going white-box is
right, and I checked it: releaseWindow is called with registry.get(paneId), so any nested
release on the same pane necessarily has a null known, which entangles the depth invariant and
the terminal-preservation invariant in one black-box scenario. But the test is in the same package,
so dropping private from the record and its two methods gets the same isolation with none of the
reflection. Six helpers deleted, and a rename now breaks the build instead of a test run. The record
stays nested and non-public, so nothing outside dev.ltms.fleet.session can see it.

I re-ran the baseline and all four mutations after that refactor, not before — the numbers above
are from the refactored tree. The tree I pushed is byte-identical
(4826b35e77a62f29a00d2356c2e957193e3ca91d) to the one I built, so the 2047-test run covers exactly
what is on main.

One thing the worker found that is worth keeping

Worktrees.hasUncommitted fires twice per release() — the pre-stop check and
dirtyImmediatelyBeforeRemoval's post-stop re-check (fleetd #316). Both land inside the teardown
window, so capturing either is valid for these tests, but anyone writing a future test that hooks
hasUncommitted should know it is not a single-shot probe.

A wiki/9-Implementation.md entry follows in a separate commit.

## Lead verification and merge Merged into `main` as `38f4fd6`, with one review commit of my own on top, `8d3f10d`. Pushed (`3fc39b9..8d3f10d`). **The merge needed its own build.** Merge tree `338f5c16…` was **not** the branch tree `1412e4cc…`, because `main` moved under this branch (#715 landed first). The worker's green build did not cover the merge, so I built it. **Merge build:** `mvn -o clean install`, Maven exit 0, `BUILD SUCCESS`, `Tests run: 2047, Failures: 0, Errors: 0, Skipped: 0` (2040 on `main` + 7 new). ### Mutations, on the merge | # | Mutation | Result | |---|---|---| | M1 | `leave()` → `return null` (a plain `Set` instead of a depth count) | **KILLED** by `releasingLeaveStepsDownADepthGreaterThanOneInsteadOfRemovingIt` — **1 failure, that test only** | | M2 | `enter()` drops the prior terminal when it has no session of its own | **KILLED** by `releasingEnterPreservesThePriorTerminalWhenTheOverlappingCallHasNoSessionOfItsOwn` — **1 failure, that test only** | | M3 | `spawnedMemberRole` stops consulting the releasing map | **KILLED** by 2 tests: the `SessionManagerTest` unit pin and `aPaneMidTeardownResolvesAsItsOwnRoleConsultingNoTabMap` (`expected: <WORKER> but was: <PRIMARY>` — the lead tab won) | | M4 | `MemberRegistry.released` no longer unbinds the architect slot | **KILLED** by `aReleasingArchitectIsDemotedToWorkerInsideTheTeardownWindow` (`expected: <WORKER> but was: <ARCHITECT>`), plus `onlyArchitectsBindAndReleaseMakesTheirSlotReusable` | M1 and M2 each fail **exactly one** test. That is the thing I asked for: the two invariants were previously pinned by a single assertion, and they are now separable. M4 confirms the architect test really depends on the unbind rather than on anything incidental, and the second red is extra coverage I did not know was there. ### My review commit, `8d3f10d` Two things, both in `SessionManager` and its test. **The `Releasing` javadoc named the wrong caller.** It said a losing `releaseIfCurrent` CAS is the call that arrives with no `known` session. `releaseIfCurrent` is only called from the reaper, always with a non-null `expected`, so it always has one. The call that really passes `null` is an overlapping `release` that finds the registry entry already gone. The same wrong claim had been copied into a test's failure message, where whoever hits the failure would read it, so I fixed both. **Reflection replaced with a compile-time binding.** The two split tests reached `Releasing.enter` and `leave` through six `setAccessible` helpers. The worker's reasoning for going white-box is right, and I checked it: `releaseWindow` is called with `registry.get(paneId)`, so any nested `release` on the same pane necessarily has a `null` `known`, which entangles the depth invariant and the terminal-preservation invariant in one black-box scenario. But the test is in the same package, so dropping `private` from the record and its two methods gets the same isolation with none of the reflection. Six helpers deleted, and a rename now breaks the build instead of a test run. The record stays nested and non-public, so nothing outside `dev.ltms.fleet.session` can see it. I re-ran the baseline and all four mutations **after** that refactor, not before — the numbers above are from the refactored tree. The tree I pushed is byte-identical (`4826b35e77a62f29a00d2356c2e957193e3ca91d`) to the one I built, so the 2047-test run covers exactly what is on `main`. ### One thing the worker found that is worth keeping `Worktrees.hasUncommitted` fires **twice** per `release()` — the pre-stop check and `dirtyImmediatelyBeforeRemoval`'s post-stop re-check (fleetd #316). Both land inside the teardown window, so capturing either is valid for these tests, but anyone writing a future test that hooks `hasUncommitted` should know it is not a single-shot probe. A `wiki/9-Implementation.md` entry follows in a separate commit.
ltms closed this issue 2026-10-04 10:26:25 +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#702
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?