fix(tracing): serialize concurrent trace finalization - #6713
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesTraceBatchManager now uses reentrant locking for nested batch finalization, with the finalization logic extracted into a lock-held helper. A concurrency test verifies that simultaneous calls send events and finalize the backend only once. Trace batch finalization
Sequence Diagram(s)sequenceDiagram
participant CallerThreads
participant TraceBatchManager
participant BackendFinalization
CallerThreads->>TraceBatchManager: call finalize_batch()
TraceBatchManager->>TraceBatchManager: acquire _finalize_lock
TraceBatchManager->>TraceBatchManager: run _finalize_batch()
TraceBatchManager->>BackendFinalization: finalize backend batch
BackendFinalization-->>TraceBatchManager: return finalization result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
AI assistance is disclosed in the PR body. GitHub does not grant this fork contributor permission to set upstream labels; could a maintainer please add the required |
cbbad69 to
3e0de95
Compare
Summary
TraceBatchManager.finalize_batch()performs an irreversible sequence — snapshotthe event buffer, send it, finalize the backend batch, then clear local state —
but only the backend-finalization step ran under a lock. Two overlapping callers
could therefore send the same event snapshot before either one cleared the
buffer.
This change holds
_finalize_lockacross the whole publicfinalize_batch()transaction. The lock becomes an
RLockrather than moving the existing one,because
finalize_batch()reaches_finalize_backend_batch(), which alreadytakes the lock, and because
FirstTimeTraceHandler.handle_execution_completion()calls
_finalize_backend_batch()directly and must keep its currentserialization.
Why the callers overlap
TraceCollectionListeneris a singleton (trace_listener.py:149-157) holding oneshared
TraceBatchManager. The event bus runs synchronous handlers on aThreadPoolExecutor(max_workers=10)(event_bus.py:179-182andevent_bus.py:633), with one submission per emitted event, so handlers for twodifferent events run on different threads. Concurrent crews therefore reach the
terminal handlers at
trace_listener.py:311,:317,:330and:336— each ofwhich calls
finalize_batch()— at the same time, against that single sharedmanager. A second finalizer now serializes behind the first, including its
existing backend I/O, instead of racing it.
Scope
This serializes overlapping
finalize_batch()callers. Two things itdeliberately does not do:
FirstTimeTraceHandler.handle_execution_completion(), whichcalls
_send_events_to_backend()outside the lock(
first_time_trace_handler.py:128), so that path can still overlap aconcurrent
finalize_batch(). Covering it means taking a batch-manager lockfrom the handler, which is a wider change than this fix needs, so I left it
out. Happy to follow up if you would like it included.
There is no public API or dependency change.
Reproduction
lib/crewai/tests/tracing/test_tracing.py::TestTraceListenerSetup::test_finalize_batch_sends_events_once_when_called_concurrentlyraces two public finalizers around a successful event send. With this patch
reverted it fails on every run (5/5):
The test captures worker exceptions, so a thread error cannot let it pass
silently. It is the public-transaction sibling of the existing
test_finalize_backend_batch_is_serialized, which already covers the private_finalize_backend_batch()step.Verification
lib/crewai/tests/is excluded from ruff and mypy inpyproject.toml, so thosethree checks run against the changed source file.
The new test also passed 12 out of 12 runs in isolation.
AI assistance disclosure
This PR was prepared with AI coding assistance and reviewed by the contributor
before submission. GitHub does not permit this fork contributor to apply labels
in the upstream repository, so maintainer assistance is needed to add the
required
llm-generatedlabel.