Fix late cleanup-failure error line overwriting an already-successful job done
The 'line' handler applied a t:'error' event immediately and unconditionally, even when a t:'done' had already arrived (deferred as pendingDone until close). A job whose real work finished successfully (io.done() called, e.g. leads already committed by the child) could still end up persisted and reported as 'error' if something in the child's finally-block cleanup (closing the lease/db) threw afterward, since main().catch() calls io.fail() + process.exit(1) in that case — discarding a real success summary for a cosmetic cleanup failure. Fix distinguishes this from a genuinely unexplained nonzero exit (process killed/crashed with no chance to report anything), which two existing tests deliberately require to still override a done: a new lateErrorAfterDone flag is only set when an explicit error line follows a done, and close only trusts the done over a nonzero exit code when that flag is set. Found via a full-project audit.
This commit is contained in:
+26
-1
@@ -65,6 +65,14 @@ export function createJobManager({ spawn: spawnProcess = spawn, killGraceMs = KI
|
||||
cancelRequested: false
|
||||
,ownerToken,
|
||||
pendingDone: null,
|
||||
// true quando uma linha "error" explicita chegou depois de um "done"
|
||||
// ja recebido: sinal de que o filho reportou sucesso e so depois falhou
|
||||
// (ex.: limpeza no finally), nao uma falha real do trabalho. Distingue
|
||||
// esse caso de um exit/close com codigo != 0 SEM nenhuma linha "error"
|
||||
// (processo morto sem chance de reportar nada), que continua corrigindo
|
||||
// um "done" para erro — ver os dois testes correspondentes em
|
||||
// jobs-manager.test.mjs.
|
||||
lateErrorAfterDone: false,
|
||||
exitCode: null,
|
||||
exitSignal: null,
|
||||
cleanupScheduled: false
|
||||
@@ -92,6 +100,18 @@ export function createJobManager({ spawn: spawnProcess = spawn, killGraceMs = KI
|
||||
job.pendingDone = event;
|
||||
return;
|
||||
} else if (event.t === 'error') {
|
||||
// Um "done" ja recebido significa que o trabalho relevante (ex.:
|
||||
// leads gravados no banco pelo proprio filho) ja terminou com
|
||||
// sucesso. Um "error" chegando depois disso normalmente vem de uma
|
||||
// falha na limpeza final (ex.: fechar a conexao do banco no finally)
|
||||
// apos main() ja ter retornado — nao deve sobrescrever um resultado
|
||||
// que ja foi bem-sucedido. O "close" cuida do status terminal usando
|
||||
// pendingDone.
|
||||
if (job.pendingDone) {
|
||||
job.lateErrorAfterDone = true;
|
||||
job.stderrTail = (job.stderrTail + (event.message || '')).slice(-4096);
|
||||
return;
|
||||
}
|
||||
job.status = job.cancelRequested ? 'cancelled' : 'error';
|
||||
job.finishedAt = new Date().toISOString();
|
||||
persistTerminal(job, job.status, job.status === 'error' ? event.message || 'processo filho falhou' : null);
|
||||
@@ -138,7 +158,12 @@ export function createJobManager({ spawn: spawnProcess = spawn, killGraceMs = KI
|
||||
job.exitCode = code ?? job.exitCode;
|
||||
job.exitSignal = signal ?? job.exitSignal;
|
||||
if (job.status === 'running') {
|
||||
const cleanDone = job.exitCode === 0 && job.pendingDone;
|
||||
// Confia no "done" mesmo com saida != 0 apenas quando uma linha
|
||||
// "error" explicita chegou depois dele (lateErrorAfterDone) — sinal
|
||||
// de que o filho terminou o trabalho e so falhou depois, na limpeza.
|
||||
// Uma saida != 0 sem nenhuma linha "error" (processo morto sem poder
|
||||
// reportar nada) continua corrigindo o "done" para erro.
|
||||
const cleanDone = Boolean(job.pendingDone) && (job.exitCode === 0 || job.lateErrorAfterDone);
|
||||
job.status = cleanDone
|
||||
? (job.pendingDone.summary?.cancelled ? 'cancelled' : 'done')
|
||||
: (job.cancelRequested ? 'cancelled' : 'error');
|
||||
|
||||
@@ -165,6 +165,41 @@ test('linha done drenada entre exit e close resulta em done', async () => {
|
||||
assert.equal(jobs.getJob(jobId).lastEvent.summary.late, true);
|
||||
});
|
||||
|
||||
// Cenario real do capture-job.mjs: main() chama io.done() apos o trabalho
|
||||
// terminar (leads ja gravados pelo proprio filho), mas o finally (fechar
|
||||
// lease/db) falha depois disso — main().catch escreve uma linha "error"
|
||||
// explicita e sai com codigo != 0. O done ja reportado nao pode virar erro
|
||||
// so por causa da falha de limpeza que veio depois dele.
|
||||
test('erro explicito apos done (falha de limpeza) nao sobrescreve o done', async () => {
|
||||
const db = openDb(':memory:');
|
||||
const runId = Number(db.prepare("INSERT INTO capture_runs (params_json, status) VALUES ('{}', 'running')").run().lastInsertRowid);
|
||||
const child = new EventEmitter();
|
||||
child.stdout = new PassThrough();
|
||||
child.stderr = new PassThrough();
|
||||
child.stdin = { write() {} };
|
||||
child.kill = () => {};
|
||||
const jobs = createJobManager({ spawn: () => child, db });
|
||||
|
||||
const jobId = jobs.startJob({ type: 'captura', script: 'fake', meta: { runId } });
|
||||
const events = [];
|
||||
jobs.subscribe(jobId, (event) => events.push(event));
|
||||
|
||||
child.stdout.write('{"t":"done","summary":{"new":42}}\n');
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
child.stdout.write('{"t":"error","message":"falha ao fechar conexao do banco"}\n');
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
assert.equal(jobs.getJob(jobId).status, 'running');
|
||||
|
||||
child.emit('exit', 1, null);
|
||||
child.emit('close', 1, null);
|
||||
|
||||
assert.equal(jobs.getJob(jobId).status, 'done');
|
||||
assert.equal(events.at(-1).t, 'done');
|
||||
assert.equal(events.at(-1).summary.new, 42);
|
||||
assert.equal(db.prepare('SELECT status FROM capture_runs WHERE id = ?').get(runId).status, 'done');
|
||||
db.close();
|
||||
});
|
||||
|
||||
test('manager trata erro do stream stdout como falha terminal persistida', async () => {
|
||||
const db = openDb(':memory:');
|
||||
const exportId = Number(db.prepare("INSERT INTO exports (tipo, status) VALUES ('excel', 'running')").run().lastInsertRowid);
|
||||
|
||||
Reference in New Issue
Block a user