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

fleetd #702: mark a pane as mid-teardown so a caller still resolves as a member #724

Closed
agent wants to merge 0 commits from worker/702-4f5c7f-2 into main
pull from: worker/702-4f5c7f-2
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-04 09:27:35 +02:00
Member
Copy Link
Copy Source

Fixes #702.

A caller from a pane being torn down was briefly absent from the session registry, so CallerResolver fell through to a lead/architect tab map for the same terminal and could resolve the wrong role.

Fix: SessionManager now keeps a depth-counted "releasing" marker per pane (a ConcurrentHashMap written via compute, not a Set), written before the registry removal and cleared in a finally wrapping the removal through releaseRemoved. A depth count is required rather than a Set because two threads can race teardown of the same pane; a Set-based unmark by the losing thread would reopen the window while the winning thread is still mid-teardown.

Both removal sites (the unconditional release() and the idle reaper's CAS releaseIfCurrent()) go through one shared helper, so all four entry routes (release, the context-cap release inside completeTurn, reapIdle, drainSnapshot) are covered.

One reader: SessionManager.spawnedMemberRole checks the live registry first (via the existing no-copy findByTerminal), then the releasing marker. FleetdAssembly now wires this method reference in place of its own untested inline lambda (also fixes the untested-wiring shape #589 swept for, and drops a roster() list copy + stream from the per-request hot path).

CallerResolver itself is unchanged — its contract already fit; teaching the auth layer about teardown would have been the wrong direction.

Rejected alternative (noted for reviewers): a RELEASING registry state was considered and rejected, because roster() has 18 distinct read sites (metrics, health monitor, heartbeat, spawn capacity, fleet_list) that would all be affected; the marker approach touches only one reader.

Tests: SessionManagerTest gets 4 new tests directly exercising spawnedMemberRole, including a nested/overlapping-release test proving the depth count (not a plain Set) is required. CallerResolverTest gets 1 new integration test proving the property no tab map is consulted during the teardown window, with a mandatory control (resolved via the same lead tab map, taken outside the window) proving the in-window assertion is not vacuous.

Build: mvn -o clean install from the worktree — Tests run: 2031, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Caveat for review: I did not add a FleetdAssembly-boot-level wiring test for the method-reference change at the old line ~485; I judged a full daemon-assembly integration test disproportionate and relied on the SessionManagerTest unit coverage plus the CallerResolverTest integration test (which exercises the production sessions::spawnedMemberRole reference directly) as wiring coverage instead. Flagging this explicitly rather than folding it in silently, per the brief.

Unverified: whether herdr ever reuses a terminal_id was left unverified per the brief's own allowance — the depth-count marker makes this a narrow race rather than a leak either way, but I did not do a definitive code-level check to settle it.

Documentation: this is an internal contract change that belongs in wiki/9-Implementation.md, but wiki/ is an uninitialized submodule in this worktree, so I did not attempt it. See the PR description / fleet_reply for the draft text.

Fixes #702. A caller from a pane being torn down was briefly absent from the session registry, so CallerResolver fell through to a lead/architect tab map for the same terminal and could resolve the wrong role. **Fix**: SessionManager now keeps a depth-counted "releasing" marker per pane (a ConcurrentHashMap written via compute, not a Set), written before the registry removal and cleared in a finally wrapping the removal through releaseRemoved. A depth count is required rather than a Set because two threads can race teardown of the same pane; a Set-based unmark by the losing thread would reopen the window while the winning thread is still mid-teardown. Both removal sites (the unconditional release() and the idle reaper's CAS releaseIfCurrent()) go through one shared helper, so all four entry routes (release, the context-cap release inside completeTurn, reapIdle, drainSnapshot) are covered. **One reader**: SessionManager.spawnedMemberRole checks the live registry first (via the existing no-copy findByTerminal), then the releasing marker. FleetdAssembly now wires this method reference in place of its own untested inline lambda (also fixes the untested-wiring shape #589 swept for, and drops a roster() list copy + stream from the per-request hot path). CallerResolver itself is unchanged — its contract already fit; teaching the auth layer about teardown would have been the wrong direction. **Rejected alternative** (noted for reviewers): a RELEASING registry state was considered and rejected, because roster() has 18 distinct read sites (metrics, health monitor, heartbeat, spawn capacity, fleet_list) that would all be affected; the marker approach touches only one reader. **Tests**: SessionManagerTest gets 4 new tests directly exercising spawnedMemberRole, including a nested/overlapping-release test proving the depth count (not a plain Set) is required. CallerResolverTest gets 1 new integration test proving the property **no tab map is consulted** during the teardown window, with a mandatory control (resolved via the same lead tab map, taken outside the window) proving the in-window assertion is not vacuous. **Build**: mvn -o clean install from the worktree — Tests run: 2031, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. **Caveat for review**: I did not add a FleetdAssembly-boot-level wiring test for the method-reference change at the old line ~485; I judged a full daemon-assembly integration test disproportionate and relied on the SessionManagerTest unit coverage plus the CallerResolverTest integration test (which exercises the production sessions::spawnedMemberRole reference directly) as wiring coverage instead. Flagging this explicitly rather than folding it in silently, per the brief. **Unverified**: whether herdr ever reuses a terminal_id was left unverified per the brief's own allowance — the depth-count marker makes this a narrow race rather than a leak either way, but I did not do a definitive code-level check to settle it. **Documentation**: this is an internal contract change that belongs in wiki/9-Implementation.md, but wiki/ is an uninitialized submodule in this worktree, so I did not attempt it. See the PR description / fleet_reply for the draft text.
agent added 1 commit 2026-10-04 09:27:35 +02:00
fleetd #702: mark a pane as mid-teardown so a caller still resolves as a member
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 49s
Details
CI / build (pull_request) Failing after 1m51s
Details
efeffb4ab7 
A caller from a pane being torn down used to be briefly absent from the
session registry, so CallerResolver fell through to a lead/architect tab map
for the same terminal and could resolve the wrong role.

SessionManager now keeps a depth-counted "releasing" marker per pane, written
before the registry removal and cleared in a finally once release finishes.
A depth count (not a Set) is needed because two threads can race teardown of
the same pane; a Set-based unmark by the losing thread would reopen the
window while the winning thread is still mid-teardown. Both removal sites
(the unconditional release() and the idle reaper's CAS releaseIfCurrent())
go through one shared helper, so all four entry routes (release, the
context-cap release in completeTurn, reapIdle, drainSnapshot) are covered.

SessionManager.spawnedMemberRole is the one reader: it checks the live
registry first (via the existing no-copy findByTerminal), then the releasing
marker. FleetdAssembly now wires this method reference instead of its own
untested inline lambda, which also drops a roster() list copy + stream from
the per-request hot path. CallerResolver is unchanged — its contract already
fit.
agent added 1 commit 2026-10-04 10:04:07 +02:00
fleetd #702: pin the architect-demotion window and split a thin test
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 1m3s
Details
CI / build (pull_request) Failing after 2m8s
Details
efab9b8c49 
Add a CallerResolverTest case proving a releasing architect resolves as
WORKER while still inside releaseRemoved's teardown window, with a
control resolve outside the window that must stay ARCHITECT so the
in-window assertion cannot pass against a slot that was never bound.

Split spawnedMemberRoleSurvivesAnOverlappingReleaseThatUnmarksEarly into
two SessionManagerTest cases, each reaching Releasing.enter/leave
directly through reflection so a mutation to one invariant (the depth
count in leave, or enter's prior-terminal preservation) can only fail
its own test.
ltms commented 2026-10-04 10:26:19 +02:00
Owner
Copy Link
Copy Source

Merged locally into main as 38f4fd6, plus one review commit of mine, 8d3f10d. Pushed
(3fc39b9..8d3f10d).

The merge tree was not the branch tree (main had moved — #715 landed first), so I built the
merge myself: mvn -o clean install, Maven exit 0, BUILD SUCCESS,
Tests run: 2047, Failures: 0, Errors: 0, Skipped: 0. Four mutations, all killed; M1 and M2 each
failed exactly one test, which is what the split was for.

My review commit fixes a javadoc claim that named the wrong caller (and the same claim copied into a
test's failure message), and replaces the six reflection helpers with a compile-time binding by
dropping private from Releasing. Full record on #702 (comment 18817).

We merge locally, so Gitea does not close this PR by itself. Closing it by hand.

Merged locally into `main` as `38f4fd6`, plus one review commit of mine, `8d3f10d`. Pushed (`3fc39b9..8d3f10d`). The merge tree was **not** the branch tree (`main` had moved — #715 landed first), so I built the merge myself: `mvn -o clean install`, Maven exit 0, `BUILD SUCCESS`, `Tests run: 2047, Failures: 0, Errors: 0, Skipped: 0`. Four mutations, all killed; M1 and M2 each failed exactly one test, which is what the split was for. My review commit fixes a javadoc claim that named the wrong caller (and the same claim copied into a test's failure message), and replaces the six reflection helpers with a compile-time binding by dropping `private` from `Releasing`. Full record on #702 (comment 18817). We merge locally, so Gitea does not close this PR by itself. Closing it by hand.
ltms closed this pull request 2026-10-04 10:26:22 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 1m3s
Details
CI / build (pull_request) Failing after 2m8s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#724
Delete Branch "worker/702-4f5c7f-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?