Skip to content

Commit c285e94

Browse files
committed
fix(parity): bound the external validators so a stall fails instead of hanging
A Pipeline Parity hl7 (ips) job sat for 30+ minutes on a single line of output and had to be cancelled by hand. Three compounding gaps, none of them in the validators themselves: 1. spawnAndCapture settled only on the child's close/error. The validators print nothing until they finish, so a subprocess blocked on the network was indistinguishable from one doing work, and the promise never settled. It now kills the child after a wall-clock bound and rejects with the byte count and stderr tail, so the failure names itself. Override with PARITY_VALIDATOR_TIMEOUT_MS; default 15 minutes, against single-digit minutes for a whole package. 2. No job in _parity-tests.yml declared timeout-minutes, so each inherited GitHub's 6-hour default. Without the manual cancel that job would have burned six hours. internal gets 45 (it generates what the others reuse), firely and hl7 get 30. 3. downloadValidator handled only 'error' on its release download, so a stalled socket waited forever too. Now setTimeout + destroy at 120s. Verified by reproducing the failure rather than reasoning about it: runHL7Batch pointed at a stand-in batch-validate.js that sleeps forever and prints nothing — the real shape of the stuck job — rejects after exactly the configured bound with "produced no result within 3s and was killed. Captured 0 bytes of stdout". Not the cause, though both were suspects: fhirpath 5.1.0 introduced a one-hour pending-request timeout, but Phase 3 runs the external HL7 Java validator and fhirpath only drives our internal ones. And the job logged SKIP_GENERATION with resources loaded from a manifest, so no emitter ran in it at all. Still open, seen in the same log and not addressed here: while validating ips, the exclusion line listed 27 classes belonging to other packages (AUCore*, ISiK*, KBV*, USCoreVitalSigns*). Either a global summary printed once or getExcludedFilePaths is not filtered per package.
1 parent e9855bf commit c285e94

3 files changed

Lines changed: 50 additions & 4 deletions

File tree

.github/workflows/_parity-tests.yml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ jobs:
113113
internal:
114114
needs: [setup]
115115
runs-on: ubuntu-latest
116+
# Generates the packages the other two jobs reuse, so it is the longest of the
117+
# three; bounded for the same reason as hl7 below.
118+
timeout-minutes: 45
116119
permissions:
117120
contents: read
118121
strategy:
@@ -239,6 +242,9 @@ jobs:
239242
needs: [setup, internal, resolve-versions]
240243
if: ${{ !cancelled() }}
241244
runs-on: ubuntu-latest
245+
# Same exposure as hl7: an external validator subprocess that can stall on the
246+
# network with no output.
247+
timeout-minutes: 30
242248
permissions:
243249
contents: read
244250
strategy:
@@ -368,6 +374,11 @@ jobs:
368374
needs: [setup, internal, resolve-versions]
369375
if: ${{ !cancelled() }}
370376
runs-on: ubuntu-latest
377+
# The external validator runs as a subprocess with no output while it works, so
378+
# a network stall inside it reads as a live job. Without a bound it inherits
379+
# GitHub's 6-hour default: one hl7 (ips) job sat for 30+ minutes on nothing
380+
# before being cancelled by hand. A whole package takes single-digit minutes.
381+
timeout-minutes: 30
371382
permissions:
372383
contents: read
373384
strategy:

scripts/hl7-validator/batch-validate.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,12 @@ async function downloadValidator(version) {
115115
return new Promise((resolve, reject) => {
116116
const file = createWriteStream(jarPath);
117117

118+
// A stalled socket had no bound: only 'error' was handled, so a hung release
119+
// download waited forever and the whole parity job waited with it.
120+
const DOWNLOAD_TIMEOUT_MS = 120_000;
121+
118122
const download = (downloadUrl) => {
119-
httpsGet(downloadUrl, (response) => {
123+
const request = httpsGet(downloadUrl, (response) => {
120124
if (response.statusCode === 302 || response.statusCode === 301) {
121125
// Follow redirect
122126
download(response.headers.location);
@@ -135,6 +139,10 @@ async function downloadValidator(version) {
135139
}).on('error', (err) => {
136140
reject(err);
137141
});
142+
request.setTimeout(DOWNLOAD_TIMEOUT_MS, () => {
143+
request.destroy();
144+
reject(new Error(`Validator download stalled for ${DOWNLOAD_TIMEOUT_MS / 1000}s: ${downloadUrl}`));
145+
});
138146
};
139147

140148
download(url);

src/test/parity/validatorAdapters.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,19 @@ function parseValidatorJson(jsonOutput: string, label: string): Map<string, Vali
6565
}
6666

6767
/** Spawn a child process, capture stdout/stderr, return stdout on close. */
68+
/**
69+
* Wall-clock bound on an external validator.
70+
*
71+
* The validators emit nothing until they finish, so a stall inside one is
72+
* indistinguishable from work in progress. Settling only on `close`/`error` meant
73+
* a validator blocked on the network hung the run indefinitely: one hl7 (ips) job
74+
* sat for 30+ minutes on a single line of output before being cancelled by hand,
75+
* and the workflow's jobs had no timeout to stop it either.
76+
*
77+
* A whole package is single-digit minutes, so this is generous.
78+
*/
79+
const VALIDATOR_TIMEOUT_MS = Number(process.env.PARITY_VALIDATOR_TIMEOUT_MS ?? 15 * 60 * 1000);
80+
6881
function spawnAndCapture(
6982
cmd: string,
7083
args: string[],
@@ -78,17 +91,31 @@ function spawnAndCapture(
7891
});
7992
let out = '';
8093
let err = '';
94+
let settled = false;
95+
const finish = (fn: () => void) => { if (!settled) { settled = true; clearTimeout(timer); fn(); } };
96+
97+
// unref() so a pending timer cannot itself hold the process open.
98+
const timer = setTimeout(() => {
99+
child.kill('SIGKILL');
100+
const errTail = err.length > 500 ? `...${err.slice(-500)}` : err;
101+
finish(() => reject(new Error(
102+
`${opts.label} produced no result within ${Math.round(VALIDATOR_TIMEOUT_MS / 1000)}s and was killed. `
103+
+ `Captured ${out.length} bytes of stdout. stderr: ${errTail || '(none)'}`,
104+
)));
105+
}, VALIDATOR_TIMEOUT_MS);
106+
timer.unref?.();
107+
81108
child.stdout.on('data', d => { out += d.toString(); });
82109
child.stderr.on('data', d => { err += d.toString(); });
83-
child.on('error', reject);
110+
child.on('error', e => finish(() => reject(e)));
84111
child.on('close', (code) => {
85112
if (err && opts.verbose) console.log(`${opts.label} stderr:`, err);
86113
if (code !== 0 && !out.trim()) {
87114
const errTail = err.length > 500 ? `...${err.slice(-500)}` : err;
88-
reject(new Error(`${opts.label} exited with code ${code}. stderr: ${errTail}`));
115+
finish(() => reject(new Error(`${opts.label} exited with code ${code}. stderr: ${errTail}`)));
89116
return;
90117
}
91-
resolve(out);
118+
finish(() => resolve(out));
92119
});
93120
});
94121
}

0 commit comments

Comments
 (0)