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

A member that loses the architect slot race still runs on the architect charter while the gate treats it as a worker #226

New Issue
Closed
opened 2026-09-01 10:42:14 +02:00 by ltms · 1 comment
ltms commented 2026-09-01 10:42:14 +02:00
Owner
Copy Link
Copy Source

Found by the reviewer on PR #223 (#123), and independently reported by that PR's author as a known residual. I checked the path in the code before filing.

#123 closed the config-gap case: asking for role=architect on a profile that no configured slot carries is now refused before anything spawns. This is the race case it deliberately left open, and it is worth its own ticket because the fix is a different shape.

The path

SessionManager.acquire (around SessionManager.java:208):

  1. requireSlotFor(ARCHITECT, profile) passes — the config does carry a slot for this profile.
  2. launcher.spawn(new SpawnRequest(..., ARCHITECT)) runs. HerdrPeerLauncher.spawnInternal builds the member's charter from the requested role, so the process starts on the architect charter and reads the architect agent definition.
  3. memberLifecycle.acquired(...) then finds every matching slot already bound to a different terminal, logs the new WARN, and returns DEV.
  4. The session is recorded as dev, and fleet_whoami resolves it as a worker.

So the member reads "you are an architect" and is told "you are a worker" by the one call the charter instructs it to trust. That is the original #123 symptom, surviving in a narrower window.

What #123 did and did not fix

It is strictly better than before, which is why PR #223 was merged rather than held:

before #123 after #123 after this ticket
roster (GET /members, fleet_list) says architect — a lie says dev — honest honest
log INFO, reads as bookkeeping WARN, names the terminal not reached
charter the member reads architect architect matches the bound role
fleet_whoami worker worker worker

The remaining damage is bounded. The member acts on an architect charter, so it will attempt architect-only calls and the authorization gate will refuse them — loud and self-correcting, the recoverable direction. It is not silent the way the pre-#123 roster lie was.

Why a pre-check cannot close it

requireSlotFor answers "does the config carry a slot for this profile", which is stable. It cannot answer "will a slot still be free after the launch", because that depends on a concurrent spawn that may not have happened yet. Widening the pre-check to test occupancy just moves the window; it does not remove it.

The reviewer's suggested shape is the right one: reserve a matching slot before launching, bind that reservation after the launch, and release it if the launch fails. Then a spawn that would lose the race is refused before a process exists, and a spawn that starts is guaranteed the role its charter describes.

Acceptance

  • A spawn that cannot hold an architect slot never reaches the launcher, so no member is ever started on a charter that does not match the role it will hold.
  • A launch failure releases the reservation — a failed spawn must not leak a slot, or the pool shrinks silently with every failure. Say in the PR how you convinced yourself of this; it is the way a reserve-then-bind fix usually goes wrong.
  • The existing acquired fallback and its WARN stay. A reservation narrows the window; claiming it removes it is the mistake to avoid, and the honest answer is still needed if the impossible happens.
  • A test drives a real spawn through SessionManager.acquire with the slot contended, and asserts on the charter the member actually received, not only on the roster role. PR #223's tests assert the roster and the WARN but not the charter — that gap is exactly what let this survive.
  • Watch each test fail before keeping it.

Related: #123 / PR #223 (the config-gap half, merged) · #113 (why the test must drive the real spawn, not MemberRegistry.bind).

Found by the reviewer on PR #223 (#123), and independently reported by that PR's author as a known residual. I checked the path in the code before filing. #123 closed the **config-gap** case: asking for `role=architect` on a profile that no configured slot carries is now refused before anything spawns. This is the **race** case it deliberately left open, and it is worth its own ticket because the fix is a different shape. ## The path `SessionManager.acquire` (around `SessionManager.java:208`): 1. `requireSlotFor(ARCHITECT, profile)` passes — the config *does* carry a slot for this profile. 2. `launcher.spawn(new SpawnRequest(..., ARCHITECT))` runs. `HerdrPeerLauncher.spawnInternal` builds the member's charter **from the requested role**, so the process starts on the architect charter and reads the architect agent definition. 3. `memberLifecycle.acquired(...)` then finds every matching slot already bound to a different terminal, logs the new WARN, and returns `DEV`. 4. The session is recorded as `dev`, and `fleet_whoami` resolves it as a worker. So the member reads "you are an architect" and is told "you are a worker" by the one call the charter instructs it to trust. That is the original #123 symptom, surviving in a narrower window. ## What #123 did and did not fix It is strictly better than before, which is why PR #223 was merged rather than held: | | before #123 | after #123 | after this ticket | |---|---|---|---| | roster (`GET /members`, `fleet_list`) | says `architect` — a lie | says `dev` — honest | honest | | log | INFO, reads as bookkeeping | WARN, names the terminal | not reached | | charter the member reads | architect | **architect** | matches the bound role | | `fleet_whoami` | worker | worker | worker | The remaining damage is bounded. The member acts on an architect charter, so it will attempt architect-only calls and the authorization gate will refuse them — loud and self-correcting, the recoverable direction. It is not silent the way the pre-#123 roster lie was. ## Why a pre-check cannot close it `requireSlotFor` answers "does the config carry a slot for this profile", which is stable. It cannot answer "will a slot still be free after the launch", because that depends on a concurrent spawn that may not have happened yet. Widening the pre-check to test occupancy just moves the window; it does not remove it. The reviewer's suggested shape is the right one: **reserve a matching slot before launching, bind that reservation after the launch, and release it if the launch fails.** Then a spawn that would lose the race is refused before a process exists, and a spawn that starts is guaranteed the role its charter describes. ## Acceptance - A spawn that cannot hold an architect slot never reaches the launcher, so no member is ever started on a charter that does not match the role it will hold. - A launch failure releases the reservation — a failed spawn must not leak a slot, or the pool shrinks silently with every failure. Say in the PR how you convinced yourself of this; it is the way a reserve-then-bind fix usually goes wrong. - The existing `acquired` fallback and its WARN stay. A reservation narrows the window; claiming it removes it is the mistake to avoid, and the honest answer is still needed if the impossible happens. - A test drives a real spawn through `SessionManager.acquire` with the slot contended, and asserts on the **charter the member actually received**, not only on the roster role. PR #223's tests assert the roster and the WARN but not the charter — that gap is exactly what let this survive. - Watch each test fail before keeping it. Related: #123 / PR #223 (the config-gap half, merged) · #113 (why the test must drive the real spawn, not `MemberRegistry.bind`).
ltms commented 2026-09-02 03:05:04 +02:00
Author
Owner
Copy Link
Copy Source

Fixed in PR #228, merged to main (cc919aa).

SessionManager.acquire now reserves a matching architect slot before it launches, binds that reservation to the terminal after the launch, and releases it on the failure paths. A spawn that would lose the race is refused before a process exists, so no member is ever started on a charter it will not hold. MemberRegistry grew reserve/bind/release, and its existing bind check now also rejects a slot that is merely reserved.

I checked the locking, because a fix that closes a race is the worst place to open one: every method in MemberRegistry uses synchronized (terminalToSlot), including the new ones, so reservedSlots is never touched outside that monitor. The field's javadoc claimed "guarded by this", which is what made me check — that stale comment is fixed here too.

What I did not take on trust

The worker reported watching the restored fallback test fail first, but quoted expected ARCHITECT but was DEV — the wrong way round for a test that asserts DEV. So I proved it myself: removed the fallback from SessionManager.acquired and re-ran that test alone.

AssertionFailedError: a failed reservation bind must use the fallback
  ==> expected: <DEV> but was: <ARCHITECT>

The test does guard the fallback. The report just transcribed the message backwards. Its decorator delegates acquired to the real MemberRegistry, so the WARN assertion is genuine too, not stubbed.

I also had the worker drop a claim from its PR body. It said the charter receipt "fingerprints exact delivered charter bytes", but HerdrPeerLauncher:453 composes the receipt from the requested role, so receipt.role() is an echo and that assertion would have passed before the fix. The real proof of the charter criterion is the other test's assertFalse(herdr.called("agent.start")): no process starts, so no charter is delivered.

Behaviour change worth knowing

fleet_spawn{role: "architect"} with every slot taken now throws instead of quietly handing back a dev. That is the point of the ticket, but it is visible. Recorded on the Features page.

The acquired fallback and its WARN stay, as the ticket required — a reservation narrows the window, and claiming it removes it is the mistake to avoid.

Verified on main after merge: BUILD SUCCESS, 1100 tests, 0 failures, 0 skipped. Live on the redeployed daemon (pid 29196, jar 4a0e2c8f0eef); a real dev spawn reached idle, so the reservation step did not break the common path.

Fixed in PR #228, merged to `main` (cc919aa). `SessionManager.acquire` now **reserves** a matching architect slot before it launches, **binds** that reservation to the terminal after the launch, and **releases** it on the failure paths. A spawn that would lose the race is refused before a process exists, so no member is ever started on a charter it will not hold. `MemberRegistry` grew `reserve`/`bind`/`release`, and its existing bind check now also rejects a slot that is merely reserved. I checked the locking, because a fix that closes a race is the worst place to open one: every method in `MemberRegistry` uses `synchronized (terminalToSlot)`, including the new ones, so `reservedSlots` is never touched outside that monitor. The field's javadoc claimed "guarded by `this`", which is what made me check — that stale comment is fixed here too. ## What I did not take on trust The worker reported watching the restored fallback test fail first, but quoted `expected ARCHITECT but was DEV` — the wrong way round for a test that asserts `DEV`. So I proved it myself: removed the fallback from `SessionManager.acquired` and re-ran that test alone. ``` AssertionFailedError: a failed reservation bind must use the fallback ==> expected: <DEV> but was: <ARCHITECT> ``` The test does guard the fallback. The report just transcribed the message backwards. Its decorator delegates `acquired` to the real `MemberRegistry`, so the WARN assertion is genuine too, not stubbed. I also had the worker drop a claim from its PR body. It said the charter receipt "fingerprints exact delivered charter bytes", but `HerdrPeerLauncher:453` composes the receipt from the **requested** role, so `receipt.role()` is an echo and that assertion would have passed before the fix. The real proof of the charter criterion is the other test's `assertFalse(herdr.called("agent.start"))`: no process starts, so no charter is delivered. ## Behaviour change worth knowing `fleet_spawn{role: "architect"}` with every slot taken now **throws** instead of quietly handing back a `dev`. That is the point of the ticket, but it is visible. Recorded on the Features page. The `acquired` fallback and its WARN stay, as the ticket required — a reservation narrows the window, and claiming it removes it is the mistake to avoid. Verified on `main` after merge: BUILD SUCCESS, 1100 tests, 0 failures, 0 skipped. Live on the redeployed daemon (pid 29196, jar `4a0e2c8f0eef`); a real dev spawn reached `idle`, so the reservation step did not break the common path.
ltms closed this issue 2026-09-02 03:05:10 +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#226
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?