Compare commits
1 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Dai Ha
|
667254df47 |
@@ -1,102 +0,0 @@
|
||||
---
|
||||
name: hunter
|
||||
description: Defect-hunt procedure for a fleetd worker — sweep an assigned package for real bugs and report several ranked findings without fixing anything. Load this when the lead asks you to hunt or audit a scope rather than review one diff. Do NOT load `reviewer` for this; the two want different output.
|
||||
---
|
||||
|
||||
# Hunter worker — procedure
|
||||
|
||||
The turn contract (one `fleet_reply`, `fleet_ask` for the lead's decisions, honest reporting,
|
||||
never merge) is in **`CLAUDE.md` → Bridge communication → Worker** and already applies.
|
||||
|
||||
**This skill is not `reviewer`.** `reviewer` judges one diff and reports the *single* most
|
||||
important issue in about 90 words. A hunt sweeps a whole package and reports *several* findings
|
||||
in a long structured form. Loading both gives you two contradictory output contracts, and the
|
||||
usual result is a worker that writes a good report into its terminal and ends the turn without
|
||||
sending it. Load exactly one.
|
||||
|
||||
## 0. Read this before you read code: how the report gets home
|
||||
|
||||
Your terminal reaches nobody. The lead sees **only** the text inside your `fleet_reply` call.
|
||||
|
||||
A long report is exactly the case where this goes wrong, so plan for it:
|
||||
|
||||
- **Write the report into the `fleet_reply` argument itself.** Do not compose it in your terminal
|
||||
and then summarise it into the call.
|
||||
- If the report is long, **send it anyway** — one `fleet_reply` with everything.
|
||||
- If you end the turn without replying, the bridge scrapes your pane instead. That scrape carries
|
||||
at most the last 4000 characters, and on a hunt it usually captures the tail of the lead's own
|
||||
brief rather than your findings. The lead then has nothing and has to ask you again.
|
||||
|
||||
## 1. Change nothing
|
||||
|
||||
A hunt is read-only. Do not edit a production file, do not "quickly fix" what you find, and do
|
||||
not run a formatter. You may run the build and tests to *check* a claim, and you should say so
|
||||
when you did.
|
||||
|
||||
## 2. Read the whole scope first
|
||||
|
||||
Read every file in the assigned package before you judge any of it. A defect that a caller
|
||||
elsewhere in the same package makes unreachable is not a defect, and you cannot know that from
|
||||
one file.
|
||||
|
||||
Stay inside the scope. If a defect there depends on a class outside it, read that class to
|
||||
confirm — but the defect itself must live in the scope you were given.
|
||||
|
||||
## 3. The bar — this matters more than the count
|
||||
|
||||
**Name the path into the bad state.** Say which caller, in which state, reaches it. A defect on
|
||||
paper is not a reachable defect. If you cannot name that path, keep the finding but mark it
|
||||
`unproven` and say exactly what you could not check. Do not drop it, and do not dress it up.
|
||||
|
||||
**Say which direction the harm goes.** Data loss, privilege escalation and silent wrong answers
|
||||
are worth reporting even when the window is narrow. A finding whose worst outcome is a worse log
|
||||
line is not worth a block.
|
||||
|
||||
Two workers once ran the same scope: the one that applied the direction-of-harm filter found ten
|
||||
real defects, the one that did not found none. Fewer findings the lead can act on beat many the
|
||||
lead has to triage.
|
||||
|
||||
## 4. Shapes that have produced real merged fixes here
|
||||
|
||||
Read for these first:
|
||||
|
||||
1. **A one-way gate.** A guard added after an incident closes only the direction that incident
|
||||
came from. Do not only ask what closes the gate — ask **which states still open it**.
|
||||
2. **A value read once, then used later to authorise something destructive**, after something
|
||||
else has had a chance to change it.
|
||||
3. **A failure downgraded to a value that looks like a legitimate result** — `-1`, `null`, an
|
||||
empty list, `false` — which a caller then trusts.
|
||||
4. **A lock held for one half of a read-modify-write and not the other**, or two collections
|
||||
updated under different locks.
|
||||
5. **A comment or javadoc stating an invariant the code no longer keeps.** Comments are
|
||||
load-bearing in this repo; a stale one has already caused a bug.
|
||||
|
||||
## 5. What you cannot check, and must not claim you did
|
||||
|
||||
- `fleetd/fleetd.yaml` is gitignored and **absent from your worktree**. You cannot read it. If a
|
||||
finding depends on live configuration, name the key and say you could not check it.
|
||||
- `.mcp.json`, `opencode.json` and `.autoenv` in your worktree are neutralised stubs, not the
|
||||
repo's real files.
|
||||
- The `wiki/` submodule pointer is months old. Do not cite it.
|
||||
|
||||
Reporting a fact you took from the lead's brief as something you measured yourself is a false
|
||||
report, even when the fact is correct. Say where each fact came from.
|
||||
|
||||
## 6. The report — what goes in `fleet_reply`
|
||||
|
||||
One block per finding, most severe first:
|
||||
|
||||
```
|
||||
FINDING N — <one line>
|
||||
file:line
|
||||
Path in: <which caller, in which state, reaches this>
|
||||
Direction: <data loss | escalation | silent wrong answer | outage | ...>
|
||||
Window/trigger: <when it actually happens>
|
||||
Confidence: <confirmed by reading | unproven — say what you could not check>
|
||||
Why nothing else catches it: <the guard or test you checked, and why it misses>
|
||||
```
|
||||
|
||||
End with one line naming every file you read, so the lead knows the denominator.
|
||||
|
||||
**Nothing clears the bar?** Reply `NO FINDINGS`, name the files you read, and say what you ruled
|
||||
out. A clean sweep is a valid result; an invented defect is worse than none.
|
||||
@@ -10,11 +10,6 @@ never merge) is in **`CLAUDE.md` → Bridge communication → Worker** and alrea
|
||||
skill is only the *review procedure*: how to work the scope, and the exact shape of what you
|
||||
send back.
|
||||
|
||||
**Wrong skill for a sweep.** This one reviews *one* diff or scope and reports the *single* most
|
||||
important issue. If the lead asked you to hunt or audit a whole package for several defects, load
|
||||
`hunter` instead and ignore this file — the two want different output, and following both is how a
|
||||
worker ends its turn with a good report that never gets sent.
|
||||
|
||||
## 1. Read the whole scope before you judge
|
||||
|
||||
The delegation names your scope — a file, a diff, a PR, a function. **Read all of it first.**
|
||||
|
||||
@@ -200,13 +200,8 @@ must obey belongs in the charter, not here.
|
||||
adapter, with a message naming the credential and the remaining seconds ("cooling off after
|
||||
repeated backend errors") — distinct wording from a quarantine refusal, so don't conflate the
|
||||
two when reading a spawn failure.
|
||||
- **Skills available to delegate:** `implementer` (worktree → commit → push → own PR),
|
||||
`reviewer` (one diff → one structured finding) and `hunter` (sweep a package → several ranked
|
||||
findings, change nothing). Name exactly one in every delegation. **`reviewer` and `hunter` are
|
||||
not interchangeable** — `reviewer` caps the answer at one finding in about 90 words, so naming
|
||||
it for a multi-finding sweep hands the worker two contradictory output contracts. That has
|
||||
already cost three workers' turns: each wrote a good report to its terminal and ended the turn
|
||||
with no `fleet_reply`, and the scrape returned the tail of the brief instead.
|
||||
- **Skills available to delegate:** `implementer` (worktree → commit → push → own PR) and
|
||||
`reviewer` (scoped review → one structured finding). Name one in every delegation.
|
||||
- **Primary-side skills** (not delegation playbooks — a worker cannot use them):
|
||||
`port-to-opencode` (make an OpenCode session a participant in this workspace) and
|
||||
`fleets-status` (report every fleet that shares one LavinMQ instance).
|
||||
|
||||
@@ -236,15 +236,7 @@ public final class CallerResolver {
|
||||
// loopback-trust: same-host callers that are not workers are the primary. A non-loopback
|
||||
// caller is anonymous even here — and startup refuses that combination anyway
|
||||
// (FleetConfig.validateAuthExposure), so this is defence in depth, not the control.
|
||||
//
|
||||
// fleetd #317: "not a worker" must not be conflated with "identity unresolved". The real
|
||||
// primary is a real process — its pid resolves (c.resolved()), it just owns no herdr pane.
|
||||
// A caller whose peer-PID lookup failed (LsofPeerPidLookup's -1 sentinel — on any failure,
|
||||
// silently including "lsof found no match") has no such pid, and PaneLocator's own javadoc
|
||||
// already names what happens if that case is handed the primary role: a worker→primary
|
||||
// escalation. So an unresolved caller is refused (ANONYMOUS — the same clean, already-tested
|
||||
// "authenticated as nothing" outcome used everywhere else in this method), never promoted.
|
||||
return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous();
|
||||
return isLoopback(remoteAddr) ? Principal.primary(c.pid()) : Principal.anonymous();
|
||||
}
|
||||
|
||||
private boolean presentedTokenMatches(String authorizationHeader) {
|
||||
|
||||
@@ -39,18 +39,12 @@ import java.util.function.Supplier;
|
||||
* keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:},
|
||||
* {@code spawnReadyTimeoutMs} / {@code spawnReadyPollMs}, {@code quarantineCooldownSeconds}
|
||||
* (CB-578 stage B — baked once into the {@code BackendQuarantine} built at startup),
|
||||
* {@code guard:}, {@code worktreeRoot:} and {@code worktreeGroup:} (both baked once into the
|
||||
* {@code GitWorktrees} built at {@code Fleetd.java:251} and never rebuilt — fleetd #323
|
||||
* instance 2 found {@code worktreeGroup} missing from this list and from
|
||||
* {@link #changedDeferredKeys}), adding or removing a profile (a new backend needs its own launcher,
|
||||
* {@code guard:}, {@code worktreeRoot:}, adding or removing a profile (a new backend needs its own launcher,
|
||||
* which is constructed once), <em>and an existing profile's launch settings</em> —
|
||||
* {@code model}, {@code baseUrl}, {@code argv}, {@code env}, {@code mcpUrl},
|
||||
* {@code exhaustedPattern} (CB-578 stage A — compiled once into {@code Fleetd.main}'s
|
||||
* pattern map at startup), {@code errorPattern} (fleetd #201 Unit 5 — compiled once into
|
||||
* {@code Fleetd.main}'s backend-error pattern map at startup, the same way),
|
||||
* {@code ideProjectDir} / {@code ideOpenCommand} / {@code autoCompactWindow} (fleetd #323
|
||||
* instance 1 — all three are read at spawn off the same frozen profile map and were missing
|
||||
* from {@link #sameLaunchSettings}), and the rest of {@link #sameLaunchSettings}.
|
||||
* {@code Fleetd.main}'s backend-error pattern map at startup, the same way), and the rest.
|
||||
* {@code credentialId} (CB-578 stage B) is NOT on
|
||||
* this list — it is read live off the config supplier at every quarantine check and
|
||||
* exhaustion event, exactly like {@code weight} / {@code maxLoad}, so it is hot instead.
|
||||
@@ -227,13 +221,6 @@ public final class ConfigRef implements Supplier<FleetConfig> {
|
||||
if (!Objects.equals(old.worktreeRoot(), fresh.worktreeRoot())) {
|
||||
changed.add("worktreeRoot");
|
||||
}
|
||||
// Baked into the same GitWorktrees as worktreeRoot (Fleetd.java:251) and never rebuilt
|
||||
// either — see the class doc. Missing this check was fleetd #323 instance 2: a reload
|
||||
// that changed only worktreeGroup reported "config reloaded" with nothing deferred, and
|
||||
// newly provisioned worktrees kept the old sharing behaviour.
|
||||
if (!Objects.equals(old.worktreeGroup(), fresh.worktreeGroup())) {
|
||||
changed.add("worktreeGroup");
|
||||
}
|
||||
if (!Objects.equals(old.spawnReadyTimeoutMs(), fresh.spawnReadyTimeoutMs())
|
||||
|| !Objects.equals(old.spawnReadyPollMs(), fresh.spawnReadyPollMs())) {
|
||||
changed.add("spawnReady*");
|
||||
@@ -278,34 +265,14 @@ public final class ConfigRef implements Supplier<FleetConfig> {
|
||||
}
|
||||
|
||||
/**
|
||||
* {@link FleetConfig.Profile} record components deliberately left out of
|
||||
* {@link #sameLaunchSettings} because they are read <em>live</em>, not baked in at spawn — see
|
||||
* the class doc's <em>Hot</em> bullet. {@code weight} and {@code maxLoad} are read live by the
|
||||
* placement policy on every spawn; {@code credentialId} is read live by
|
||||
* {@code CompositePeerLauncher} and the CB-578 stage B exhaustion sink. Nothing else is
|
||||
* excluded — see {@code sameLaunchSettingsComparesEveryProfileComponentOrExcludesIt} in
|
||||
* {@code ConfigRefProfileCoverageTest}, which enumerates every {@code Profile} record component
|
||||
* by reflection and fails the build if one is neither compared below nor named here.
|
||||
* Whether two versions of a profile would launch a peer identically. Compares every component
|
||||
* the launcher reads at spawn; {@code weight}, {@code maxLoad} and {@code credentialId} are
|
||||
* excluded because those are read live (by the placement policy and, for credentialId, by
|
||||
* {@code CompositePeerLauncher}/the CB-578 stage B exhaustion sink) and really do take effect on
|
||||
* the next spawn.
|
||||
*/
|
||||
static final Set<String> LAUNCH_SETTINGS_EXCLUDED = Set.of("weight", "maxLoad", "credentialId");
|
||||
|
||||
/**
|
||||
* Whether two versions of a profile would launch a peer identically.
|
||||
*
|
||||
* <p>This must compare every {@link FleetConfig.Profile} record component except the three in
|
||||
* {@link #LAUNCH_SETTINGS_EXCLUDED}. That is not a claim this javadoc can make good on by
|
||||
* itself — a javadoc saying "compares every component" is exactly what fleetd #323 found to be
|
||||
* false for three fields (and a sibling method's field list, for a fourth). The actual
|
||||
* guarantee comes from {@code ConfigRefProfileCoverageTest}: it enumerates every record
|
||||
* component of {@code FleetConfig.Profile} by reflection, mutates each one not in
|
||||
* {@code LAUNCH_SETTINGS_EXCLUDED} on a base profile, and asserts this method reports a
|
||||
* difference — so a new component that is neither compared here nor added to
|
||||
* {@code LAUNCH_SETTINGS_EXCLUDED} (with a reason) fails that test by name, rather than
|
||||
* silently reporting "config reloaded" for a value the daemon never picked up.
|
||||
*/
|
||||
static boolean sameLaunchSettings(FleetConfig.Profile a, FleetConfig.Profile b) {
|
||||
return Objects.equals(a.profile(), b.profile())
|
||||
&& Objects.equals(a.baseUrl(), b.baseUrl())
|
||||
private static boolean sameLaunchSettings(FleetConfig.Profile a, FleetConfig.Profile b) {
|
||||
return Objects.equals(a.baseUrl(), b.baseUrl())
|
||||
&& Objects.equals(a.model(), b.model())
|
||||
&& Objects.equals(a.configDir(), b.configDir())
|
||||
&& Objects.equals(a.tokenEnv(), b.tokenEnv())
|
||||
@@ -331,16 +298,6 @@ public final class ConfigRef implements Supplier<FleetConfig> {
|
||||
// fleetd #201 Unit 5: errorPattern is compiled once into Fleetd.main's backend-error
|
||||
// pattern map at startup (see BackendErrorPatternLookup wiring), the same way
|
||||
// exhaustedPattern is — a reload never re-reads it either.
|
||||
&& Objects.equals(a.errorPattern(), b.errorPattern())
|
||||
// fleetd #323 instance 1: ideProjectDir and ideOpenCommand are read at spawn off the
|
||||
// same frozen profile map as ideMcpUrl above (ClaudeCodeLauncher.java:267/269,
|
||||
// OpenCodeLauncher.java:474/480/486) and were missing from this comparison.
|
||||
&& Objects.equals(a.ideProjectDir(), b.ideProjectDir())
|
||||
&& Objects.equals(a.ideOpenCommand(), b.ideOpenCommand())
|
||||
// fleetd #323 instance 1: autoCompactWindow is read at spawn the same way
|
||||
// (ClaudeCodeLauncher.java:926, OpenCodeLauncher.java:650). Comparing it here only
|
||||
// makes the reload REPORT that a restart is needed — it deliberately does not make
|
||||
// autoCompactWindow take effect live, which is a separate, larger change.
|
||||
&& Objects.equals(a.autoCompactWindow(), b.autoCompactWindow());
|
||||
&& Objects.equals(a.errorPattern(), b.errorPattern());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,25 +36,6 @@ public final class ConnectionIdentity {
|
||||
* primary / an off-host client) and its {@code pid} (or {@code -1} if not resolvable).
|
||||
*/
|
||||
public record Caller(String terminal, long pid) {
|
||||
|
||||
/**
|
||||
* Whether the OS peer-PID lookup actually succeeded — {@code false} means {@code pid} is
|
||||
* the {@code -1} sentinel, not a real process id, so this caller's identity could not be
|
||||
* established at all. That is a different fact from a real pid that simply owns no worker
|
||||
* pane (the primary's own connection): the primary is {@code resolved()} and has a
|
||||
* {@code null terminal}; an unresolvable caller is {@code !resolved()} and also has a
|
||||
* {@code null terminal}. The two look identical through {@link #terminal} alone, which is
|
||||
* exactly how fleetd #317 happened — a failed {@code lsof} lookup and a genuine primary both
|
||||
* fell through to {@code Principal.primary(...)}.
|
||||
*
|
||||
* <p>Centralised here, next to the sentinel it tests, for the same reason
|
||||
* {@link ConnectionIdentity#isLoopback} is centralised rather than left for each caller to
|
||||
* reimplement: a raw {@code pid > 0} check duplicated at every call site is precisely the
|
||||
* "one rule, two copies" shape that let #305 drift.
|
||||
*/
|
||||
public boolean resolved() {
|
||||
return pid > 0;
|
||||
}
|
||||
}
|
||||
|
||||
/** Resolve the caller's terminal and PID from one peer-PID lookup. */
|
||||
@@ -90,27 +71,13 @@ public final class ConnectionIdentity {
|
||||
* {@code 127.0.0.1:8765} with a source address of {@code 127.0.0.2} — measured on the Linux
|
||||
* fleet host, where binding that source succeeds.
|
||||
*
|
||||
* <p><strong>What excluding an address costs, stated as it is today.</strong> This paragraph
|
||||
* used to say that narrowing this range turned a worker into the lead, and that widening the
|
||||
* check was what closed the hole. That was true only while there were <em>two</em> definitions
|
||||
* that disagreed: {@code ConnectionIdentity} skipped the identity lookup for {@code 127.0.0.2}
|
||||
* while {@code CallerResolver} read the same address as loopback and granted the primary role.
|
||||
* #305 removed the second copy, and with one shared definition the old sentence no longer holds.
|
||||
*
|
||||
* <p>Measured on 2026-09-04 by narrowing this method back to exactly {@code 127.0.0.1} and
|
||||
* running {@code CallerResolverTest} and {@code ConnectionIdentityTest}: a caller from
|
||||
* {@code 127.0.0.2} then resolves to {@code ANONYMOUS}, not {@code PRIMARY} — for a worker
|
||||
* ({@code aWorkerOnAnyLoopbackSourceAddressIsStillAWorkerNotThePrimary}) and for a non-worker
|
||||
* ({@code aNonWorkerOnAnyLoopbackSourceAddressIsStillThePrimary}) alike. Excluding an address
|
||||
* now <em>refuses</em> its caller; it does not promote one.
|
||||
*
|
||||
* <p>So keep the whole range, but for the plain reason: a genuine worker or primary that
|
||||
* connects from {@code 127.0.0.2} must be identifiable at all, and narrowing this predicate
|
||||
* locks it out. That is an outage, and an outage is the direction to fail in — which is exactly
|
||||
* why the range must not be narrowed casually and also why doing so is no longer a security
|
||||
* hole. This predicate still does not decide whether a caller is trusted; it decides whether the
|
||||
* caller's identity is <em>resolved at all</em>. What makes an unresolved caller safe is
|
||||
* {@link Caller#resolved()} (#317), not this method.
|
||||
* <p><strong>Being strict here does not make the daemon safer; it makes it unsafe.</strong>
|
||||
* That reads backwards, so it is worth stating plainly. This predicate does not decide whether
|
||||
* a caller is trusted — it decides whether the caller's identity is <em>resolved at all</em>.
|
||||
* Returning false means {@link #resolve} answers "no terminal", and downstream a caller with no
|
||||
* terminal is treated as the primary under loopback-trust. So every address excluded here is an
|
||||
* address on which a worker silently becomes the lead. Widening a check normally weakens it;
|
||||
* widening this one is what closes the hole.
|
||||
*/
|
||||
public static boolean isLoopback(String addr) {
|
||||
if (addr == null) {
|
||||
|
||||
@@ -41,14 +41,6 @@ public final class LsofPeerPidLookup implements PeerPidLookup {
|
||||
if (!p.waitFor(2, TimeUnit.SECONDS)) {
|
||||
p.destroyForcibly();
|
||||
}
|
||||
if (found < 0) {
|
||||
// fleetd #317: this is the silent path — lsof ran clean and simply reported no
|
||||
// matching process (e.g. queried before the OS socket table settles). Previously
|
||||
// this logged nothing at all, which is exactly why the escalation went unnoticed;
|
||||
// the exception path below already logs. A caller now refused because of this is
|
||||
// still refused (never promoted) — this line only makes the refusal diagnosable.
|
||||
log.debug("lsof peer-pid lookup for port {} found no matching process", port);
|
||||
}
|
||||
return found;
|
||||
} catch (Exception e) {
|
||||
log.debug("lsof peer-pid lookup for port {} failed: {}", port, e.getMessage());
|
||||
|
||||
@@ -386,6 +386,31 @@ public final class SessionManager implements TurnListener {
|
||||
// from the registry with no pane stop is an orphaned pane — a live terminal burning a fleet
|
||||
// slot that no longer appears in the roster and can never be reclaimed.
|
||||
launcher.stop(paneId);
|
||||
if (removed != null && !preserveWorktree && removed.worktree() != null) {
|
||||
// fleetd #316: the `dirty` read above ran while the worker could still write to this
|
||||
// worktree, so a stale `false` must not be trusted to authorise the --force removal
|
||||
// below. Re-read the worktree's state one more time, right here — immediately before
|
||||
// the one step that would destroy it, and only on the path that is actually about to
|
||||
// do that (invariant 4: no second unconditional `git status` on a release that already
|
||||
// decided to preserve). By now `launcher.stop` has returned, so this read reflects
|
||||
// whatever the worker managed to write up to and including its teardown, not whatever
|
||||
// it had written at release-start time.
|
||||
if (dirtyImmediatelyBeforeRemoval(removed)) {
|
||||
preserveWorktree = true;
|
||||
// The pre-stop snapshot above never ran for this session (the pre-stop read said
|
||||
// clean), so this is the only chance to get the newly-discovered work into
|
||||
// refs/wip/* rather than leaving the on-disk preserve as the sole copy. Best-effort,
|
||||
// like every other snapshot attempt — trySnapshot logs and swallows its own failure.
|
||||
String lateSnapshotRef = trySnapshot(removed, cause);
|
||||
log.warn("release {} preserves worktree {} for pane={} terminal={}: it reported "
|
||||
+ "clean before the pane stopped but dirty immediately before removal — the "
|
||||
+ "worker wrote to it during teardown, and --force removing it now would "
|
||||
+ "have destroyed that work{}",
|
||||
cause, removed.worktree(), paneId, removed.terminalId(),
|
||||
lateSnapshotRef == null ? "" : " (snapshotted to refs/wip/" + removed.branch()
|
||||
+ " commit=" + lateSnapshotRef + ")");
|
||||
}
|
||||
}
|
||||
if (removed != null && !preserveWorktree && removed.worktree() != null) {
|
||||
// fleetd #283: this is the one cleanup step in this method that used to be bare. By the
|
||||
// time it runs, the registry entry, the retained handle, and the pane are all already
|
||||
@@ -404,6 +429,24 @@ public final class SessionManager implements TurnListener {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #316: the read that actually authorises {@code worktrees.remove}, taken with the
|
||||
* worker's pane already stopped. Fails toward preserving (returns {@code true}) on any
|
||||
* exception — the same rule the pre-stop check applies (CB-581): once we can no longer tell
|
||||
* whether the worktree is dirty, preserving costs disk while deleting on a guess can destroy
|
||||
* work that has no other copy.
|
||||
*/
|
||||
private boolean dirtyImmediatelyBeforeRemoval(MemberSession removed) {
|
||||
try {
|
||||
return worktrees.hasUncommitted(removed.worktree());
|
||||
} catch (RuntimeException e) {
|
||||
log.warn("release could not re-check worktree {} for pane={} terminal={} immediately "
|
||||
+ "before removal; preserving it rather than risk destroying unsaved work: {}",
|
||||
removed.worktree(), removed.paneId(), removed.terminalId(), e.toString());
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Best-effort snapshot of a dirty worktree into {@code refs/wip/<branch>} (CB-578 stage C). A
|
||||
* failure here must never escalate: the caller has already decided to preserve the worktree
|
||||
|
||||
@@ -103,54 +103,6 @@ class CallerResolverTest {
|
||||
assertEquals(Role.PRIMARY, p.role(), "the historical behaviour, now an explicit choice");
|
||||
}
|
||||
|
||||
// ── fleetd #317: an unresolvable caller must never be promoted to the primary ──────────────────
|
||||
// #305 closed the trigger where a resolved pid matched no pane *and* had no ancestry walk to
|
||||
// save it. This is the other trigger PaneLocator's javadoc names: the pid never resolves at
|
||||
// all — LsofPeerPidLookup returns -1 on any failure, including (silently) "lsof found no
|
||||
// match" — so there is no candidate pid for an ancestry walk to even attempt.
|
||||
|
||||
/**
|
||||
* The failing-without-the-fix case. Before #317's fix, {@code c.terminal() == null} was the
|
||||
* only test in the loopback-trust fallback, and an unresolved pid produces exactly that same
|
||||
* {@code null} terminal as a genuine primary — so this caller was handed
|
||||
* {@code Principal.primary(...)}, a real worker's failed lookup becoming indistinguishable from
|
||||
* the lead.
|
||||
*/
|
||||
@Test
|
||||
void aFailedPeerPidLookupIsRefusedNotPromotedToPrimary() {
|
||||
ConnectionIdentity unresolved = new ConnectionIdentity(new PaneLocator(herdr), _ -> -1);
|
||||
Principal p = new CallerResolver(unresolved).resolve("127.0.0.1", 55555, null);
|
||||
|
||||
assertEquals(Role.ANONYMOUS, p.role(),
|
||||
"an unresolvable caller must never be silently promoted to the primary");
|
||||
}
|
||||
|
||||
/**
|
||||
* The companion invariant #317 must not break: a caller whose lookup genuinely succeeded, and
|
||||
* who simply owns no herdr pane — the real primary's own connection — is still the primary.
|
||||
* This is {@link #loopbackTrustTreatsANonWorkerLoopbackCallerAsThePrimary} pinned again here,
|
||||
* named for #317 and placed next to the test it must be distinguished from: same {@code null}
|
||||
* terminal, opposite verdict, because {@code Caller.resolved()} tells them apart.
|
||||
*/
|
||||
@Test
|
||||
void aRealPidThatOwnsNoPaneIsStillThePrimaryNotRefused() {
|
||||
Principal p = new CallerResolver(nonWorkerIdentity()).resolve("127.0.0.1", 55555, null);
|
||||
|
||||
assertEquals(Role.PRIMARY, p.role());
|
||||
}
|
||||
|
||||
/** #317 point 4: token mode never consults {@code c.pid()}, so a failed lookup must not change it. */
|
||||
@Test
|
||||
void tokenModeIsUndisturbedByAnUnresolvedLookup() {
|
||||
ConnectionIdentity unresolved = new ConnectionIdentity(new PaneLocator(herdr), _ -> -1);
|
||||
CallerResolver r = new CallerResolver(unresolved, true, "s3cret");
|
||||
|
||||
assertEquals(Role.ANONYMOUS, r.resolve("127.0.0.1", 55555, null).role(),
|
||||
"no credential is still just ANONYMOUS, as before #317 — unchanged by the lookup failing");
|
||||
assertEquals(Role.PRIMARY, r.resolve("127.0.0.1", 55555, "Bearer s3cret").role(),
|
||||
"a valid token still authenticates the primary even though the peer-pid lookup failed");
|
||||
}
|
||||
|
||||
@Test
|
||||
void tokenModeRefusesANonWorkerCallerThatPresentsNoToken() {
|
||||
Principal p = new CallerResolver(nonWorkerIdentity(), true, "s3cret")
|
||||
|
||||
@@ -1,187 +0,0 @@
|
||||
package dev.ltms.fleet.config;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.lang.reflect.Constructor;
|
||||
import java.lang.reflect.RecordComponent;
|
||||
import java.util.Arrays;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.TreeSet;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
|
||||
/**
|
||||
* fleetd #323: {@code ConfigRef.sameLaunchSettings} javadoc used to claim it "compares every
|
||||
* component the launcher reads at spawn". It did not — {@code ideProjectDir}, {@code
|
||||
* ideOpenCommand} and {@code autoCompactWindow} were all baked in at daemon startup (see
|
||||
* {@code ClaudeCodeLauncher}/{@code OpenCodeLauncher}) and missing from the comparison, so a reload
|
||||
* that changed only one of them reported "config reloaded" and the running daemon kept the old
|
||||
* value.
|
||||
*
|
||||
* <p>This class is the mechanism the issue asked for: it enumerates every record component of
|
||||
* {@link FleetConfig.Profile} by reflection and proves — by actually mutating a base profile one
|
||||
* field at a time and calling the real method — that each component is either compared by
|
||||
* {@link ConfigRef#sameLaunchSettings} or named in {@link ConfigRef#LAUNCH_SETTINGS_EXCLUDED} with
|
||||
* a reason. A new profile field that is neither fails this test by name, not a hand-maintained list
|
||||
* going stale.
|
||||
*/
|
||||
class ConfigRefProfileCoverageTest {
|
||||
|
||||
private static final RecordComponent[] COMPONENTS = FleetConfig.Profile.class.getRecordComponents();
|
||||
|
||||
/**
|
||||
* One valid, non-blank value per record component — "the a value". None of these trip any
|
||||
* defaulting/normalization in {@code Profile}'s compact constructor (see {@code
|
||||
* FleetConfig.java}), so what goes in is what {@code sameLaunchSettings} sees back out.
|
||||
*/
|
||||
private static final Map<String, Object> BASE = baseValues();
|
||||
|
||||
/** The same shape, each value distinct from {@link #BASE} — "the b value". */
|
||||
private static final Map<String, Object> ALT = altValues();
|
||||
|
||||
private static Map<String, Object> baseValues() {
|
||||
Map<String, Object> v = new LinkedHashMap<>();
|
||||
v.put("profile", "sonnet");
|
||||
v.put("baseUrl", "http://gx00.gw:8000");
|
||||
v.put("model", "sonnet");
|
||||
v.put("configDir", "/config/a");
|
||||
v.put("tokenEnv", "TOKEN_A");
|
||||
v.put("argv", List.of("claude", "--flag-a"));
|
||||
v.put("placement", "tab");
|
||||
v.put("workspace", "workspace-a");
|
||||
v.put("tabLabel", "label-a");
|
||||
v.put("mcpUrl", "http://mcp-a");
|
||||
v.put("cwd", "/cwd/a");
|
||||
v.put("parityOverlay", List.of(".env", ".env.a"));
|
||||
v.put("gitTokenEnv", "GIT_TOKEN_A");
|
||||
v.put("gitHostEnv", "GITEA_HOST_A");
|
||||
v.put("kind", "claude-code");
|
||||
v.put("env", Map.of("K", "A"));
|
||||
v.put("weight", 1.0f);
|
||||
v.put("maxLoad", 5);
|
||||
v.put("subscription", Boolean.TRUE);
|
||||
v.put("exhaustedPattern", "usage limit a");
|
||||
v.put("credentialId", "cred-a");
|
||||
v.put("ideMcpUrl", "http://ide-mcp-a");
|
||||
v.put("ideProjectDir", "modules/a");
|
||||
v.put("ideOpenCommand", "open-cmd-a {dir}");
|
||||
v.put("autoCompactWindow", 150000);
|
||||
v.put("errorPattern", "error a");
|
||||
assertNamesMatchComponents(v);
|
||||
return v;
|
||||
}
|
||||
|
||||
private static Map<String, Object> altValues() {
|
||||
Map<String, Object> v = new LinkedHashMap<>();
|
||||
v.put("profile", "sonnet-b");
|
||||
v.put("baseUrl", "http://gx01.gw:8000");
|
||||
v.put("model", "haiku");
|
||||
v.put("configDir", "/config/b");
|
||||
v.put("tokenEnv", "TOKEN_B");
|
||||
v.put("argv", List.of("claude", "--flag-b"));
|
||||
v.put("placement", "weighted");
|
||||
v.put("workspace", "workspace-b");
|
||||
v.put("tabLabel", "label-b");
|
||||
v.put("mcpUrl", "http://mcp-b");
|
||||
v.put("cwd", "/cwd/b");
|
||||
v.put("parityOverlay", List.of(".env", ".env.b"));
|
||||
v.put("gitTokenEnv", "GIT_TOKEN_B");
|
||||
v.put("gitHostEnv", "GITEA_HOST_B");
|
||||
v.put("kind", "opencode");
|
||||
v.put("env", Map.of("K", "B"));
|
||||
v.put("weight", 2.0f);
|
||||
v.put("maxLoad", 9);
|
||||
v.put("subscription", Boolean.FALSE);
|
||||
v.put("exhaustedPattern", "usage limit b");
|
||||
v.put("credentialId", "cred-b");
|
||||
v.put("ideMcpUrl", "http://ide-mcp-b");
|
||||
v.put("ideProjectDir", "modules/b");
|
||||
v.put("ideOpenCommand", "open-cmd-b {dir}");
|
||||
v.put("autoCompactWindow", 250000);
|
||||
v.put("errorPattern", "error b");
|
||||
assertNamesMatchComponents(v);
|
||||
return v;
|
||||
}
|
||||
|
||||
private static void assertNamesMatchComponents(Map<String, Object> values) {
|
||||
Set<String> componentNames = new TreeSet<>();
|
||||
for (RecordComponent rc : COMPONENTS) {
|
||||
componentNames.add(rc.getName());
|
||||
}
|
||||
assertEquals(componentNames, new TreeSet<>(values.keySet()),
|
||||
"this test's value map has drifted from FleetConfig.Profile's actual components — "
|
||||
+ "update BASE/ALT alongside the record");
|
||||
}
|
||||
|
||||
private static FleetConfig.Profile profileOf(Map<String, Object> values) throws ReflectiveOperationException {
|
||||
Class<?>[] types = Arrays.stream(COMPONENTS).map(RecordComponent::getType).toArray(Class<?>[]::new);
|
||||
Object[] args = Arrays.stream(COMPONENTS)
|
||||
.map(rc -> values.get(rc.getName()))
|
||||
.toArray();
|
||||
Constructor<FleetConfig.Profile> ctor = FleetConfig.Profile.class.getDeclaredConstructor(types);
|
||||
return ctor.newInstance(args);
|
||||
}
|
||||
|
||||
/** {@code BASE} with exactly one named component swapped for its {@code ALT} value. */
|
||||
private static FleetConfig.Profile mutate(String componentName) throws ReflectiveOperationException {
|
||||
Map<String, Object> values = new LinkedHashMap<>(BASE);
|
||||
values.put(componentName, ALT.get(componentName));
|
||||
return profileOf(values);
|
||||
}
|
||||
|
||||
/**
|
||||
* The mechanism fleetd #323 asked for: enumerate {@link FleetConfig.Profile}'s record
|
||||
* components, mutate each non-excluded one, and prove {@code sameLaunchSettings} actually
|
||||
* notices — not just that some hand-maintained list claims it does. Prints the denominator
|
||||
* (total / compared / excluded) the issue required: a checker that cannot state its own
|
||||
* denominator is the failure this repo keeps hitting.
|
||||
*/
|
||||
@Test
|
||||
void sameLaunchSettingsComparesEveryProfileComponentOrExcludesIt() throws ReflectiveOperationException {
|
||||
int total = COMPONENTS.length;
|
||||
Set<String> excluded = ConfigRef.LAUNCH_SETTINGS_EXCLUDED;
|
||||
|
||||
Set<String> allNames = new TreeSet<>();
|
||||
for (RecordComponent rc : COMPONENTS) {
|
||||
allNames.add(rc.getName());
|
||||
}
|
||||
assertTrue(allNames.containsAll(excluded),
|
||||
"ConfigRef.LAUNCH_SETTINGS_EXCLUDED names a component that does not exist on "
|
||||
+ "FleetConfig.Profile — check for a typo: " + excluded);
|
||||
|
||||
FleetConfig.Profile base = profileOf(BASE);
|
||||
List<String> uncovered = new java.util.ArrayList<>();
|
||||
int compared = 0;
|
||||
for (RecordComponent rc : COMPONENTS) {
|
||||
String name = rc.getName();
|
||||
if (excluded.contains(name)) {
|
||||
continue;
|
||||
}
|
||||
FleetConfig.Profile mutated = mutate(name);
|
||||
if (ConfigRef.sameLaunchSettings(base, mutated)) {
|
||||
uncovered.add(name);
|
||||
} else {
|
||||
compared++;
|
||||
}
|
||||
}
|
||||
|
||||
System.out.printf(
|
||||
"ConfigRef.sameLaunchSettings coverage — %d Profile components total, %d compared, "
|
||||
+ "%d excluded (%s)%n",
|
||||
total, compared, excluded.size(), excluded);
|
||||
|
||||
assertEquals(List.of(), uncovered,
|
||||
"these FleetConfig.Profile components changed but ConfigRef.sameLaunchSettings "
|
||||
+ "reported no difference — add each one to the comparison (it is read at "
|
||||
+ "spawn and baked in until a restart) or to ConfigRef.LAUNCH_SETTINGS_EXCLUDED "
|
||||
+ "with a reason it is genuinely read live: " + uncovered);
|
||||
assertEquals(total, compared + excluded.size(),
|
||||
"every FleetConfig.Profile record component must be either compared or excluded — "
|
||||
+ total + " components, " + compared + " compared, " + excluded.size()
|
||||
+ " excluded");
|
||||
}
|
||||
}
|
||||
@@ -434,78 +434,6 @@ class ConfigRefTest {
|
||||
assertEquals("provider 5xx", ref.get().profiles().get("sonnet").errorPattern());
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #323 instance 1: {@code ideProjectDir} is read at spawn off the frozen profile map
|
||||
* (see {@code ClaudeCodeLauncher}/{@code OpenCodeLauncher}) exactly like {@code model}, but was
|
||||
* missing from {@code sameLaunchSettings} — a reload changing only this field used to report a
|
||||
* bare "config reloaded" and the running daemon kept launching with the old value.
|
||||
*/
|
||||
@Test
|
||||
void changingAProfilesIdeProjectDirIsReportedAsDeferred(@TempDir Path dir) throws Exception {
|
||||
Path f = dir.resolve("fleetd.yaml");
|
||||
Files.writeString(f, """
|
||||
bind:
|
||||
host: 127.0.0.1
|
||||
port: 8765
|
||||
herdrSocket: ~/.config/herdr/herdr.sock
|
||||
profiles:
|
||||
sonnet:
|
||||
baseUrl: http://gx00.gw:8000
|
||||
model: sonnet
|
||||
ideProjectDir: fleetd
|
||||
guard:
|
||||
offSubscriptionHosts:
|
||||
- gx00.gw
|
||||
""");
|
||||
ConfigRef ref = refFor(f);
|
||||
|
||||
Files.writeString(f, """
|
||||
bind:
|
||||
host: 127.0.0.1
|
||||
port: 8765
|
||||
herdrSocket: ~/.config/herdr/herdr.sock
|
||||
profiles:
|
||||
sonnet:
|
||||
baseUrl: http://gx00.gw:8000
|
||||
model: sonnet
|
||||
ideProjectDir: fleetd-renamed
|
||||
guard:
|
||||
offSubscriptionHosts:
|
||||
- gx00.gw
|
||||
""");
|
||||
ConfigRef.Outcome out = ref.reload();
|
||||
|
||||
assertTrue(out.applied());
|
||||
assertEquals(1, out.deferred().size(), out.deferred().toString());
|
||||
assertTrue(out.deferred().getFirst().contains("sonnet"), out.deferred().toString());
|
||||
assertTrue(out.deferred().getFirst().contains("launch settings"), out.deferred().toString());
|
||||
// The snapshot still carries the new value — a restart is what makes it take effect.
|
||||
assertEquals("fleetd-renamed", ref.get().profiles().get("sonnet").ideProjectDir());
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #323 instance 2: {@code worktreeGroup} is baked into the same {@code GitWorktrees}
|
||||
* as {@code worktreeRoot} (Fleetd.java:251) and never rebuilt, but only {@code worktreeRoot}
|
||||
* was on {@code changedDeferredKeys} — a reload changing only the group reported a bare
|
||||
* "config reloaded" and newly provisioned worktrees kept the old sharing behaviour.
|
||||
*/
|
||||
@Test
|
||||
void changingWorktreeGroupIsReportedAsDeferred(@TempDir Path dir) throws Exception {
|
||||
Path f = dir.resolve("fleetd.yaml");
|
||||
Files.writeString(f, yaml("worktreeGroup: devgroup\n"));
|
||||
ConfigRef ref = refFor(f);
|
||||
|
||||
Files.writeString(f, yaml("worktreeGroup: devgroup2\n"));
|
||||
ConfigRef.Outcome out = ref.reload();
|
||||
|
||||
assertTrue(out.applied());
|
||||
assertEquals(java.util.List.of("worktreeGroup"), out.deferred());
|
||||
assertTrue(out.summary().contains("needs a restart") || out.summary().contains("need a restart"),
|
||||
out.summary());
|
||||
// The snapshot still carries the new value — a restart is what makes it take effect.
|
||||
assertEquals("devgroup2", ref.get().worktreeGroup());
|
||||
}
|
||||
|
||||
@Test
|
||||
void aFixedRefHasNoFileAndRefusesToReload() {
|
||||
FleetConfig cfg = new FleetConfig(null, null, null, null, null, null,
|
||||
|
||||
@@ -43,22 +43,6 @@ class ConnectionIdentityTest {
|
||||
assertNull(with(_ -> 999_999).callerTerminal("127.0.0.1", 55555));
|
||||
}
|
||||
|
||||
@Test
|
||||
void callerIsUnresolvedWhenThePeerPidLookupFails() {
|
||||
// fleetd #317: LsofPeerPidLookup returns -1 on any failure — a fork error, or (silently)
|
||||
// simply no matching lsof line. Caller.resolved() is the one place that sentinel is tested.
|
||||
ConnectionIdentity.Caller c = with(_ -> -1).resolve("127.0.0.1", 55555);
|
||||
assertFalse(c.resolved(), "a -1 pid means the lookup failed, not that this pid owns no pane");
|
||||
}
|
||||
|
||||
@Test
|
||||
void callerIsResolvedWhenThePidIsRealEvenThoughItOwnsNoPane() {
|
||||
// The primary's own connection: a real, lsof-found pid that just isn't a worker pane. This
|
||||
// must read as "resolved" — the distinction #317 turns on.
|
||||
ConnectionIdentity.Caller c = with(_ -> 999_999).resolve("127.0.0.1", 55555);
|
||||
assertTrue(c.resolved());
|
||||
}
|
||||
|
||||
@Test
|
||||
void resolvesTheCallersPidAndCwd() {
|
||||
// CB-112: the primary maps to no pane, but its PID and cwd are still readable.
|
||||
|
||||
@@ -86,12 +86,32 @@ class SessionManagerTest {
|
||||
private volatile RuntimeException hasUncommittedFailure;
|
||||
private volatile RuntimeException snapshotFailure;
|
||||
private final java.util.concurrent.atomic.AtomicLong snapshotSeq = new java.util.concurrent.atomic.AtomicLong();
|
||||
/** fleetd #316: successive {@code hasUncommitted} answers, one per call, last one sticky
|
||||
* once exhausted — models a worktree whose state changes between reads. Empty (the
|
||||
* default) falls back to the plain {@link #dirty} flag, so every existing test using this
|
||||
* fake keeps returning one fixed answer. */
|
||||
private final List<Boolean> dirtySequence = new java.util.concurrent.CopyOnWriteArrayList<>();
|
||||
private final java.util.concurrent.atomic.AtomicInteger hasUncommittedCalls =
|
||||
new java.util.concurrent.atomic.AtomicInteger();
|
||||
|
||||
RecordingWorktrees dirty(boolean dirty) {
|
||||
this.dirty = dirty;
|
||||
return this;
|
||||
}
|
||||
|
||||
/** fleetd #316: return {@code answers[0]} on the first {@code hasUncommitted} call,
|
||||
* {@code answers[1]} on the second, and so on; the last element repeats after that. */
|
||||
RecordingWorktrees dirtySequence(boolean... answers) {
|
||||
for (boolean a : answers) {
|
||||
dirtySequence.add(a);
|
||||
}
|
||||
return this;
|
||||
}
|
||||
|
||||
int hasUncommittedCallCount() {
|
||||
return hasUncommittedCalls.get();
|
||||
}
|
||||
|
||||
RecordingWorktrees failHasUncommittedWith(RuntimeException e) {
|
||||
this.hasUncommittedFailure = e;
|
||||
return this;
|
||||
@@ -127,9 +147,13 @@ class SessionManagerTest {
|
||||
|
||||
@Override
|
||||
public boolean hasUncommitted(String worktreePath) {
|
||||
int call = hasUncommittedCalls.getAndIncrement();
|
||||
if (hasUncommittedFailure != null) {
|
||||
throw hasUncommittedFailure;
|
||||
}
|
||||
if (!dirtySequence.isEmpty()) {
|
||||
return dirtySequence.get(Math.min(call, dirtySequence.size() - 1));
|
||||
}
|
||||
return dirty;
|
||||
}
|
||||
|
||||
@@ -1207,6 +1231,80 @@ class SessionManagerTest {
|
||||
+ "dirty check threw");
|
||||
}
|
||||
|
||||
// --- fleetd #316: the dirty check must be re-taken after the worker is stopped, not trusted
|
||||
// stale from before it ------------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
void releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval() {
|
||||
// Models the exact race #316 reports: hasUncommitted answers clean while the worker is
|
||||
// still running (call 1), the worker then writes new work, and by the time release is
|
||||
// about to force-remove the worktree a second read (call 2) would see it as dirty. Without
|
||||
// the fix this test fails: release() never re-reads and force-removes the worktree anyway.
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
RecordingWorktrees worktrees = new RecordingWorktrees().dirtySequence(false, true);
|
||||
SessionManager sessions = sessionManager(herdr, worktrees);
|
||||
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
|
||||
new WorktreeRequest("cb-316a", null));
|
||||
|
||||
sessions.release(s.paneId());
|
||||
|
||||
assertTrue(worktrees.removeCalls().isEmpty(),
|
||||
"a worktree that turned dirty between the pre-stop read and removal must be preserved");
|
||||
assertEquals(2, worktrees.hasUncommittedCallCount(),
|
||||
"the fix re-reads hasUncommitted exactly once more, immediately before removal");
|
||||
}
|
||||
|
||||
@Test
|
||||
void releaseSnapshotsWorkFoundOnlyByTheLateRecheck() {
|
||||
// #316's second half: the pre-stop dirty=false means trySnapshot never ran for this
|
||||
// session, so the late-discovered work would otherwise have no refs/wip/* copy at all —
|
||||
// only the on-disk preserve. The re-check path must snapshot it too.
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
RecordingWorktrees worktrees = new RecordingWorktrees().dirtySequence(false, true);
|
||||
SessionManager sessions = sessionManager(herdr, worktrees);
|
||||
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
|
||||
new WorktreeRequest("cb-316b", null));
|
||||
|
||||
sessions.release(s.paneId());
|
||||
|
||||
assertEquals(java.util.List.of(s.worktree()), worktrees.snapshotCalls(),
|
||||
"the newly-dirty worktree is snapshotted even though the pre-stop check saw it clean");
|
||||
}
|
||||
|
||||
@Test
|
||||
void releaseStillRemovesAWorktreeThatStaysCleanOnTheLateRecheck() {
|
||||
// The ordinary, non-racing case: nothing else changes behaviour when the second read
|
||||
// agrees with the first.
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
RecordingWorktrees worktrees = new RecordingWorktrees().dirty(false);
|
||||
SessionManager sessions = sessionManager(herdr, worktrees);
|
||||
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
|
||||
new WorktreeRequest("cb-316c", null));
|
||||
|
||||
sessions.release(s.paneId());
|
||||
|
||||
assertEquals(java.util.List.of(s.worktree()), worktrees.removeCalls(),
|
||||
"a worktree that is still clean on the late recheck is removed as before");
|
||||
}
|
||||
|
||||
@Test
|
||||
void releaseNeverReChecksAWorktreeAlreadyPreservedByTheFirstDirtyCheck() {
|
||||
// Invariant 4 from #316: no second unconditional git status. A release that already
|
||||
// decided to preserve (the ordinary CB-576 dirty path) must not pay for a second read.
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
RecordingWorktrees worktrees = new RecordingWorktrees().dirty(true);
|
||||
SessionManager sessions = sessionManager(herdr, worktrees);
|
||||
MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null,
|
||||
new WorktreeRequest("cb-316d", null));
|
||||
|
||||
sessions.release(s.paneId());
|
||||
|
||||
assertEquals(1, worktrees.hasUncommittedCallCount(),
|
||||
"a release that already preserves on the first read must not re-check before "
|
||||
+ "skipping the removal it was never going to do");
|
||||
assertTrue(worktrees.removeCalls().isEmpty());
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #283 defect 1 changed this test's own premise, so its assertions are updated along
|
||||
* with the production fix. Before #283, the middle session's worktree-removal failure escaped
|
||||
|
||||
Reference in New Issue
Block a user