Skip to content

Commit b1817fe

Browse files
os-litantclaude
andauthored
test(cli): hoist the serve-probe child-lifecycle attribution into one helper, and pin it against a driven dead child (#15902)
* test(cli): hoist the serve-probe child-lifecycle attribution and pin it Three more e2e files here fetched a spawned `os serve` with no read of the child's fate on the failing path: `child.on('exit')` fed the READINESS promise only, so a death after readiness reached vitest as a bare `TypeError: fetch failed` with no exit code, no stdout and no stderr. The idiom that repaired the fourth file is hoisted into `test/helpers/serve-process.ts` as `probeThroughChild()` and all four files now consume it, so the family has one definition rather than four copies — and one pin, `serve-probe-child-attribution.test.ts`, which drives a real child that accepts the whole request and then FINs. No skip, no todo, no quarantine, no timeout bump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * test(cli): spell the probe child as ChildProcessByStdio, not WithoutNullStreams The pin spawns with `stdio: ['ignore', 'pipe', 'pipe']`, so the handle has no `stdin` and the cast was a type error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * test(cli): the attribution pin's driver declares its child env `check:cli-test-child-env` requires every child spawned from packages/cli/test to declare its environment. The driver reads no variable of its own, so `childEnv()` — the environment minus the vitest worker family — is the honest declaration rather than an omitted `env` key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent bca21f7 commit b1817fe

6 files changed

Lines changed: 834 additions & 237 deletions

packages/cli/test/helpers/serve-process.ts

Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -927,3 +927,240 @@ export function runServe(
927927
child.on('exit', () => finish());
928928
});
929929
}
930+
931+
// ─────────────────────────────────────────────────────────────────────────
932+
// CHILD-LIFECYCLE ATTRIBUTION FOR AN HTTP PROBE AGAINST A SPAWNED `os serve`
933+
//
934+
// ## The defect this section removes
935+
//
936+
// A file here boots `os serve`, waits for its banner, and then `fetch`es it.
937+
// Every one of them fed `child.on('exit')` into the READINESS promise ONLY, so
938+
// once readiness had settled a later death was invisible: the rejection reached
939+
// vitest as a bare `TypeError: fetch failed` with no exit code, no stdout and no
940+
// stderr. One such occurrence was recorded in a merge-group run and could not be
941+
// attributed at all — the probe had discarded the only two artefacts that decide
942+
// between "the child died" and "a live child dropped a connection".
943+
//
944+
// ## What the recorded signature actually said
945+
//
946+
// Three shapes measured side by side on this container, with the same request
947+
// the origin probes send:
948+
//
949+
// nothing listening on the port ECONNREFUSED, and NO socket object
950+
// (bytesWritten/bytesRead both undefined)
951+
// listener accepts, then FINs UND_ERR_SOCKET "other side closed",
952+
// bytesWritten 321, bytesRead 0
953+
// server torn down mid-request UND_ERR_SOCKET "other side closed",
954+
// bytesWritten 345, bytesRead 0
955+
//
956+
// `bytesWritten: 349, bytesRead: 0` — what the merge-group run recorded — says
957+
// the connection was ESTABLISHED, the whole request was delivered, and the peer
958+
// closed it without answering. Only the second and third match; the FIRST is
959+
// what a genuine readiness race looks like. So the child was serving when the
960+
// request arrived, and something ended the connection after it. ⛔ The repair is
961+
// therefore not a longer wait and ⛔ not a bigger timeout.
962+
//
963+
// It matters that a silent death is reachable at all: there is no
964+
// `process.on('uncaughtException')` and no `'unhandledRejection'` handler
965+
// anywhere in this repo's product source, so a late async throw inside the child
966+
// — or an OOM kill on a six-shard CI box — takes the process down with no HTTP
967+
// answer and exactly this client-side signature.
968+
//
969+
// ## The bound, and what it can and cannot hide
970+
//
971+
// ⛔ The retry is not the first move and cannot be reached before the child has
972+
// been asked whether it is alive. On a transport failure the probe waits up to
973+
// `settleMs` for the child's `exit` event — the FIN reaches the client on the
974+
// network's schedule and `exit` arrives on the event loop's, so reading
975+
// `exitCode` at the instant `fetch` rejects reports a dead child as ALIVE — and
976+
// then:
977+
//
978+
// • child EXITED ⇒ ⛔ never retried. Fails at once, naming the exit code and
979+
// signal and quoting the child's whole stdout and stderr.
980+
// • failure is NOT `UND_ERR_SOCKET` ⇒ ⛔ never retried. Rethrown with the
981+
// child's output attached. ECONNREFUSED (nothing listening), a timeout, a
982+
// DNS or a TLS fault each still fail on their first occurrence.
983+
// • child ALIVE and `UND_ERR_SOCKET` ⇒ absorbed, up to `attempts` in total.
984+
//
985+
// ⛔ CANNOT hide a wrong ANSWER. The loop absorbs transport failures only; a
986+
// `200` where the pin wants `403` is returned to the caller untouched, on the
987+
// first attempt, every time. Nor can it hide a boot that never reaches the
988+
// banner: that is the caller's readiness rejection, which this does not touch.
989+
//
990+
// ⚠️ CAN hide a LIVE `os serve` that occasionally destroys a connection without
991+
// answering it. That is a real product-defect class and this bound does not
992+
// remove it. What it does is make every occurrence print
993+
// `[<label>] absorbed a transport failure` with the full socket signature, so a
994+
// recurring one is a `grep` away in a CI log instead of a merge-queue eviction.
995+
// ⛔ If that line starts appearing, the finding is in `os serve` and belongs in a
996+
// card against it — do not raise `attempts` to make it stop.
997+
//
998+
// ⛔ And the line these files may never cross, restated because they are slow and
999+
// their subject is unglamorous: no `.skip`, no `.todo`, no quarantine list, no
1000+
// retry that swallows a failure into silence. A flake in a required shard is a
1001+
// harness bug to diagnose, never a licence to stop measuring.
1002+
// ─────────────────────────────────────────────────────────────────────────
1003+
1004+
/** How many times one probe may be re-sent, in total, across all attempts. */
1005+
export const PROBE_ATTEMPTS = 3;
1006+
1007+
/**
1008+
* How long a probe gives the child's `exit` event before it will call the child
1009+
* alive — see the section above for why that answer cannot be read
1010+
* synchronously off `exitCode`.
1011+
*
1012+
* Two seconds because this is a LOCAL child whose exit has already happened by
1013+
* the time its FIN reached us; the wait covers event-loop delivery, not process
1014+
* teardown. It is paid ONLY on a failing attempt, so a green run never waits.
1015+
*/
1016+
export const CHILD_EXIT_SETTLE_MS = 2_000;
1017+
1018+
/** The child's lifecycle state, as far as this process can observe it. */
1019+
export interface ChildFate {
1020+
exited: boolean;
1021+
code: number | null;
1022+
signal: NodeJS.Signals | null;
1023+
}
1024+
1025+
/**
1026+
* The child surface these lifecycle reads need, spelled structurally.
1027+
*
1028+
* Deliberately NOT `ChildProcess`: the files here spawn with different `stdio`
1029+
* tuples and so hold differently-typed handles (`ChildProcessByStdio<null,
1030+
* Readable, Readable>`, `ChildProcessWithoutNullStreams`). Naming only the four
1031+
* members actually read keeps every one of them assignable without a cast.
1032+
*/
1033+
export interface LifecycleChild {
1034+
exitCode: number | null;
1035+
signalCode: NodeJS.Signals | null;
1036+
once(event: 'exit', listener: () => void): unknown;
1037+
off(event: 'exit', listener: () => void): unknown;
1038+
}
1039+
1040+
export function fateOf(child: LifecycleChild): ChildFate {
1041+
return {
1042+
exited: child.exitCode !== null || child.signalCode !== null,
1043+
code: child.exitCode,
1044+
signal: child.signalCode,
1045+
};
1046+
}
1047+
1048+
/** Ask the child whether it is still running, allowing `ms` for the answer. */
1049+
export function settleChildFate(child: LifecycleChild, ms: number): Promise<ChildFate> {
1050+
const already = fateOf(child);
1051+
if (already.exited) return Promise.resolve(already);
1052+
return new Promise<ChildFate>((settle) => {
1053+
const onExit = () => {
1054+
clearTimeout(timer);
1055+
settle(fateOf(child));
1056+
};
1057+
const timer = setTimeout(() => {
1058+
child.off('exit', onExit);
1059+
settle(fateOf(child));
1060+
}, ms);
1061+
child.once('exit', onExit);
1062+
});
1063+
}
1064+
1065+
/** `cause.code` of a `fetch` rejection, when it carries one. */
1066+
export function transportCode(err: unknown): string | undefined {
1067+
const cause = (err as { cause?: { code?: unknown } } | undefined)?.cause;
1068+
return typeof cause?.code === 'string' ? cause.code : undefined;
1069+
}
1070+
1071+
/**
1072+
* One line naming what the transport did — the exact fields the recorded
1073+
* failure had to be diagnosed from, kept together so the NEXT occurrence needs
1074+
* no second run to be readable.
1075+
*/
1076+
export function transportSignature(err: unknown): string {
1077+
const cause = (err as {
1078+
cause?: { code?: unknown; message?: unknown; socket?: Record<string, unknown> };
1079+
} | undefined)?.cause;
1080+
const socket = cause?.socket;
1081+
const wire = socket
1082+
? ` socket: bytesWritten=${String(socket.bytesWritten)} bytesRead=${String(socket.bytesRead)}`
1083+
+ ` local=${String(socket.localAddress)}:${String(socket.localPort)}`
1084+
+ ` remote=${String(socket.remoteAddress)}:${String(socket.remotePort)}`
1085+
: ' socket: none — the connection was never established';
1086+
return `${(err as Error)?.message ?? String(err)}`
1087+
+ ` [cause ${String(cause?.code ?? 'none')}: ${String(cause?.message ?? 'none')}]${wire}`;
1088+
}
1089+
1090+
export interface ChildProbeOptions {
1091+
/** The spawned `os serve` the request is addressed to. */
1092+
child: LifecycleChild;
1093+
/**
1094+
* Everything the child has printed so far. Read at THROW time, not at call
1095+
* time, so a failure carries the crash the child printed on its way down
1096+
* rather than the buffer as it stood when the probe started.
1097+
*/
1098+
transcript: () => string;
1099+
/** The file's own tag, for the greppable absorbed-failure line. */
1100+
label: string;
1101+
/** What is being asked of the child, e.g. `the origin probe on port 41234`. */
1102+
what: string;
1103+
attempts?: number;
1104+
settleMs?: number;
1105+
}
1106+
1107+
/**
1108+
* Run one HTTP exchange against a spawned `os serve` and, if the transport
1109+
* fails, say WHY — naming the child's exit status and quoting everything it
1110+
* printed. See the section above for the bound and its fences.
1111+
*
1112+
* `request` must perform the whole exchange it wants attributed, body read
1113+
* included: a connection torn down mid-body rejects out of `res.json()` /
1114+
* `res.text()`, not out of `fetch()`, and a thunk that returns the bare
1115+
* `Response` leaves that half unattributed.
1116+
*/
1117+
export async function probeThroughChild<T>(
1118+
options: ChildProbeOptions,
1119+
request: () => Promise<T>,
1120+
): Promise<T> {
1121+
const attempts = options.attempts ?? PROBE_ATTEMPTS;
1122+
const settleMs = options.settleMs ?? CHILD_EXIT_SETTLE_MS;
1123+
const absorbed: string[] = [];
1124+
1125+
for (let attempt = 1; attempt <= attempts; attempt++) {
1126+
try {
1127+
return await request();
1128+
} catch (probeErr) {
1129+
const signature = transportSignature(probeErr);
1130+
// ⛔ Ask the child FIRST, and wait for the answer: a dead child must never
1131+
// reach the absorb branch below.
1132+
const fate = await settleChildFate(options.child, settleMs);
1133+
if (fate.exited) {
1134+
throw new Error(
1135+
`os serve DIED while answering ${options.what} — exit code ${String(fate.code)}, `
1136+
+ `signal ${String(fate.signal)}. This is the CHILD's failure, not a dropped socket, so `
1137+
+ 'it is NOT retried; the transcript below is what it printed on its way down.\n'
1138+
+ `attempt ${attempt}/${attempts}: ${signature}${options.transcript()}`,
1139+
);
1140+
}
1141+
if (transportCode(probeErr) !== 'UND_ERR_SOCKET') {
1142+
throw new Error(
1143+
`${options.what} failed with a transport error this harness does not absorb — only `
1144+
+ 'UND_ERR_SOCKET against a still-running child is absorbed, and only '
1145+
+ `${attempts} times (see PROBE_ATTEMPTS).\n`
1146+
+ `attempt ${attempt}/${attempts}: ${signature}${options.transcript()}`,
1147+
);
1148+
}
1149+
absorbed.push(`attempt ${attempt}/${attempts}: ${signature}`);
1150+
// Loud on purpose. An absorbed failure that printed nothing would be the
1151+
// "retry into silence" this harness forbids; this line is what makes a
1152+
// recurring one greppable in a CI log.
1153+
console.error(
1154+
`[${options.label}] absorbed a transport failure from a STILL-RUNNING os serve during `
1155+
+ `${options.what} (attempt ${attempt}/${attempts}) — ${signature}`,
1156+
);
1157+
}
1158+
}
1159+
1160+
throw new Error(
1161+
`${options.what} never completed a request against os serve after ${attempts} attempts. The `
1162+
+ `child was still running ${settleMs} ms after EVERY one of them, so this is a live server `
1163+
+ 'dropping connections, not a dead child — the finding is in os serve, not in this '
1164+
+ `harness.\n${absorbed.join('\n')}${options.transcript()}`,
1165+
);
1166+
}

0 commit comments

Comments
 (0)