Repository navigation
Conversation
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: e2b0fa85eb
ℹ️ 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 (@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 (@codex) address that feedback".
| value = self._serializer(value) | ||
| f.write(json.dumps(value).encode("utf-8")) | ||
|
|
||
| os.replace(temporary_path, file_path) |
There was a problem hiding this comment.
On Windows, if another thread or process is still inside get() with the destination file open, os.replace can raise PermissionError because ordinary Python file handles do not share deletion. The exception is swallowed below and the temporary file is removed, so set() silently loses the update and leaves a stale cache entry; the new concurrency test finishes its read before allowing execution to reach this line and therefore does not exercise this overlap. Retry sharing violations or otherwise coordinate publication with readers on this supported platform.
Useful? React with 👍 / 👎.
| return None | ||
|
|
||
| paths = [os.path.join(self._dir, f) for f in os.listdir(self._dir)] | ||
| paths = [os.path.join(self._dir, f) for f in os.listdir(self._dir) if not f.startswith(_TEMP_PREFIX)] |
There was a problem hiding this comment.
If the process is killed or the host crashes after NamedTemporaryFile(delete=False) is created, the finally block cannot unlink it. Every later eviction filters out all paths with this prefix, and constructing a new cache performs no cleanup, so repeated interrupted writes accumulate partial or complete cache payloads outside max_size and can eventually fill a persistent cache volume. Distinguish or reap stale temporary files instead of excluding them forever.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
DiskCache.set()opens the live cache entry withwbbefore serialization. A serializer/JSON error therefore destroys the previous usable value, and a concurrent reader can see an incomplete gzip file and get a cache miss while a replacement is still being prepared. Concurrent eviction can also unlink an entry that another writer has not finished writing.Write each replacement to a unique temporary file in the cache directory, close the complete gzip stream, then publish it with
os.replace. Failed writes retain the existing entry and clean up the temporary file. Eviction ignores in-progress staging files. Publication retries Windows access/sharing violations up to five attempts with 150 ms total backoff, then retains the old value on failure. The first write per cache instance, and at most once an hour afterward, removes staging files older than 24 hours, including in unbounded caches. A write taking longer than a day may expire under this best-effort policy. Existing SHA-256 entry names, gzip/JSON contents, maximum-size behavior, and best-effort error handling are preserved.Validation (pinned Mise Python 3.14.7 / uv 0.12.10 / Ruff 0.15.21):
dec33a8, the four new regressions fail while the 11 existing disk-cache tests pass. The cases cover JSON encoding failure, a failing custom serializer, a reader during replacement, and eviction during a second pending write.ThreadPoolExecutorwith event-coordinated serializers. The original four tests use no mocks. Review follow-up additionally injects OS sharing errors to exercise retry success and bounded failure, checks permanent permission errors, and exits a real subprocess mid-write to test stale staging-file cleanup. A Windows-only test holds a real reader open during replacement; it has not been run on Windows here. The eviction test sets an explicit old mtime to avoid filesystem clock ties.nox -s test_core -- --vcr-record=none: 1,098 passed, 107 skipped, 12 xfailed. Optional provider packages remain absent as required by the session.make fixup: all repository hooks pass. Changed-file Pylint with the repository's--errors-onlysetting andgit diff --checkpass.No dependencies, credentials, cassettes, network APIs, or generated files changed. This branch is based independently on main and does not depend on #964.
AI assistance disclosure: Codex prepared the code, tests, and description and ran the checks above. No human code review or approval is claimed.