CB-586: the retention sweep never ran — Long.MIN_VALUE overflowed the gate
SessionReaper.lastWipSweepNanos started at Long.MIN_VALUE as a "never swept yet" sentinel. That sentinel cannot be compared by subtraction. nanoTime() is positive on this platform, so `now - Long.MIN_VALUE` wraps to a large negative number, the gate `delta < WIP_SWEEP_INTERVAL_NANOS` reads it as "swept moments ago", and the method returns before the assignment that would have fixed the field. The sweep never ran once, for the life of the process, and nothing in the log said so. Measured: System.nanoTime() = 31305820625625 (positive) now - Long.MIN_VALUE = -9223340731034150183 interval (6h in nanos) = 21600000000000 gate 'delta < interval' -> true => returns early, every iteration, forever Fix: a separate `sweptOnce` boolean holds "never yet", so the subtraction only runs once both operands come from nanoTime. The first pass always sweeps — a restart is a fine moment for it, the 24h age floor keeps it safe, and the feature becomes observable right after a redeploy instead of six hours later. The existing tests all passed because they call SessionManager.sweepWipRefs directly, which walks around the gate. The new test asserts through the reaper loop instead: it spawns a worktree session, starts the reaper, and requires a real prune call at the seam with the 24h floor intact. Removing the fix makes it fail with "it never reached the seam". 860 tests, mvn clean install, BUILD SUCCESS.
This commit is contained in:
@@ -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() {
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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 <em>before</em>
|
||||
* 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.
|
||||
*
|
||||
* <p>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();
|
||||
|
||||
Reference in New Issue
Block a user