Skip to content

fix(task): publish jobs before worker spawn - #711

Closed
ALV0612 wants to merge 1 commit into
openai:mainfrom
ALV0612:fix/issue-620-publish-job-before-worker
Closed

fix(task): publish jobs before worker spawn#711
ALV0612 wants to merge 1 commit into
openai:mainfrom
ALV0612:fix/issue-620-publish-job-before-worker

Conversation

@ALV0612

@ALV0612 ALV0612 commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • publish the complete pid-less queued job record before starting its detached worker
  • wait for the child spawn acknowledgement before reporting queued success
  • let the worker exclusively own the running transition and PID registration, preventing parent/worker state regression
  • persist a terminal failed job and return non-zero when spawn fails or yields no PID
  • include initial runTrackedJob persistence inside its failure boundary

Root cause

enqueueBackgroundTask() called spawnDetachedTaskWorker() before writing the job record consumed by task-worker. On Windows, parent-side spawn() was measured taking 716-878 ms while the child reached readStoredJob() in about 327 ms, so a normal single dispatch could lose the race, exit with “No stored job found,” and leave a permanent queued record.

This change removes the race by construction rather than adding sleeps or retries. The parent first publishes all worker input with pid: null; after process creation is acknowledged, the worker performs the existing queued → running transition and records process.pid itself.

Testing

  • node --test tests/background-dispatch.test.mjs tests/tracked-jobs.test.mjs — 5/5 pass
  • existing background task integration test — pass
  • npm test — 96/96 pass
  • npm run build — pass
  • npm run check-version — pass
  • git diff --check — pass

Regression coverage includes:

  • complete request exists synchronously before spawn
  • worker-authored running state cannot be regressed by the parent
  • spawn error converges job file and state index to failed
  • spawn acknowledgement without a finite PID fails closed
  • initial running-state persistence failure prevents runner execution
  • normal detached task remains resolvable through status/result

Fixes #620

@ALV0612
ALV0612 requested a review from a team September 2, 2026 02:54

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a1d6f4f00

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

...job,
status: "queued",
phase: "queued",
pid: null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Prevent queued cancellation from losing the worker PID

When /codex:cancel runs after this queued record is published but before the worker writes its running state, cancellation reads this null PID, so it cannot terminate the already-spawned process and merely records the job as cancelled. The worker then unconditionally enters runTrackedJob, overwrites that record as running, and executes the task; this can cause a cancelled background --write task 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 👍 / 👎.

@ALV0612

ALV0612 commented Sep 2, 2026

Copy link
Copy Markdown
Author

Closing this PR because a deeper backlog audit found that open PR #667 already implements the same publish-before-spawn ordering for background jobs. I missed it initially because #667 does not reference issue #620 in its title/body or timeline. Avoiding duplicate maintainer work; the independent tests here remain available if useful.

@ALV0612 ALV0612 closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant