Recover Codex jobs across workspace scopes - #667
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: 43e61ea88b
ℹ️ 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".
| status: "queued", | ||
| phase: "queued", | ||
| pid: child.pid ?? null, | ||
| pid: null, |
There was a problem hiding this comment.
When cancel runs after enqueue returns but before the worker enters runTrackedJob, this null PID makes terminateProcessTree a no-op. The worker does not check whether the stored job was cancelled, so it can subsequently overwrite the cancelled record as running and continue executing, including a --write task. Persist before spawning as intended, but update the queued record with child.pid before returning so cancellation can terminate the worker during this window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e06cd2c243
ℹ️ 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 crossWorkspace = findCrossWorkspaceJob(reference); | ||
| if (!crossWorkspace) { | ||
| throw error; | ||
| } | ||
| return { | ||
| workspaceRoot: crossWorkspace.workspaceRoot, | ||
| job: enrichJob(crossWorkspace.job, { maxProgressLines: options.maxProgressLines }) |
There was a problem hiding this comment.
When this fallback finds an active job from another workspace, the rendered status includes /codex:cancel <id>, but resolveCancelableJob still searches only listJobs(workspaceRoot). Consequently, a background task— including one running with --write—can be monitored from the recovered scope but cannot be stopped there; explicit cancellation should use the same cross-workspace lookup with an active-status predicate.
Useful? React with 👍 / 👎.
|
Addressed the cancellation race in e06cd2c:
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Addressed the cross-workspace cancellation finding in 93beb53:
Targeted cross-scope cancellation test passed. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93beb533be
ℹ️ 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) => job.status === "queued" || job.status === "running" | ||
| ); | ||
| if (crossWorkspace) { | ||
| return crossWorkspace; |
There was a problem hiding this comment.
When this fallback resolves a running job from another workspace, handleCancel still calls interruptAppServerTurn(cwd, ...) with the invocation workspace rather than the returned workspaceRoot (codex-companion.mjs:1012). For a job using a workspace-scoped shared broker, that looks up the wrong broker.json and sends the interrupt to a new or unrelated app server; killing the detached worker can then leave the brokered turn—potentially a --write task—running after the command records it as cancelled. Pass the owning workspace to the interrupt call.
Useful? React with 👍 / 👎.
|
Addressed the owning-broker finding in 7e107ec:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e107ec166
ℹ️ 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 launchRecord = { ...queuedRecord, pid: child.pid ?? null }; | ||
| writeJobFile(job.workspaceRoot, job.id, launchRecord); | ||
| upsertJob(job.workspaceRoot, launchRecord); | ||
| fs.writeFileSync(startGate, "ready\n", "utf8"); |
There was a problem hiding this comment.
When another process cancels the job after the initial queued record is published but before this launch record is saved, handleCancel sees a queued job with a null PID and records it as cancelled, but these lines then overwrite that state back to queued and release the worker. The worker consequently observes the overwritten status and executes the task, including a possible --write task, despite cancellation having succeeded. The new queued-before-spawn ordering is fresh evidence of this distinct race; re-read the stored status before overwriting it or make registration and cancellation atomic.
Useful? React with 👍 / 👎.
|
Addressed the startup cancellation race in 87db4e2. The bootstrap job file is now kept out of the shared state index until the detached worker PID is known. The first publicly discoverable Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Fixed rescue result delivery in 18cdf7a. Root cause: the rescue subagent could infer companion The contract now separates the two layers:
Validation:
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
CLAUDE_PROJECT_DIRwhen--cwdis omittedProblem
A Claude rescue subagent can launch Codex from a parent/home directory while the target repository is only available through
CLAUDE_PROJECT_DIR. The background job then lands in the wrong workspace registry. Laterstatus <job-id>andresult <job-id>from the target repository report that the job disappeared even though Codex completed it. The old enqueue order also spawned the worker before its job file existed.Verification
CHANNEL_OK