[Backport stable-26-2] PR #49452: [issue-#49169] (-) Make prev-node tracing state lock-free in TExecutionContext - #50489
Conversation
Run Extra TestsRun additional tests for this PR. You can customize:
|
lberserq
changed the title
|
Conflict resolved: dropped the The C++ part of the backport is byte-identical to the original commit 709ca02; all APIs it relies on ( |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
AI Review SummaryVerdict: ✅ No critical issues found Critical issuesNo critical issues found. Other findings
Detailed analysisThe PR correctly addresses the SIGSEGV crash (issue #49169) caused by concurrent Correctness of the atomic approach:
Behavioral equivalence:
Callers of GetPrevCategoryName outside the execution loop:
Remaining NO_SANITIZE_THREAD:
The backport is clean; the conflict with This review was generated automatically. Critical issues require attention; other findings are advisory. |
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
| std::atomic<TPrevNodeState> PrevNode = {}; | ||
|
|
||
| TString RenderCategoryName(const TPrevNodeState& state) const { | ||
| if (!state.Defined) { |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
Stop() resets ProgramIterator and ExecutionVisitor but does not reset Program. This is intentionally needed so that RenderCategoryName can still resolve node IDs after execution stops (e.g., in OnEmptyStageData / BuildStageResult). However, this creates a lifetime asymmetry: Program outlives the rest of the execution state, and a second call to Start() would hit AFL_VERIFY(!Program).
Consider adding a brief comment in Stop() explaining why Program is deliberately kept alive, to prevent future maintainers from "cleaning it up" and breaking GetPrevCategoryName() post-Stop calls.
| } | ||
| AFL_VERIFY(Program); | ||
| auto it = Program->GetNodes().find(state.NodeId); | ||
| AFL_VERIFY(it != Program->GetNodes().end())("node_id", state.NodeId); |
There was a problem hiding this comment.
Severity: Minor
Confidence: Low
RenderCategoryName reads the non-atomic StartCategoryName field. This is safe because SetStartCategoryName is only called once during Start(), which completes before any Execute() can create a concurrent continuation. However, the safety invariant (that StartCategoryName is fully written before any concurrent read) is implicit -- it depends on the call order in DoExecuteInplace and is not enforced by the atomic protocol itself.
This is acceptable for the current code structure but worth noting as a maintenance concern.
| StartCategoryName = std::move(name); | ||
| } | ||
|
|
||
| void SetPrevNodeTracing(const ui32 nodeId, const TConclusion<NArrow::NSSA::IResourceProcessor::EExecutionResult>& conclusion) { |
There was a problem hiding this comment.
Severity: Nit
Confidence: High
SetStartCategoryName accepts only TString&& (rvalue reference). The call site SetStartCategoryName(step.GetPrevName()) works because GetPrevName() returns TString by value, but this rvalue-only signature is fragile -- if a future caller needs to pass an lvalue TString, they would need to add std::move() or a copy. Consider accepting const TString& with an internal copy, or provide both overloads, for a more conventional API.
2 participants
Changelog entry
[issue-#49169] (-) Make prev-node tracing state lock-free in TExecutionContext
Description for reviewers
Original PR(s)
Metadata
Conflicts Require Manual Resolution
This PR contains merge conflicts that require manual resolution.
Files with conflicts:
How to resolve conflicts:
After resolving conflicts:
[CONFLICT]if conflicts are resolved)Git Cherry-Pick Log
PR was created by cherry-pick workflow run