Skip to content

fix(tracing): serialize concurrent trace finalization - #6713

Open
CTWalk wants to merge 1 commit into
crewAIInc:mainfrom
CTWalk:fix/serialize-trace-finalization
Open

fix(tracing): serialize concurrent trace finalization#6713
CTWalk wants to merge 1 commit into
crewAIInc:mainfrom
CTWalk:fix/serialize-trace-finalization

Conversation

@CTWalk

@CTWalk CTWalk commented Jul 29, 2026

Copy link
Copy Markdown

Summary

TraceBatchManager.finalize_batch() performs an irreversible sequence — snapshot
the 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_lock across the whole public finalize_batch()
transaction. The lock becomes an RLock rather than moving the existing one,
because finalize_batch() reaches _finalize_backend_batch(), which already
takes the lock, and because FirstTimeTraceHandler.handle_execution_completion()
calls _finalize_backend_batch() directly and must keep its current
serialization.

Why the callers overlap

TraceCollectionListener is a singleton (trace_listener.py:149-157) holding one
shared TraceBatchManager. The event bus runs synchronous handlers on a
ThreadPoolExecutor(max_workers=10) (event_bus.py:179-182 and
event_bus.py:633), with one submission per emitted event, so handlers for two
different events run on different threads. Concurrent crews therefore reach the
terminal handlers at trace_listener.py:311, :317, :330 and :336 — each of
which calls finalize_batch() — at the same time, against that single shared
manager. 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 it
deliberately does not do:

  • It does not claim exactly-once delivery across an ambiguous network response.
  • It does not cover FirstTimeTraceHandler.handle_execution_completion(), which
    calls _send_events_to_backend() outside the lock
    (first_time_trace_handler.py:128), so that path can still overlap a
    concurrent finalize_batch(). Covering it means taking a batch-manager lock
    from 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_concurrently
races two public finalizers around a successful event send. With this patch
reverted it fails on every run (5/5):

AssertionError: Expected 'send_trace_events' to have been called once. Called 2 times.

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

uv run pytest lib/crewai/tests/tracing/test_tracing.py -q
# 40 passed, 1 skipped

uv run pytest lib/crewai/tests/test_flow_conversation.py -q
# 47 passed, 21 skipped

uv run ruff check lib/crewai/src/crewai/events/listeners/tracing/trace_batch_manager.py
uv run ruff format --check lib/crewai/src/crewai/events/listeners/tracing/trace_batch_manager.py
uv run mypy lib/crewai/src/crewai/events/listeners/tracing/trace_batch_manager.py
# clean

lib/crewai/tests/ is excluded from ruff and mypy in pyproject.toml, so those
three 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-generated label.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 442de1b7-8414-458a-80b2-fc55ce1886d4

📥 Commits

Reviewing files that changed from the base of the PR and between ceed4a3 and 3e0de95.

📒 Files selected for processing (2)
  • lib/crewai/src/crewai/events/listeners/tracing/trace_batch_manager.py
  • lib/crewai/tests/tracing/test_tracing.py

📝 Walkthrough

Walkthrough

Changes

TraceBatchManager 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

Layer / File(s) Summary
Reentrant finalization flow
lib/crewai/src/crewai/events/listeners/tracing/trace_batch_manager.py
finalize_batch() acquires an RLock and delegates to _finalize_batch(), allowing nested finalization under the same lock.
Concurrent finalization validation
lib/crewai/tests/tracing/test_tracing.py
A coordinated two-thread test verifies that concurrent finalization sends events and finalizes the trace batch exactly once without errors.

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
Loading

Suggested reviewers: thecybertech

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly reflects the main change: serializing concurrent trace finalization.
Description check ✅ Passed The description directly explains the tracing concurrency fix, scope, and verification.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@CTWalk

CTWalk commented Jul 29, 2026

Copy link
Copy Markdown
Author

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 llm-generated label?

@CTWalk
CTWalk force-pushed the fix/serialize-trace-finalization branch from cbbad69 to 3e0de95 Compare July 30, 2026 15:22
@CTWalk
CTWalk marked this pull request as ready for review July 30, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant