-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(task): publish jobs before worker spawn #711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
ALV0612
wants to merge
1
commit into
openai:main
from
ALV0612:fix/issue-620-publish-job-before-worker
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import { readJobFile, resolveJobFile, upsertJob, writeJobFile } from "./state.mjs"; | ||
| import { nowIso } from "./tracked-jobs.mjs"; | ||
|
|
||
| function errorMessage(error) { | ||
| return error instanceof Error ? error.message : String(error); | ||
| } | ||
|
|
||
| function persistSpawnFailure(workspaceRoot, jobId, error) { | ||
| const message = errorMessage(error); | ||
| const completedAt = nowIso(); | ||
| let existing = { id: jobId }; | ||
| try { | ||
| existing = readJobFile(resolveJobFile(workspaceRoot, jobId)); | ||
| } catch { | ||
| // Preserve the launch error even if the prepublished record cannot be reread. | ||
| } | ||
| const failedRecord = { | ||
| ...existing, | ||
| id: jobId, | ||
| status: "failed", | ||
| phase: "failed", | ||
| pid: null, | ||
| errorMessage: message, | ||
| completedAt | ||
| }; | ||
| writeJobFile(workspaceRoot, jobId, failedRecord); | ||
| upsertJob(workspaceRoot, { | ||
| id: jobId, | ||
| status: "failed", | ||
| phase: "failed", | ||
| pid: null, | ||
| errorMessage: message, | ||
| completedAt | ||
| }); | ||
| } | ||
|
|
||
| function awaitSpawn(child) { | ||
| if (!child || typeof child.once !== "function") { | ||
| return Promise.reject(new Error("Detached task worker did not return a child process.")); | ||
| } | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const onSpawn = () => { | ||
| child.off?.("error", onError); | ||
| resolve(child); | ||
| }; | ||
| const onError = (error) => { | ||
| child.off?.("spawn", onSpawn); | ||
| reject(error); | ||
| }; | ||
| child.once("spawn", onSpawn); | ||
| child.once("error", onError); | ||
| }); | ||
| } | ||
|
|
||
| export async function dispatchBackgroundJob({ job, request, logFile, spawnWorker }) { | ||
| const queuedRecord = { | ||
| ...job, | ||
| status: "queued", | ||
| phase: "queued", | ||
| pid: null, | ||
| logFile, | ||
| request | ||
| }; | ||
|
|
||
| // The worker's first operation is to read this record, so publish it before | ||
| // process creation. The worker owns the later running transition and PID. | ||
| writeJobFile(job.workspaceRoot, job.id, queuedRecord); | ||
| upsertJob(job.workspaceRoot, queuedRecord); | ||
|
|
||
| let child; | ||
| try { | ||
| child = spawnWorker(); | ||
| await awaitSpawn(child); | ||
| if (!Number.isFinite(child.pid)) { | ||
| throw new Error("Detached task worker spawned without a process ID."); | ||
| } | ||
| child.unref?.(); | ||
| } catch (error) { | ||
| persistSpawnFailure(job.workspaceRoot, job.id, error); | ||
| throw error; | ||
| } | ||
|
|
||
| return { | ||
| jobId: job.id, | ||
| status: "queued", | ||
| title: job.title, | ||
| summary: job.summary, | ||
| logFile | ||
| }; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| import { EventEmitter } from "node:events"; | ||
| import fs from "node:fs"; | ||
| import path from "node:path"; | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
|
|
||
| import { dispatchBackgroundJob } from "../plugins/codex/scripts/lib/background-dispatch.mjs"; | ||
| import { makeTempDir } from "./helpers.mjs"; | ||
| import { | ||
| listJobs, | ||
| readJobFile, | ||
| resolveJobFile, | ||
| upsertJob, | ||
| writeJobFile | ||
| } from "../plugins/codex/scripts/lib/state.mjs"; | ||
|
|
||
| function createJob(workspaceRoot) { | ||
| return { | ||
| id: "task-ordering-test", | ||
| kind: "task", | ||
| kindLabel: "rescue", | ||
| title: "Codex Task", | ||
| workspaceRoot, | ||
| jobClass: "task", | ||
| summary: "ordering test", | ||
| write: false, | ||
| createdAt: "2026-09-02T00:00:00.000Z" | ||
| }; | ||
| } | ||
|
|
||
| function fakeChild(schedule, pid = 4242) { | ||
| const child = new EventEmitter(); | ||
| child.pid = pid; | ||
| child.unrefCalled = false; | ||
| child.unref = () => { | ||
| child.unrefCalled = true; | ||
| }; | ||
| process.nextTick(() => schedule(child)); | ||
| return child; | ||
| } | ||
|
|
||
| test("dispatchBackgroundJob publishes the complete request before spawning", async () => { | ||
| const workspace = makeTempDir(); | ||
| const job = createJob(workspace); | ||
| const request = { cwd: workspace, prompt: "inspect the race", write: false }; | ||
| const logFile = path.join(workspace, "task.log"); | ||
| fs.writeFileSync(logFile, "queued\n", "utf8"); | ||
| let observedAtSpawn = null; | ||
| let child = null; | ||
|
|
||
| const payload = await dispatchBackgroundJob({ | ||
| job, | ||
| request, | ||
| logFile, | ||
| spawnWorker() { | ||
| observedAtSpawn = readJobFile(resolveJobFile(workspace, job.id)); | ||
| child = fakeChild((value) => value.emit("spawn")); | ||
| return child; | ||
| } | ||
| }); | ||
|
|
||
| assert.equal(observedAtSpawn.status, "queued"); | ||
| assert.equal(observedAtSpawn.pid, null); | ||
| assert.deepEqual(observedAtSpawn.request, request); | ||
| assert.equal(observedAtSpawn.logFile, logFile); | ||
| assert.equal(payload.jobId, job.id); | ||
| assert.equal(payload.status, "queued"); | ||
| assert.equal(child.unrefCalled, true); | ||
| assert.equal(listJobs(workspace)[0].status, "queued"); | ||
| }); | ||
|
|
||
| test("dispatchBackgroundJob never regresses a worker-authored running record", async () => { | ||
| const workspace = makeTempDir(); | ||
| const job = createJob(workspace); | ||
| const request = { cwd: workspace, prompt: "run immediately", write: false }; | ||
| const logFile = path.join(workspace, "task.log"); | ||
| fs.writeFileSync(logFile, "queued\n", "utf8"); | ||
|
|
||
| await dispatchBackgroundJob({ | ||
| job, | ||
| request, | ||
| logFile, | ||
| spawnWorker() { | ||
| const queued = readJobFile(resolveJobFile(workspace, job.id)); | ||
| const running = { | ||
| ...queued, | ||
| status: "running", | ||
| phase: "starting", | ||
| pid: 4242, | ||
| startedAt: "2026-09-02T00:00:01.000Z" | ||
| }; | ||
| writeJobFile(workspace, job.id, running); | ||
| upsertJob(workspace, running); | ||
| return fakeChild((value) => value.emit("spawn")); | ||
| } | ||
| }); | ||
|
|
||
| const stored = readJobFile(resolveJobFile(workspace, job.id)); | ||
| assert.equal(stored.status, "running"); | ||
| assert.equal(stored.pid, 4242); | ||
| assert.equal(listJobs(workspace)[0].status, "running"); | ||
| assert.equal(listJobs(workspace)[0].pid, 4242); | ||
| }); | ||
|
|
||
| test("dispatchBackgroundJob persists a terminal failure when spawn fails", async () => { | ||
| const workspace = makeTempDir(); | ||
| const job = createJob(workspace); | ||
| const request = { cwd: workspace, prompt: "cannot spawn", write: false }; | ||
| const logFile = path.join(workspace, "task.log"); | ||
| fs.writeFileSync(logFile, "queued\n", "utf8"); | ||
|
|
||
| await assert.rejects( | ||
| dispatchBackgroundJob({ | ||
| job, | ||
| request, | ||
| logFile, | ||
| spawnWorker() { | ||
| return fakeChild((value) => value.emit("error", new Error("spawn denied"))); | ||
| } | ||
| }), | ||
| /spawn denied/ | ||
| ); | ||
|
|
||
| const stored = readJobFile(resolveJobFile(workspace, job.id)); | ||
| const indexed = listJobs(workspace)[0]; | ||
| for (const record of [stored, indexed]) { | ||
| assert.equal(record.status, "failed"); | ||
| assert.equal(record.phase, "failed"); | ||
| assert.equal(record.pid, null); | ||
| assert.equal(record.errorMessage, "spawn denied"); | ||
| assert.ok(record.completedAt); | ||
| } | ||
| }); | ||
|
|
||
|
|
||
| test("dispatchBackgroundJob rejects a spawn acknowledgement without a process ID", async () => { | ||
| const workspace = makeTempDir(); | ||
| const job = createJob(workspace); | ||
| const request = { cwd: workspace, prompt: "missing pid", write: false }; | ||
| const logFile = path.join(workspace, "task.log"); | ||
| fs.writeFileSync(logFile, "queued\n", "utf8"); | ||
|
|
||
| await assert.rejects( | ||
| dispatchBackgroundJob({ | ||
| job, | ||
| request, | ||
| logFile, | ||
| spawnWorker() { | ||
| return fakeChild((value) => value.emit("spawn"), Number.NaN); | ||
| } | ||
| }), | ||
| /spawned without a process ID/ | ||
| ); | ||
|
|
||
| const stored = readJobFile(resolveJobFile(workspace, job.id)); | ||
| assert.equal(stored.status, "failed"); | ||
| assert.equal(stored.pid, null); | ||
| assert.match(stored.errorMessage, /without a process ID/); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
/codex:cancelruns after this queued record is published but before the worker writes its running state, cancellation reads thisnullPID, so it cannot terminate the already-spawned process and merely records the job as cancelled. The worker then unconditionally entersrunTrackedJob, overwrites that record as running, and executes the task; this can cause a cancelled background--writetask to still modify the workspace. Preserve a cancellable PID after spawn or make the worker honor a cancelled record before transitioning to running.Useful? React with 👍 / 👎.