Repository navigation
fix(decorators): start no SWR refresh thread in a child forked without at-fork hooks (LAB-9129) - #573
Conversation
…t at-fork hooks A child forked from C (uWSGI without --py-call-uwsgi-fork-hooks, with the app loaded in the master) skips CPython's after-fork repair, so a thread started there can hang in Thread.start() or crash the interpreter. L1 cleanup, the invalidation listener and the metrics worker already start nothing there; the two sync SWR refresh dispatches still did. Both now check l1_cache._forked_without_hooks() on a stale hit, before they take a refresh slot: one getpid() on that path, none on a plain hit. The stale value is served until the entry expires, and the first read after that recomputes it. Nothing logs on the skipped path. The async paths schedule a task, not a thread, and are unchanged. The new test forks through libc's fork() in a fresh interpreter and records Thread.start() in the child: it asks for no refresh after a C fork and for both after os.fork(). The fresh-interpreter runner moves to tests/utils/fork_helpers.py, shared with the L1 cleanup C-fork test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Summary by CodeRabbit
WalkthroughThe synchronous wrapper now suppresses L1 and L2 stale-entry refresh threads in forked children without at-fork hooks. Those children serve stale values until hard expiry, then recompute on a later read. Fork tests compare this behaviour with ChangesSynchronous refresh after fork
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Forked children can serve stale values without starting refresh threads, then recompute after expiry. No merge-blocking issue was identified. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description provides detailed motivation, implementation, testing, and documentation context, but it does not follow the required template. It omits the Type of Change, Security Checklist, Testing… | Rewrite the description using the repository template. Add all required headings and complete the relevant checkboxes, including the bug-fix type, security checks, test status, backward-compatibility statement, and any additional reviewer n… |
| Docstring Coverage | |
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preventing synchronous SWR refresh threads in children forked without at-fork hooks. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description provides detailed motivation, implementation, testing, and documentation context, but it does not follow the required template. It omits the Type of Change, Security Checklist, Testing checklist, Backward Compatibility, and Additional Notes sections.
Resolution
Rewrite the description using the repository template. Add all required headings and complete the relevant checkboxes, including the bug-fix type, security checks, test status, backward-compatibility statement, and any additional reviewer notes.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
In a child forked from C, such as a uWSGI worker without
--py-call-uwsgi-fork-hookswith the app loaded in the master (uWSGI's default), cachekit already starts no L1 cleanup thread, no invalidation listener and no metrics worker, because such a fork skips CPython's after-fork repair and a thread started there can hang inThread.start()or crash the interpreter. The two sync stale-while-revalidate refresh dispatches still started one: an L1-only (backend=None) function's stale hit startedcachekit-swr-refresh, and an L2 SWR function's stale hit startedcachekit-swr-revalidate. SWR is on by default.Change
decorators/wrapper.pynow checkl1_cache._forked_without_hooks()on a stale hit, before they take a refresh slot or deep-copy the arguments. That is onegetpid()on a stale hit that would refresh; the plain hit path is unchanged. In such a child the stale value is served until the entry expires (L1-only) or hard-expires (L2stale_ttl), and the first read after that recomputes it. Nothing logs on the skipped path, because the same fork skipslogging's at-fork lock reset.ObjectCache.get_with_swrset is left in place, so later stale hits on that entry ask for no refresh and pay nogetpid(). The marker goes with the entry throughremove(), so the cache's marker invariant (a marker always matches the live entry's generation) holds.asyncio.create_task, not a thread, and are unchanged.Tests
tests/unit/test_swr_decorator.py::TestSWRForkIsolation::test_child_forked_without_hooks_serves_stale_and_starts_no_refresh, parametrized over libc'sfork()(throughctypes.PyDLL(None).fork) andos.fork(). A fresh interpreter warms an L1-only and an L2 SWR function, ends its other threads and forks. The child recordsThread.start()instead of starting, makes both entries stale, and calls each once. After the C fork it asks for no thread, serves both stale values without recomputing them, and logs nothing at WARNING or above. Afteros.fork()it asks for both refreshes.['cachekit-swr-refresh']and['cachekit-swr-revalidate']respectively). Theos.fork()case fails if either gate fires in a child the at-fork hooks reached.run_fresh,print_outcome) moves totests/utils/fork_helpers.pyand is shared with the L1 cleanup C-fork test intests/unit/test_l1_memory_bounds.py.os.fork()) at 20 concurrent runs. Unit, critical, docs, doctest and markdown-docs suites pass on CPython 3.14.7, and the fork-related suites on 3.12.12.Docs:
docs/free-threading.md(the SWR andObjectCacherows),docs/features/l1-invalidation.md(the uWSGI and L1-only fork paragraphs) and theObjectCachedocstring. Companion docs-site change: cachekit-io/docs#174, to merge once this ships in a release.Closes LAB-9129