fix(core): resolve race condition with concurrent tool spans#1034
Conversation
Fixed a race condition where tools running in parallel would overwrite each other's parentToolSpan in the shared systemContext. The fix passes parentToolSpan through execution options instead of systemContext, ensuring each tool receives its unique span. Backward compatibility is maintained by checking both options and systemContext.
🦋 Changeset detectedLatest commit: 84b10cb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughParent tool span propagation is moved from the shared systemContext to per-execution options (executionOptions.parentToolSpan), with fallbacks to systemContext for compatibility; subagent delegation and planning paths were updated and tests added to ensure distinct spans during concurrent tool/subagent execution. Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent
participant ToolA as Tool A
participant ToolB as Tool B
participant OTel as OpenTelemetry
Agent->>Agent: create per-call spans\nset executionOptions.parentToolSpan
Agent->>ToolA: call with executionOptions.parentToolSpan
Agent->>ToolB: call with executionOptions.parentToolSpan
par Concurrent Execution
ToolA->>OTel: start span "tool.execution:toolA"\n(parent = executionOptions.parentToolSpan)
ToolB->>OTel: start span "tool.execution:toolB"\n(parent = executionOptions.parentToolSpan)
ToolA->>ToolA: async work / await
ToolB->>ToolB: async work / await
end
ToolA->>Agent: return result
ToolB->>Agent: return result
Agent->>OTel: spans are isolated and correlated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/agent/subagent/index.ts`:
- Around line 764-766: The code currently casts options to any to extract
parentToolSpan; instead add parentToolSpan to the declared type for the options
parameter so you can read (options.parentToolSpan) without casting. Update the
options interface/type used by the function that calls createTool (the options
parameter in index.ts where parentToolSpan is read) to include parentToolSpan?:
Span | undefined, then remove the (options as any) cast and use const
parentToolSpan = options.parentToolSpan before calling createTool so the call
site remains fully typed.
In `@packages/core/src/planagent/plan-agent.ts`:
- Around line 773-775: The parentSpan assignment duplicates the same resolution
logic already stored in toolSpan (declared earlier on line ~748); replace the
inline re-derived expression ((executeOptions as any).parentToolSpan ...
operationContext.systemContext.get("parentToolSpan")) with the existing toolSpan
variable when setting parentSpan to avoid divergence and duplication, keeping
the type as Span | undefined (cast if necessary) and ensuring parentSpan uses
toolSpan as its source of truth.
🧹 Nitpick comments (4)
packages/core/src/planagent/plan-agent.ts (1)
748-750: Sameas anycast issue as flagged inplanning/index.ts.See the comment on
packages/core/src/planagent/planning/index.tslines 151-153 regarding theas anycast and the suggestion to extract a shared helper.packages/core/src/agent/concurrent-tool-spans.spec.ts (2)
100-106: Strengthen span assertions to validate actualSpanobjects.
expect(spanA).toBeDefined()passes for any truthy value. Since this test's purpose is to validate that distinct OpenTelemetry spans are created per tool, consider asserting on a knownSpaninterface method (e.g.,spanA.spanContext()returns a validSpanContext) rather than relying on an internal.nameproperty that may not exist on allSpanimplementations.Suggested improvement
expect(spanA).toBeDefined(); expect(spanB).toBeDefined(); // Spans shouldn't be the same expect(spanA).not.toBe(spanB); - expect((spanA as any).name).toContain("tool.execution:toolA"); - expect((spanB as any).name).toContain("tool.execution:toolB"); + // Verify these are real Span objects with distinct span contexts + const ctxA = spanA!.spanContext(); + const ctxB = spanB!.spanContext(); + expect(ctxA.spanId).toBeTruthy(); + expect(ctxB.spanId).toBeTruthy(); + expect(ctxA.spanId).not.toBe(ctxB.spanId);
27-30: Consider restoring mocks after each test.Adding
afterEach(() => vi.restoreAllMocks())prevents mock state from leaking if more tests are added to this file later.packages/core/src/planagent/planning/index.ts (1)
151-153:as anycast obscures runtime-added properties; addparentToolSpanto the type definition for clarity.The
parentToolSpanproperty is dynamically set at runtime (seeagent.ts:5057) and accessed in multiple places, but it's not declared inToolExecuteOptions. While the[key: string]: anyindex signature allows this, theas anycast obscures the intent. AddingparentToolSpan?: SpantoToolExecuteOptionswould make the type explicit and eliminate the cast.This pattern appears in at least six locations (
planning/index.ts,plan-agent.ts×2,retriever/tools/index.ts,agent.ts,subagent/index.ts), making it a good candidate for extraction into a shared helper function to avoid duplication.
|
Hey @klakpin , |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/agent/subagent/index.ts">
<violation number="1" location="packages/core/src/agent/subagent/index.ts:764">
P2: delegate_task uses the tool-creation parentToolSpan instead of the per-call execute options, so concurrent calls reuse a stale/undefined span and fall back to shared systemContext, reintroducing the race the PR aims to fix.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@omeraplak thanks for the quick feedback! When all jobs failed for the first time due to the GitHub issue initially I was really impressed by how small change could destroy everything.. I am never trusting GitHub workflows again |
2 participants
#1033
Fixed a race condition where tools running in parallel would overwrite each other's parentToolSpan in the shared systemContext. The fix passes parentToolSpan through execution options instead of systemContext, ensuring each tool receives its unique span. Backward compatibility is maintained by checking both options and systemContext.
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
When multiple tools are called the spans are propagated incorrectly - the underlying actions inside tool (for example subagent invocation) have parent span of the last invoked tool.
What is the new behavior?
Each tool correctly traces the parent span.
Summary by cubic
Fixes a race condition where parallel tools overwrote each other’s parent spans during tracing. Parent spans now flow through per-call execution options (with systemContext fallback) and propagate correctly to subagents and other tools.
Written for commit 84b10cb. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation