Skip to content

Commit 16ff5d8

Browse files
committed
fix(host-kit): keep a null CIM field printing empty and the birth stamp culture-pinned
The row body claimed a culture-invariant stamp while ToString took no culture argument, so a host calendar could reformat the same process birth between the daemon's publish and a client's verify. And Formatting a null CreationDate (the System Idle Process row) raises mid-pipeline: exit 1 blanks the whole snapshot rather than that row, turning one odd process into unknown evidence for every pid. The row rule is now what it claims: null fields print empty, never raise; birth is formatted with the invariant culture. Reaper coverage gains the zombie identity it never seeded, and owner-liveness guards collapse into one place so the single snapshot path stops paying two kill(pid, 0) per poll.
1 parent bad805c commit 16ff5d8

5 files changed

Lines changed: 106 additions & 10 deletions

File tree

‎packages/host-kit/src/internal/host-process-windows.test.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,30 @@ test('a failed or thrown CIM query is unknown evidence, as a failed ps is', () =
103103
});
104104
});
105105

106+
test('the CIM row body never lets one null field raise, and pins the invariant culture', () => {
107+
windowsPlatform(() => {
108+
// The System Idle Process has no CreationDate. Formatting that row must
109+
// print an empty stamp, not raise: a terminating error mid-pipeline exits
110+
// 1 and blanks the ENTIRE snapshot, turning one odd process into unknown
111+
// evidence for every pid.
112+
cimAnswers(`0|0||System Idle Process\r\n4343|4321||cmd.exe\r\n${DAEMON_ROW}\r\n`);
113+
const observations = readHostProcessIdentityObservations([DAEMON_PID, 4343]);
114+
assert.deepEqual(observations.get(DAEMON_PID), { state: 'R', startTime: DAEMON_START });
115+
assert.equal(
116+
observations.get(4343),
117+
undefined,
118+
'a null-birth row is unknown, not a false birth',
119+
);
120+
});
121+
const command = String(mockRunCmdSync.mock.calls[0]![1][3]);
122+
assert.match(command, /if \(\$_\.CreationDate\)/, 'null birth prints empty instead of raising');
123+
assert.match(
124+
command,
125+
/CultureInfo\]::InvariantCulture/,
126+
'custom format follows the host calendar',
127+
);
128+
});
129+
106130
test('the Windows batch snapshot answers one observation query for every selected pid', () => {
107131
windowsPlatform(() => {
108132
cimAnswers(
@@ -161,7 +185,7 @@ test('the Windows process list parses CIM rows through the injected command runn
161185
runCommand: async (cmd, args, options) => {
162186
calls.push({ cmd, args, timeoutMs: options.timeoutMs });
163187
return {
164-
stdout: `\uFEFF${DAEMON_ROW}\r\n0|0||\r\nnot a row\r\n`,
188+
stdout: `\uFEFF0|0||System Idle Process\r\n${DAEMON_ROW}\r\n0|0||\r\nnot a row\r\n`,
165189
stderr: '',
166190
exitCode: 0,
167191
};

‎packages/host-kit/src/internal/host-process.ts‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -258,14 +258,22 @@ function isWindowsHostPlatform(): boolean {
258258
return hostPlatform() === 'win32';
259259
}
260260

261-
// Rows are `pid|ppid|creation|command` with a culture-invariant UTC creation
262-
// stamp, so the value recorded in daemon.json compares equal on every read of
263-
// the same process lifetime. Newlines inside a command line are flattened so
261+
// Rows are `pid|ppid|creation|command`. The creation stamp is formatted with
262+
// the invariant culture, so the value recorded in daemon.json compares equal
263+
// on every read of the same process lifetime regardless of host locale. The
264+
// row rule is that a null field prints empty and never raises: the CIM table
265+
// carries rows without a CreationDate (the System Idle Process), and a
266+
// terminating error mid-pipeline would exit 1 and blank the whole snapshot
267+
// instead of that one row. Newlines inside a command line are flattened so
264268
// one process is one row; the command may still contain `|`, which only ever
265269
// lands in the final capture group.
266270
const WINDOWS_PROCESS_ROW_BODY =
267-
" | ForEach-Object { '{0}|{1}|{2}|{3}' -f $_.ProcessId, $_.ParentProcessId," +
268-
" $_.CreationDate.ToUniversalTime().ToString('yyyyMMddHHmmssfffffff')," +
271+
' | ForEach-Object {' +
272+
' $birth = if ($_.CreationDate) {' +
273+
" $_.CreationDate.ToUniversalTime().ToString('yyyyMMddHHmmssfffffff'," +
274+
' [System.Globalization.CultureInfo]::InvariantCulture)' +
275+
" } else { '' };" +
276+
" '{0}|{1}|{2}|{3}' -f $_.ProcessId, $_.ParentProcessId, $birth," +
269277
String.raw` ($_.CommandLine -replace '[\r\n]+', ' ') }`;
270278

271279
function windowsProcessQueryArgs(pids: readonly number[]): string[] {

‎packages/host-kit/src/internal/owned-process-record.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,42 @@ test('startup interrupts an exact simctl recorder before clearing its record', a
166166
}
167167
});
168168

169+
test('startup treats a zombie identity as missing without signaling it', async () => {
170+
// The reaper asks one identity read the zombie question, and a zombie has
171+
// already terminated: it is cleaned as missing, never signaled. Before this
172+
// case the fixture could seed zombiePids but never did, so every facts.zombie
173+
// branch ran uncovered.
174+
const stateDir = mkdtempForTestSync('agent-device-owned-process-zombie-reap-');
175+
const store = createOwnedProcessRecordStore({
176+
stateDir,
177+
sessionsDir: path.join(stateDir, 'sessions'),
178+
resolveSessionDir: (sessionId) => path.join(stateDir, 'sessions', sessionId),
179+
});
180+
const record = {
181+
pid: 303,
182+
startTime: 'start-303',
183+
command: 'command-303',
184+
purpose: 'managed-web-browser',
185+
};
186+
processState.alive.add(record.pid);
187+
processState.zombiePids.add(record.pid);
188+
processState.starts.set(record.pid, record.startTime);
189+
processState.commands.set(record.pid, record.command);
190+
const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true);
191+
192+
try {
193+
store.replace({ kind: 'daemon' }, [record]);
194+
195+
const summary = await reapOwnedProcessRecordsAtStartup(store);
196+
197+
assert.deepEqual(summary.missingPids, [record.pid]);
198+
assert.equal(killSpy.mock.calls.length, 0);
199+
assert.equal(store.read().length, 0);
200+
} finally {
201+
fs.rmSync(stateDir, { recursive: true, force: true });
202+
}
203+
});
204+
169205
test('startup retains a record when the pid identity was reused', async () => {
170206
const stateDir = mkdtempForTestSync('agent-device-owned-process-reuse-');
171207
const store = createOwnedProcessRecordStore({

‎packages/host-kit/src/internal/owner-identity-liveness.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,11 @@ test('a pid outside the native range is unknown without a liveness probe', () =>
8484
assert.equal(mockIsProcessZombie.mock.calls.length, 0);
8585
assert.equal(mockReadProcessStartTime.mock.calls.length, 0);
8686
});
87+
88+
test('the snapshot path probes liveness for the owner pid exactly once', () => {
89+
// The guards were once duplicated across the snapshot entry and the shared
90+
// judge, paying two kill(pid, 0) per poll on the very path whose single
91+
// snapshot exists to stop double probing.
92+
assert.equal(classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: 'start-a' } }), 'live');
93+
assert.equal(mockIsProcessAlive.mock.calls.length, 1);
94+
});

‎packages/host-kit/src/internal/owner-identity.ts‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,11 @@ export function classifyOwnerLiveness(params: {
7474
// read as two probes double the cost of every lock-ownership poll on hosts
7575
// where a probe starts a fresh process tool, and one snapshot also judges both
7676
// facts at the same instant instead of stitching two separate host answers.
77-
if (!isProcessPid(params.owner.pid)) return 'unknown';
78-
if (!isProcessAlive(params.owner.pid)) return 'owner-process-dead';
77+
const guarded = guardOwnerPid(params.owner);
78+
if (guarded) return guarded;
7979
const observation =
8080
readHostProcessIdentityObservations([params.owner.pid]).get(params.owner.pid) ?? null;
81-
return classifyOwnerLivenessFromObservation(params, observation);
81+
return classifyLiveOwnerFromObservation(params, observation);
8282
}
8383

8484
export function classifyOwnerLivenessFromObservation(
@@ -88,9 +88,29 @@ export function classifyOwnerLivenessFromObservation(
8888
},
8989
observation?: HostProcessIdentityObservation | null,
9090
): OwnerLiveness {
91-
const { owner, stateDir } = params;
91+
// The snapshot was taken before this call, so the guards run again here: a
92+
// claim whose owner died between snapshot and judgment must read dead.
93+
const guarded = guardOwnerPid(params.owner);
94+
if (guarded) return guarded;
95+
return classifyLiveOwnerFromObservation(params, observation);
96+
}
97+
98+
/** The verdicts provable from the pid alone; undefined means the caller may probe on. */
99+
function guardOwnerPid(owner: Pick<OwnerIdentity, 'pid' | 'startTime'>): OwnerLiveness | undefined {
92100
if (!isProcessPid(owner.pid)) return 'unknown';
93101
if (!isProcessAlive(owner.pid)) return 'owner-process-dead';
102+
return undefined;
103+
}
104+
105+
/** Judges an owner the caller has established as a live, valid pid; probes no liveness. */
106+
function classifyLiveOwnerFromObservation(
107+
params: {
108+
owner: Pick<OwnerIdentity, 'pid' | 'startTime'>;
109+
stateDir?: string;
110+
},
111+
observation?: HostProcessIdentityObservation | null,
112+
): OwnerLiveness {
113+
const { owner, stateDir } = params;
94114
if (observation !== undefined ? observation?.state.startsWith('Z') : isProcessZombie(owner.pid)) {
95115
return 'owner-process-dead';
96116
}

0 commit comments

Comments
 (0)