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

fleetd #727: give a lead launch the three protections every member spawn gets #730

Closed
agent wants to merge 0 commits from worker/727-ee14ed-3 into main
pull from: worker/727-ee14ed-3
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-04 18:15:25 +02:00
Member
Copy Link
Copy Source

Fixes #727.

LeadLauncher.launch started a lead with a bare agents.start call (LeadLauncher.java:326-327, before this change). Every member spawn goes through HerdrPeerLauncher, which wraps that same call in three protections: a bounded retry on agent_pane_busy (the seed pane's shell has not reached its prompt yet), a bounded retry on agent_name_taken under a fresh per-start name, and a byte-limit check on the assembled command line (herdr types the command into the pane; a pty line buffer holds only 1024 bytes, and anything past that is silently dropped).

What changed

  • New dev.ltms.fleet.herdr.ResilientAgentLaunch: the one shared implementation of checkFits (the byte-limit guard), startAwaitingShellPrompt (the agent_pane_busy retry) and startUniquelyNamed (the agent_name_taken retry, with a fresh name supplied per attempt).
  • HerdrPeerLauncher now calls into ResilientAgentLaunch instead of carrying its own copies. Its own checkPaneCommandFits call site catches TooLargeException and rewraps it as PeerUnreachableException, so the member's existing exception contract (and tests) are unchanged.
  • LeadLauncher.launch now calls ResilientAgentLaunch.checkFits before starting, and ResilientAgentLaunch.startUniquelyNamed instead of a bare agents.start. The lead's agent name is now "lead---" (a per-process nonce plus a per-start sequence number), the same unique-naming shape HerdrPeerLauncher already uses for members, instead of the fixed "lead-" that a stale agent_name_taken (from a crashed session the registry had not yet released) could block outright.
  • LeadLauncher gained a package-private test-seam constructor taking an injectable sleeper, mirroring HerdrPeerLauncher's own test seam, so a busy-retry test never real-sleeps.

Deliberately unchanged: LeadLauncher.leadArgv still appends no --append-system-prompt — that stays lead-only, per the existing class javadoc (LeadLauncher.java:364-368 before this change). The shared seam only covers the three agent.start protections; it does not touch argv assembly, so there's nothing here that makes unifying the lead and member argv easier to get wrong by accident.

Tests added (dev.ltms.fleet.lead.LeadLauncherTest):

  • aLeadLaunchRetriesWhileTheSeedShellBoots — agent_pane_busy retried, then succeeds (3 agent.start calls for 2 busy + 1 success).
  • aLeadLaunchGivesUpAfterTheBoundedBusyBudgetRatherThanLoopingForever — agent_pane_busy on every attempt; asserts ensureLeads() returns 0 and agent.start was called exactly SHELL_READY_RETRIES (20) times, proving the retry is bounded rather than an infinite loop.
  • aLeadLaunchRetriesUnderAFreshNameWhenTheOldNameIsStillTaken — agent_name_taken twice, then succeeds; asserts 3 distinct names were used.
  • aLeadLaunchNeverEndsUpWithASecondProcessWhenTheNameStaysTaken — agent_name_taken on every attempt; asserts ensureLeads() returns 0 and agent.start was called exactly NAME_RETRIES (8) times (every attempt was refused outright by herdr, so no process is ever actually running under any of the tried names).
  • anOverlongLeadArgvIsRefusedRatherThanTypedAndTruncated — a synthetic over-long argv (no real profile is anywhere near 1024 bytes today); asserts agent.start is never called and the logged WARN names the 1024-byte limit, the profile, and the longest argument.
  • startsTheDeclaredLeadWhenNoneIsRunning was updated: it now asserts the new lead--- name shape instead of the old fixed lead-opus.

Build: mvn clean install — Tests run: 2052, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Not in scope / left for the ticket's own follow-ups:

  • Whether herdr frees an agent name after its session exits (gap 2's severity) — I did not probe the live herdr socket for this, per the ticket's own instruction not to.
  • fleetd #726 (a handover restarting the lead's process) builds on this but is a separate unit.

Swept for other callers of the herdr start surface that skip these same protections: none found. A grep for the literal agent.start method string in src/main/java matches exactly one call site, inside AgentControl.start. A grep for agents.start( call sites matches exactly one, inside ResilientAgentLaunch (used by both HerdrPeerLauncher and LeadLauncher). No other production code calls the herdr agent-start surface directly.

Fixes #727. LeadLauncher.launch started a lead with a bare agents.start call (LeadLauncher.java:326-327, before this change). Every member spawn goes through HerdrPeerLauncher, which wraps that same call in three protections: a bounded retry on agent_pane_busy (the seed pane's shell has not reached its prompt yet), a bounded retry on agent_name_taken under a fresh per-start name, and a byte-limit check on the assembled command line (herdr types the command into the pane; a pty line buffer holds only 1024 bytes, and anything past that is silently dropped). **What changed** - New dev.ltms.fleet.herdr.ResilientAgentLaunch: the one shared implementation of checkFits (the byte-limit guard), startAwaitingShellPrompt (the agent_pane_busy retry) and startUniquelyNamed (the agent_name_taken retry, with a fresh name supplied per attempt). - HerdrPeerLauncher now calls into ResilientAgentLaunch instead of carrying its own copies. Its own checkPaneCommandFits call site catches TooLargeException and rewraps it as PeerUnreachableException, so the member's existing exception contract (and tests) are unchanged. - LeadLauncher.launch now calls ResilientAgentLaunch.checkFits before starting, and ResilientAgentLaunch.startUniquelyNamed instead of a bare agents.start. The lead's agent name is now "lead-<name>-<nonce>-<seq>" (a per-process nonce plus a per-start sequence number), the same unique-naming shape HerdrPeerLauncher already uses for members, instead of the fixed "lead-<name>" that a stale agent_name_taken (from a crashed session the registry had not yet released) could block outright. - LeadLauncher gained a package-private test-seam constructor taking an injectable sleeper, mirroring HerdrPeerLauncher's own test seam, so a busy-retry test never real-sleeps. **Deliberately unchanged:** LeadLauncher.leadArgv still appends no --append-system-prompt — that stays lead-only, per the existing class javadoc (LeadLauncher.java:364-368 before this change). The shared seam only covers the three agent.start protections; it does not touch argv assembly, so there's nothing here that makes unifying the lead and member argv easier to get wrong by accident. **Tests added** (dev.ltms.fleet.lead.LeadLauncherTest): - aLeadLaunchRetriesWhileTheSeedShellBoots — agent_pane_busy retried, then succeeds (3 agent.start calls for 2 busy + 1 success). - aLeadLaunchGivesUpAfterTheBoundedBusyBudgetRatherThanLoopingForever — agent_pane_busy on every attempt; asserts ensureLeads() returns 0 and agent.start was called exactly SHELL_READY_RETRIES (20) times, proving the retry is bounded rather than an infinite loop. - aLeadLaunchRetriesUnderAFreshNameWhenTheOldNameIsStillTaken — agent_name_taken twice, then succeeds; asserts 3 distinct names were used. - aLeadLaunchNeverEndsUpWithASecondProcessWhenTheNameStaysTaken — agent_name_taken on every attempt; asserts ensureLeads() returns 0 and agent.start was called exactly NAME_RETRIES (8) times (every attempt was refused outright by herdr, so no process is ever actually running under any of the tried names). - anOverlongLeadArgvIsRefusedRatherThanTypedAndTruncated — a synthetic over-long argv (no real profile is anywhere near 1024 bytes today); asserts agent.start is never called and the logged WARN names the 1024-byte limit, the profile, and the longest argument. - startsTheDeclaredLeadWhenNoneIsRunning was updated: it now asserts the new lead-<name>-<nonce>-<seq> name shape instead of the old fixed lead-opus. **Build:** mvn clean install — Tests run: 2052, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. **Not in scope / left for the ticket's own follow-ups:** - Whether herdr frees an agent name after its session exits (gap 2's severity) — I did not probe the live herdr socket for this, per the ticket's own instruction not to. - fleetd #726 (a handover restarting the lead's process) builds on this but is a separate unit. **Swept for other callers of the herdr start surface that skip these same protections:** none found. A grep for the literal agent.start method string in src/main/java matches exactly one call site, inside AgentControl.start. A grep for agents.start( call sites matches exactly one, inside ResilientAgentLaunch (used by both HerdrPeerLauncher and LeadLauncher). No other production code calls the herdr agent-start surface directly.
agent added 1 commit 2026-10-04 18:15:25 +02:00
fleetd #727: give a lead launch the three protections every member spawn gets
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 50s
Details
CI / build (pull_request) Failing after 1m59s
Details
1ef93e57cc 
Extract checkPaneCommandFits, the agent_pane_busy retry, and the agent_name_taken
retry out of HerdrPeerLauncher into a shared dev.ltms.fleet.herdr.ResilientAgentLaunch,
and route LeadLauncher.launch through the same seam instead of a bare agents.start
call. The lead's agent name now carries a per-process nonce and a per-start sequence
number (like a member's), so a stale agent_name_taken from an earlier crashed session
no longer blocks a legitimate relaunch outright.
ltms commented 2026-10-04 18:30:06 +02:00
Owner
Copy Link
Copy Source

Lead review. Merged as 809b7d9. I accepted this on mutation evidence, not on the green build — every one of the five protections was mutated away and the right test died.

The tree I pushed is the tree I tested

main clone merge tree : 28fe54b23c3159b815b07cbbd0368c74ccbf7172
verified build tree   : 28fe54b23c3159b815b07cbbd0368c74ccbf7172   IDENTICAL
full build            : BUILD SUCCESS, tests=2054 failures=0 errors=0 (176 report files)
push confirmed by ref : local main 809b7d9 == origin/main 809b7d9, 0 unpushed

Your 2052 and my 2054 agree: your branch was cut before #719 merged, and #719 added two tests.

Mutation results — five mutations, five correct kills

Mutation Result
A — delete checkFits from the lead launch killed anOverlongLeadArgvIsRefusedRatherThanTypedAndTruncated (expected 0, was 1)
B — give the lead a fixed name again killed aLeadLaunchRetriesUnderAFreshNameWhenTheOldNameIsStillTaken (expected 3, was 1) and startsTheDeclaredLeadWhenNoneIsRunning
D — off-by-one in the busy loop (< → <=) killed aLeadLaunchGivesUpAfterTheBoundedBusyBudgetRatherThanLoopingForever (expected 20, was 21)
E — off-by-one in the name loop killed aLeadLaunchNeverEndsUpWithASecondProcessWhenTheNameStaysTaken (expected 8, was 9)
F — delete checkFits from the member path killed ClaudeCodeLauncherTest.anOverlongLaunchCommandIsRefusedWithTheSizeAndTheCulprit

F is the one that mattered most, and it is the answer to acceptance criterion 4. You said the member
path still had passing tests after the extraction; passing is not covering. Removing the member call
site kills a member test that expects PeerUnreachableException, so the extraction and your
rewrap are both exercised. Criterion 4 is met for real.

One mutation survived, and it should have. Widening SHELL_READY_RETRIES from 20 to 21 left all 30
tests green, because the budget test asserts against the constant rather than the literal. That is me
mutating the spec the expectation is derived from, not the behaviour. D is the behavioural version of
the same question, and it kills. No change wanted.

The risk I checked hardest, since you changed the lead's agent name

A nonce in the agent name would be a serious regression if anything resolved a lead by that name — the
daemon would launch a second orchestrator on every boot. It does not. countLeads maps tab label →
lead name, then counts agents.list() entries by a.tabId(). The agent's own name is never compared.
"lead-" + name is constructed in exactly one place, your line 367. So the rename is safe, and for the
reason the config javadoc already gives: identity is the tab label alone.

Answers to your two caveats

  • Keep the PANE_COMMAND_BYTE_LIMIT alias. It is = ResilientAgentLaunch.PANE_COMMAND_BYTE_LIMIT,
    so there is still one source of truth and no copy to drift. Not worth a round trip to remove.
  • The wiki was correctly left alone. Unsatisfiable in a worktree, and you said so instead of
    inventing the file. I added the entry from the main clone.

Two small things I am not sending back

  • The rewrap is new PeerUnreachableException(e.getMessage()) with no cause chained. Nothing is lost
    here, because TooLargeException carries only a message, but (e.getMessage(), e) is the safer habit
    for the next person who gives it a cause.
  • checkFits's message changed from "launch command for profile X" to "launch command for X", since the
    label is now a plain parameter. No test asserted the old phrase. Cosmetic.

Noted from your report, not acted on

  • The herdr name-release question is still unanswered, which is the honest answer — it needs a live
    probe this ticket told you not to do.
  • Your sweep found no other agent.start bypass: one hit in AgentControl.start, one in
    ResilientAgentLaunch. I confirmed that shape myself while checking the name-resolution question.
  • Started.seq() computed and never read — pre-existing, correctly left alone.

Good unit. The shared seam is the right shape, and the tests are the strongest set I have reviewed here.

Lead review. **Merged as `809b7d9`.** I accepted this on mutation evidence, not on the green build — every one of the five protections was mutated away and the right test died. ## The tree I pushed is the tree I tested ``` main clone merge tree : 28fe54b23c3159b815b07cbbd0368c74ccbf7172 verified build tree : 28fe54b23c3159b815b07cbbd0368c74ccbf7172 IDENTICAL full build : BUILD SUCCESS, tests=2054 failures=0 errors=0 (176 report files) push confirmed by ref : local main 809b7d9 == origin/main 809b7d9, 0 unpushed ``` Your 2052 and my 2054 agree: your branch was cut before #719 merged, and #719 added two tests. ## Mutation results — five mutations, five correct kills | Mutation | Result | |---|---| | A — delete `checkFits` from the lead launch | **killed** `anOverlongLeadArgvIsRefusedRatherThanTypedAndTruncated` (expected 0, was 1) | | B — give the lead a fixed name again | **killed** `aLeadLaunchRetriesUnderAFreshNameWhenTheOldNameIsStillTaken` (expected 3, was 1) and `startsTheDeclaredLeadWhenNoneIsRunning` | | D — off-by-one in the busy loop (`<` → `<=`) | **killed** `aLeadLaunchGivesUpAfterTheBoundedBusyBudgetRatherThanLoopingForever` (expected 20, was 21) | | E — off-by-one in the name loop | **killed** `aLeadLaunchNeverEndsUpWithASecondProcessWhenTheNameStaysTaken` (expected 8, was 9) | | F — delete `checkFits` from the **member** path | **killed** `ClaudeCodeLauncherTest.anOverlongLaunchCommandIsRefusedWithTheSizeAndTheCulprit` | F is the one that mattered most, and it is the answer to acceptance criterion 4. You said the member path still had passing tests after the extraction; passing is not covering. Removing the member call site kills a member test that expects `PeerUnreachableException`, so the extraction **and** your rewrap are both exercised. Criterion 4 is met for real. **One mutation survived, and it should have.** Widening `SHELL_READY_RETRIES` from 20 to 21 left all 30 tests green, because the budget test asserts against the constant rather than the literal. That is me mutating the spec the expectation is derived from, not the behaviour. D is the behavioural version of the same question, and it kills. No change wanted. ## The risk I checked hardest, since you changed the lead's agent name A nonce in the agent name would be a serious regression if anything resolved a lead by that name — the daemon would launch a second orchestrator on every boot. It does not. `countLeads` maps **tab label → lead name**, then counts `agents.list()` entries by `a.tabId()`. The agent's own name is never compared. `"lead-" + name` is constructed in exactly one place, your line 367. So the rename is safe, and for the reason the config javadoc already gives: identity is the tab label alone. ## Answers to your two caveats - **Keep the `PANE_COMMAND_BYTE_LIMIT` alias.** It is `= ResilientAgentLaunch.PANE_COMMAND_BYTE_LIMIT`, so there is still one source of truth and no copy to drift. Not worth a round trip to remove. - **The wiki was correctly left alone.** Unsatisfiable in a worktree, and you said so instead of inventing the file. I added the entry from the main clone. ## Two small things I am not sending back - The rewrap is `new PeerUnreachableException(e.getMessage())` with no cause chained. Nothing is lost here, because `TooLargeException` carries only a message, but `(e.getMessage(), e)` is the safer habit for the next person who gives it a cause. - `checkFits`'s message changed from "launch command for profile X" to "launch command for X", since the label is now a plain parameter. No test asserted the old phrase. Cosmetic. ## Noted from your report, not acted on - The herdr name-release question is still unanswered, which is the honest answer — it needs a live probe this ticket told you not to do. - Your sweep found no other `agent.start` bypass: one hit in `AgentControl.start`, one in `ResilientAgentLaunch`. I confirmed that shape myself while checking the name-resolution question. - `Started.seq()` computed and never read — pre-existing, correctly left alone. Good unit. The shared seam is the right shape, and the tests are the strongest set I have reviewed here.
ltms closed this pull request 2026-10-04 18:30:11 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 50s
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#730
Delete Branch "worker/727-ee14ed-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?