diff --git a/devlog/_plan/260819_unclaimed_bug_selection/170_2114_2108_fences.md b/devlog/_plan/260819_unclaimed_bug_selection/170_2114_2108_fences.md new file mode 100644 index 0000000000..59564854cb --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/170_2114_2108_fences.md @@ -0,0 +1,103 @@ +# 170 — #2114 and #2108: the fence that could not re-ask + +Both reports are the same shape wearing two operating systems: a probe that +could not get an answer, a verdict of `unknown`, and a fence that holds for the +whole process. Every native request 503s and only `ocx restart` clears it. + +Shipped as PR #2130, five commits. + +## The three changes + +**#2114 — the Linux session bus.** `inspectSystemd` called every non-zero exit +`unknown`. Its own comment was right that a non-zero status means the question +never reached the bus — and that is exactly why the verdict was wrong. It is +evidence about the bus, not about who owns this home. + +Widening on the exit code alone fails open: with the bus down `systemctl` +cannot see a foreign unit either, so "no answer" would read as "no owner" on a +machine that has one. The classification asks the disk instead, which needs no +bus. + +**#2108 (a) — an unaskable WinSW query.** WinSW is optional, and a +scheduler-only install has neither its XML nor its exe on disk. A timed-out +`sc.exe query` still returned `unknown`, which outranks the disk. With both +assets gone there is nothing for a registration to belong to. + +**#2108 (b) — the fence re-asks.** `startServer` takes the verdict once and +holds it, which is why waiting never helped and restart always did. Correct for +`foreign-ownership` — a foreign owner is a fact. Wrong for +`ownership-unknown`, which says the probe could not answer. + +## What the audit caught, three rounds running + +This is the part worth keeping. The functional idea was right in round one; the +**ownership model was wrong three times in a row**, and each time the symptom +was identical — a fence with no way to re-ask, which is #2108 returning by +another route. + +| Round | Defect | Why it slipped | +|---|---|---| +| 1 | `inspectSystemdOffline` checked ONE path, so a foreign unit in `~/.local/share/systemd/user` or an XDG override was invisible → `ownership: owned` on a foreign host | The bus-up path never had the hole; I reused its constant, not its coverage | +| 2 | The probe decremented the refcount AND the fence's own `release()` decremented again → a fence another holder owned got lifted | `Math.max(0, ...)` floored it, so it under-counted silently instead of going negative | +| 2 | A spent hook stayed in the map, so no LATER fence could install one | The "do not reset the budget" guard was right; its scope was not | +| 3 | Owner-identity guard was correct but **untested** — replacing it with `if (true)` left every suite green | A third route to the same wedge, with nothing pinning it | + +Round 1's fail-open is the one that mattered: I wrote "a unit naming a foreign +home stays present and still blocks" in a commit message, and the auditor proved +it end-to-end as `ownership = owned`. The claim was true only for a unit at the +canonical path. + +The fix that finally held is smaller than either attempt: the hook delegates to +the fence's own idempotent `release()`. One fence, one payment, whoever triggers +it. Both earlier versions were modelling the ownership in the wrong place. + +## Guards, and the ones that were not guards + +Every branch was ablated. Three that looked like guards were not: + +- the multi-unit conflict branch — a fail-closed decision with nothing asserting + it; ablating it left the suite green +- "foreign never retries" — double-implemented, so removing either half alone + stayed green; only removing both failed +- the owner-identity check — round 3's finding, above + +All three now have tests driven red against the real code. + +## Stated rather than hidden + +**Locale.** systemd localizes the bus-failure strings, so a non-English host +will not match and keeps the old `unknown`. That fences rather than admits, +which is the safe direction, but the #2114 fix does not reach every affected +user. Forcing `LC_ALL=C` on the probe would remove the caveat and is the +obvious follow-up; it is not done here because it changes every `systemctl` +call the module makes. + +**Coverage.** These are Windows and Linux platform paths. The suites that prove +them ran on macOS and on `ssh lidge`; the platform CI legs are the check that +actually matters. + +**A hostile `XDG_CONFIG_HOME`** redirects the offline check to an +attacker-chosen directory. The auditor raised it and then cleared it: reaching +that requires controlling the proxy's own environment, at which point +`CODEX_HOME` is equally controllable and the comparison is moot. No privilege +boundary is crossed. + +## Coordination with #2029 + +Draft PR #2029 edits the same function for #1939 and classifies two other bus +messages as `absent`. This branch never touched it or its branch. The two +agree in direction; if it lands first this reconciles rather than replaces. + +## Verification + +``` +full suite on ssh lidge 13,524 pass / 15 skip / 0 fail across 855 files +probe + startup + boundary 96 pass / 0 fail +bun x tsc --noEmit exit 0 +bun run privacy:scan exit 0 +``` + +One CI failure was investigated and dismissed on evidence: `keyring ubuntu` +stalled eight minutes on an apt mirror and was cancelled by the job timeout — +an infrastructure fault with no relation to the diff. + diff --git a/src/codex/native-profile-startup.ts b/src/codex/native-profile-startup.ts index bf1349aafa..e873579197 100644 --- a/src/codex/native-profile-startup.ts +++ b/src/codex/native-profile-startup.ts @@ -15,6 +15,7 @@ import { import { withNativeMainExclusiveClaim } from "./native-main-claim"; import { scrubNativeMainAuthTempResidues } from "./native-main-auth-temp"; import { NATIVE_STAGE_SWEEP_INTERVAL_MS } from "./native-profile-stage-store"; +import type { NativeCodexOwnership } from "../integrations/native/ownership-preflight"; export type NativeMainStartupGateSnapshot = | { status: "ready"; homeId: string | null } @@ -310,6 +311,75 @@ export function startNativeMainStartupLifecycle( }; } +/** + * How many times a service-ownership fence will re-ask before it stops asking (#2108). + * + * A host that is permanently unaskable must not re-probe on every request forever, and a + * host that recovers usually does so within the first few. The budget belongs to the + * REASON, not to an individual fence: raising a second fence deliberately does not hand + * out a fresh allowance, or a caller looping over fences could spin the probe forever. + * It is dropped when the last fence for that reason releases. + */ +export const NATIVE_MAIN_OWNERSHIP_RETRY_LIMIT = 5; + +/** Reprobe hooks for the fences currently held, keyed by the reason they were raised for. */ +const serviceOwnershipReprobes = new Map(); + +interface ServiceOwnershipReprobe { + readonly probe: () => NativeCodexOwnership; + attempts: number; + /** The fence that installed this hook; only its own release may drop the entry. */ + readonly owner: NativeMainStartupLifecycle; + /** Releases the fence that installed this hook, exactly once. */ + readonly spend: () => void; +} + +/** Test-only: the retry budget is module state and would otherwise leak across tests. */ +export function __resetNativeMainOwnershipRetries(): void { + for (const entry of serviceOwnershipReprobes.values()) entry.attempts = 0; +} + +/** + * Re-ask whether this host is still unownable, and drop the fence if it is not. + * + * `startServer` takes the ownership verdict once, at boot, and holds it for the process + * lifetime. For `foreign-ownership` that is correct — a foreign owner is a fact, and + * re-asking would only hand a determined caller a second chance. For `ownership-unknown` + * it is wrong: that verdict means the probe could not answer, so waiting cannot help, + * which is precisely why the #2108 reporter had to run `ocx restart` after every reboot. + * + * The re-probe is demand-driven rather than timed: it runs when something asks whether + * native-main is fenced, which is usually a request but is also the background token + * guardian's warmup. It is capped so a permanently unaskable host cannot spin. + * + * The probe is synchronous `spawnSync` with a bounded timeout, and this function is on a + * request path, so the cap is what keeps a wedged host from paying that cost repeatedly. + */ +function reprobeServiceOwnership(reason: NativeMainServiceOwnershipBlockReason): boolean { + if (reason !== "ownership-unknown") return false; + const entry = serviceOwnershipReprobes.get(reason); + if (!entry) return false; + if (entry.attempts >= NATIVE_MAIN_OWNERSHIP_RETRY_LIMIT) return false; + entry.attempts += 1; + let answer: NativeCodexOwnership; + try { + answer = entry.probe(); + } catch { + // An inspection that throws is not evidence the host became ownable. + return false; + } + if (answer !== "owned") return false; + // Release through the fence that installed this hook, and only that one. + // + // Several servers can hold a fence for the same reason while only one carries a hook, so + // clearing the shared refcount here would unblock fences this probe never spoke for. + // Decrementing here directly is just as wrong the other way: that fence's own release() + // would then pay a second time for one fence, leaving the count short. Delegating to the + // fence's idempotent release keeps exactly one payment per fence. + entry.spend(); + return true; +} + function activeServiceOwnershipBlockReason(): NativeMainServiceOwnershipBlockReason | null { if ((serviceOwnershipRefs.get("foreign-ownership") ?? 0) > 0) return "foreign-ownership"; if ((serviceOwnershipRefs.get("ownership-unknown") ?? 0) > 0) return "ownership-unknown"; @@ -325,10 +395,11 @@ function serviceOwnershipSnapshot( /** Close native-main admission without resolving or creating any CODEX_HOME artifacts. */ export function blockNativeMainStartupForUnownedServiceHome( reason: NativeMainServiceOwnershipBlockReason, + options?: { reprobe?: () => NativeCodexOwnership }, ): NativeMainStartupLifecycle { serviceOwnershipRefs.set(reason, (serviceOwnershipRefs.get(reason) ?? 0) + 1); let released = false; - return { + const lifecycle: NativeMainStartupLifecycle = { homeId: null, settled: Promise.resolve(serviceOwnershipSnapshot(reason)), async release() { @@ -337,8 +408,25 @@ export function blockNativeMainStartupForUnownedServiceHome( const remaining = Math.max(0, (serviceOwnershipRefs.get(reason) ?? 0) - 1); if (remaining === 0) serviceOwnershipRefs.delete(reason); else serviceOwnershipRefs.set(reason, remaining); + if (serviceOwnershipReprobes.get(reason)?.owner === lifecycle) { + serviceOwnershipReprobes.delete(reason); + } }, }; + // Do NOT reset an existing budget: keying the reprobe by reason means a caller raising + // fences in a loop would otherwise be handed a fresh allowance each time and could spin + // the probe forever. But once the holder is gone its entry is removed above, so a LATER + // fence installs its own hook — a server started after an earlier probe must not be left + // needing `ocx restart`, which is the very symptom this exists to remove. + if (options?.reprobe && reason === "ownership-unknown" && !serviceOwnershipReprobes.has(reason)) { + serviceOwnershipReprobes.set(reason, { + probe: options.reprobe, + attempts: 0, + owner: lifecycle, + spend: () => { void lifecycle.release(); }, + }); + } + return lifecycle; } export function bindNativeMainStartupLifecycle(server: object, lifecycle: NativeMainStartupLifecycle): void { @@ -353,7 +441,13 @@ export async function releaseNativeMainStartupLifecycle(server: object): Promise } export function isNativeMainTrafficBlocked(): boolean { - return activeServiceOwnershipBlockReason() !== null || snapshot.status === "blocked"; + const reason = activeServiceOwnershipBlockReason(); + if (reason !== null && reprobeServiceOwnership(reason)) { + // The host became ownable after boot (#2108): the fence lifts here rather than + // waiting for the restart the reporter had to perform by hand. + return activeServiceOwnershipBlockReason() !== null || snapshot.status === "blocked"; + } + return reason !== null || snapshot.status === "blocked"; } /** diff --git a/src/server/index.ts b/src/server/index.ts index 8f43b8fdf1..b33e3f1750 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -705,6 +705,11 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server inspectStartupOwnership(deps).ownership }, ) : { homeId: null, diff --git a/src/service-manager-probe.ts b/src/service-manager-probe.ts index 8446c78ae5..670e55e4f8 100644 --- a/src/service-manager-probe.ts +++ b/src/service-manager-probe.ts @@ -179,6 +179,86 @@ function unitEnvValue(body: string, key: string): string | null { return null; } +/** + * Did `systemctl --user` fail because the session bus could not be reached at all? + * + * These are the shapes reported on #2114 and #1939. The distinction that matters is + * "the question never left the machine" versus "systemd answered and said no" — only + * the former licenses reading the disk instead. + * + * **Locale caveat, stated rather than hidden:** systemd localizes these strings, so a + * non-English host will not match and keeps the old `unknown`. That is the safe + * direction — it fences rather than admits — but it does mean the fix does not reach + * every affected user. Forcing `LC_ALL=C` on the probe would remove the caveat and is + * the obvious follow-up; it is not done here because it changes every systemctl call + * this module makes, not just this branch. + */ +function busUnreachable(stderr: string): boolean { + const err = stderr.trim(); + return err.includes("Failed to connect to bus") + || err.includes("Failed to connect to user scope bus") + || err.includes("Failed to get D-Bus connection") + || err.includes("DBUS_SESSION_BUS_ADDRESS") + || err.includes("System has not been booted with systemd"); +} + +/** + * Ownership from the unit file alone, for when the bus cannot answer (#2114). + * + * A unit file is proof of installation that does not require a running bus, and the homes + * it names are what ownership is actually decided on. What the disk cannot tell us is + * whether systemd has the unit LOADED, so this reports `registration: "absent"` — the + * honest reading of "no running manager has it" — rather than inventing a live state. + * + * A foreign home therefore still blocks, which is the whole reason this consults the disk + * instead of widening the exit code. + */ +function systemdUserUnitSearchPaths(home: string): string[] { + // systemd's user search path is not one directory. Checking only the canonical one and + // calling the rest absent is a fail-open: with the bus down a foreign unit in any other + // search dir is invisible, and "no answer" would be read as "no owner". + const xdgConfig = process.env.XDG_CONFIG_HOME?.trim(); + const xdgData = process.env.XDG_DATA_HOME?.trim(); + const dirs = [ + xdgConfig ? join(xdgConfig, "systemd", "user") : join(home, ".config", "systemd", "user"), + join(home, ".config", "systemd", "user"), + xdgData ? join(xdgData, "systemd", "user") : join(home, ".local", "share", "systemd", "user"), + join(home, ".local", "share", "systemd", "user"), + ]; + return [...new Set(dirs)].map(dir => join(dir, `${TASK}.service`)); +} + +function inspectSystemdOffline(home: string): ServiceManagerInstallation { + const candidates = systemdUserUnitSearchPaths(home); + const found = candidates.filter(path => artifactPresence(path) === "present"); + if (candidates.some(path => artifactPresence(path) === "unreadable")) { + return unknown("the session bus is unreachable and a systemd unit could not be read"); + } + if (found.length === 0) return { kind: "absent" }; + if (found.length > 1) { + return unknown("the session bus is unreachable and more than one systemd unit file claims this proxy"); + } + const definitionPath = found[0]!; + let body: string; + try { + body = readFileSync(definitionPath, "utf-8"); + } catch (error) { + return unknown(`the session bus is unreachable and the systemd unit could not be read: ${String(error)}`); + } + return { + kind: "present", + claims: [{ + backend: "systemd", + definitionPath, + homes: { + codexHome: unitEnvValue(body, "CODEX_HOME"), + opencodexHome: unitEnvValue(body, "OPENCODEX_HOME"), + }, + registration: "absent", + }], + }; +} + function inspectLaunchd(deps: Required>): ServiceManagerInstallation { const definitionPath = join(deps.home, "Library", "LaunchAgents", `${LABEL}.plist`); @@ -269,6 +349,15 @@ function inspectSystemd(deps: Required>): Servic if (shown.status !== 0) { // A missing unit still exits ZERO and says not-found; a non-zero status means // the question never reached the bus. + // + // That is evidence about the BUS, not evidence that a foreign service owns this home + // (#2114). Calling it `unknown` fences native-main for the whole process, so a laptop + // with no session bus answers every native request with a 503 until `ocx restart`. + // + // Widening on the exit code alone would fail open, because with the bus down systemctl + // cannot see a foreign unit either. So ask the disk, which needs no bus, and fall back + // to `unknown` for every other non-zero exit. + if (busUnreachable(shown.stderr)) return inspectSystemdOffline(deps.home); return unknown(`systemctl show exited ${String(shown.status)}: ${shown.stderr.trim()}`); } @@ -729,6 +818,16 @@ function walkWinswChain( const registration = probeWinswRegistration(deps); if (xml === "absent" && exe === "absent" && registration === "absent") return { kind: "absent" }; + // A query we could not ask is a question about a service that cannot exist: WinSW is an + // optional backend, and with neither its XML nor its exe on disk there is nothing for a + // registration to belong to. Fencing here on an `sc.exe` timeout is one of the two + // triggers behind #2108, where a scheduler-only install answers 503 until `ocx restart`. + // + // The disk outranks the unaskable query only when BOTH assets are gone. Either one + // present means a real install may be there and the old `unknown` still holds. + if (registration === "unknown" && xml === "absent" && exe === "absent") { + return { kind: "absent" }; + } if (registration === "unknown") { return unknown("the native WinSW service registration could not be verified"); } diff --git a/tests/codex-service-manager-probe.test.ts b/tests/codex-service-manager-probe.test.ts index d1c5035360..0082f19723 100644 --- a/tests/codex-service-manager-probe.test.ts +++ b/tests/codex-service-manager-probe.test.ts @@ -274,7 +274,10 @@ describe("could not ask is not an answer", () => { * reached the bus, which is the opposite conclusion. */ test("a non-zero systemctl status is unknown even though a missing unit exits zero", () => { - const { run } = recorder(() => ({ status: 1, stderr: "Failed to connect to bus" })); + // Amended for #2114, not deleted: the rule still holds for every non-zero exit whose + // stderr does not prove the bus itself was unreachable. The bus-down family is handled + // by reading the unit file instead, and is asserted separately below. + const { run } = recorder(() => ({ status: 1, stderr: "Job for opencodex-proxy.service failed" })); expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).toBe("unknown"); }); @@ -1008,3 +1011,163 @@ describe("ownership refuses what it cannot prove", () => { expect(result.ownership).toBe("owned"); }); }); + +/* + * #2114: systemctl exists and runs, but the user session bus does not answer. + * + * The old branch called every non-zero exit `unknown`, which fences native-main for the + * whole process — the reporter's 503. But "the question never reached the bus" is evidence + * about the BUS, not evidence that a foreign service owns this home. + * + * Widening the exit code alone would fail open, because with the bus down systemctl cannot + * see a foreign unit either. So the classification asks the DISK, which needs no bus. + */ +describe("systemd probe: the bus is unreachable (#2114)", () => { + const BUS_DOWN = "Failed to connect to user scope bus via local transport: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined"; + + test("no unit file on disk means nothing can own this home — absent, not fenced", () => { + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).toBe("absent"); + }); + + test("a unit naming THIS home is still ours, read off disk", () => { + const definitionPath = writeUnit(join(home, ".codex"), join(home, ".opencodex")); + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + const result = inspectServiceManagerInstallation({ run, platform: "linux", home }); + + expect(result.kind).toBe("present"); + // Registration is genuinely unknowable with the bus down; the claim must not invent it. + expect(result.kind === "present" && result.claims[0]?.definitionPath).toBe(definitionPath); + expect(result.kind === "present" && result.claims[0]?.homes.codexHome).toBe(join(home, ".codex")); + }); + + // The guard that keeps this fail-closed. A foreign unit is exactly the case the old + // `unknown` existed to protect, and it must survive the widening. + test("a unit naming a FOREIGN home still blocks", () => { + writeUnit("/other/.codex", "/other/.opencodex"); + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + const result = inspectServiceManagerInstallation({ run, platform: "linux", home }); + + expect(result.kind).toBe("present"); + expect(result.kind === "present" && result.claims[0]?.homes.codexHome).toBe("/other/.codex"); + }); + + test("a non-bus failure is untouched — it stays unknown", () => { + const { run } = recorder(() => ({ status: 1, stderr: "Job for opencodex-proxy.service failed" })); + + expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).toBe("unknown"); + }); +}); + +/* + * #2108: a reboot leaves native-main fenced until `ocx restart`. + * + * One trigger is a timed-out `sc.exe query`. WinSW is an optional backend, and a + * scheduler-only install has neither of its assets on disk — but a query that timed out + * returns "unknown", which outranks the disk and fences the whole process. + * + * With BOTH assets absent there is nothing for a WinSW registration to belong to, so a + * failed query is a question about a service that cannot exist. + */ +describe("WinSW probe: a timed-out query with no assets (#2108)", () => { + test("no xml and no exe means absent, even when the query could not be asked", () => { + // Only the WinSW query is unaskable. The scheduler answers absent for itself, so the + // whole verdict turns on whether the WinSW half fences over assets that are not on disk. + const { runRaw } = recorder((file, args) => args[0] === "/query" + ? { status: 1, stderr: "ERROR: The system cannot find the file specified." } + : { timedOut: true }); + + const result = inspectServiceManagerInstallation({ + platform: "win32", home, runRaw, + winswStatus: () => "unknown", + }); + + expect(result.kind).toBe("absent"); + }); + + test("an unaskable query with WinSW assets present is still unknown", () => { + const dir = join(home, ".opencodex", "winsw"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "opencodex-proxy.xml"), "opencodex-proxy"); + writeFileSync(join(dir, "opencodex-proxy.exe"), "MZ"); + + const { runRaw } = recorder(() => ({ timedOut: true })); + + const result = inspectServiceManagerInstallation({ + platform: "win32", home, runRaw, + winswStatus: () => "unknown", + }); + + expect(result.kind).toBe("unknown"); + }); +}); + +/* + * The fail-open an audit caught in the first cut of the #2114 fix. + * + * systemd's user search path is not one directory: ~/.local/share/systemd/user and + * $XDG_CONFIG_HOME/systemd/user are also live, and system-level units are never in the + * user path at all. With the bus down, a foreign unit in any of those is invisible. + * + * Returning "absent" because ONE path was empty produced ownership: owned on a host a + * foreign service owns — exactly the failure the disk check was supposed to prevent. + */ +describe("bus-down absence must mean absence everywhere systemd looks (#2114)", () => { + const BUS_DOWN = "Failed to connect to user scope bus via local transport: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined"; + + test("a foreign unit in the other user search dir still blocks", () => { + const dir = join(home, ".local", "share", "systemd", "user"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "opencodex-proxy.service"), [ + "[Service]", + 'Environment="CODEX_HOME=/other/.codex"', + 'Environment="OPENCODEX_HOME=/other/.opencodex"', + ].join("\n")); + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + const result = inspectServiceManagerInstallation({ run, platform: "linux", home }); + + expect(result.kind).not.toBe("absent"); + }); + + test("XDG_CONFIG_HOME is honored the way systemd honors it", () => { + const xdg = join(home, "xdg-config"); + const dir = join(xdg, "systemd", "user"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "opencodex-proxy.service"), '[Service]\nEnvironment="CODEX_HOME=/other/.codex"'); + const previous = process.env.XDG_CONFIG_HOME; + process.env.XDG_CONFIG_HOME = xdg; + try { + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).not.toBe("absent"); + } finally { + if (previous === undefined) delete process.env.XDG_CONFIG_HOME; + else process.env.XDG_CONFIG_HOME = previous; + } + }); + + test("a genuinely empty disk is still absent", () => { + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).toBe("absent"); + }); + + // The multi-unit branch is a fail-closed decision that nothing pinned: ablating it left the + // suite green. Two units on disk with the bus down is genuinely ambiguous — neither can be + // confirmed as the live definition — so it must refuse rather than pick one. + test("two units in different search dirs refuse rather than choose", () => { + const a = join(home, ".config", "systemd", "user"); + const b = join(home, ".local", "share", "systemd", "user"); + mkdirSync(a, { recursive: true }); + mkdirSync(b, { recursive: true }); + writeFileSync(join(a, "opencodex-proxy.service"), `[Service]\nEnvironment="CODEX_HOME=${join(home, ".codex")}"`); + writeFileSync(join(b, "opencodex-proxy.service"), '[Service]\nEnvironment="CODEX_HOME=/other/.codex"'); + const { run } = recorder(() => ({ status: 1, stderr: BUS_DOWN })); + + expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).toBe("unknown"); + }); +}); diff --git a/tests/native-profile-startup.test.ts b/tests/native-profile-startup.test.ts index f8129f3d27..fe59dc2224 100644 --- a/tests/native-profile-startup.test.ts +++ b/tests/native-profile-startup.test.ts @@ -34,7 +34,10 @@ import { initializeNativeMainStartupGate, isNativeMainTrafficBlocked, nativeMainStartupGateSnapshot, + NATIVE_MAIN_OWNERSHIP_RETRY_LIMIT, + __resetNativeMainOwnershipRetries, } from "../src/codex/native-profile-startup"; +import type { NativeCodexOwnership } from "../src/integrations/native/ownership-preflight"; import { tryAcquireNativeMainProfileClaim, tryClaimNativeMainProfileForTurn, @@ -623,3 +626,222 @@ describe("native-main startup journal gate", () => { } }, 20_000); }); + +/* + * #2108: after a Windows reboot the fence never lifts until `ocx restart`. + * + * `startServer` takes the ownership verdict ONCE and holds it for the process lifetime. + * That is right for `foreign-ownership` — a foreign owner is a fact, and re-asking would + * only give a determined caller a second chance. It is wrong for `ownership-unknown`, + * which means the probe could not answer: waiting cannot help, which is exactly why the + * reporter had to restart. + * + * The retry is deliberately NOT automatic-on-a-timer. It re-probes when a native request + * actually arrives, so an idle proxy does no work, and it is capped so a permanently + * unaskable host cannot spin. + */ +describe("an unknown service-ownership fence is retryable (#2108)", () => { + afterEach(() => { + __resetNativeMainOwnershipRetries(); + }); + + test("a later successful probe reopens the gate without a restart", () => { + let answer: NativeCodexOwnership = "unknown"; + const fence = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => answer, + }); + try { + expect(isNativeMainTrafficBlocked()).toBe(true); + + answer = "owned"; + + expect(isNativeMainTrafficBlocked()).toBe(false); + } finally { + void fence.release(); + } + }); + + test("a foreign owner is a fact, not a question — it never retries", () => { + let asked = 0; + const fence = blockNativeMainStartupForUnownedServiceHome("foreign-ownership", { + reprobe: () => { asked += 1; return "owned"; }, + }); + try { + expect(isNativeMainTrafficBlocked()).toBe(true); + expect(isNativeMainTrafficBlocked()).toBe(true); + expect(asked).toBe(0); + } finally { + void fence.release(); + } + }); + + test("a host that stays unaskable stops being asked", () => { + let asked = 0; + const fence = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => { asked += 1; return "unknown"; }, + }); + try { + for (let i = 0; i < 25; i++) isNativeMainTrafficBlocked(); + + expect(isNativeMainTrafficBlocked()).toBe(true); + expect(asked).toBeLessThanOrEqual(NATIVE_MAIN_OWNERSHIP_RETRY_LIMIT); + } finally { + void fence.release(); + } + }); + + test("with no reprobe wired the fence behaves exactly as before", () => { + const fence = blockNativeMainStartupForUnownedServiceHome("ownership-unknown"); + try { + expect(isNativeMainTrafficBlocked()).toBe(true); + } finally { + void fence.release(); + } + }); +}); + +/* + * Two defects an audit found in the first cut of the retryable fence, both from keying the + * reprobe by REASON while the fence refcount is per-fence. + */ +describe("the retryable fence respects its own refcount (#2108)", () => { + afterEach(() => { + __resetNativeMainOwnershipRetries(); + }); + + test("raising a second fence does not hand out a fresh retry budget", () => { + let asked = 0; + const probe = () => { asked += 1; return "unknown" as NativeCodexOwnership; }; + const first = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { reprobe: probe }); + for (let i = 0; i < 20; i++) isNativeMainTrafficBlocked(); + const afterFirst = asked; + + const second = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { reprobe: probe }); + try { + for (let i = 0; i < 20; i++) isNativeMainTrafficBlocked(); + + // A caller raising fences in a loop must not be able to spin the probe forever. + expect(asked).toBe(afterFirst); + } finally { + void first.release(); + void second.release(); + } + }); + + test("one successful probe does not lift a fence it never spoke for", () => { + const hookless = blockNativeMainStartupForUnownedServiceHome("ownership-unknown"); + const hooked = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => "owned" as NativeCodexOwnership, + }); + try { + isNativeMainTrafficBlocked(); + + // The hookless fence is still held, so traffic stays blocked until IT releases. + expect(isNativeMainTrafficBlocked()).toBe(true); + } finally { + void hooked.release(); + void hookless.release(); + } + }); +}); + +/* + * The audit noted the "foreign never retries" property was single-covered: the guard exists + * in two places, and ablating either alone stayed green. This pins the OUTCOME rather than + * one of the two implementations, so removing either is caught. + */ +describe("a foreign fence is never reopened by a probe (#2108)", () => { + afterEach(() => { + __resetNativeMainOwnershipRetries(); + }); + + test("a foreign fence stays closed even when the host reports owned", () => { + const fence = blockNativeMainStartupForUnownedServiceHome("foreign-ownership", { + reprobe: () => "owned" as NativeCodexOwnership, + }); + try { + for (let i = 0; i < 10; i++) isNativeMainTrafficBlocked(); + + expect(isNativeMainTrafficBlocked()).toBe(true); + expect(nativeMainStartupGateSnapshot()).toMatchObject({ status: "blocked", reason: "foreign-ownership" }); + } finally { + void fence.release(); + } + }); +}); + +/* + * The double-decrement a second audit round found: the probe paid for the hooked fence, and + * then that fence's own release() paid for it again. One fence, two decrements, so a fence + * another holder still owns was lifted. Plus the wedge: once a hook was spent, no LATER + * fence could install one, which is the #2108 symptom returning by another route. + */ +describe("a spent reprobe leaves the refcount coherent (#2108)", () => { + afterEach(() => { + __resetNativeMainOwnershipRetries(); + }); + + test("the hooked fence's release does not pay twice for the same fence", () => { + const hookless = blockNativeMainStartupForUnownedServiceHome("ownership-unknown"); + const hooked = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => "owned" as NativeCodexOwnership, + }); + try { + isNativeMainTrafficBlocked(); + void hooked.release(); + + // The hookless fence is still held by its owner and must keep traffic closed. + expect(isNativeMainTrafficBlocked()).toBe(true); + } finally { + void hookless.release(); + } + expect(isNativeMainTrafficBlocked()).toBe(false); + }); + + test("a fence raised after a spent probe still gets to re-ask", () => { + const first = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => "owned" as NativeCodexOwnership, + }); + isNativeMainTrafficBlocked(); + void first.release(); + + let asked = 0; + const later = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => { asked += 1; return "owned" as NativeCodexOwnership; }, + }); + try { + isNativeMainTrafficBlocked(); + + // A server started after an earlier probe must not be stuck needing `ocx restart`. + expect(asked).toBeGreaterThan(0); + expect(isNativeMainTrafficBlocked()).toBe(false); + } finally { + void later.release(); + } + }); + + // The wedge, by a third route. If a NON-owner fence's release dropped the entry, the + // owner's hook would be destroyed and the fence stuck until `ocx restart` — the #2108 + // symptom. This class of bug recurred across three audit rounds, so the guard that + // prevents it is pinned rather than merely present. + test("a non-owner release does not destroy the owner's hook", () => { + let asked = 0; + const owner = blockNativeMainStartupForUnownedServiceHome("ownership-unknown", { + reprobe: () => { asked += 1; return "owned" as NativeCodexOwnership; }, + }); + const other = blockNativeMainStartupForUnownedServiceHome("ownership-unknown"); + try { + void other.release(); + + isNativeMainTrafficBlocked(); + + expect(asked).toBe(1); + } finally { + void owner.release(); + } + }); +}); + +/* + * The multi-unit conflict branch was a fail-closed decision with nothing pinning it. + */