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

CB-613: one worker branch holds a teardown test that never reached main, and 55 stale branches hide it #116

New Issue
Closed
opened 2026-08-17 14:25:58 +02:00 by ltms · 1 comment
ltms commented 2026-08-17 14:25:58 +02:00
Owner
Copy Link
Copy Source

Found in a pre-tag sweep on 2026-08-17 of every worker branch on the forge.

What the sweep found

56 remote worker/* branches exist. 7 hold commits not on main. Of those:

  • 4 re-landed under different SHAs — CB-548, CB-553, CB-568, CB-566. Confirmed by git cherry, which reports their patches as already upstream.
  • 2 are on main in improved form — CB-573's TurnToken exists there with the same javadoc; CB-577's fail-on-terminal-health shipped with failTarget made a required constructor parameter rather than the branch's optional overload.
  • 1 is genuinely absent.

The lost work

worker/cb576-01a04b-17 (c393600, 2026-08-15) adds one test and a fake helper:

@Test
void releaseStillStopsPaneAndNotifiesWhenWorktreeIsGone()

It asserts that when a worktree is already gone — operator cleanup, git worktree prune, a half-completed earlier release — teardown still:

  1. fires notifyReleased (CB-516's fast-fail for a blocked bridge_send caller),
  2. stops the pane, so it is not orphaned,
  3. falls through to the already-gone-tolerant remove.

Neither the test nor its FakeWorktrees.markGone helper exists on main. Verified by grepping the whole test tree for the method name and for markGone: no hits.

Severity: coverage gap, not a live defect

The behaviour itself is safe on main today, and safer than the branch assumed. CB-581 later restructured SessionManager.release so the notify-and-stop path runs in a finally block (SessionManager.java:298-304), which makes the already-gone case correct by construction rather than by a checked branch. The javadoc there cites CB-576 by name.

So nothing is broken. What is missing is the regression test that would catch it if someone reorganises that method again — and this is exactly the teardown-path case that has bitten this repo before: a diff that registers state on entry and clears it on only some of the exits.

What to do

  1. Port releaseStillStopsPaneAndNotifiesWhenWorktreeIsGone and FakeWorktrees.markGone onto current main. The assertions may need adjusting to CB-581's finally structure — port the intent, not the diff.
  2. Prove it is a real guard: remove or bypass the finally and confirm the test fails. A test that passes both with and without the protection is not protecting anything.
  3. Then delete the stale branches. 55 of the 56 hold nothing that is not on main. They are why a single missing test took a scripted sweep to find. Keep worker/cb576-01a04b-17 until step 1 lands.

Acceptance criteria

  1. The test exists on main and fails when the teardown ordering is broken.
  2. Every worker branch whose content is on main is deleted from the forge.
  3. A note on how this is prevented: either branches are deleted at merge, or a periodic sweep reports branches with unmerged commits. Deleting 55 branches once and letting them accumulate again solves nothing.

Milestone

2.0. No behaviour is wrong on one host, and the protection the test describes is already in the code by construction.

Related

The general lesson is already recorded: count the exits from a method that registers state, not just the happy path. CB-581 fixed this instance; this ticket restores the test that would prove it stays fixed.

Found in a pre-tag sweep on 2026-08-17 of every worker branch on the forge. ## What the sweep found 56 remote `worker/*` branches exist. 7 hold commits not on `main`. Of those: - **4 re-landed under different SHAs** — CB-548, CB-553, CB-568, CB-566. Confirmed by `git cherry`, which reports their patches as already upstream. - **2 are on `main` in improved form** — CB-573's `TurnToken` exists there with the same javadoc; CB-577's fail-on-terminal-health shipped with `failTarget` made a **required** constructor parameter rather than the branch's optional overload. - **1 is genuinely absent.** ## The lost work `worker/cb576-01a04b-17` (`c393600`, 2026-08-15) adds one test and a fake helper: ```java @Test void releaseStillStopsPaneAndNotifiesWhenWorktreeIsGone() ``` It asserts that when a worktree is **already gone** — operator cleanup, `git worktree prune`, a half-completed earlier release — teardown still: 1. fires `notifyReleased` (CB-516's fast-fail for a blocked `bridge_send` caller), 2. stops the pane, so it is not orphaned, 3. falls through to the already-gone-tolerant `remove`. Neither the test nor its `FakeWorktrees.markGone` helper exists on `main`. Verified by grepping the whole test tree for the method name and for `markGone`: no hits. ## Severity: coverage gap, not a live defect The behaviour itself is safe on `main` today, and safer than the branch assumed. CB-581 later restructured `SessionManager.release` so the notify-and-stop path runs in a **`finally`** block (`SessionManager.java:298-304`), which makes the already-gone case correct by construction rather than by a checked branch. The javadoc there cites CB-576 by name. So nothing is broken. What is missing is the regression test that would catch it if someone reorganises that method again — and this is exactly the teardown-path case that has bitten this repo before: a diff that registers state on entry and clears it on only some of the exits. ## What to do 1. Port `releaseStillStopsPaneAndNotifiesWhenWorktreeIsGone` and `FakeWorktrees.markGone` onto current `main`. The assertions may need adjusting to CB-581's `finally` structure — port the *intent*, not the diff. 2. Prove it is a real guard: remove or bypass the `finally` and confirm the test fails. A test that passes both with and without the protection is not protecting anything. 3. **Then delete the stale branches.** 55 of the 56 hold nothing that is not on `main`. They are why a single missing test took a scripted sweep to find. Keep `worker/cb576-01a04b-17` until step 1 lands. ## Acceptance criteria 1. The test exists on `main` and fails when the teardown ordering is broken. 2. Every worker branch whose content is on `main` is deleted from the forge. 3. A note on how this is prevented: either branches are deleted at merge, or a periodic sweep reports branches with unmerged commits. Deleting 55 branches once and letting them accumulate again solves nothing. ## Milestone **2.0.** No behaviour is wrong on one host, and the protection the test describes is already in the code by construction. ## Related The general lesson is already recorded: count the **exits** from a method that registers state, not just the happy path. CB-581 fixed this instance; this ticket restores the test that would prove it stays fixed.
ltms added this to the 2.0 — one operation centre, many hosts milestone 2026-08-17 14:25:58 +02:00
ltms commented 2026-09-03 11:18:57 +02:00
Author
Owner
Copy Link
Copy Source

Parts 1 and 2 are done. PR #260 is merged to main as 3bad9f5. Full build: 1258 tests, 0 failures.

Acceptance criterion 1 is only partly met, and I want that on the record.

The criterion asked for a test that "fails when the teardown ordering is broken" — that is, a test that guards the finally block in SessionManager.release. The new test does not do that. I read release() myself to check.

Here is why. For an already-gone worktree, hasUncommitted returns false cleanly. It never throws, because GitWorktrees.hasUncommitted has a Files.exists guard. So on this path no exception is ever raised. That makes "inside finally" and "at the end of try" the same thing. No test on this scenario can tell them apart.

The worker was honest about this and said so in the PR. To get a red it added a dirty gate on top of the move. That proves the assertion runs. It does not prove the finally is load-bearing.

What the test does guard, and it is worth keeping. I ran my own mutation, one a person could plausibly write:

if (false && removed != null && !preserveWorktree && removed.worktree() != null) {

The test failed at WorktreeSessionManagerTest.java:289, 3 of 22 in the class. So the test does hold real behaviour: when a member's worktree is already gone, release still notifies the blocked caller, still stops the pane, and still falls through to the tolerant remove. That is the CB-576 regression this ticket wanted back.

Still open, and left for the operator: part 3, branch cleanup. The ticket said ~56 branches. The real count is 116 remote worker/* branches. 105 have their content on main already. 11 do not: cb-157, cb-161, cb-164 (x2), cb-172, cb-175, cb-633 (x2), cb573b, cb576, cb577. I deleted nothing. Deleting a branch is not reversible from here, so that is the operator's call, and the 11 need a look first.

Closing for parts 1 and 2. Part 3 should be its own ticket if it is still wanted.

Parts 1 and 2 are done. PR #260 is merged to `main` as `3bad9f5`. Full build: 1258 tests, 0 failures. **Acceptance criterion 1 is only partly met, and I want that on the record.** The criterion asked for a test that "fails when the teardown ordering is broken" — that is, a test that guards the `finally` block in `SessionManager.release`. The new test does **not** do that. I read `release()` myself to check. Here is why. For an already-gone worktree, `hasUncommitted` returns `false` cleanly. It never throws, because `GitWorktrees.hasUncommitted` has a `Files.exists` guard. So on this path no exception is ever raised. That makes "inside `finally`" and "at the end of `try`" the same thing. No test on this scenario can tell them apart. The worker was honest about this and said so in the PR. To get a red it added a `dirty` gate on top of the move. That proves the assertion runs. It does not prove the `finally` is load-bearing. **What the test does guard, and it is worth keeping.** I ran my own mutation, one a person could plausibly write: ```java if (false && removed != null && !preserveWorktree && removed.worktree() != null) { ``` The test failed at `WorktreeSessionManagerTest.java:289`, 3 of 22 in the class. So the test does hold real behaviour: when a member's worktree is already gone, `release` still notifies the blocked caller, still stops the pane, and still falls through to the tolerant `remove`. That is the CB-576 regression this ticket wanted back. **Still open, and left for the operator:** part 3, branch cleanup. The ticket said ~56 branches. The real count is 116 remote `worker/*` branches. 105 have their content on `main` already. 11 do not: `cb-157`, `cb-161`, `cb-164` (x2), `cb-172`, `cb-175`, `cb-633` (x2), `cb573b`, `cb576`, `cb577`. I deleted nothing. Deleting a branch is not reversible from here, so that is the operator's call, and the 11 need a look first. Closing for parts 1 and 2. Part 3 should be its own ticket if it is still wanted.
ltms closed this issue 2026-09-03 11:19:02 +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 2.0 — one operation centre, many hosts
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#116
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?