From b6b219c8714b4b5d925e334f108f8efdb601ac8e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 00:56:02 +0900 Subject: [PATCH 1/7] fix(probe): read the unit off disk when the session bus cannot answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A laptop where systemctl exists but the user session bus does not respond gets every native request answered with 503 until ocx restart. inspectSystemd called every non-zero exit unknown, ownership-preflight turned that into ownership unknown, and startServer fenced native-main for the process lifetime. The comment on that branch was right that a non-zero status means the question never reached the bus. That is the reason the verdict is wrong: it is evidence about the bus, not evidence that a foreign service owns this home. Widening on the exit code alone would fail open. With the bus down systemctl cannot see a foreign unit either, so "no answer" would be read as "no owner" on a machine that genuinely has one. The fix asks the disk instead, which needs no bus: the unit file is proof of installation, and the homes it names are what ownership is actually decided on. No unit file means absent. A unit naming a foreign home stays present and still blocks. Registration is reported as absent on this path rather than invented. The disk cannot say whether systemd has the unit loaded, and guessing there is how a stale claim would slip through. Refs #2114 Known limitation, stated rather than hidden: systemd localizes these stderr 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 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. Coordination: open draft PR #2029 edits the same function for #1939 and classifies two other bus messages as absent. This branch does not touch that PR or its branch. Its two classifications and this one agree in direction; if it lands first, this reconciles with it rather than replacing it. Verification: red-driven — three assertions fail before the change (present vs unknown), with the non-bus control passing throughout. 54 pass / 0 fail on the probe suite, 14 pass / 0 fail on service-probe-docker and native-profile-startup, tsc --noEmit exit 0. The pre-existing assertion that pinned this shape as unknown is amended to a non-bus stderr rather than deleted, so the rule it protects still holds. --- src/service-manager-probe.ts | 69 +++++++++++++++++++++++ tests/codex-service-manager-probe.test.ts | 55 +++++++++++++++++- 2 files changed, 123 insertions(+), 1 deletion(-) diff --git a/src/service-manager-probe.ts b/src/service-manager-probe.ts index 8446c78ae5..a300bbeaed 100644 --- a/src/service-manager-probe.ts +++ b/src/service-manager-probe.ts @@ -179,6 +179,66 @@ 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 inspectSystemdOffline(definitionPath: string): ServiceManagerInstallation { + const presence = artifactPresence(definitionPath); + if (presence === "absent") return { kind: "absent" }; + if (presence === "unreadable") { + return unknown("the session bus is unreachable and the systemd unit could not be read"); + } + 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 +329,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(definitionPath); return unknown(`systemctl show exited ${String(shown.status)}: ${shown.stderr.trim()}`); } diff --git a/tests/codex-service-manager-probe.test.ts b/tests/codex-service-manager-probe.test.ts index d1c5035360..92484dfed9 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,53 @@ 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"); + }); +}); From 7e95fc6f44cd71c76768c1a7018d258a5c2940c1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 00:57:23 +0900 Subject: [PATCH 2/7] fix(probe): an unaskable WinSW query with no assets on disk is absence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One of the two triggers behind #2108: after a reboot, a scheduler-only Windows install answers every native request with 503 until ocx restart. WinSW is an optional backend. A scheduler-only install has neither its XML nor its exe on disk, but the probe still runs sc.exe query for it, and a query that timed out returned unknown. That verdict outranks the disk, ownership-preflight turns it into ownership unknown, and startServer fences native-main for the whole process lifetime. A query we could not ask is a question about a service that cannot exist. With both assets absent there is nothing for a registration to belong to, so the disk answers it. 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 — that is the guard, and it has its own test. Refs #2108 This is the narrow half of phase 2. The broader change, making a boot-time unknown retryable while OCX_SERVICE=1 instead of a process-lifetime verdict, is not in this commit. Verification: red-driven, and ablated afterwards — reverting just the new branch puts the assertion back to failing, so the test is bound to this code rather than passing incidentally. 56 pass / 0 fail on the probe suite, tsc --noEmit exit 0. --- src/service-manager-probe.ts | 10 ++++++ tests/codex-service-manager-probe.test.ts | 43 +++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/service-manager-probe.ts b/src/service-manager-probe.ts index a300bbeaed..7cd61dea9d 100644 --- a/src/service-manager-probe.ts +++ b/src/service-manager-probe.ts @@ -798,6 +798,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 92484dfed9..615f1e68ff 100644 --- a/tests/codex-service-manager-probe.test.ts +++ b/tests/codex-service-manager-probe.test.ts @@ -1061,3 +1061,46 @@ describe("systemd probe: the bus is unreachable (#2114)", () => { 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"); + }); +}); From e95b8cf680fdfa3515fccde6836c00130c51c1a8 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 00:59:53 +0900 Subject: [PATCH 3/7] fix(codex): let an unknown ownership fence re-ask instead of holding for the process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The other half of #2108. A Windows reboot leaves native-main fenced and every native request answers 503 until ocx restart, which is the one thing that reliably cures it. The reason restart works is the whole bug: startServer takes the ownership verdict once, at boot, and holds it for the process lifetime. Waiting cannot help, because nothing re-asks. That is correct for foreign-ownership. A foreign owner is a fact, and re-asking would only hand a determined caller a second chance at a boundary that exists to refuse it. It is wrong for ownership-unknown, which does not say this host is unownable — it says the probe could not answer. So an unknown fence now carries a reprobe hook and lifts itself when the host becomes answerable. Three properties keep that honest: - foreign never retries, and the hook is not even recorded for it - the re-probe runs when a native request arrives, not on a timer, so an idle proxy does no work - attempts are capped, so a permanently unaskable host stops asking rather than re-probing on every request forever A fence constructed without the hook behaves exactly as before, which keeps every existing caller and test on the old path. Refs #2108 Verification: red-driven with the new exports staged as no-ops, so the failing assertion was the behavior rather than a module-load error — 1 fail on the reopen case with all three guards passing, then 4 pass. 73 pass / 0 fail across native-profile-startup and the probe suite, 106 pass / 0 fail across core-lab-boundary, native-main-owner-lifetime, native-profile-drain-server, service-probe-docker and server-auth. tsc --noEmit exit 0, privacy:scan exit 0. core-lab-boundary is in that list deliberately: this adds an import to native-profile-startup, and that test walks the runtime import graph to prove the core request path still cannot reach src/lab. --- src/codex/native-profile-startup.ts | 67 +++++++++++++++++++++++- src/server/index.ts | 5 ++ tests/native-profile-startup.test.ts | 76 ++++++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 1 deletion(-) diff --git a/src/codex/native-profile-startup.ts b/src/codex/native-profile-startup.ts index bf1349aafa..d53587e34a 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,59 @@ 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 cap is per fence, and it is + * reset by `release()`, so a restarted server starts fresh. + */ +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; +} + +/** 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 happens when a native request actually arrives rather than on a timer, so + * an idle proxy does no work, and it is capped so a permanently unaskable host cannot spin. + */ +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; + serviceOwnershipRefs.delete(reason); + serviceOwnershipReprobes.delete(reason); + 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,8 +379,12 @@ 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); + if (options?.reprobe && reason === "ownership-unknown") { + serviceOwnershipReprobes.set(reason, { probe: options.reprobe, attempts: 0 }); + } let released = false; return { homeId: null, @@ -337,6 +395,7 @@ 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 (remaining === 0) serviceOwnershipReprobes.delete(reason); }, }; } @@ -353,7 +412,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/tests/native-profile-startup.test.ts b/tests/native-profile-startup.test.ts index f8129f3d27..87d43bde39 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,76 @@ 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(); + } + }); +}); From 82fd8106b224c5c8dc3d9e84230eeb4eb75b1730 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 01:21:32 +0900 Subject: [PATCH 4/7] fix(probe,codex): close the fail-open and the refcount bug an audit found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial audit of the first cut returned fail on three real defects. All three are fixed here, and the first one is the reason this commit exists. FAIL-OPEN. The bus-down path consulted exactly one path, /.config/systemd/user/opencodex-proxy.service, and returned absent when it was missing. But systemd's user search path is not one directory: ~/.local/share/systemd/user and $XDG_CONFIG_HOME/systemd/user are also live. A foreign unit in either was invisible with the bus down, so inspectNativeCodexOwnership returned owned on a host a foreign service owns — the exact failure the disk check was supposed to prevent, reached through a narrower door. The bus-up path never had this hole, and that asymmetry is what made it a bug rather than a limitation. The offline check now walks every search directory systemd itself honors, including the XDG overrides. Two units found is unknown, not a guess. RESETTABLE RETRY BUDGET. The reprobe was keyed by reason but re-created with attempts: 0 on every fence, so a caller raising fences in a loop was handed a fresh allowance each time — measured 5 then 10. The budget now belongs to the reason and is only dropped when the last fence for it releases. A PROBE LIFTING FENCES IT NEVER SPOKE FOR. On success the reprobe deleted the whole refcount. With two servers fenced and only one carrying a hook, one probe unblocked both, and the hookless fence's own release() then decremented a counter that no longer existed. The hook now clears only its own fence and marks itself spent; the remaining fences keep traffic closed until each releases itself, which is what the refcount is for. Also from the same audit: the "foreign never retries" property was single-covered — the guard exists in two places and ablating either alone stayed green. A test now pins the outcome rather than one implementation, and was driven red by removing both. Two docstrings claimed things the code does not do: the cap is not reset by release(), and the reprobe is demand-driven rather than request-only, since the background token guardian also asks. Refs #2114, Refs #2108 Verification: every fix red-driven first — the fail-open reproduced as two failing assertions against a foreign unit in the other search dirs, the two fence defects as their own failures. 92 pass / 0 fail across native-profile-startup, the probe suite and core-lab-boundary. tsc --noEmit exit 0, privacy:scan exit 0. --- src/codex/native-profile-startup.ts | 33 ++++++++--- src/service-manager-probe.ts | 34 ++++++++--- tests/codex-service-manager-probe.test.ts | 52 +++++++++++++++++ tests/native-profile-startup.test.ts | 70 +++++++++++++++++++++++ 4 files changed, 175 insertions(+), 14 deletions(-) diff --git a/src/codex/native-profile-startup.ts b/src/codex/native-profile-startup.ts index d53587e34a..572f2debf7 100644 --- a/src/codex/native-profile-startup.ts +++ b/src/codex/native-profile-startup.ts @@ -315,8 +315,10 @@ 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 cap is per fence, and it is - * reset by `release()`, so a restarted server starts fresh. + * 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; @@ -326,6 +328,8 @@ const serviceOwnershipReprobes = new Map NativeCodexOwnership; attempts: number; + /** Set once the probe has already spent this hook's fence, so it cannot spend it twice. */ + cleared?: boolean; } /** Test-only: the retry budget is module state and would otherwise leak across tests. */ @@ -342,13 +346,18 @@ export function __resetNativeMainOwnershipRetries(): void { * 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 happens when a native request actually arrives rather than on a timer, so - * an idle proxy does no work, and it is capped so a permanently unaskable host cannot spin. + * 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.cleared) return false; if (entry.attempts >= NATIVE_MAIN_OWNERSHIP_RETRY_LIMIT) return false; entry.attempts += 1; let answer: NativeCodexOwnership; @@ -359,8 +368,15 @@ function reprobeServiceOwnership(reason: NativeMainServiceOwnershipBlockReason): return false; } if (answer !== "owned") return false; - serviceOwnershipRefs.delete(reason); - serviceOwnershipReprobes.delete(reason); + // Only the hook's OWN fence is cleared. Several servers can hold a fence for the same + // reason and only one of them may carry a hook, so lifting the shared refcount here + // would unblock fences this probe never spoke for — and their own release() would then + // decrement a counter that no longer exists. The remaining fences keep traffic closed + // until each releases itself, which is what the refcount is for. + entry.cleared = true; + const remaining = Math.max(0, (serviceOwnershipRefs.get(reason) ?? 0) - 1); + if (remaining === 0) serviceOwnershipRefs.delete(reason); + else serviceOwnershipRefs.set(reason, remaining); return true; } @@ -382,7 +398,10 @@ export function blockNativeMainStartupForUnownedServiceHome( options?: { reprobe?: () => NativeCodexOwnership }, ): NativeMainStartupLifecycle { serviceOwnershipRefs.set(reason, (serviceOwnershipRefs.get(reason) ?? 0) + 1); - if (options?.reprobe && reason === "ownership-unknown") { + // 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; the budget belongs to the reason, not to the individual fence. + if (options?.reprobe && reason === "ownership-unknown" && !serviceOwnershipReprobes.has(reason)) { serviceOwnershipReprobes.set(reason, { probe: options.reprobe, attempts: 0 }); } let released = false; diff --git a/src/service-manager-probe.ts b/src/service-manager-probe.ts index 7cd61dea9d..670e55e4f8 100644 --- a/src/service-manager-probe.ts +++ b/src/service-manager-probe.ts @@ -213,12 +213,32 @@ function busUnreachable(stderr: string): boolean { * A foreign home therefore still blocks, which is the whole reason this consults the disk * instead of widening the exit code. */ -function inspectSystemdOffline(definitionPath: string): ServiceManagerInstallation { - const presence = artifactPresence(definitionPath); - if (presence === "absent") return { kind: "absent" }; - if (presence === "unreadable") { - return unknown("the session bus is unreachable and the systemd unit could not be read"); - } +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"); @@ -337,7 +357,7 @@ function inspectSystemd(deps: Required>): Servic // 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(definitionPath); + if (busUnreachable(shown.stderr)) return inspectSystemdOffline(deps.home); return unknown(`systemctl show exited ${String(shown.status)}: ${shown.stderr.trim()}`); } diff --git a/tests/codex-service-manager-probe.test.ts b/tests/codex-service-manager-probe.test.ts index 615f1e68ff..7cb0486bce 100644 --- a/tests/codex-service-manager-probe.test.ts +++ b/tests/codex-service-manager-probe.test.ts @@ -1104,3 +1104,55 @@ describe("WinSW probe: a timed-out query with no assets (#2108)", () => { 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"); + }); +}); diff --git a/tests/native-profile-startup.test.ts b/tests/native-profile-startup.test.ts index 87d43bde39..79f4030c6f 100644 --- a/tests/native-profile-startup.test.ts +++ b/tests/native-profile-startup.test.ts @@ -699,3 +699,73 @@ describe("an unknown service-ownership fence is retryable (#2108)", () => { } }); }); + +/* + * 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(); + } + }); +}); From 9d49763406c80e9ebd5e7b42faf07cf92d93cd4f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 01:29:44 +0900 Subject: [PATCH 5/7] fix(codex): let the fence pay for itself, so the probe cannot pay twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the audit found that my round-1 repair traded one refcount bug for another. Both are gone now, and the shape is simpler than either attempt. DOUBLE-DECREMENT. The probe decremented the refcount directly, and the hooked fence's own release() then decremented again for the same fence. The count went short by one, so releasing the hooked fence lifted a fence another holder still owned. The Math.max(0, ...) floor is what hid it: the count never went negative, it just silently under-counted. The first fix had exactly the same failure mode one step later in the lifecycle, which is the tell that the ownership was modelled in the wrong place. The hook now delegates to the fence's own idempotent release() instead of touching the refcount. One fence, one payment, whoever triggers it. WEDGE. A spent hook stayed in the map, so the "do not reset an existing budget" guard refused to install a hook for any LATER fence. A server started after an earlier probe got no reprobe at all and was back to needing ocx restart, which is the #2108 symptom returning by another route. The entry is now dropped by its own owner's release, so the next fence installs its own hook while a live holder still cannot be handed a fresh allowance. Also pinned: the multi-unit conflict branch in the offline systemd path. Two units on disk with the bus down is genuinely ambiguous — neither can be confirmed as the live definition — so it refuses rather than picking one. That was a fail-closed decision with nothing asserting it; ablating it left the suite green, and now it does not. Refs #2114, Refs #2108 Verification: both defects red-driven first — the double-decrement as a fence that should still block reading unblocked, the wedge as a later fence never being asked. 95 pass / 0 fail across native-profile-startup, the probe suite and core-lab-boundary. The new multi-unit guard was ablated to confirm it is not vacuous. tsc --noEmit exit 0, privacy:scan exit 0. --- src/codex/native-profile-startup.ts | 50 ++++++++++++--------- tests/codex-service-manager-probe.test.ts | 15 +++++++ tests/native-profile-startup.test.ts | 55 +++++++++++++++++++++++ 3 files changed, 100 insertions(+), 20 deletions(-) diff --git a/src/codex/native-profile-startup.ts b/src/codex/native-profile-startup.ts index 572f2debf7..e873579197 100644 --- a/src/codex/native-profile-startup.ts +++ b/src/codex/native-profile-startup.ts @@ -328,8 +328,10 @@ const serviceOwnershipReprobes = new Map NativeCodexOwnership; attempts: number; - /** Set once the probe has already spent this hook's fence, so it cannot spend it twice. */ - cleared?: boolean; + /** 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. */ @@ -357,7 +359,6 @@ function reprobeServiceOwnership(reason: NativeMainServiceOwnershipBlockReason): if (reason !== "ownership-unknown") return false; const entry = serviceOwnershipReprobes.get(reason); if (!entry) return false; - if (entry.cleared) return false; if (entry.attempts >= NATIVE_MAIN_OWNERSHIP_RETRY_LIMIT) return false; entry.attempts += 1; let answer: NativeCodexOwnership; @@ -368,15 +369,14 @@ function reprobeServiceOwnership(reason: NativeMainServiceOwnershipBlockReason): return false; } if (answer !== "owned") return false; - // Only the hook's OWN fence is cleared. Several servers can hold a fence for the same - // reason and only one of them may carry a hook, so lifting the shared refcount here - // would unblock fences this probe never spoke for — and their own release() would then - // decrement a counter that no longer exists. The remaining fences keep traffic closed - // until each releases itself, which is what the refcount is for. - entry.cleared = true; - const remaining = Math.max(0, (serviceOwnershipRefs.get(reason) ?? 0) - 1); - if (remaining === 0) serviceOwnershipRefs.delete(reason); - else serviceOwnershipRefs.set(reason, remaining); + // 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; } @@ -398,14 +398,8 @@ export function blockNativeMainStartupForUnownedServiceHome( options?: { reprobe?: () => NativeCodexOwnership }, ): NativeMainStartupLifecycle { serviceOwnershipRefs.set(reason, (serviceOwnershipRefs.get(reason) ?? 0) + 1); - // 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; the budget belongs to the reason, not to the individual fence. - if (options?.reprobe && reason === "ownership-unknown" && !serviceOwnershipReprobes.has(reason)) { - serviceOwnershipReprobes.set(reason, { probe: options.reprobe, attempts: 0 }); - } let released = false; - return { + const lifecycle: NativeMainStartupLifecycle = { homeId: null, settled: Promise.resolve(serviceOwnershipSnapshot(reason)), async release() { @@ -414,9 +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 (remaining === 0) serviceOwnershipReprobes.delete(reason); + 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 { diff --git a/tests/codex-service-manager-probe.test.ts b/tests/codex-service-manager-probe.test.ts index 7cb0486bce..0082f19723 100644 --- a/tests/codex-service-manager-probe.test.ts +++ b/tests/codex-service-manager-probe.test.ts @@ -1155,4 +1155,19 @@ describe("bus-down absence must mean absence everywhere systemd looks (#2114)", 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 79f4030c6f..95aca6b688 100644 --- a/tests/native-profile-startup.test.ts +++ b/tests/native-profile-startup.test.ts @@ -769,3 +769,58 @@ describe("a foreign fence is never reopened by a probe (#2108)", () => { } }); }); + +/* + * 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 multi-unit conflict branch was a fail-closed decision with nothing pinning it. + */ From a674b90a26faa5686a03b34e0fb1d8f1f169e7e9 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 01:34:55 +0900 Subject: [PATCH 6/7] test(codex): pin the owner-identity guard on the reprobe hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of the audit passed, with one gap: replacing the owner-identity check with `if (true)` left all three suites green. The guard is correct and nothing would have caught it being broken. That matters more than an ordinary coverage gap, because it is the same wedge this fix has now grown three times by three different routes: first a spent hook staying in the map, then the entry being dropped early by a double-decrement, and now a non-owner fence's release destroying the owner's hook. Each time the symptom is identical — a fence with no way to re-ask, which is #2108 returning. The test drives that third route directly and was ablated to confirm it fails without the guard. Refs #2108 --- tests/native-profile-startup.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/native-profile-startup.test.ts b/tests/native-profile-startup.test.ts index 95aca6b688..fe59dc2224 100644 --- a/tests/native-profile-startup.test.ts +++ b/tests/native-profile-startup.test.ts @@ -819,6 +819,27 @@ describe("a spent reprobe leaves the refcount coherent (#2108)", () => { 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(); + } + }); }); /* From 50386c14871bd0511392573f7997454de98f9946 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 01:56:19 +0900 Subject: [PATCH 7/7] docs(devlog): record the fence fixes and the ownership model that was wrong three times --- .../170_2114_2108_fences.md | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/170_2114_2108_fences.md 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. +