fix(sentry): guard runtime context lifecycle per coroutine - #1085
fix(sentry): guard runtime context lifecycle per coroutine#1085huangdijia wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![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: adb7a6b2dd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (\Hyperf\Engine\Coroutine::id() <= 0) { | ||
| return; |
There was a problem hiding this comment.
When SentrySdk::startContext() is called from a console, CLI, or other path where Coroutine::id() is -1, this return prevents the promised isolated runtime context from being created. Subsequent hub or scope changes therefore mutate the global fallback, and endContext() becomes a no-op instead of flushing and removing that context. The process-level store can still be cleaned normally when callers pair startContext() with endContext(); callers omitting the latter should not cause all non-coroutine lifecycle calls to lose isolation.
Useful? React with 👍 / 👎.
| $channelA->push('started'); | ||
| $channelA->pop(); // Wait for the "end A" signal. |
There was a problem hiding this comment.
Because each channel has capacity one, coroutine A immediately consumes its own started value in the following pop() instead of waiting for the parent, and coroutine B does the same. Both child coroutines can therefore finish before the parent reaches lines 92–93, so this test never has two active contexts concurrently and cannot detect the cross-coroutine regression it claims to cover. Use separate command/acknowledgement channels or otherwise ensure only the parent consumes the started signal.
AGENTS.md reference: AGENTS.md:L40-L42
Useful? React with 👍 / 👎.
1 participant
问题
startContext()在主协程（非协程环境，Hyperf\Engine\Coroutine::id()返回 -1）执行时，条目会写入进程级 context 存储；若endContext()未执行则永久残留，存在内存溢出/条目残留风险。getExecutionContextKey()返回固定常量PROCESS_EXECUTION_CONTEXT_KEY（sentry.process.execution_context），key 不含协程信息，存在跨协程/主协程映射残留风险。修改
startContext()开头增加守卫：当前不在协程环境（\Hyperf\Engine\Coroutine::id() <= 0）时直接return。主协程的 context 存储是进程级，start 后无法随协程回收，回退 global context 更安全。getExecutionContextKey()改为包含协程 id：sprintf('%s.%d', self::PROCESS_EXECUTION_CONTEXT_KEY, \Hyperf\Engine\Coroutine::id())。key 按协程隔离，防止跨协程/主协程映射残留；CoArrayObject本就按当前协程取存储，正常行为不变。startContext()仅在协程内生效，非协程环境使用 global fallback。createHubFromBaseHub、flush、removeContextById、generateRuntimeContextId、global context 逻辑）一律未改动。测试
新增
tests/Sentry/RuntimeContextLifecycleTest.php（直接new RuntimeContextManager+ mockHubInterface/ClientInterface，避免容器 make；class_map 注入仅在 Hyperf 运行时生效，测试进程不生效，故测试文件显式加载该 class_map 文件以验证被测实现）：startContext()后hasActiveContext()为 true，endContext()后为 false；#[NonCoroutine]，非Swoole\Coroutine\run内）startContext()后hasActiveContext()为 false，getCurrentContext()回退 global；startContext()，getCurrentContext()->getId()不同且互不影响（一个endContext()后另一个hasActiveContext()仍为 true）；startContext()幂等，endContext()一次即清理。验证
vendor/bin/pest tests/Sentry：47 passed（基线 43 + 新增 4），覆盖本组件全部 sentry 测试（tests/Pest.php中uses()->group('sentry')->in('Sentry')，该目录全部用例即 sentry group 成员）。注：本开发 worktree 的vendor为指向主 checkout 的符号链接，vendor/bin/pest --group=sentry会在收集阶段先后加载主 checkout 与 worktree 两份tests/Helpers/HelpersTest.php（顶层均声明enum TestEnum），纯 main 无任何改动时同样触发该 fatal，属环境问题，故以完整 sentry 测试目录运行验证。vendor/bin/php-cs-fixer fix --dry-run --diff：0 个文件需修复。git diff --check：通过。composer analyse src/sentry：No errors。