fix: preserve primary error across join map cancellation - #27614
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Two blocking correctness issues remain on this exact head.
-
pkg/vm/message/joinMapMsg.go:663 unconditionally returns context.Cause(ctx). This bypasses the query-cancellation classification in normalizeScopeRunError. A reachable query context created with context.WithTimeoutCause has Err() == DeadlineExceeded but Cause() == its diagnostic MO error. With a real operator blocked in ReceiveJoinMap until that deadline, this head returns only "internal error: internal executor exec"; DeadlineExceeded is absent. The identical end-to-end test passes on base a5877b6. The existing compile contract explicitly preserves DeadlineExceeded so callers can classify timeout first and attach the diagnostic cause afterward. Keep query deadlines as DeadlineExceeded; only promote the pipeline-local sibling cause after distinguishing query cancellation.
-
The patch only fixes the ctxDone winner and therefore does not close issue #27524. MessageReceiver checks queued messages before ctx.Done(). If a canceled HashBuild first publishes NewJoinMapBuildErrorResult(context.Canceled), ReceiveJoinMap consumes that terminal message and returns "internal error: context canceled" without entering the changed branch, even when the pipeline cancel cause is the FK violation. I reproduced this deterministically by queueing the canceled BuildError under a context canceled with NewErrFKNoReferencedRow2. The expected FK error is not in the returned chain. This ordering is reachable through HashBuild.finalizeBuildFailure -> publishBuildError, and the issue log at joinMapDependency.go:57 shows that exact context-canceled snapshot path.
Please handle cancellation precedence at the common producer/aggregation boundary for both message-first and ctx-first orderings, while preserving query deadline classification. Add deterministic tests for both interleavings plus the timeout control, and public multi-CN/Proxy regression evidence for the SQL-visible FK error contract.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 378294be52619a1c8d90f2d129414a0e929b5d24 against base a5877b6126c949e6c9f9e71a899d08e229cf7476. Two concrete error-precedence paths remain incorrect.
-
The new ctx-done branch unconditionally returns
context.Cause(ctx). Forcontext.WithTimeoutCause,ctx.Err()isDeadlineExceededwhile the cause is diagnostic detail. Returning the cause here bypassesnormalizeScopeRunError, whose explicit contract preservesDeadlineExceededso callers can classify query timeout. A join-map consumer blocked until a query deadline now returns only the diagnostic MO error. Preserve deadline classification and promote a pipeline-local sibling cause only after distinguishing query cancellation. -
MessageReceiver.ReceiveMessagechecks queued messages beforectx.Done(). If canceled HashBuild publishesNewJoinMapBuildErrorResult(context.Canceled)first,ReceiveJoinMapResultreturns that terminal result and never executes the changed ctx-done branch;ReceiveJoinMapthen returns generic context-canceled even when the pipeline cancellation cause is the FK violation. This is the producer/message-first interleaving from the issue path.
Resolve context-shaped dependency failures against the pipeline cause at a boundary shared by both orderings, without replacing independent operator errors or query deadlines. Add deterministic ctx-first and message-first tests plus a WithTimeoutCause control; the current test covers only ctx-first cancellation.
matrix-meow
added
size/M
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head ed5ed4d. The restored unit matrix closes the prior white-box coverage gap and passes under repeated race testing, but the distributed regression still does not exercise the reported cancellation race. Requesting changes for the inline blocker.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 0404ace, including the full review/thread history and the increment since ed5ed4d. The distributed regression now restores the issue's late-failure shape: 10,000 rows, only the final FK missing, five consecutive attempts with FK-specific errors and zero-row atomicity checks, followed by a successful valid FK insert on the same connection. This closes my prior blocker. The complete production diff remains coherent across ctx-first/message-first cancellation, joined causes, QueryInterrupted, query deadlines, and independent producer deadlines. Validation passed: affected package list/build/vet/tests for message, hashbuild, hashjoin, and compile; 20 race repetitions of the cancellation/deadline/concurrent message matrix; diff check; and the exact-head multi-CN Proxy BVT check is green.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 0404acef886339ae603ba410fdccab3e32e67eb9. The cancellation precedence is now applied at the shared JoinMap receive boundary, so context-first and queued-message-first outcomes agree without mutating the immutable broadcast result. Substantive sibling causes survive cancellation-shaped fallout, while query and independent producer deadlines remain classifiable as DeadlineExceeded; joined and QueryInterrupted forms are covered. The concurrent-consumer test closes the publication/waiter lifecycle, and the repeated 10,000-row late-FK BVT verifies the SQL-visible error, atomic rollback, and same-connection recovery. Exact-head CI is green; no blocking correctness, lifecycle, or boundedness finding remains.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 0404ace. The change preserves the primary join-map failure while using cancellation errors only as fallback; success, cancellation, concurrent error reporting, and cleanup paths remain coherent. CI is green. No blocking correctness, liveness, resource-lifecycle, compatibility, or performance issue found.
Merge Queue Status
This pull request spent 26 minutes 19 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #27614 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27524
What this PR does / why we need it: