fix(tracing): stop set_tracing_processor_configs deadlocking on its own lock - #543
michaelxu2288 wants to merge 1 commit into
Conversation
…wn lock TracingProcessorManager.set_processor_configs takes self.lock and then calls add_processor_config for each config, which takes the same lock again. self.lock was a threading.Lock, which is not reentrant, so the first call to the exported set_tracing_processor_configs() blocked forever: on an ACP server it never finishes startup, on a worker it never reaches worker.run(). Use an RLock so the batch still registers under one lock acquisition and the nested add can re-enter it. add_processor_config is unchanged. The new test registers two configs from a thread and fails on the old lock (the thread is still blocked after 5 s); it passes with the RLock.
| return manager | ||
|
|
||
|
|
||
| def _finishes(target: Any, timeout: float = 5.0) -> bool: |
There was a problem hiding this comment.
_finishes uses 5.0 as an inline timeout. The repository requires magic numbers to be stored as class or instance variables with descriptive names. Name this deadline to satisfy that requirement before merging.
Rule Used: Store magic numbers as class or instance variables with descriptive names rather than using them inline in the code. (source)
Learned From
scaleapi/scaleapi#126388
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/lib/core/tracing/test_tracing_processor_manager.py
Line: 27
Comment:
**Test deadline has no name**
`_finishes` uses `5.0` as an inline timeout. The repository requires magic numbers to be stored as class or instance variables with descriptive names. Name this deadline to satisfy that requirement before merging.
**Rule Used:** Store magic numbers as class or instance variables with descriptive names rather than using them inline in the code. ([source](https://app.greptile.com/scale-ai/-/custom-context?memory=002e0051-41ad-46c1-9098-47433c580150))
**Learned From**
[scaleapi/scaleapi#126388](https://github.com/scaleapi/scaleapi/pull/126388)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| thread = threading.Thread(target=target, daemon=True) | ||
| thread.start() | ||
| thread.join(timeout) | ||
| return not thread.is_alive() |
There was a problem hiding this comment.
If registration raises in the worker thread, _finishes returns True because the thread has stopped. The test then fails on a later list check instead of showing the registration error. Capture the worker error and raise it after the join.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/lib/core/tracing/test_tracing_processor_manager.py
Line: 31
Comment:
**Worker errors look successful**
If registration raises in the worker thread, `_finishes` returns `True` because the thread has stopped. The test then fails on a later list check instead of showing the registration error. Capture the worker error and raise it after the join.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Problem
set_tracing_processor_configs()(exported fromagentex.lib.core.tracing.tracing_processor_manager) never returns.TracingProcessorManager.set_processor_configstakesself.lockand then callsadd_processor_configfor each config, which takes the same lock again.self.lockis athreading.Lock, which is not reentrant, so the first config blocks forever:Called at ACP startup, the server never finishes booting; called before a worker starts, it never reaches
worker.run(). Nothing inside the SDK calls the plural API today, which is how it went unnoticed, but it is exported next toadd_tracing_processor_config.Fix
self.lockbecomes athreading.RLock. The batch still registers under one lock acquisition and the nestedadd_processor_configcan re-enter it.add_processor_configitself is unchanged. Two-line diff.set_processor_configsappends to the registered processors, the same as callingadd_processor_configonce per config. I kept that behaviour rather than making it replace the existing list; happy to change that if "set" was meant literally.Verification
tests/lib/core/tracing/test_tracing_processor_manager.pyregisters two configs from a thread with a 5 s join.AssertionError: set_processor_configs blocked on the manager's own lock.uv run pytest -n 0 tests/lib/core/tracing/test_tracing_processor_manager.py tests/lib/core/tracing/test_span_queue.py tests/lib/core/tracing/processors: 89 passed.ruff checkandpyrightclean on both files.The lock fix appears sound, but the test must satisfy the repository’s named-number rule before merging.
Fix with agent prompt
Summary
The tracing processor manager now uses a reentrant lock so batch config registration can finish while it keeps registration protected.
Reviews (1) · Last reviewed commit: "fix(tracing): stop set_tracing_processor..."