diff --git a/bridged/src/main/java/dev/ltms/bridged/session/SessionReaper.java b/bridged/src/main/java/dev/ltms/bridged/session/SessionReaper.java index 8f8b643..5d66713 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/SessionReaper.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/SessionReaper.java @@ -27,7 +27,16 @@ public final class SessionReaper { private final long intervalMillis; private volatile boolean running; private Thread thread; - private volatile long lastWipSweepNanos = Long.MIN_VALUE; + /** + * When the retention sweep last ran, and whether it ever has. The flag is not a convenience: + * a "never yet" sentinel value cannot be compared by subtraction. {@code Long.MIN_VALUE} was + * the obvious choice and it silently overflows — {@code System.nanoTime()} is positive on this + * platform, so {@code now - Long.MIN_VALUE} wraps to a large negative number, the interval gate + * reads it as "swept moments ago", and it returns before ever assigning the field. The sweep + * then never runs at all, for the life of the process, with nothing in the log to say so. + */ + private volatile boolean sweptOnce; + private volatile long lastWipSweepNanos; /** Construct a reaper with the default 5-second polling interval. */ public SessionReaper(SessionManager sessions, long idleTtlSeconds) { @@ -68,7 +77,11 @@ public final class SessionReaper { */ private void maybeSweepWipRefs() { long now = System.nanoTime(); - if (now - lastWipSweepNanos < WIP_SWEEP_INTERVAL_NANOS) { + // The first pass always sweeps: a restart is a fine moment to sweep, the 24h age floor + // makes it safe, and it means the feature is observable right after a redeploy instead of + // six hours later. Only after that does the interval gate apply, and by then both operands + // come from nanoTime, so the subtraction is well-defined. + if (sweptOnce && now - lastWipSweepNanos < WIP_SWEEP_INTERVAL_NANOS) { return; } try { @@ -80,7 +93,10 @@ public final class SessionReaper { } catch (RuntimeException e) { log.warn("refs/wip retention sweep failed; continuing", e); } + // Set even when the sweep threw, so a broken repo is retried on the slow cadence rather + // than hammering git on every 5-second iteration. lastWipSweepNanos = now; + sweptOnce = true; } private void sleep() { diff --git a/bridged/src/test/java/dev/ltms/bridged/session/SessionReaperTest.java b/bridged/src/test/java/dev/ltms/bridged/session/SessionReaperTest.java index 027178f..5220055 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/SessionReaperTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/SessionReaperTest.java @@ -11,10 +11,12 @@ import org.junit.jupiter.api.Test; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -90,6 +92,59 @@ class SessionReaperTest { return ticks.get() >= n; } + /** + * CB-586: the retention sweep must actually reach the seam when the loop runs. + * + *
The sweep's own tests call {@code SessionManager.sweepWipRefs} directly, which walks + * around the reaper's interval gate — and the gate is where it broke. {@code lastWipSweepNanos} + * started at {@code Long.MIN_VALUE}, so {@code now - lastWipSweepNanos} overflowed to a large + * negative number, the gate read that as "swept moments ago", and it returned before + * the assignment that would have fixed the field. The sweep never ran once, for the life of + * the process, and every direct-call test still passed. + * + *
So this asserts through the loop: spawn a worktree session (which is what tells the + * manager which repo holds {@code refs/wip/*}), start the reaper, and require a real prune call + * to arrive at the fake. + */ + @Test + void theLoopActuallyRunsTheWipRetentionSweep() throws InterruptedException { + FakeHerdr herdr = new FakeHerdr(); + FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt"); + SessionManager sessions = new SessionManager(launcher(herdr), worktrees); + // Without a worktree session the repo is unknown and the sweep is a legitimate no-op, so + // this step is what makes the assertion below meaningful rather than vacuous. + sessions.acquire("ltms-local", null, "/caller/proj", null, new WorktreeRequest("cb-586", null)); + assertTrue(worktrees.pruneCalls().isEmpty(), "nothing has swept before the reaper starts"); + + SessionReaper reaper = new SessionReaper(sessions, IDLE_TTL_SECONDS, SHORT_INTERVAL_MILLIS); + reaper.start(); + try { + long deadline = System.currentTimeMillis() + 3000; + while (worktrees.pruneCalls().isEmpty() && System.currentTimeMillis() < deadline) { + Thread.sleep(10); + } + } finally { + reaper.stop(); + } + + assertFalse(worktrees.pruneCalls().isEmpty(), + "the reaper loop must run the refs/wip retention sweep; it never reached the seam"); + assertEquals("/repo", worktrees.pruneCalls().getFirst().repoRoot(), + "the sweep must target the repo the fleet's worktrees came from"); + assertEquals(TimeUnit.HOURS.toMillis(24), worktrees.pruneCalls().getFirst().minAgeMillis(), + "the 24h age floor is the safety rule and must reach the seam intact"); + } + + /** As {@link #launcher()}, on a caller-supplied herdr so the test can inspect it. */ + private static ClaudeCodeLauncher launcher(FakeHerdr herdr) { + BridgedConfig.Profile cfg = new BridgedConfig.Profile( + "ltms-local", "http://gx00.gw:8000", "coder", null, "BRIDGED_WORKER_TOKEN", + List.of("ccs", "ltms-local"), "tab", "bridged-workers", + "worker: {profile} #{n}", null, null, null); + return new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null); + } + @Test void stopIsIdempotentAndSafeBeforeStart() { SessionReaper reaper = reaper();