Repository navigation
fix(session): release managed sidecar cache on Windows closeStrict to prevent handle leaks - #6568
Conversation
… prevent handle leaks Root cause: SessionManager.closeStrict() only conditionally released the managed sidecar cache (which holds directory handles to .gjc). On Windows, these unclosed handles prevented test cleanup from deleting temporary directories, causing EPERM/EBUSY errors on Windows CI after each test. Fix: Always release managed sidecar cache when closeStrict succeeds, ensuring file handles are released even when resident blob cleanup is skipped due to session state. The sidecar cache is auxiliary to session persistence and safe to release unconditionally when close succeeds. Reverts pointless retry backoff added in #6566, which was masking the underlying handle leak. With the real issue fixed, retries are no longer needed. Fixes #5321 (Windows task-cache-key test failures). Lore-id: fix-windows-task-cache-key-handle Constraint: must release sidecar handles before returning from closeStrict Tested: locally on Linux; awaiting Windows CI verification Scope-risk: narrow (SessionManager.closeStrict path only) Reversibility: safe (adds explicit cleanup)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 7d66fc56d7
ℹ️ 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".
| } else { | ||
| try { | ||
| await retryWithBackoff(() => fsPromises.rm(entryPath, { force: true }), 5, 10); | ||
| await fsPromises.rm(entryPath, { force: true }); |
There was a problem hiding this comment.
On Windows, the replacement cleanup cannot release the handle blamed by this change: #managedSidecarRoot() returns before creating #managedSidecarCacheStore when process.platform === "win32" (session-manager.ts:12991-12995), and any successful close satisfying the new guard has already called #releaseClosedSessionState() and therefore the same release method at lines 17251-17266. Consequently, removing the backoff here restores the original immediate EPERM/EBUSY failure mode in Windows CI without changing Windows resource ownership; retain the retries until the actual Windows handle owner is identified and closed.
Useful? React with 👍 / 👎.
|
@probepark @snowykr Root-cause follow-up to #6566: the retry backoff did not fix the Windows — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR adds sidecar-cache release handling to SessionManager.closeStrict() and removes retry backoff from the task-cache-key test cleanup. No actionable merge-blocking defect was demonstrated: the no-retry cleanup passed in the exact-head Windows CI run.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
The PR attributes the Windows handle leak to the managed sidecar cache, but #managedSidecarRoot() returns undefined on Windows (packages/coding-agent/src/session/session-manager.ts:12991-13003), so the new release call cannot release that cache there. On platforms where it is acquired, successful close already reaches #releaseManagedSidecarCache() through #releaseClosedSessionState() (16960-16978). The passing Windows test confirms cleanup succeeded at this head, but does not establish that the new release caused it. Keep the root-cause explanation aligned with the actual Windows handle owner if further failures occur.
CI / Verification
For reviewed head 7d66fc56d7ce72ee695bd10c869cab1fe19fdacc, the Windows dev:doctor/session-path regression job passed, including task-cache-key.test.ts with retries removed; affected-path validation and virtual integration validation also passed. Overall, 22 checks succeeded and 6 were skipped. No local tests were run.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The Windows sidecar-cache explanation conflicts with the existing platform guard; no merge-blocking contract violation was demonstrated. See non-blocking observation. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Close/dispose and retry paths were traced; no exact-head failure was established, and Windows cleanup passed without retries. |
| A3 — Security / Privacy / Trust | APPROVED |
No new authority boundary, untrusted path, or sensitive-data exposure was introduced. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head Windows regression and affected-path gates passed; no blocking verification defect. |
| A5 — Context / Compatibility / Platform | APPROVED |
Consumers and platform guards were traced; no persisted-format or packaging incompatibility was found. |
Limitations
The base revision’s Windows regression lane was skipped, so there is no comparable base result to establish whether this test previously failed or prove the new release mechanism caused the exact-head pass.
2 participants
Root cause: SessionManager.closeStrict() only conditionally released the managed sidecar cache (which holds directory handles to .gjc). On Windows, these unclosed handles prevented test cleanup from deleting temporary directories, causing EPERM/EBUSY errors on Windows CI.
Fix: Always release managed sidecar cache when closeStrict succeeds, ensuring file handles are released even when resident blob cleanup is skipped. The sidecar cache is auxiliary to session persistence and safe to release unconditionally when close succeeds.
Also removes pointless retry backoff from task-cache-key test that was masking this issue.
Related to #5321 (Windows task-cache-key test failures) and #6566 (retry backoff attempt).