fix(core): survive a stale Windows log deleted mid-cleanup (#1211) - #1215
fix(core): survive a stale Windows log deleted mid-cleanup (#1211)#1215overgoy wants to merge 2 commits into
Conversation
…hines-co#1211) Windows log files are per-PID, so every launch prunes the same shared directory, and concurrent launches are the normal case for the Claude Code plugin: several editor sessions plus a session-start hook each start a process. The unlink in _cleanup_windows_log_files is guarded against another launch removing a file first; the stat in the sort key that precedes it was not, so FileNotFoundError escaped through setup_logging and ended the process during logging setup. When that process was the MCP server, the client saw the stdio connection close a couple of seconds after launch and the session silently had no basic-memory tools, while the bundled skills and slash commands went on instructing against tools that were not there. The sort key now treats a file that has already gone as oldest, which puts it in the delete slice, where unlink is already guarded against the same race. The test stands in for the racing launch by deleting the file during the stat it is about to be sorted by. Signed-off-by: overgoy <32526203+overgoy@users.noreply.github.com>
|
|
![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: a00200e624
ℹ️ 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".
|
|
||
| def stat_with_a_racing_deletion(self: Path, *args: Any, **kwargs: Any) -> Any: | ||
| # Stand in for another launch pruning the same shared directory between the glob and the stat. | ||
| if self == vanishing and vanishing.exists(): |
There was a problem hiding this comment.
On the supported Python 3.12 runtime, Path.exists() is implemented by calling self.stat(), so after this test monkeypatches utils.Path.stat, this condition recursively re-enters stat_with_a_racing_deletion instead of checking whether the file still exists. That makes the new regression test fail with RecursionError before it exercises the cleanup fix on Python 3.12; use the saved real_stat, os.path.exists, or a one-shot flag for the existence check instead.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
Path.exists() is implemented by calling stat(), which is the method the test patches, so the existence check inside the patched stat re-entered itself and the test failed with RecursionError before it exercised the fix. A one-shot flag deletes the file exactly once instead, touching the filesystem not at all during the stat. It passed locally on 3.14, where this does not recurse, and the project supports 3.12 upwards. Verified on 3.12 and 3.13. Signed-off-by: overgoy <32526203+overgoy@users.noreply.github.com>
overgoy
changed the title
|
Thanks — the recursion report is correct. Also retitled from |
|
Thank you, @overgoy — your production fix is now merged through #1218 as the distinct contributor-authored commit b712db8. Your authorship, DCO sign-off, and the original a00200e commit provenance were preserved. I also saw the follow-up test commit 6d8a45c you pushed while the replacement PR was already in CI. We had independently made the same one-shot-flag correction for the Path.exists() recursion; the merged fixture additionally keeps Path.is_file() on the real filesystem so the simulated deletion is guaranteed to occur during the sort-key stat and directly exercises the production guard. With the fix and regression coverage merged, I am closing this PR as superseded. Thanks again for finding and fixing the Windows logging startup race — and for responding quickly to the review feedback. |
3 participants
_cleanup_windows_log_filescan kill the process during logging setup.Windows log files are per-PID, so every launch prunes the same shared directory, and concurrent launches are the normal case with the Claude Code plugin: several editor sessions plus a session-start hook each start a process. The
unlinkis guarded against another launch removing a file first; thestatin the sort key that precedes it is not, soFileNotFoundErrorescapes throughsetup_logging. When that process is the MCP server, the client sees the stdio connection close a couple of seconds after launch and the session silently has no basic-memory tools, while the bundled skills and slash commands go on instructing against tools that are not there.The sort key now treats a file that has already gone as oldest, which puts it in the delete slice where
unlinkalready handles the same race. The regression test stands in for the racing launch by deleting the file during thestatit is about to be sorted by.Fixes #1211.
Tested.
tests/utils/passes (166 passed). The fulltests/run is 2407 passed, 32 skipped, with one failure intests/cli/test_ci_commands.py::test_setup_writes_workflow_config_and_prompt; that one fails identically on a clean checkout of the same commit in my environment (a missing external binary in a slim container) and is unrelated to this change.Disclosure: prepared with an AI coding assistant, per the collaborative workflow described in CONTRIBUTING.