Compare commits

...

3 Commits

Author SHA1 Message Date
Dai Ha 8308c0b68f fleetd #116: recover the already-gone-worktree teardown regression test
CI / contract (pull_request) Successful in 1m16s
CI / build (pull_request) Successful in 1m23s
Ports the intent of the lost CB-576 commit c393600 (worker/cb576-01a04b-17,
never merged, package dev.ltms.bridged.*) onto main's dev.ltms.fleet.*
tree. Adds FakeWorktrees.markGone (tracks which add()'d worktree paths
still "exist", mirroring GitWorktrees.hasUncommitted's Files.exists guard
for the already-gone case) and a regression test,
releaseStillStopsPaneAndNotifiesWhenWorktreeIsGone, asserting that
SessionManager.release still fires notifyReleased, stops the pane, and
falls through to remove() when the worktree is already gone.
2026-09-03 16:11:27 +07:00
Dai Ha 3b3063eb2b #252: the route scrape counted commented-out registrations as live
CI / build (push) Successful in 1m44s
CI / contract (push) Successful in 2m21s
Follow-up to #259, found by verifying the guard rather than trusting it.

The scrape read FleetApp's raw source, so a registration disabled with `//`
still matched. Measured: commenting out `app.get("/tasks/{ticket}", ...)` left
the test GREEN, while deleting the same line was caught. Only the commented-out
shape was blind, and it is the silent direction — the inventory would keep
claiming a route the server no longer serves.

Drop whole-line comments before scraping. Only lines starting with //, * or /*
are dropped, deliberately not every // on a line: that would also cut a string
literal containing // (a URL) and could silently delete a real registration
sharing the line. The remaining gap is a trailing comment beside real code; no
registration here has that shape, and the vacuity test catches a scrape that
loses registrations wholesale.

Proof: with the fix, the comment-out mutation fails naming
"Removed ... [GET /tasks/{ticket}]"; reverted, FleetApp.java confirmed clean.
mvn clean install: 0 compile errors, 1257 tests, BUILD SUCCESS.
2026-09-03 15:59:35 +07:00
Dai Ha df9086263d Merge #259: guard the REST route inventory against drift (#252) 2026-09-03 15:56:15 +07:00
3 changed files with 77 additions and 3 deletions
@@ -7,6 +7,7 @@ import java.util.Locale;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
@@ -68,6 +69,32 @@ class RestRouteInventoryTest {
"GET /tasks/{ticket}"
);
private static final Pattern ROUTE_CALL =
Pattern.compile("app\\.(get|post|delete|put|patch)\\(\\s*\"([^\"]+)\"");
/**
* Drop whole-line comments before scraping. Without this the scrape reads commented-out code as
* live: a registration disabled with {@code //} still matched, so the route stayed in the
* inventory while the server no longer served it — a silent false PASS, measured on 2026-09-03
* by commenting out {@code app.get("/tasks/{ticket}", ...)} and watching this test stay green.
* Deleting the same line was caught correctly, so only the commented-out shape was blind.
*
* <p>Only lines whose first non-blank characters are {@code //}, {@code *} or {@code /*} are
* dropped — deliberately NOT every {@code //} anywhere on a line, because that would also cut a
* string literal containing {@code //} (a URL) and could silently delete a real registration
* sharing that line. The remaining gap is a trailing comment on the same line as real code; no
* registration in this file has that shape, and the vacuity test below would catch a scrape that
* lost registrations wholesale.
*/
private static String withoutCommentLines(String source) {
return source.lines()
.filter(line -> {
String t = line.stripLeading();
return !(t.startsWith("//") || t.startsWith("*") || t.startsWith("/*"));
})
.collect(Collectors.joining("\n"));
}
/**
* Every {@code app.<verb>("<path>")} call in {@link FleetApp}'s source, as {@code "VERB path"}.
* This matches inside an {@code if (...) { ... }} block just as well as a top-level statement —
@@ -75,8 +102,7 @@ class RestRouteInventoryTest {
* catches {@code GET /metrics} (registered conditionally on {@code metrics != null}).
*/
private static Set<String> routesTheServerRegisters() throws Exception {
String source = Files.readString(REST_SOURCE);
Matcher m = Pattern.compile("app\\.(get|post|delete|put|patch)\\(\\s*\"([^\"]+)\"").matcher(source);
Matcher m = ROUTE_CALL.matcher(withoutCommentLines(Files.readString(REST_SOURCE)));
Set<String> found = new LinkedHashSet<>();
while (m.find()) {
found.add(m.group(1).toUpperCase(Locale.ROOT) + " " + m.group(2));
@@ -45,6 +45,9 @@ public final class FakeWorktrees implements Worktrees {
private final List<String> overlayShareOrder = new CopyOnWriteArrayList<>();
private final Set<String> existingPaths = ConcurrentHashMap.newKeySet();
private final Set<String> trackedPaths = ConcurrentHashMap.newKeySet();
/** Worktree paths that currently exist, mirroring GitWorktrees' {@code Files.exists} check for
* the already-gone case (CB-576 review, fleetd #116). */
private final Set<String> worktreePaths = ConcurrentHashMap.newKeySet();
private final AtomicLong snapshotSeq = new AtomicLong();
private volatile RuntimeException addFailure;
private volatile RuntimeException snapshotFailure;
@@ -115,7 +118,15 @@ public final class FakeWorktrees implements Worktrees {
}
// The branch already carries a unique nonce, so the derived path is distinct per acquire
// without an extra counter — keep it a pure function of the branch the test can predict.
return prefix + "/" + branch.replace('/', '_');
String path = prefix + "/" + branch.replace('/', '_');
worktreePaths.add(path);
return path;
}
/** Model an operator / {@code git worktree prune} removing the worktree before release. */
public FakeWorktrees markGone(String worktreePath) {
worktreePaths.remove(worktreePath);
return this;
}
@Override
@@ -125,6 +136,11 @@ public final class FakeWorktrees implements Worktrees {
@Override
public boolean hasUncommitted(String worktreePath) {
// A path that was never added, or was marked gone, is reported clean, mirroring
// GitWorktrees' already-gone guard — never an error, so teardown still completes.
if (!worktreePaths.contains(worktreePath)) {
return false;
}
return dirty;
}
@@ -258,6 +258,38 @@ class WorktreeSessionManagerTest {
}
}
/**
* CB-576 review (fleetd #116). A worktree that is already gone (operator cleanup,
* {@code git worktree prune}, an earlier half-completed release) must not break teardown.
* {@code hasUncommitted} reports the missing path clean (mirroring {@code GitWorktrees}), so
* {@code release} still runs {@code notifyReleased} (the CB-516 fast-fail for a blocked
* {@code fleet_send} caller) and {@code launcher.stop} (so the pane is not orphaned), and falls
* through to the already-gone-tolerant {@code remove}. Since CB-581 this all happens because the
* notify-and-stop work sits in {@code release}'s {@code finally}/post-try block rather than a
* checked branch — this test pins that shape by construction.
*/
@Test
void releaseStillStopsPaneAndNotifiesWhenWorktreeIsGone() {
FakeHerdr herdr = new FakeHerdr();
FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt");
SessionManager sessions = new SessionManager(workerService(herdr), worktrees);
List<SessionManager.ReleaseDetail> released = new java.util.concurrent.CopyOnWriteArrayList<>();
sessions.onRelease(released::add);
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
new WorktreeRequest("cb-576g", null));
worktrees.markGone(s.worktree());
sessions.release(s.paneId());
assertEquals(1, released.size(),
"notifyReleased must still fire when the worktree is already gone (CB-516)");
assertEquals(s.terminalId(), released.getFirst().terminalId());
assertTrue(herdr.called("pane.close"),
"the pane must still be stopped when the worktree is already gone");
assertEquals(1, worktrees.removeCalls().size(),
"release still calls the already-gone-tolerant remove");
}
@Test
void drainAllPreservesWorktreeOfIdleSession() {
FakeHerdr herdr = new FakeHerdr();