Repository navigation
fix(decorators): restore the stats context on every sync exit, interrupts included (LAB-8741) - #568
Conversation
…upts included sync_wrapper restored the function-stats ContextVar by hand on each exit, so a BaseException raised by the caller-thread backend I/O (the L2 read, backend resolution, the invalidation listener start) skipped every reset: KeyboardInterrupt, a worker timeout's SystemExit or gevent.Timeout left the var set in the caller's context, and later code there reported that function's stats. One try/finally now holds the only reset, as in async_wrapper, and the 18 hand resets are gone. The three try blocks that only reset and re-raise go with them; every other except clause is unchanged. On a degraded path (key generation, breaker rejection, client creation or the L2 read failed) the uncached function now runs with the var set, as it already does on the miss path and on every async path. The backend-init leak test patched get_backend_provider in cache_handler, a binding the wrapper never reads, so it never reached the provider. It now patches the wrapper's binding and asserts the provider was called.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
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 |
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}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…solate leak tests The comments on the L1 guard's explicit `except DecryptionAuthenticationError: raise`, sync and async, said it keeps a tamper raise reaching the caller even if a later edit wraps the read path in a broad `except Exception`. An outer handler would still catch the re-raise. The clause only keeps the raise ahead of an `except Exception` later added to the same inner try. Both comments now say that, and name the test that pins the raise. test_context_leak_regression.py gets an autouse fixture that starts each test with no stats context and restores it after, so one leaking test fails alone instead of failing every later None check in the same worker.
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:
|
…text The test ran the decorated call in a copied context because an interrupt left the decorator's stats context set. The sync wrapper now restores it on every exit, so the call runs directly and the test asserts the context is restored.
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:
|
1 participant
Summary
An interrupt that leaves a sync
@cachecall from the decorator's own backend I/O leaves the function-stats ContextVar set in the caller's context. That coversKeyboardInterrupt, a worker timeout'sSystemExitandgevent.Timeout, raised from the L2 read, backend resolution or the invalidation listener start.sync_wrapperrestored the var by hand on each exit (18reset_current_function_stats(token)calls). Only three sat in afinally, and theexcept Exceptionhandlers around the backend I/O never see aBaseException. A process that survives the interrupt (a gevent greenlet, a REPL or notebook after Ctrl-C) then keeps attributing stats to that function outside any decorated call. The CachekitIO backend's metrics headers are one reader of the var.sync_wrappernow matchesasync_wrapper: onetry/finallyopens straight afterset_current_function_stats, and itsfinallyholds the only reset.ContextVar.resetraisesRuntimeErroron a token already used, so the hand resets had to go rather than sit beside afinally.Review with whitespace hidden: most of the diff is the body moving one level in.
What changed
wrapper.py,sync_wrapper. The body moves under onetry/finally. All 17 resets outside the miss path'sfinallyare deleted, along with the twotry/finallyblocks that only reset. Threetry: … except Exception: raisewrappers existed only to reset before re-raising, around_l2_scope()and the two interop checks; with the reset gone they are no-ops, so they are removed.async_wrappercalls the same three bare. Every otherexceptclause is unchanged, including the fail-closed re-raises (DecryptionAuthenticationError,KeyringConfigurationError,InteropError,UnsupportedTenantError). The L1 guard's explicitexcept DecryptionAuthenticationError: raisestays, as it does inasync_wrapper. Its comment, and the async twin's, now state exactly what it guards: it keeps the tamper raise ahead of anyexcept Exceptionlater added to that innertry. It cannot stop a broad handler wrapped around the read path from outside; the fail-closed L1 tests pin that. Comments that described the hand resets are removed or updated.test_context_reset_on_interrupt_from_backend_ioraisesKeyboardInterruptandSystemExitfrom the L2 read (backend.get) and from backend resolution (the provider'sget_backend). It runs in the test's own context, not a copied one. All 4 cases fail onmainand pass here.test_async_context_reset_on_interrupt_from_backend_resolutionis the async pin. The decorated coroutine is awaited directly, so it runs in the test's task, and the test passes onmaintoo. It raises from backend resolution, not the L2 read. A miss reads L2 inside its single-flight task, and asyncio re-raises an interrupt from a task out of the event loop, not into the awaiting caller.test_context_leak_regression.pyhas an autouse fixture that starts each test with no stats context and restores it after, so one leaking test fails alone instead of failing every laterNonecheck in the worker.test_an_interrupt_through_the_decorator_propagates_as_itself_and_spares_the_breaker(test_redis_error_frames.py) ran the decorated call incontextvars.copy_context().run(...)to contain this leak. It now calls directly and asserts the context is restored. It fails againstmain'swrapper.pyand passes here.test_context_reset_on_backend_init_failurepatchedcachekit.cache_handler.get_backend_provider, a binding the wrapper never reads (it imports the function), so the provider was never called. It now patchescachekit.decorators.wrapper.get_backend_provider, as the rest of the suite does, and asserts the provider was called.Verification
sync_wrapperagainstmainremoves the reset statements on both sides. With that done, every survivingexceptclause (14) is identical to itsmaincounterpart. The 3 removed clauses are eachexcept Exception: raisewith no sibling handlers. The whole function is identical once do-nothingtryblocks are inlined. A deliberately narrowedexcept (InteropError, KeyringConfigurationError)fails the check. It compares the parsed AST, not text, so a re-indentedraisecannot hide ingit diff -w.tests/unit+tests/critical, withmainmerged in: 6202 passed, 18 skipped, 1 xfailed.tests/suite against a local Redis, compared withmainon the same Redis: the only failures that differ are timing-based performance tests, which flake onmainas well. The tests that failed under-n 4pass when run serially.main→ this branch: L1-only hit 3787 → 3888 ns; backed L1 hit 14210 → 13333 ns. Both are within noise.ruff check,ruff format --checkandbasedpyrightare clean; basedpyright reports 38 warnings, the same count as onmain.No docs change: no documentation describes when the stats context is reset.