Uh oh!
There was an error while loading. Please reload this page.
fix: guard module-level state with RLock (closes #4) - #27
Conversation
Two threads racing install() could both observe _installed=False and both run the patch path, with the second wrapping the first. A later uninstall() would unwrap only one layer, leaving a recost wrapper in place even though _installed=False. Add a module-level RLock around install/uninstall/is_installed so the patched-method state cannot drift away from the _installed flag. Adds a regression test that reliably reproduced the wrapper-leak on the unfixed code. Refs #4
Two threads racing init() could both observe _handle=None and both proceed past the dispose-previous guard, with the first thread's handle orphaned (its timer thread + transport leak) once the second overwrites _handle. Add a module-level RLock around init() and RecostHandle.dispose() so _handle mutation and the install/uninstall calls they make stay atomic against each other. The lock nests safely with the interceptor's _install_lock — _init_lock is always acquired first, then _install_lock via the transitive install()/uninstall() call. Adds a regression test that reliably reproduced the orphan-handle leak on the unfixed code (17 of 80 handles orphaned in a typical run). Tests use RecostConfig(enabled=False) so init/dispose pairs do not spin up transport/timer threads — the lock invariant under test does not depend on those resources being live, and avoiding them keeps the test under one second instead of four minutes. Refs #4
Move `sys` / `threading` imports introduced by the new race-test classes to the top of their files (ruff E402) and rewrite the `config_factory` lambda as a `def` (ruff E731). No behavior change. Refs #4
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary
Closes#4.
Adds two module-level
RLocks to guard the singleton state sharedacross SDK lifecycle calls:
_install_lockinrecost/_interceptor.py— protectsinstall()/uninstall()/is_installed()so the patched-methodstate cannot drift away from the
_installedflag._init_lockinrecost/_init.py— protectsinit()andRecostHandle.dispose()so concurrent callers cannot orphanpreviously-installed handles.
The two locks are independent.
init()/dispose()always acquire_init_lockfirst, then_install_lockvia the transitiveinstall()/uninstall()call. There is no inverted ordering, sono deadlock potential.
Tests
tests/test_interceptor.py::TestInstallUninstallRace— 4 threads ×200 install/uninstall cycles, with
sys.setswitchinterval(0.000001)to force GIL yields. Pre-fix this reliably leaves a recost wrapper
on
urllib3.HTTPConnectionPool.urlopenafter the race; post-fixthe original is restored.
tests/test_init.py::TestInitDisposeRace— 4 threads × 20 initcycles with
RecostConfig(enabled=False)so the test exercises the_handlerace without paying for real transport/timer setup. Pre-fixthis reliably orphans handles (a typical run leaves 17 of 80 handles
undisposed); post-fix exactly one handle is active at any time.
Notes
sys.setswitchintervaltest discipline introducedby PR fix(aggregator): make Aggregator thread-safe (closes #1) #25 (issue Aggregator is not thread-safe — flush vs. ingest race #1).
RecostConfig(enabled=True)but the plan's own comment said "Disabled config avoids actually
starting transport / timer threads so the test stays fast and
isolated." With enabled=True each init/dispose pair takes ~3s, so 80
pairs exceeded the 30s per-thread join timeout and the test failed
for the wrong reason. Flipped to enabled=False to match the comment's
stated intent — test runs in 0.02s and still reproduces the race.
🤖 Generated with Claude Code