fix: SessionEnd/status lookups miss state written under a different CLAUDE_PLUGIN_DATA root, orphaning brokers - #659
Conversation
…LAUDE_PLUGIN_DATA root, orphaning brokers resolveStateDir() picks the state root from CLAUDE_PLUGIN_DATA when set, falling back to $TMPDIR/codex-companion when it's absent -- same workspace slug/hash either way, only the root differs. State written under one root (e.g. a broker registered while the var was unset) becomes invisible to any later lookup that resolves to the other root, since nothing checked both. For a broker specifically, that means SessionEnd never finds it to shut down -- it's orphaned permanently, and since ensureBrokerSession also can't see it, the next session spawns a duplicate broker for the same workspace, compounding the leak. The same mechanism affects job/status state (state.json, individual job detail files), not just the broker. Reads now check every candidate root (current primary, then the tmpdir fallback), not just the current invocation's primary -- writes are unchanged, still going to the primary root. Applied to loadState() (job list, status, config), readStoredJob() (individual job detail lookups), and loadBrokerSession()/clearBrokerSession(). Known remaining asymmetry: this fixes the direction with concrete evidence in the issue -- state written while CLAUDE_PLUGIN_DATA was unset, later missed by a lookup that has it set. The reverse isn't fixable this way: an unset env var carries no trace of what value it previously held, so there's nothing to check beyond the always-known tmpdir fallback. Fixes openai#636
![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: 6b102b3d29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export function clearBrokerSession(cwd) { | ||
| const stateFile = resolveBrokerStateFile(cwd); | ||
| if (fs.existsSync(stateFile)) { | ||
| fs.unlinkSync(stateFile); | ||
| for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { | ||
| if (fs.existsSync(stateFile)) { | ||
| fs.unlinkSync(stateFile); |
There was a problem hiding this comment.
When both the plugin-data and fallback roots contain broker.json—a state that existing installations can have because the old lookup behavior spawned duplicate brokers—loadBrokerSession() returns only the first record, so SessionEnd tears down only that broker. This loop then deletes both records, leaving the second broker running with its only metadata removed; similarly, ensureBrokerSession() encountering a stale primary record can erase a live fallback record before spawning another duplicate. Clear only the selected record, or load and tear down every distinct candidate before deleting their files.
Useful? React with 👍 / 👎.
…turned Both call sites (handleSessionEnd, ensureBrokerSession) act on whatever loadBrokerSession() returns -- tearing that broker down and clearing its record -- but clearBrokerSession deleted every candidate root's broker.json, not just the one that was actually torn down. That's reachable in practice: it's precisely the root-split bug's own historical fallout, where the old lookup could leave a broker registered under one root while a duplicate got spawned under the other. Deleting both records on the next cleanup erases the untorn broker's only metadata, making it permanently untrackable instead of leaving a stale-but-discoverable file behind. Thanks to Codex Review for catching this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baa5ffdcfe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (fs.existsSync(stateFile)) { | ||
| return stateFile; |
There was a problem hiding this comment.
When both roots already contain state.json—a reachable legacy state after invocations alternated between having and lacking CLAUDE_PLUGIN_DATA—this returns the primary file immediately and never reads the fallback file. If the fallback index contains a running job absent from the primary index, /codex:status, result, and cancellation lookups still cannot see it, so the cross-root orphaning this change targets remains whenever both files exist; reconcile the candidate indexes rather than treating the first existing file as the complete state.
Useful? React with 👍 / 👎.
| for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { | ||
| if (fs.existsSync(stateFile)) { | ||
| fs.unlinkSync(stateFile); | ||
| return; |
There was a problem hiding this comment.
When the primary broker.json exists but is malformed and the fallback record is valid, loadBrokerSession() skips the parse failure and returns the fallback broker, but this loop deletes the malformed primary and returns. SessionEnd therefore shuts down the fallback broker while leaving its valid record behind, causing subsequent lookups to report a stopped endpoint. Fresh evidence in the revised code is that loadBrokerSession() skips unreadable candidates while clearBrokerSession() selects solely by file existence.
Useful? React with 👍 / 👎.
… with loadBrokerSession on malformed files Two more issues from Codex Review, both real: 1. loadState() returned only the first candidate state.json found, not merged. Unlike a broker session (at most one meaningful record, so 'first found' is correct), jobs are a growing collection -- a job started while CLAUDE_PLUGIN_DATA was set and a different job started while it was unset are both real and non-conflicting. Returning only the first root's job list silently hid whichever root wasn't picked, for every status/result/cancel lookup, any time both roots happened to have a state.json. Now merges jobs from every candidate, keeping the more recently updated copy if the same id somehow appears in more than one. 2. loadBrokerSession() skips a candidate it can't parse and moves on, so it can return a fallback session while a malformed primary file exists. clearBrokerSession() selected by existence alone, so it could delete the unrelated malformed primary while leaving the valid fallback record behind -- the one actually loaded and torn down by the caller. Both functions now share a single selectBrokerState() helper (exists AND parses), so they always agree on which candidate is the selected one. Verified the loadState() merge fix doesn't have a side effect on saveState()'s own previousJobs cleanup diff (its per-job file removal resolves paths against the current-root-only resolveJobFile(), so a job living in another root is a no-op there, not a deletion) -- confirmed empirically with a throwaway repro before concluding no further change was needed there. Thanks again to Codex Review.
|
Thanks, all three caught real issues -- fixed in the two follow-up commits:
Added regression tests for each, confirmed they fail against the prior code. Full suite: 98 passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf9384f53
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ...defaultState().config, | ||
| ...(primary.config ?? {}) | ||
| }, | ||
| jobs: [...jobsById.values()] |
There was a problem hiding this comment.
When both candidate roots contain state and a caller removes a fallback-origin job, such as cleanupSessionJobs() filtering it during SessionEnd, saveState() writes the filtered collection only to the current primary file while leaving the fallback file unchanged. The next loadState() merges that fallback record back into jobsById, so session cleanup and pruning cannot permanently remove cross-root jobs; status can continue reporting a terminated job as running, even after its log was deleted. The fallback state must be migrated/updated or otherwise record deletions before returning this merged collection.
Useful? React with 👍 / 👎.
…t the primary saveState() only ever wrote the new job list to the current primary root. A job that originated entirely in a different root (e.g. added while CLAUDE_PLUGIN_DATA was unset) and later gets filtered out -- cleanupSessionJobs() during SessionEnd loads the merged view, drops jobs for the ending session, and saves the remainder -- never actually disappeared: that other root's own state.json still held its own untouched copy, and the very next loadState() merged it right back in. A removed job could keep reporting as running indefinitely. saveState() now also prunes every other candidate root's own file down to the same retained job-id set (derived from this save's own merged previousJobs diff), so a deletion sticks everywhere. New and updated jobs are unaffected -- they still only ever get written to the primary root, exactly as before; this only ever removes. Also made the individual job-detail-file cleanup in the same loop candidate-aware (resolveJobFileCandidates instead of the primary-only resolveJobFile), for the same reason. Two of the existing tests had to seed their two-root fixtures via direct file writes instead of two independent saveState() calls -- every real caller (updateState()/cleanupSessionJobs()) always derives its job list from a prior loadState(), so seeding via two disjoint, non-full-list saveState() calls doesn't reflect any real call pattern, and (correctly, now) tripped this very fix's own deletion logic during test setup. Thanks again to Codex Review.
|
Right again, and this one was more severe than the earlier finding it followed from — merging reads across roots wasn't enough on its own, since Fixed: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e349f1c09b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const parsedCandidates = resolveStateDirCandidates(cwd) | ||
| .map((stateDir) => readStateFileIfValid(path.join(stateDir, STATE_FILE_NAME))) | ||
| .filter((parsed) => parsed != null); |
There was a problem hiding this comment.
When a session's jobs exist only in the fallback root and SessionEnd runs with CLAUDE_PLUGIN_DATA set, cleanupSessionJobs() in session-lifecycle-hook.mjs still checks only resolveStateFile() (the primary path) at lines 48-50 and returns before calling this candidate-aware loader. Consequently, the hook neither terminates nor removes those jobs, leaving the background processes and records orphaned despite the new cross-root lookup; the early existence check must also consider all candidate files or be removed.
Useful? React with 👍 / 👎.
| const [primary] = parsedCandidates; | ||
| return { | ||
| ...defaultState(), | ||
| ...primary, |
There was a problem hiding this comment.
When both roots contain state, only the current primary candidate supplies config, even though the fallback candidate may contain a later or previously authoritative stopReviewGate: true. This is reachable when an older plugin invocation created a primary file with the default disabled value and /codex:setup --enable-review-gate later ran without CLAUDE_PLUGIN_DATA; subsequent stop hooks with the variable set read the stale primary value and silently skip the explicitly enabled review gate. The configuration needs reconciliation or migration rather than unconditional primary selection.
Useful? React with 👍 / 👎.
1 participant
Fixes #636.
What's wrong
resolveStateDir()picks the state root fromCLAUDE_PLUGIN_DATAwhen set, falling back to$TMPDIR/codex-companionwhen it's absent — same workspace slug/hash either way, only the root differs. State written under one root (e.g. a broker registered while the var was unset) becomes invisible to any later lookup that resolves to the other root, since nothing checked both. For a broker specifically, that meansSessionEndnever finds it to shut down — it's orphaned permanently, and sinceensureBrokerSessionalso can't see it, the next session spawns a duplicate broker for the same workspace, compounding the leak. The same mechanism affects job/status state (state.json, individual job detail files), not just the broker.Fix
Reads now check every candidate root (current primary, then the tmpdir fallback), not just the current invocation's primary — writes are unchanged, still going to the primary root. Applied consistently to
loadState()(job list, status, config),readStoredJob()(individual job detail lookups), andloadBrokerSession()/clearBrokerSession().Known remaining asymmetry, called out honestly: this fixes the direction with concrete evidence in the issue — state written while
CLAUDE_PLUGIN_DATAwas unset, later missed by a lookup that has it set. The reverse (written while set, later looked up unset) isn't fixable this way: an unset env var carries no trace of what value it previously held, so there's nothing to check beyond the always-known tmpdir fallback. Closing that direction fully would need something like your suggested fix #1 (persist the resolved root, e.g. insidebroker.json) — happy to take a pass at that too if it's wanted, but wanted to keep this PR to the achievable, lower-risk fix first.Testing
Added regression tests for
loadState,readStoredJob, andloadBrokerSession/clearBrokerSessioneach finding/clearing state across the root split; confirmed all fail on unpatched code. Full suite: 95 passed, 0 regressions.