Skip to content

Commit f85a722

Browse files
authored
Fix bridge test races and improve Linux child tracking (#43)
* Observe expected workspace lock rejections before releasing test barriers Assisted-by: codex-cli reason:post-merge-ci-rejection-race Docs-Impact: Test synchronization only; runtime behavior is unchanged. * Report early delegation failures in cross-process lock test Assisted-by: codex-cli reason:ci-startup-timeout-diagnosis * Make provenance deadline coverage independent of preflight duration Register a deterministic deadline check after the trace handle opens, while keeping the end-to-end absolute-budget and no-worker assertions. Export the existing setup helper without changing its runtime behavior. Assisted-by: codex-cli reason:windows-ci-phase-timing * Bind Linux child identities before completing the parent task scan Capture visible child identities while the original parent can still be revalidated. Preserve fail-closed behavior for unknown children and recycled parent PIDs, avoiding evidence loss when short-lived Git helpers exit during later task reads. Assisted-by: codex-cli reason:linux-containment-race
1 parent f585fb5 commit f85a722

6 files changed

Lines changed: 159 additions & 16 deletions

File tree

‎plugins/Hylouis233/cli-agent-bridge/README.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,3 +238,12 @@ capture truncation, and Codex prompt delimiters on Windows and POSIX.
238238
## License
239239

240240
MIT. See LICENSE. Upstream credits: see NOTICE.
241+
242+
### Process-tracker regression coverage
243+
244+
The internal Linux tracker tests bind each newly visible child to its start identity while its
245+
original parent can still be revalidated, before enumerating the parent's remaining tasks.
246+
This preserves evidence for short-lived Git helpers without accepting unknown descendants or
247+
children of a recycled parent PID. Pending children without verifiable identities still fail
248+
closed. This test-path improvement does not enable Linux production delegation; the supported
249+
kernel containment boundary remains Windows Job Objects.

‎plugins/Hylouis233/cli-agent-bridge/process-tree.mjs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -587,6 +587,26 @@ async function linuxTrackedProcessSnapshot(
587587
// Preserve evidence from every torn attempt. A later complete task
588588
// pass cannot make a previously observed child safe to forget.
589589
pendingChildren.add(childPid);
590+
// Bind a newly observed child before reading more task files. A short
591+
// Git helper and its parent can both disappear during those reads;
592+
// deferring identity capture would lose evidence that is available now.
593+
if (!treeState.knownStarts.has(childPid)) {
594+
const child = await readLinuxStat(childPid, procRoot, fsOps);
595+
if (child === null) return null;
596+
if (child && child.parentPid === pid &&
597+
/^\d+$/u.test(child.startIdentity) && /^\d+$/u.test(item.startIdentity) &&
598+
BigInt(child.startIdentity) >= BigInt(item.startIdentity)) {
599+
const anchor = await readLinuxStat(pid, procRoot, fsOps);
600+
if (anchor === null) return null;
601+
if (anchor?.startIdentity === item.startIdentity) {
602+
treeState.knownPids.add(childPid);
603+
treeState.knownStarts.set(childPid, child.startIdentity);
604+
// Ownership is now bound to this immutable identity. Later
605+
// reparenting must not turn it back into an unverified candidate.
606+
enqueue(childPid);
607+
}
608+
}
609+
}
590610
}
591611
}
592612
try {

‎plugins/Hylouis233/cli-agent-bridge/server.mjs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1384,7 +1384,7 @@ export function backendGitProvenanceEnvironment(tracePath, baseEnvironment = pro
13841384
return env;
13851385
}
13861386

1387-
async function createBackendGitProvenance(baseEnvironment = process.env, options = {}) {
1387+
export async function createBackendGitProvenance(baseEnvironment = process.env, options = {}) {
13881388
let root = "";
13891389
let tracePath = "";
13901390
let handle = null;

‎plugins/Hylouis233/cli-agent-bridge/tests/process-tree.test.mjs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,3 +1058,61 @@ test("BSD process snapshots reject truncation and malformed records", async () =
10581058
runUtility: async () => ({ exitCode: 0, stdout: valid + "malformed\n" }),
10591059
}), null);
10601060
});
1061+
1062+
test("Linux binds a visible child before scanning its parent's remaining tasks", async () => {
1063+
let alive = true;
1064+
const fsOps = {
1065+
readdir: async target => target === "/fixture-proc/100/task"
1066+
? [100, 101].map(pid => ({ name: String(pid), isDirectory: () => true })) : [],
1067+
readFile: async target => {
1068+
if (target.endsWith("/100/stat") && alive) {
1069+
return procStatLine(100, { parent: 1, group: 100, startIdentity: 10 });
1070+
}
1071+
if (target.endsWith("/200/stat") && alive) {
1072+
return procStatLine(200, { parent: 100, group: 100, startIdentity: 20 });
1073+
}
1074+
if (target.endsWith("/100/task/100/children")) return "200\n";
1075+
if (target.endsWith("/100/task/101/children")) {
1076+
alive = false; // Both short-lived processes exit during a later task read.
1077+
return "\n";
1078+
}
1079+
throw missingProcessError();
1080+
},
1081+
};
1082+
const state = {
1083+
knownPids: new Set([100]), knownStarts: new Map([[100, "10"]]),
1084+
runMarker: "fixture-run", markerObservationGraceMs: 0,
1085+
};
1086+
await refreshProcessTree({ pid: 100 }, state, { platform: "linux", procRoot: "/fixture-proc", fsOps });
1087+
assert.equal(state.knownStarts.get(200), "20", "retain identity while the original parent is still observable");
1088+
assert.equal(state.processIdentityUncertain, undefined);
1089+
assert.equal(await isProcessTreeAlive({ pid: 100 }, state, {
1090+
platform: "linux", procRoot: "/fixture-proc", fsOps,
1091+
probeProcessGroup: () => { throw missingProcessError("ESRCH"); },
1092+
}), false);
1093+
});
1094+
1095+
test("Linux early child binding rejects a parent whose PID identity changed", async () => {
1096+
let reads = 0;
1097+
const fsOps = {
1098+
readdir: async target => target === "/fixture-proc/100/task"
1099+
? [{ name: "100", isDirectory: () => true }] : [],
1100+
readFile: async target => {
1101+
if (target.endsWith("/100/stat")) {
1102+
return procStatLine(100, { parent: 1, group: 100, startIdentity: ++reads === 1 ? 10 : 30 });
1103+
}
1104+
if (target.endsWith("/200/stat")) {
1105+
return procStatLine(200, { parent: 100, group: 100, startIdentity: 40 });
1106+
}
1107+
if (target.endsWith("/100/task/100/children")) return "200\n";
1108+
throw missingProcessError();
1109+
},
1110+
};
1111+
const state = {
1112+
knownPids: new Set([100]), knownStarts: new Map([[100, "10"]]), runMarker: "fixture-run",
1113+
};
1114+
await refreshProcessTree({ pid: 100 }, state, { platform: "linux", procRoot: "/fixture-proc", fsOps });
1115+
assert.equal(state.knownPids.has(200), false);
1116+
assert.equal(state.knownStarts.get(200), undefined);
1117+
assert.equal(state.processIdentityUncertain, true);
1118+
});

‎plugins/Hylouis233/cli-agent-bridge/tests/server.test.mjs‎

Lines changed: 59 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
import {
2323
backendConfigurationControlEnvironment, backendConfigurationReaderEnvironment,
2424
backendEntryFromProbe, backendGitProvenanceEnvironment,
25-
closestExistingBase, committedDelta,
25+
closestExistingBase, committedDelta, createBackendGitProvenance,
2626
gitCommonDirectory, gitWorktreeRoot, loadBackends, markWorkspaceQuarantined,
2727
populateCommitishCache, readBackendGitProvenance, readBoundedRegularFile,
2828
readRepositoryLockActivity, readWorkspaceQuarantine, repositoryLockKey,
@@ -1778,7 +1778,7 @@ test("cancellation requires a fresh workspace_status to reveal earlier edits", a
17781778
assert.ok(statusOut.git.changedFiles.includes(changedFile), JSON.stringify(statusOut.git));
17791779
});
17801780

1781-
test("provenance setup obeys cancellation and deadline before worker launch", async (context) => {
1781+
test("pre-launch setup obeys cancellation and the overall deadline", async (context) => {
17821782
const setupRoot = await mkdtemp(path.join(os.tmpdir(), "cli-agent-trace-setup-test-"));
17831783
const startedFile = path.join(setupRoot, "started.txt");
17841784
context.after(() => rm(setupRoot, { recursive: true, force: true }));
@@ -1804,14 +1804,61 @@ test("provenance setup obeys cancellation and deadline before worker launch", as
18041804
name: "expired-trace-setup", eventFile,
18051805
}, { timeoutMs: 5_000 }), 51_006);
18061806
assert.equal(timed.result.structuredContent.timedOut, true, JSON.stringify(timed));
1807-
assert.match(timed.result.structuredContent.error, /preparing Git provenance/iu);
1807+
// The absolute request budget includes preflight. A slow runner can spend
1808+
// it in an earlier stage; the phase-specific deadline is exercised below.
1809+
assert.match(timed.result.structuredContent.error, /worker never started/iu);
18081810
assert.deepEqual(await events(eventFile), [], "trace setup interruption must precede worker launch");
18091811
for (const traceRoot of (await readFile(startedFile, "utf8")).trim().split(/\r?\n/u)) {
18101812
await assert.rejects(access(traceRoot), /ENOENT/u,
18111813
"interrupted provenance setup must close its handle and remove its private root");
18121814
}
18131815
});
18141816

1817+
test("deadline during open provenance setup removes its private trace", async (context) => {
1818+
const setupRoot = await mkdtemp(path.join(os.tmpdir(), "cli-agent-trace-deadline-"));
1819+
const startedFile = path.join(setupRoot, "started.txt");
1820+
const settings = {
1821+
NODE_ENV: "test",
1822+
CLI_AGENT_BRIDGE_TEST_TRACE_CREATION_DELAY_MS: "60000",
1823+
CLI_AGENT_BRIDGE_TEST_TRACE_CREATION_STARTED_FILE: startedFile,
1824+
};
1825+
const saved = Object.fromEntries(Object.keys(settings).map(key => [key, process.env[key]]));
1826+
Object.assign(process.env, settings);
1827+
context.after(async () => {
1828+
context.mock.timers.reset();
1829+
for (const [key, value] of Object.entries(saved)) {
1830+
if (value === undefined) delete process.env[key]; else process.env[key] = value;
1831+
}
1832+
await rm(setupRoot, { recursive: true, force: true });
1833+
});
1834+
// Keep real filesystem I/O; advance only the deadline after the trace handle
1835+
// is open, so machine-dependent Git startup cannot select the wrong phase.
1836+
context.mock.timers.enable({ apis: ["Date", "setTimeout"], now: Date.now() });
1837+
const rejected = assert.rejects(
1838+
createBackendGitProvenance({}, { deadline: Date.now() + 5000 }),
1839+
{ message: "delegation deadline exceeded" },
1840+
);
1841+
const waitForIO = async predicate => {
1842+
const deadline = performance.now() + 5000;
1843+
while (!await predicate()) {
1844+
assert.ok(performance.now() < deadline, "filesystem barrier was not reached");
1845+
await new Promise(resolve => setImmediate(resolve));
1846+
}
1847+
};
1848+
let traceRoot;
1849+
await waitForIO(async () => {
1850+
try { traceRoot = (await readFile(startedFile, "utf8")).trim(); return !!traceRoot; }
1851+
catch (error) { if (error.code === "ENOENT") return false; throw error; }
1852+
});
1853+
await access(path.join(traceRoot, "git.trace"));
1854+
context.mock.timers.tick(5000);
1855+
await rejected;
1856+
await waitForIO(async () => {
1857+
try { await access(traceRoot); return false; }
1858+
catch (error) { if (error.code === "ENOENT") return true; throw error; }
1859+
});
1860+
});
1861+
18151862
test("shutdown waits for a late provenance-setup cleanup", async (context) => {
18161863
const setupRoot = await mkdtemp(path.join(os.tmpdir(), "cli-agent-trace-shutdown-test-"));
18171864
const startedFile = path.join(setupRoot, "started.txt");
@@ -2467,12 +2514,17 @@ test("canonical worktree locking serializes independent server processes", async
24672514
try {
24682515
await secondClient.initialize();
24692516
const eventFile = path.join(tempRoot, "cross-process-events.jsonl");
2517+
let firstResult;
24702518
const first = client.request("tools/call", taskArguments(workspace, {
24712519
name: "first-server", eventFile, delayMs: 800, writeFile: "first-server.txt",
2472-
}));
2473-
await waitFor(async () => (await events(eventFile)).some(
2474-
(item) => item.name === "first-server" && item.event === "start",
2475-
));
2520+
})).then((response) => { firstResult = response; return response; });
2521+
await waitFor(async () => {
2522+
if (firstResult) assert.equal(firstResult.result?.structuredContent?.ok, true,
2523+
"first server failed before its start event: " + JSON.stringify(firstResult));
2524+
return (await events(eventFile)).some(
2525+
(item) => item.name === "first-server" && item.event === "start",
2526+
);
2527+
});
24762528
const second = secondClient.request("tools/call", taskArguments(workspace, {
24772529
name: "second-server", eventFile, delayMs: 10, writeFile: "second-server.txt",
24782530
}, { allowDirty: true }));

‎plugins/Hylouis233/cli-agent-bridge/tests/workspace-lock.test.mjs‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -558,16 +558,18 @@ test("post-CAS cancellation reconciles the committed owner before returning", as
558558
} else process.env.CLI_AGENT_BRIDGE_TEST_WORKSPACE_CAS_RESULT_RELEASE_FILE = saved.release;
559559
});
560560
const cancel = cancellationToken();
561-
const acquisition = tryAcquireGitWorkspaceLock({
561+
// Observe the expected rejection before releasing the CAS barrier: the
562+
// operation can reject before the asynchronous release-file write resolves.
563+
const acquisitionRejected = assert.rejects(tryAcquireGitWorkspaceLock({
562564
cwd: repo, key, cancel, heartbeatMs: 60_000,
563-
});
565+
}), WorkspaceLockCancelledError);
564566
await waitForFile(startedFile);
565567
assert.match(await git(repo, ["rev-parse", ref]), /^[0-9a-f]{40,64}$/u,
566568
"the real acquisition CAS must commit before cancellation");
567569
await writeFile(blocker, "intentional compensating-delete failure\n");
568570
cancel.cancel();
569571
await writeFile(releaseFile, "release\n");
570-
await assert.rejects(acquisition, WorkspaceLockCancelledError);
572+
await acquisitionRejected;
571573
assert.match(await git(repo, ["rev-parse", ref]), /^[0-9a-f]{40,64}$/u,
572574
"a failed exact delete must leave a recoverable owner ref");
573575
await rm(blocker, { force: true });
@@ -604,14 +606,14 @@ test("post-CAS deadline reconciliation deletes the exact committed owner", async
604606
});
605607
const passive = { cancelled: false, promise: new Promise(() => {}), subscribe: () => () => {} };
606608
const deadline = Date.now() + 3_000;
607-
const acquisition = tryAcquireGitWorkspaceLock({
609+
const acquisitionRejected = assert.rejects(tryAcquireGitWorkspaceLock({
608610
cwd: repo, key, cancel: passive, deadline, heartbeatMs: 60_000,
609-
});
611+
}), WorkspaceLockDeadlineError);
610612
await waitForFile(startedFile);
611613
assert.match(await git(repo, ["rev-parse", ref]), /^[0-9a-f]{40,64}$/u);
612614
await new Promise((resolve) => setTimeout(resolve, Math.max(0, deadline - Date.now() + 20)));
613615
await writeFile(releaseFile, "release\n");
614-
await assert.rejects(acquisition, WorkspaceLockDeadlineError);
616+
await acquisitionRejected;
615617
await assert.rejects(
616618
execFileAsync("git", ["rev-parse", "--verify", ref], { cwd: repo }), /Command failed/u,
617619
);
@@ -654,13 +656,15 @@ test("interrupted state CAS cleanup covers both exact commit outcomes", async (c
654656
}
655657
});
656658
const cancel = cancellationToken();
657-
const update = result.lease.markWorkerStarting({ cancel });
659+
const updateRejected = assert.rejects(
660+
result.lease.markWorkerStarting({ cancel }), WorkspaceLockCancelledError,
661+
);
658662
await waitForFile(startedFile);
659663
const candidateOid = await git(repo, ["rev-parse", ref]);
660664
assert.notEqual(candidateOid, previousOid, "the real state CAS must commit its candidate OID");
661665
cancel.cancel();
662666
await writeFile(releaseFile, "release\n");
663-
await assert.rejects(update, WorkspaceLockCancelledError);
667+
await updateRejected;
664668
delete process.env.CLI_AGENT_BRIDGE_TEST_WORKSPACE_CAS_RESULT_STARTED_FILE;
665669
delete process.env.CLI_AGENT_BRIDGE_TEST_WORKSPACE_CAS_RESULT_RELEASE_FILE;
666670
process.env.CLI_AGENT_BRIDGE_TEST_WORKSPACE_RELEASE_READBACK_FAILURES = "1";

0 commit comments

Comments
 (0)