Fix background job zombie entries and enhance --fresh clearing#435
Fix background job zombie entries and enhance --fresh clearing#435SparshGarg999 wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6397249e7
ℹ️ 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".
| const state = loadState(cwd); | ||
| const now = Date.now(); | ||
| const nextJobs = state.jobs.filter((job) => { | ||
| if (job.status !== "running" && job.updatedAt) { |
There was a problem hiding this comment.
Queued background jobs are active elsewhere (enqueueBackgroundTask writes status queued, and status/cancel treat queued as active). With this predicate, any queued job whose worker has not transitioned to running within 30 minutes, such as after delayed startup or machine sleep, is removed and saveState deletes its job JSON/log, leaving /codex:status and /codex:cancel unable to see it even though the worker may still run. Restrict GC to explicit terminal statuses instead of every non-running status.
Useful? React with 👍 / 👎.
|
|
||
| export function clearTerminalJobs(cwd) { | ||
| const state = loadState(cwd); | ||
| const nextJobs = state.jobs.filter((job) => job.status === "running"); |
There was a problem hiding this comment.
This is used by task --fresh, but a background job can still be in the queued state before the detached worker rewrites it as running. Because saveState deletes files for jobs not retained, running a fresh task during that window removes the queued job's request/log and can make the worker fail with “No stored job found” or make the active job impossible to cancel. Keep both queued and running jobs, or filter only known terminal statuses.
Useful? React with 👍 / 👎.
rajpratham1
left a comment
rajpratham1
left a comment
There was a problem hiding this comment.
The intent is good, but I don't think this implementation is safe enough yet. The review bot has already pointed out two correctness issues that can cause active jobs to disappear.
Major issues
- Queued jobs are incorrectly garbage collected
Current logic:
if (job.status !== "running" && job.updatedAt) {
...
return false;
}
This treats every non-running job as terminal.
But elsewhere the codebase treats queued as an active state.
Possible scenario:
User starts a background task.
Job is stored as queued.
Machine sleeps or startup is delayed.
After 30 minutes, listJobs() runs.
The queued job is deleted.
Worker eventually starts but its metadata/logs are gone.
That is data loss.
GC should only remove known terminal states such as:
completed
failed
cancelled/stopped
not every non-running status.
- clearTerminalJobs() also removes queued jobs
Current code:
job.status === "running"
This means --fresh deletes every queued job.
A queued worker is still active.
Removing it can cause:
missing job metadata
failed worker startup
/codex:cancel no longer works
/codex:status can't find the job
Again, terminal states should be filtered explicitly instead of keeping only "running".
Tests miss these cases
The new tests only cover:
running
completed
failed
They should also include a queued job to verify that:
listJobs() preserves queued jobs
clearTerminalJobs() preserves queued jobs
Without those tests, the regression remains undetected.
What is good
Adding automatic cleanup for stale terminal jobs is a reasonable improvement.
Calling clearTerminalJobs() for --fresh makes sense.
Test coverage for completed/failed cleanup is good.
…icit terminal statuses, preserving queued jobs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6ae0ea4a1
ℹ️ 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".
| }); | ||
| if (nextJobs.length !== state.jobs.length) { | ||
| state.jobs = nextJobs; | ||
| saveState(cwd, state); |
There was a problem hiding this comment.
When listJobs is called while there is any terminal job older than 30 minutes, this newly-added save writes the entire state object that was loaded before filtering. In a normal background run, a detached worker updates the same state via upsertJob as it moves from queued/running to completed; if /codex:status, /codex:result, or the stop hook calls listJobs between that load and this save, the GC write can overwrite the worker's newer status/thread fields with the stale snapshot, leaving a completed job indexed as running or otherwise losing progress. Re-load/mutate under a lock or remove only expired IDs from the current state.
Useful? React with 👍 / 👎.
2 participants
This PR fixes background job state machine retaining zombie entries (Issue #410) in
openai/codex-plugin-cc:Auto-Garbage Collection (GC) for Job State (Issue Background job state machine retains zombie entries;
--freshdoes not clear them #410):listJobs()(insidestate.mjs) that automatically filters out completed, failed, or stopped jobs older than 30 minutes. The associated JSON state files and log files on disk are pruned automatically during this process via the existingsaveStatelogic.Enhanced
--freshFlag Behavior:--freshflag did not actively clean up existing terminal-state jobs, meaning stale jobs still interfered or showed up in/codex:status.clearTerminalJobs(cwd)function to wipe all terminal-state jobs (completed, failed, or stopped) while keeping active/running jobs. This is invoked incodex-companion.mjs'shandleTaskwhen the--freshoption is passed, ensuring a completely clean slate for new runs.Added Unit Tests:
tests/state.test.mjsverifying thatlistJobscorrectly prunes expired jobs andclearTerminalJobsremoves only terminal jobs.tests/state.test.mjswhere multiple test cases resolved to the same temp workspace directory on Windows (due to parent directory traversing for Git repo roots), by callinginitGitRepo(workspace)on the unique temp workspaces.