fix(server): make a job's terminal outcome immutable on the parent side (#2264 P3)

Defense-in-depth complement to the worker terminal-claim: the launcher's message
handler and the job manager's updateJob both lacked a terminal-state guard, so a
late worker IPC message (a SIGTERM-driven 'error' after 'complete', or vice versa)
could re-release the repo lock and flip the reported status. (Touches parent-side
files outside the original PR diff — deliberate, clearly-scoped.)

- analyze-job.ts updateJob: drop any update once the job is already terminal (the
  transition INTO terminal still applies, since status isn't terminal yet then).
- analyze-launch.ts message handler: return early when the job is already terminal,
  mirroring its sibling exit handler.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
This commit is contained in:
Gergo Magyar 2026-06-21 13:13:34 +00:00
parent 7e1e5828a2
commit 717359c74a
3 changed files with 45 additions and 0 deletions

View file

@ -101,6 +101,12 @@ export class JobManager {
const job = this.jobs.get(id);
if (!job) return;
// Once a job is terminal (complete/failed) its outcome is immutable — drop any
// later update so a worker `complete` racing a SIGTERM-driven `error` (or vice
// versa) can't flip a reported result (#2264 P3). The transition INTO a terminal
// state still applies because `job.status` is not yet terminal at that point.
if (this.isTerminal(job.status)) return;
Object.assign(job, update);
if (this.isTerminal(job.status)) {

View file

@ -81,6 +81,13 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) {
});
child.on('message', (msg: WorkerMessage) => {
// Ignore any message once the job is terminal — a late worker message (a
// SIGTERM-driven `error` after `complete`, or vice versa) must not
// re-release the repo lock or flip the reported status. Mirrors the `exit`
// handler guard below; pairs with the worker's terminal-claim (#2264 P3).
const current = jobManager.getJob(job.id);
if (!current || current.status === 'complete' || current.status === 'failed') return;
if (msg.type === 'progress') {
jobManager.updateJob(job.id, {
status: 'analyzing',

View file

@ -128,4 +128,36 @@ describe('JobManager', () => {
it('cancelJob returns false for unknown job', () => {
expect(manager.cancelJob('nonexistent')).toBe(false);
});
// #2264 P3: a job's terminal outcome is immutable, so a late worker message (a
// SIGTERM-driven `error` after `complete`, or vice versa) cannot flip it.
describe('terminal-state immutability (#2264 P3)', () => {
it('keeps complete when a later failed update arrives', () => {
const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' });
manager.updateJob(job.id, { status: 'analyzing' });
manager.updateJob(job.id, { status: 'complete', repoName: 'repo' });
manager.updateJob(job.id, { status: 'failed', error: 'Analysis cancelled' });
expect(manager.getJob(job.id)!.status).toBe('complete');
});
it('keeps failed when a later complete update arrives', () => {
const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' });
manager.updateJob(job.id, { status: 'analyzing' });
manager.updateJob(job.id, { status: 'failed', error: 'Analysis cancelled' });
manager.updateJob(job.id, { status: 'complete', repoName: 'repo' });
const after = manager.getJob(job.id)!;
expect(after.status).toBe('failed');
expect(after.error).toBe('Analysis cancelled');
});
it('emits no further event for a post-terminal update', () => {
const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' });
const events: Array<{ phase: string }> = [];
manager.onProgress(job.id, (data) => events.push(data));
manager.updateJob(job.id, { status: 'complete', repoName: 'repo' });
manager.updateJob(job.id, { status: 'failed', error: 'late' });
expect(events).toHaveLength(1);
expect(events[0].phase).toBe('complete');
});
});
});