Skip to content

Commit cf54ef0

Browse files
fix(test): isolate containment fixtures from the Ubuntu runner pid
Why: - GitHub Node tests on 7c9a093 failed in broker-core, not at the 30-minute cap. Test 159 expected an active-containment-requester skip for fixture pid 2500. After public-surface the Ubuntu test-runner pid can be 2500, so containment records current-broker-process first. Changed: - Process controllers may inject currentPid. Fixtures use a synthetic pid so hardcoded process tables cannot match the runner. - The command-group requester test uses the live runner pid as the requester so that collision cannot pass vacuously. - Add coverage for the current-broker-process skip. Verification: - npm run agent:verify -- --profile spec-only --paths CHANGELOG.md --paths broker-core/containment.mjs --paths broker-core/test/broker-core.test.mjs --paths spec/build-and-test.md --session-dir task-sessions/ubuntu-ci-pid-collision - npm run agent:verify -- --profile implementation --paths CHANGELOG.md --paths broker-core/containment.mjs --paths broker-core/test/broker-core.test.mjs --paths spec/build-and-test.md --session-dir task-sessions/ubuntu-ci-pid-collision Affected: - CHANGELOG.md - broker-core/containment.mjs - broker-core/test/broker-core.test.mjs - spec/build-and-test.md Refs: - #19 - https://github.com/fiveonecode/simulator-broker/actions/runs/32717787296 Session: - task-sessions/ubuntu-ci-pid-collision
1 parent 7c9a093 commit cf54ef0

4 files changed

Lines changed: 89 additions & 11 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2727
adapter, client tests run with `--test-concurrency=1` on Ubuntu CI, and
2828
`service start` stops waiting when spawned `brokerd` exits even if a
2929
startup lock directory remains.
30+
- Broker-core process fixtures inject a synthetic controller pid so Ubuntu
31+
CI cannot classify a fixture requester as `current-broker-process` when
32+
the test-runner pid lands on a hardcoded fixture pid such as `2500`.
3033
- New project scaffolds require an iPhone for UI purposes without pinning an
3134
iOS version unless `--ios-version` is explicit.
3235
- Homebrew CLI and app metadata now require macOS 14 Sonoma, matching the app

broker-core/containment.mjs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -960,6 +960,14 @@ function defaultProcessController() {
960960
};
961961
}
962962

963+
function currentBrokerPid(processController) {
964+
const injected = processController?.currentPid;
965+
if (Number.isInteger(injected) && injected > 0) {
966+
return injected;
967+
}
968+
return process.pid;
969+
}
970+
963971
function killPid(processController, pid, signal) {
964972
if (typeof processController.killPid === "function") {
965973
return processController.killPid(pid, signal);
@@ -1004,6 +1012,7 @@ function terminateOwnedProcesses({
10041012
const actions = [];
10051013
const ownedByPid = new Map(discovery.owned.map((processRecord) => [processRecord.pid, processRecord]));
10061014
const ownerPid = discovery.owned.find((processRecord) => processRecord.reasons.includes("owner-pid"))?.pid ?? null;
1015+
const brokerPid = currentBrokerPid(processController);
10071016
const skippedRequesterPid = normalizePositiveInteger(requesterPid);
10081017
const requesterInCommandGroup = Boolean(
10091018
skippedRequesterPid
@@ -1039,7 +1048,7 @@ function terminateOwnedProcesses({
10391048
.map((processRecord) => processRecord.pid);
10401049

10411050
for (const pid of sortedPids) {
1042-
if (pid === process.pid) {
1051+
if (pid === brokerPid) {
10431052
actions.push({
10441053
ok: true,
10451054
pid,
@@ -1095,7 +1104,7 @@ function terminateOwnedProcesses({
10951104
}
10961105

10971106
for (const pid of sortedPids) {
1098-
if (pid === process.pid || pid === skippedRequesterPid || (pid === ownerPid && !killOwner)) {
1107+
if (pid === brokerPid || pid === skippedRequesterPid || (pid === ownerPid && !killOwner)) {
10991108
continue;
11001109
}
11011110
if (revalidatedOwnedPids && revalidatedOwnedPids.has(pid) === false) {

broker-core/test/broker-core.test.mjs

Lines changed: 71 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,17 @@ function readCapacityTransactions(resolvedPaths) {
236236
.map((entry) => readJson(path.join(resolvedPaths.capacityTransactionsDir, entry)));
237237
}
238238

239+
const FIXTURE_CONTROLLER_PID = 2_147_483_646;
240+
241+
function unusedFixturePid(preferred, taken = []) {
242+
const blocked = new Set([process.pid, ...taken]);
243+
let pid = preferred;
244+
while (blocked.has(pid) || pid <= 1) {
245+
pid += 1;
246+
}
247+
return pid;
248+
}
249+
239250
function makeProcessFixture(records) {
240251
const table = records.map((record) => ({
241252
alive: true,
@@ -252,6 +263,7 @@ function makeProcessFixture(records) {
252263
return {
253264
actions,
254265
controller: {
266+
currentPid: FIXTURE_CONTROLLER_PID,
255267
killPid(pid, signal) {
256268
actions.push({ pid, signal, target: "pid" });
257269
const record = table.find((candidate) => candidate.pid === pid);
@@ -6670,15 +6682,70 @@ test("forced-abort skips the active containment requester pid", () => {
66706682
});
66716683

66726684
test("forced-abort avoids command process-group termination when the requester is inside that group", () => {
6685+
const paths = makePaths();
6686+
writeBaseHostConfig(paths.hostConfigPath);
6687+
writeBaseProject(paths.projectFilePath);
6688+
const resolvedPaths = brokerPaths(paths);
6689+
const ownerPid = unusedFixturePid(1000);
6690+
const commandPid = unusedFixturePid(2000, [ownerPid]);
6691+
const requesterPid = process.pid;
6692+
const processes = makeProcessFixture([
6693+
{ command: "bash", pgid: ownerPid, pid: ownerPid, ppid: 1, rssBytes: 2 * 1024 * 1024 },
6694+
{ command: "xcodebuild test SIM-UI-1", pgid: commandPid, pid: commandPid, ppid: ownerPid, rssBytes: 40 * 1024 * 1024 },
6695+
{ command: "node client/bin/simbroker.mjs lease contain", pgid: commandPid, pid: requesterPid, ppid: commandPid, rssBytes: 5 * 1024 * 1024 },
6696+
]);
6697+
6698+
initBroker(resolvedPaths, runtimeOptions(paths, { processExists: () => true }));
6699+
const lease = acquireLeaseBroker(resolvedPaths, {
6700+
actorId: "agent-cleanup",
6701+
actorType: "agent",
6702+
ownerPgid: ownerPid,
6703+
ownerPid,
6704+
processExists: (pid) => pid === ownerPid,
6705+
processSampler: liveProcessSampler({ command: "with-broker-lease", pgid: ownerPid, pid: ownerPid }),
6706+
purposeId: "agent-ui-session",
6707+
simctlAdapter: paths.simctl.adapter,
6708+
}).lease;
6709+
registerLeaseProcessBroker(resolvedPaths, {
6710+
command: "xcodebuild test",
6711+
commandPgid: commandPid,
6712+
commandPid,
6713+
leaseId: lease.leaseId,
6714+
processExists: (pid) => pid === ownerPid,
6715+
processSampler: processes.sampler,
6716+
simctlAdapter: paths.simctl.adapter,
6717+
});
6718+
6719+
const result = containLeaseBroker(resolvedPaths, {
6720+
leaseId: lease.leaseId,
6721+
processController: processes.controller,
6722+
processExists: (pid) => pid === ownerPid,
6723+
processSampler: processes.sampler,
6724+
reason: "forced-abort",
6725+
requesterPid,
6726+
simctlAdapter: paths.simctl.adapter,
6727+
termWaitMs: 0,
6728+
});
6729+
6730+
assert.equal(result.contained, true);
6731+
assert.equal(processes.controller.currentPid, FIXTURE_CONTROLLER_PID);
6732+
assert.notEqual(processes.controller.currentPid, requesterPid);
6733+
assert.equal(processes.isAlive(requesterPid), true);
6734+
assert.equal(processes.isAlive(commandPid), false);
6735+
assert.equal(result.cleanupActions.some((action) => action.target === "process-group" && action.pgid === commandPid), false);
6736+
assert.ok(result.cleanupActions.some((action) => action.pid === requesterPid && action.skipped === true && action.why === "active-containment-requester"));
6737+
});
6738+
6739+
test("forced-abort records current-broker-process when the controller pid is lease-owned", () => {
66736740
const paths = makePaths();
66746741
writeBaseHostConfig(paths.hostConfigPath);
66756742
writeBaseProject(paths.projectFilePath);
66766743
const resolvedPaths = brokerPaths(paths);
66776744
const processes = makeProcessFixture([
66786745
{ command: "bash", pgid: 1000, pid: 1000, ppid: 1, rssBytes: 2 * 1024 * 1024 },
6679-
{ command: "xcodebuild test SIM-UI-1", pgid: 2000, pid: 2000, ppid: 1000, rssBytes: 40 * 1024 * 1024 },
6680-
{ command: "node client/bin/simbroker.mjs lease contain", pgid: 2000, pid: 2500, ppid: 2000, rssBytes: 5 * 1024 * 1024 },
6746+
{ command: "xcodebuild test SIM-UI-1", pgid: 1000, pid: 2000, ppid: 1000, rssBytes: 40 * 1024 * 1024 },
66816747
]);
6748+
processes.controller.currentPid = 2000;
66826749

66836750
initBroker(resolvedPaths, runtimeOptions(paths, { processExists: () => true }));
66846751
const lease = acquireLeaseBroker(resolvedPaths, {
@@ -6693,7 +6760,6 @@ test("forced-abort avoids command process-group termination when the requester i
66936760
}).lease;
66946761
registerLeaseProcessBroker(resolvedPaths, {
66956762
command: "xcodebuild test",
6696-
commandPgid: 2000,
66976763
commandPid: 2000,
66986764
leaseId: lease.leaseId,
66996765
processExists: (pid) => pid === 1000,
@@ -6707,16 +6773,13 @@ test("forced-abort avoids command process-group termination when the requester i
67076773
processExists: (pid) => pid === 1000,
67086774
processSampler: processes.sampler,
67096775
reason: "forced-abort",
6710-
requesterPid: 2500,
67116776
simctlAdapter: paths.simctl.adapter,
67126777
termWaitMs: 0,
67136778
});
67146779

67156780
assert.equal(result.contained, true);
6716-
assert.equal(processes.isAlive(2500), true);
6717-
assert.equal(processes.isAlive(2000), false);
6718-
assert.equal(result.cleanupActions.some((action) => action.target === "process-group" && action.pgid === 2000), false);
6719-
assert.ok(result.cleanupActions.some((action) => action.pid === 2500 && action.skipped === true && action.why === "active-containment-requester"));
6781+
assert.equal(processes.isAlive(2000), true);
6782+
assert.ok(result.cleanupActions.some((action) => action.pid === 2000 && action.skipped === true && action.why === "current-broker-process"));
67206783
});
67216784

67226785
test("forced-abort does not group-kill when the owner process group is unknown", () => {

spec/build-and-test.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,10 @@ A first extracted implementation slice now exists:
7676
checkout must not spawn one `git cat-file` per file. `simbroker service
7777
start` must fail as soon as the spawned `brokerd` exits without becoming
7878
ready; it must not keep polling for `serviceStartupTimeoutMs` just because
79-
the startup lock directory still exists.
79+
the startup lock directory still exists. Broker-core process-table
80+
fixtures inject `processController.currentPid` so hardcoded fixture
81+
PIDs cannot match the GitHub Actions test-runner pid. Containment still
82+
skips the live `process.pid` when `currentPid` is omitted.
8083
- tagged versions such as `v0.1.0-alpha.2` attach the CLI tarball, the
8184
packable `simbroker-<version>.tgz`, and the notarized
8285
`Simulator-Broker-<version>.zip` to a GitHub Release. The CLI and npm

0 commit comments

Comments
 (0)