fleetd #722: reconcile presence that arrives before registry.put #735

2026-10-04T19:23:27+02:00

agent commented

2026-10-04 19:23:27 +02:00

Fixes #722.

Both session-acquisition paths in SessionManager launch the pane before registering it. If a member's MCP contact lands in that window, presence.markPresent fires the SPAWNING -> READY transition, but it finds no registry entry yet and no-ops silently. The presence mark persists, registration then runs, and the session is left stuck in SPAWNING even though it is present and deliverable -- so reclaim/seat accounting skips it.

The ordering permits this; it has not been observed in production.

Fix

Added a private helper, SessionManager.reconcilePresence(terminalId), called right after registry.put in both the plain-spawn path (acquire, line ~257) and the worktree-spawn path (acquireWithWorktree, line ~815). It checks presence.isPresent(terminalId) and, if true, retries the SPAWNING -> READY transition via the existing onReady. One helper serves both call sites, so there is no duplicated reconciliation rule to drift.

Placement: right after registry.put, before handles.put/notifyAcquired. The only invariant that matters is that the registry entry exists (transitionByTerminal CASes against it), so the call sits immediately next to the line it depends on rather than after unrelated bookkeeping.

reordering registry.put ahead of launcher.spawn was explicitly avoided per the ticket (#702 ordering is load-bearing the other way, and the pane id comes back from spawn()).

Tests

New PresenceRacingLauncher test double (src/test/java/dev/ltms/fleet/session/PresenceRacingLauncher.java) wraps the real launcher and marks presence for the spawned terminal from inside spawn(), before acquire()'s own registry.put runs -- modeling the contact-then-register race deterministically.

SessionManagerTest#registerThenContactReachesReadyForPlainSpawn (control, today's normal order)
SessionManagerTest#contactThenRegisterStillReachesReadyForPlainSpawn (new; fails on unmodified main)
SessionManagerTest#aTerminalNeverMarkedPresentStaysSpawningAfterRegistration
WorktreeSessionManagerTest#registerThenContactReachesReadyForWorktreeSpawn (control)
WorktreeSessionManagerTest#contactThenRegisterStillReachesReadyForWorktreeSpawn (new; fails on unmodified main)

Verified both contact-then-register tests fail against unmodified main (expected READY, got SPAWNING), then pass after the fix. Verified three mutations each kill exactly the test they should and nothing else:

removing the reconcilePresence call from the plain-spawn path kills contactThenRegisterStillReachesReadyForPlainSpawn
removing it from the worktree path kills contactThenRegisterStillReachesReadyForWorktreeSpawn
dropping the isPresent condition (unconditional transition) kills aTerminalNeverMarkedPresentStaysSpawningAfterRegistration

mvn clean install: Tests run: 2075, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS.

Not covered by these tests: no test here boots a real CLI or races a real MCP connect over a live transport -- only a live spawn could prove the window is reachable at all; these tests only prove the registration code tolerates the ordering once it is forced.

Fixes #722. Both session-acquisition paths in SessionManager launch the pane before registering it. If a member's MCP contact lands in that window, presence.markPresent fires the SPAWNING -> READY transition, but it finds no registry entry yet and no-ops silently. The presence mark persists, registration then runs, and the session is left stuck in SPAWNING even though it is present and deliverable -- so reclaim/seat accounting skips it. The ordering permits this; it has not been observed in production. ## Fix Added a private helper, SessionManager.reconcilePresence(terminalId), called right after registry.put in both the plain-spawn path (acquire, line ~257) and the worktree-spawn path (acquireWithWorktree, line ~815). It checks presence.isPresent(terminalId) and, if true, retries the SPAWNING -> READY transition via the existing onReady. One helper serves both call sites, so there is no duplicated reconciliation rule to drift. Placement: right after registry.put, before handles.put/notifyAcquired. The only invariant that matters is that the registry entry exists (transitionByTerminal CASes against it), so the call sits immediately next to the line it depends on rather than after unrelated bookkeeping. reordering registry.put ahead of launcher.spawn was explicitly avoided per the ticket (#702 ordering is load-bearing the other way, and the pane id comes back from spawn()). ## Tests New PresenceRacingLauncher test double (src/test/java/dev/ltms/fleet/session/PresenceRacingLauncher.java) wraps the real launcher and marks presence for the spawned terminal from inside spawn(), before acquire()'s own registry.put runs -- modeling the contact-then-register race deterministically. - SessionManagerTest#registerThenContactReachesReadyForPlainSpawn (control, today's normal order) - SessionManagerTest#contactThenRegisterStillReachesReadyForPlainSpawn (new; fails on unmodified main) - SessionManagerTest#aTerminalNeverMarkedPresentStaysSpawningAfterRegistration - WorktreeSessionManagerTest#registerThenContactReachesReadyForWorktreeSpawn (control) - WorktreeSessionManagerTest#contactThenRegisterStillReachesReadyForWorktreeSpawn (new; fails on unmodified main) Verified both contact-then-register tests fail against unmodified main (expected READY, got SPAWNING), then pass after the fix. Verified three mutations each kill exactly the test they should and nothing else: 1. removing the reconcilePresence call from the plain-spawn path kills contactThenRegisterStillReachesReadyForPlainSpawn 2. removing it from the worktree path kills contactThenRegisterStillReachesReadyForWorktreeSpawn 3. dropping the isPresent condition (unconditional transition) kills aTerminalNeverMarkedPresentStaysSpawningAfterRegistration mvn clean install: Tests run: 2075, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS. Not covered by these tests: no test here boots a real CLI or races a real MCP connect over a live transport -- only a live spawn could prove the window is reachable at all; these tests only prove the registration code tolerates the ordering once it is forced.

agent added 1 commit 2026-10-04 19:23:27 +02:00

fleetd #722 : reconcile presence that arrives before a session's registry entry

CI / shell-tests (pull_request) Failing after 9s

Details

CI / contract (pull_request) Successful in 50s

Details

CI / build (pull_request) Failing after 1m48s

Details

CI / shell-tests (push) Failing after 9s

Details

CI / contract (push) Successful in 47s

Details

CI / build (push) Failing after 1m46s

Details

428a12af62

A member whose MCP contact lands between launcher.spawn() and registry.put()
had its presence marked, but the SPAWNING -> READY transition that markPresent
triggers found no registry entry yet and silently did nothing. The mark then
persisted while registration left the session in SPAWNING, with nothing to
retry the transition. That left the session undeliverable to reclaim/seat
accounting even though it was present and deliverable.

Add SessionManager.reconcilePresence, called right after registry.put in both
the plain-spawn and worktree-spawn paths, to retry the transition for a
terminal already marked present. One private helper serves both call sites.

Tests cover both orderings (contact-then-register and register-then-contact)
for both spawn paths, plus a terminal never marked present staying in
SPAWNING. The contact-then-register tests use a new PresenceRacingLauncher
test double that marks presence from inside spawn(), before acquire()'s own
registry.put runs.

ltms commented

2026-10-04 19:28:05 +02:00

Merged locally as 428a12a, pushed to main. Closing by hand — we merge locally, so Gitea does not close it itself. No changes requested.

This is the shape I ask for and rarely get: a test shown red on unmodified main with its failure line quoted, one helper for both call sites, and a control that kills the obvious wrong fix.

What I checked myself rather than taking from your reply

Both sites are covered, and there are only two. grep -n 'registry.put' SessionManager.java returns exactly :257 and :811, and MemberSession.State.SPAWNING is constructed at exactly :256 and :806. So the fix is complete at the site level, not just at the two sites you happened to be pointed at.

The merge is the thing I built. Merge tree 31c1477, identical to your branch's tree, so this was a fast-forward and your build was already the merge result. I built it anyway:

BUILD SUCCESS
Tests run: 2075, Failures: 0, Errors: 0, Skipped: 0
SessionManagerTest          82, Failures: 0
WorktreeSessionManagerTest  27, Failures: 0

2070 + 5 = 2075. Agrees with your count.

A mutation you did not run, and it was killed

Your three mutations all remove or weaken the new code. I wanted one that attacks the claim your javadoc makes but no test obviously asserts — that the call must come after registry.put. So I moved reconcilePresence(handle.terminalId()) to sit above registry.put in the plain path, confirmed mvn -o compile passed first so the mutation was live rather than a compile error, and ran both session test classes:

[ERROR] SessionManagerTest.contactThenRegisterStillReachesReadyForPlainSpawn
org.opentest4j.AssertionFailedError: a presence contact that lands before registry.put
  must still reach READY ==> expected: <READY> but was: <SPAWNING>

Killed. So the ordering constraint in your javadoc is enforced by a test, not only asserted in prose. File restored and confirmed byte-identical afterwards.

One gap, small enough not to send back

aTerminalNeverMarkedPresentStaysSpawningAfterRegistration exists only for the plain path. The helper is shared, so it covers today's code — but if a later change inlined the condition at the worktree site, nothing would catch it. Not worth a round trip; noting it so it is on the record.

What I could not run

ide_diagnostics answers project_not_found — the fleetd project is not open in IntelliJ — so the IDE inspections have not been run over SessionManager.java. Maven only.

A pre-existing hazard I found while reviewing this, which is NOT yours to fix

Enumerating the writers of MemberPresence (not the readers), presence.forget has exactly one wiring: FleetdAssembly.java:375 hands presence::forget to the Injector, which calls it at Injector.java:676 and :845 when a delivery attempt finds the worker gone. Neither SessionManager.release nor releaseRemoved clears it — I grepped both bodies and they contain no presence or forget at all.

So a released member's terminal stays marked present until something tries to deliver to it. Your change does not widen this: the injector's own gate is presence alone, so a stale entry already permits a delivery today with or without the reconciliation, and your fix actually improves the bookkeeping in that case by letting the session hold its seat instead of looking reclaimable.

I am filing it separately rather than asking you to touch it, for the same reason this ticket was split out of #705: a pre-existing defect hidden inside an unrelated change makes the change's diff look like the cause.

Merged locally as `428a12a`, pushed to `main`. Closing by hand — we merge locally, so Gitea does not close it itself. No changes requested. This is the shape I ask for and rarely get: a test shown red on unmodified `main` with its failure line quoted, one helper for both call sites, and a control that kills the obvious wrong fix. ### What I checked myself rather than taking from your reply **Both sites are covered, and there are only two.** `grep -n 'registry.put' SessionManager.java` returns exactly `:257` and `:811`, and `MemberSession.State.SPAWNING` is constructed at exactly `:256` and `:806`. So the fix is complete at the site level, not just at the two sites you happened to be pointed at. **The merge is the thing I built.** Merge tree `31c1477`, identical to your branch's tree, so this was a fast-forward and your build was already the merge result. I built it anyway: ``` BUILD SUCCESS Tests run: 2075, Failures: 0, Errors: 0, Skipped: 0 SessionManagerTest 82, Failures: 0 WorktreeSessionManagerTest 27, Failures: 0 ``` 2070 + 5 = 2075. Agrees with your count. ### A mutation you did not run, and it was killed Your three mutations all remove or weaken the new code. I wanted one that attacks the claim your javadoc makes but no test obviously asserts — that the call must come **after** `registry.put`. So I moved `reconcilePresence(handle.terminalId())` to sit *above* `registry.put` in the plain path, confirmed `mvn -o compile` passed first so the mutation was live rather than a compile error, and ran both session test classes: ``` [ERROR] SessionManagerTest.contactThenRegisterStillReachesReadyForPlainSpawn org.opentest4j.AssertionFailedError: a presence contact that lands before registry.put must still reach READY ==> expected: <READY> but was: <SPAWNING> ``` **Killed.** So the ordering constraint in your javadoc is enforced by a test, not only asserted in prose. File restored and confirmed byte-identical afterwards. ### One gap, small enough not to send back `aTerminalNeverMarkedPresentStaysSpawningAfterRegistration` exists only for the plain path. The helper is shared, so it covers today's code — but if a later change inlined the condition at the worktree site, nothing would catch it. Not worth a round trip; noting it so it is on the record. ### What I could not run `ide_diagnostics` answers `project_not_found` — the `fleetd` project is not open in IntelliJ — so the IDE inspections have not been run over `SessionManager.java`. Maven only. ### A pre-existing hazard I found while reviewing this, which is NOT yours to fix Enumerating the writers of `MemberPresence` (not the readers), `presence.forget` has **exactly one** wiring: `FleetdAssembly.java:375` hands `presence::forget` to the `Injector`, which calls it at `Injector.java:676` and `:845` when a delivery attempt finds the worker gone. Neither `SessionManager.release` nor `releaseRemoved` clears it — I grepped both bodies and they contain no `presence` or `forget` at all. So a released member's terminal stays marked present until something tries to deliver to it. Your change does not widen this: the injector's own gate is presence alone, so a stale entry already permits a delivery today with or without the reconciliation, and your fix actually *improves* the bookkeeping in that case by letting the session hold its seat instead of looking reclaimable. I am filing it separately rather than asking you to touch it, for the same reason this ticket was split out of #705: a pre-existing defect hidden inside an unrelated change makes the change's diff look like the cause.

ltms closed this pull request

2026-10-04 19:28:08 +02:00

ltms referenced this pull request

2026-10-04 19:28:36 +02:00

A released member's presence entry is never cleared: release() does not forget it, and only a later delivery attempt does #736

ltms referenced this pull request

2026-10-04 19:28:55 +02:00

A member whose MCP contact lands before registry.put is marked present but never leaves SPAWNING, so it holds no seat for reclaim accounting #722