#317: refuse an unresolved caller instead of promoting it to primary #320

2026-09-04T08:59:30+02:00

agent commented

2026-09-04 08:59:30 +02:00

Fixes #317.

The bug

ConnectionIdentity.resolve calls pids.pidForLocalPort(remotePort).
LsofPeerPidLookup returns -1 on any failure, and also — silently, no
log line — when lsof runs clean but simply finds no matching process.
terminalForPid(-1) then matches no pane, so terminal is null, and
CallerResolver's loopback-trust fallback could not tell that caller
apart from a genuine primary: it passed c.pid() straight into
Principal.primary(c.pid()) without ever testing it. A worker whose
lookup failed was granted SPAWN, STOP, SEND and DRAIN — the escalation
PaneLocator's own javadoc already names word for word. CB-161's
ancestry walk only helps once a candidate pid exists; a failed lookup
has none to walk.

I read #305's close-out first, per the ticket's instruction, for the
"one rule, two copies, drifted" precedent that shaped the fix below.

Correcting the hunter (per the ticket's own instruction)

Same correction the ticket's author already made: the failure is not
lsof missing a 2-second timeout. waitFor(2, TimeUnit.SECONDS) runs
after the read loop has already drained stdout to EOF, and its
result is discarded — found is returned either way. A slow lsof
blocks in readLine, not at 2 seconds. I re-read the method and
confirm that reading is correct.

The fix

ConnectionIdentity.Caller gains a resolved() predicate:

public boolean resolved() {
    return pid > 0;
}

CallerResolver's loopback-trust fallback now requires c.resolved()
before granting PRIMARY:

return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous();

An unresolved caller gets Principal.anonymous() — the same
already-tested "authenticated as nothing" outcome the resolver already
uses for a bad token or an off-host caller, so the refusal is a clean,
named, unsurprising result rather than something that looks like a bug
to the operator.

Also: the previously-silent "lsof ran clean, found no match" path in
LsofPeerPidLookup now logs at DEBUG (matching the existing
exception-path log), since the ticket's own analysis says this —
not a slow lsof — is the likelier real trigger, and it logged
nothing at all before.

The four things the ticket asked me to weigh

Is pid > 0 the right predicate, or should Caller carry an
explicit "unresolved" state? I kept the sentinel (pid == -1
already meant "not resolvable" per Caller's own javadoc — this
isn't a new encoding), but centralised the test of it as a method
on Caller itself, resolved(), rather than let CallerResolver
re-implement c.pid() > 0 inline. That mirrors the codebase's own
precedent: isLoopback is centralised in ConnectionIdentity
specifically because two independent copies of that rule drifted
and caused #305. A bare pid > 0 at the call site would be exactly
that shape again. I judged a same-record predicate method
sufficient — it's one place, not two — rather than a separate
explicit "unresolved" type, since Caller is the only place that
needs to know about the sentinel at all now.
Should the lookup retry? I decided against it. pidForLocalPort
already forks a subprocess (lsof) on every call on the hot path
(contextExtractor, every MCP call); a retry doubles that cost
exactly on the failed calls, which are also the ones most likely to
be failing because the system is already under load. A bounded,
sleep-free single retry might be worth adding later, but I don't
have evidence for how often the failure is transient vs. genuine,
and the ticket asked me to decide rather than guess — so I left it
out and documented why in the code comment. The refusal itself is
cheap and self-correcting (a refused primary calls again), so I
didn't judge the added latency worth trading for the added
complexity.
What happens when the primary's own lookup fails? It is refused
— Role.ANONYMOUS, which is already a fully-plumbed, tested outcome
(Authz.isUnauthenticated reads it as "you are nobody", a 401-style
error, not a crash or a silent hang). I did not add a distinct error
for this case; it reuses the same path a bad bearer token already
takes, so there is nothing new for the operator to learn, and it
already reads as "not authenticated" rather than "something broke".
Token mode. Confirmed unaffected: the tokenMode branch in
CallerResolver.resolve is reached only when c.terminal() == null
(unchanged), and it never reads c.pid() at all — only
presentedTokenMatches(authorizationHeader). Added
tokenModeIsUndisturbedByAnUnresolvedLookup to pin this explicitly
rather than rely on reading the code.

Invariant kept green

CallerResolverTest.loopbackTrustTreatsANonWorkerLoopbackCallerAsThePrimary
is untouched and still passes: a real pid that owns no pane (the
actual primary's own connection) is resolved() (a real lsof-found
pid, just no matching herdr pane) and is still PRIMARY. This ticket
is about an unresolvable caller, not a non-worker one — I kept the
two paths distinct rather than collapsing them.

Tests added

CallerResolverTest.aFailedPeerPidLookupIsRefusedNotPromotedToPrimary
— the failing-without-the-fix case: a loopback caller whose lookup
returns -1 must not resolve to PRIMARY.
CallerResolverTest.aRealPidThatOwnsNoPaneIsStillThePrimaryNotRefused
— the companion for invariant 2, named for #317 and placed next to
the test above so the two verdicts (same null terminal, opposite
outcome) sit side by side.
CallerResolverTest.tokenModeIsUndisturbedByAnUnresolvedLookup —
point 4 above, pinned as a test rather than left as a read-the-code
claim.
ConnectionIdentityTest.callerIsUnresolvedWhenThePeerPidLookupFails
and callerIsResolvedWhenThePidIsRealEvenThoughItOwnsNoPane — unit
tests on the new Caller.resolved() predicate itself, in the class
where it lives.

No test opens a real socket or exercises a live escalation path — all
of the above construct ConnectionIdentity with a fake PeerPidLookup
lambda, the same pattern the existing CallerResolverTest /
ConnectionIdentityTest suites already use.

Mutation proof

Reverted only the CallerResolver.java guard (kept
ConnectionIdentity.Caller.resolved() and both new test files) and
ran mvn test -Dtest=CallerResolverTest,ConnectionIdentityTest:

[ERROR] Tests run: 38, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.237 s <<< FAILURE! -- in dev.ltms.fleet.auth.CallerResolverTest
[ERROR] dev.ltms.fleet.auth.CallerResolverTest.aFailedPeerPidLookupIsRefusedNotPromotedToPrimary -- Time elapsed: 0.003 s <<< FAILURE!
org.opentest4j.AssertionFailedError: an unresolvable caller must never be silently promoted to the primary ==> expected: <ANONYMOUS> but was: <PRIMARY>

Restored the guard, then ran the full build again — matches the
"restored" build below.

Build

cd fleetd && mvn clean install, unpiped, full output read:

[INFO] Tests run: 1328, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

(#305's close-out measured main at 1310 tests before this change;
this PR adds 5.)

Caveats for review

I added a DEBUG log line to LsofPeerPidLookup for the "ran clean,
no match" case. This is not strictly required by the ticket's rules
list, but it directly answers weighing-point 3 ("confirm the refusal
is a clean named error... not something that looks like a bug") by
closing the exact silent-observability gap the ticket's own analysis
names. It changes no behaviour — same DEBUG level as the existing
exception-path log, same method, no new state.
Shape check (per the ticket's tail instruction — reported, not
fixed): grepping auth/ and mcp/ for the same "failure
downgraded to a value indistinguishable from a legitimate result"
shape, one line each:
- ConnectionIdentity.cwdForPid(long pid) returns null both when
  pid <= 0 (unresolved) and when cwds.cwdForPid(pid) itself
  returns null for a resolved pid the OS lookup simply can't find a
  cwd for (e.g. a permission error) — the same "two failure reasons,
  one sentinel" shape as this ticket, one caller away from mattering.
- MessageDigest.isEqual misuse aside, presentedTokenMatches
  returns false uniformly for "no header", "wrong scheme", "empty
  credential" and "wrong token" — a legitimate design choice here
  (all four should mean ANONYMOUS), but it's the same
  can't-tell-the-reasons-apart shape if anyone later wants to log
  which one happened.
- Authz.permits returns false for caller == null and for a
  genuinely-ANONYMOUS caller alike — again probably fine (both
  should deny), but flagged as the same collapse-of-distinct-causes
  pattern.
Not investigated further or fixed, per the ticket's instruction.

Fixes #317. ## The bug `ConnectionIdentity.resolve` calls `pids.pidForLocalPort(remotePort)`. `LsofPeerPidLookup` returns `-1` on any failure, and also — silently, no log line — when `lsof` runs clean but simply finds no matching process. `terminalForPid(-1)` then matches no pane, so `terminal` is `null`, and `CallerResolver`'s loopback-trust fallback could not tell that caller apart from a genuine primary: it passed `c.pid()` straight into `Principal.primary(c.pid())` without ever testing it. A worker whose lookup failed was granted SPAWN, STOP, SEND and DRAIN — the escalation `PaneLocator`'s own javadoc already names word for word. CB-161's ancestry walk only helps once a candidate pid exists; a failed lookup has none to walk. I read #305's close-out first, per the ticket's instruction, for the "one rule, two copies, drifted" precedent that shaped the fix below. ## Correcting the hunter (per the ticket's own instruction) Same correction the ticket's author already made: the failure is not `lsof` missing a 2-second timeout. `waitFor(2, TimeUnit.SECONDS)` runs *after* the read loop has already drained stdout to EOF, and its result is discarded — `found` is returned either way. A slow `lsof` blocks in `readLine`, not at 2 seconds. I re-read the method and confirm that reading is correct. ## The fix `ConnectionIdentity.Caller` gains a `resolved()` predicate: ```java public boolean resolved() { return pid > 0; } ``` `CallerResolver`'s loopback-trust fallback now requires `c.resolved()` before granting `PRIMARY`: ```java return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous(); ``` An unresolved caller gets `Principal.anonymous()` — the same already-tested "authenticated as nothing" outcome the resolver already uses for a bad token or an off-host caller, so the refusal is a clean, named, unsurprising result rather than something that looks like a bug to the operator. Also: the previously-silent "`lsof` ran clean, found no match" path in `LsofPeerPidLookup` now logs at DEBUG (matching the existing exception-path log), since the ticket's own analysis says this — not a slow `lsof` — is the likelier real trigger, and it logged nothing at all before. ## The four things the ticket asked me to weigh 1. **Is `pid > 0` the right predicate, or should `Caller` carry an explicit "unresolved" state?** I kept the sentinel (`pid == -1` already meant "not resolvable" per `Caller`'s own javadoc — this isn't a new encoding), but centralised the *test* of it as a method on `Caller` itself, `resolved()`, rather than let `CallerResolver` re-implement `c.pid() > 0` inline. That mirrors the codebase's own precedent: `isLoopback` is centralised in `ConnectionIdentity` specifically because two independent copies of that rule drifted and caused #305. A bare `pid > 0` at the call site would be exactly that shape again. I judged a same-record predicate method sufficient — it's one place, not two — rather than a separate explicit "unresolved" type, since `Caller` is the only place that needs to know about the sentinel at all now. 2. **Should the lookup retry?** I decided against it. `pidForLocalPort` already forks a subprocess (`lsof`) on every call on the hot path (`contextExtractor`, every MCP call); a retry doubles that cost exactly on the failed calls, which are also the ones most likely to be failing because the system is already under load. A bounded, sleep-free single retry might be worth adding later, but I don't have evidence for how often the failure is transient vs. genuine, and the ticket asked me to decide rather than guess — so I left it out and documented why in the code comment. The refusal itself is cheap and self-correcting (a refused primary calls again), so I didn't judge the added latency worth trading for the added complexity. 3. **What happens when the primary's own lookup fails?** It is refused — `Role.ANONYMOUS`, which is already a fully-plumbed, tested outcome (`Authz.isUnauthenticated` reads it as "you are nobody", a 401-style error, not a crash or a silent hang). I did not add a distinct error for this case; it reuses the same path a bad bearer token already takes, so there is nothing new for the operator to learn, and it already reads as "not authenticated" rather than "something broke". 4. **Token mode.** Confirmed unaffected: the `tokenMode` branch in `CallerResolver.resolve` is reached only when `c.terminal() == null` (unchanged), and it never reads `c.pid()` at all — only `presentedTokenMatches(authorizationHeader)`. Added `tokenModeIsUndisturbedByAnUnresolvedLookup` to pin this explicitly rather than rely on reading the code. ## Invariant kept green `CallerResolverTest.loopbackTrustTreatsANonWorkerLoopbackCallerAsThePrimary` is untouched and still passes: a real pid that owns no pane (the actual primary's own connection) is `resolved()` (a real `lsof`-found pid, just no matching herdr pane) and is still `PRIMARY`. This ticket is about an *unresolvable* caller, not a *non-worker* one — I kept the two paths distinct rather than collapsing them. ## Tests added - `CallerResolverTest.aFailedPeerPidLookupIsRefusedNotPromotedToPrimary` — the failing-without-the-fix case: a loopback caller whose lookup returns `-1` must not resolve to `PRIMARY`. - `CallerResolverTest.aRealPidThatOwnsNoPaneIsStillThePrimaryNotRefused` — the companion for invariant 2, named for #317 and placed next to the test above so the two verdicts (same `null` terminal, opposite outcome) sit side by side. - `CallerResolverTest.tokenModeIsUndisturbedByAnUnresolvedLookup` — point 4 above, pinned as a test rather than left as a read-the-code claim. - `ConnectionIdentityTest.callerIsUnresolvedWhenThePeerPidLookupFails` and `callerIsResolvedWhenThePidIsRealEvenThoughItOwnsNoPane` — unit tests on the new `Caller.resolved()` predicate itself, in the class where it lives. No test opens a real socket or exercises a live escalation path — all of the above construct `ConnectionIdentity` with a fake `PeerPidLookup` lambda, the same pattern the existing `CallerResolverTest` / `ConnectionIdentityTest` suites already use. ## Mutation proof Reverted only the `CallerResolver.java` guard (kept `ConnectionIdentity.Caller.resolved()` and both new test files) and ran `mvn test -Dtest=CallerResolverTest,ConnectionIdentityTest`: ``` [ERROR] Tests run: 38, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.237 s <<< FAILURE! -- in dev.ltms.fleet.auth.CallerResolverTest [ERROR] dev.ltms.fleet.auth.CallerResolverTest.aFailedPeerPidLookupIsRefusedNotPromotedToPrimary -- Time elapsed: 0.003 s <<< FAILURE! org.opentest4j.AssertionFailedError: an unresolvable caller must never be silently promoted to the primary ==> expected: <ANONYMOUS> but was: <PRIMARY> ``` Restored the guard, then ran the full build again — matches the "restored" build below. ## Build `cd fleetd && mvn clean install`, unpiped, full output read: ``` [INFO] Tests run: 1328, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` (#305's close-out measured `main` at 1310 tests before this change; this PR adds 5.) ## Caveats for review - I added a DEBUG log line to `LsofPeerPidLookup` for the "ran clean, no match" case. This is not strictly required by the ticket's rules list, but it directly answers weighing-point 3 ("confirm the refusal is a clean named error... not something that looks like a bug") by closing the exact silent-observability gap the ticket's own analysis names. It changes no behaviour — same DEBUG level as the existing exception-path log, same method, no new state. - **Shape check (per the ticket's tail instruction — reported, not fixed):** grepping `auth/` and `mcp/` for the same "failure downgraded to a value indistinguishable from a legitimate result" shape, one line each: - `ConnectionIdentity.cwdForPid(long pid)` returns `null` both when `pid <= 0` (unresolved) and when `cwds.cwdForPid(pid)` itself returns `null` for a resolved pid the OS lookup simply can't find a cwd for (e.g. a permission error) — the same "two failure reasons, one sentinel" shape as this ticket, one caller away from mattering. - `MessageDigest.isEqual` misuse aside, `presentedTokenMatches` returns `false` uniformly for "no header", "wrong scheme", "empty credential" and "wrong token" — a legitimate design choice here (all four *should* mean ANONYMOUS), but it's the same can't-tell-the-reasons-apart shape if anyone later wants to log *which* one happened. - `Authz.permits` returns `false` for `caller == null` and for a genuinely-`ANONYMOUS` caller alike — again probably fine (both should deny), but flagged as the same collapse-of-distinct-causes pattern. Not investigated further or fixed, per the ticket's instruction.

agent added 1 commit 2026-09-04 08:59:30 +02:00

#317 : refuse an unresolved caller instead of promoting it to primary

CI / contract (pull_request) Successful in 47s

Details

CI / build (pull_request) Successful in 1m52s

Details

53a533afb4

ConnectionIdentity.resolve() called pids.pidForLocalPort(remotePort),
which returns -1 both on a real failure and (silently, no log line)
when lsof just finds no matching process. terminalForPid(-1) then
matches no pane, so CallerResolver's loopback-trust fallback could not
tell that caller apart from a genuine primary and handed it
Principal.primary(...) — granting SPAWN, STOP, SEND and DRAIN to a
worker whose PID lookup failed. This is the escalation PaneLocator's
own javadoc already names; CB-161's ancestry walk only helps once a
candidate pid exists, and a failed lookup has none.

Fix: ConnectionIdentity.Caller gets a resolved() predicate (pid > 0),
centralised next to the -1 sentinel it tests for the same reason
isLoopback() is centralised (fleetd #305: two independent copies of
one rule already drifted once). CallerResolver's loopback-trust
fallback now requires c.resolved() before granting PRIMARY; an
unresolved caller gets Principal.anonymous() — the same already-tested
"authenticated as nothing" outcome used everywhere else in that
method, so the refusal is a clean, named, unsurprising result rather
than something that looks like a bug.

Also logs the previously-silent "lsof ran clean, found no match" case
in LsofPeerPidLookup at DEBUG, since that (not a slow lsof — the
waitFor result was already discarded) is the likelier real trigger.

loopbackTrustTreatsANonWorkerLoopbackCallerAsThePrimary is untouched
and still green: a real pid that owns no pane (the actual primary) is
still resolved() and still PRIMARY. Token mode is unaffected — it
never consults c.pid() at all.

Mutation-tested: reverting only the CallerResolver.java guard
reproduces the escalation exactly (aFailedPeerPidLookupIsRefusedNotPromotedToPrimary
fails with "expected: <ANONYMOUS> but was: <PRIMARY>").

ltms closed this pull request

2026-09-04 09:05:54 +02:00

CI / contract (pull_request) Successful in 47s

Details

CI / build (pull_request) Successful in 1m52s

Details

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#320