fix: context.Canceled propagated as non-retryable in execute() causing flaky TestBasicExampleQuery/ExecuteDataQuery#2115
fix: context.Canceled propagated as non-retryable in execute() causing flaky TestBasicExampleQuery/ExecuteDataQuery#2115asmyasnikov with Copilot wants to merge 8 commits into
Conversation
summaryInferred base version: v3.135.10 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2115 +/- ##
==========================================
- Coverage 75.16% 75.06% -0.11%
==========================================
Files 427 427
Lines 36974 36989 +15
==========================================
- Hits 27792 27764 -28
- Misses 7993 8026 +33
- Partials 1189 1199 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…estBasicExampleQuery/ExecuteDataQuery When a session dies between the ctx.Done() check and the newResult() call in execute(), the executeCtx gets cancelled via AfterFunc, causing newResult to return context.Canceled. Previously this was propagated as a non-retryable error. Now we detect this pattern (error is context.Canceled) and wrap it as retryable so the pool can retry with a fresh session. Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/f82324d6-e778-432b-a6f2-6a3f95686166 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR aims to make execute() treat a context.Canceled returned from newResult() as retryable, addressing a race where session cancellation between the ctx.Done() check and the first Recv() caused flaky query failures instead of a retry.
Changes:
- Update
internal/query/execute_query.goto wrapcontext.CanceledfromnewResult()as a retryable error. - Extend
CancelWhileExecuteininternal/query/execute_query_test.goto assert the returned error is marked retryable. - Add a changelog entry describing the retryability fix for query operations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/query/execute_query.go | Adds retryable wrapping for context.Canceled returned after newResult(). |
| internal/query/execute_query_test.go | Updates a cancellation test to assert retryable error classification. |
| CHANGELOG.md | Documents the race-condition fix in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
None.
The fix correctly addresses the race window between the ctx.Done() select (line 153) and the newResult call (line 164) in execute(). When a session dies in that window, AfterFunc cancels executeCtx, causing newResult to return context.Canceled — previously non-retryable, now correctly wrapped with xerrors.Retryable. The approach is consistent with the existing ctx.Done() select block above it, and ctx.Err() is guaranteed non-nil at that point since executeCtx cancellation is triggered exclusively by AfterFunc(ctx, executeCancel). The CloseOnContextCancel mechanism in streamResult.nextPart further ensures that context.Canceled propagates even when cancellation occurs mid-Recv().
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…race edge case Adds a test where Recv() returns context.Canceled directly while the parent ctx is still alive (user did NOT cancel). This is exactly the production race: session death cancels the merged session ctx which fires executeCancel, but the user context is untouched. Previously the code wrapped ctx.Err() (nil) which would produce an invalid retryable error; the current fix uses executeCtx.Err() with a fallback to err, which this test verifies correctly produces a retryable context.Canceled. Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/bcf414c2-20d4-4c82-93c0-86a7ab4d0605 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Review SummaryVerdict: ✅ No critical issues found Critical issuesNo critical issues found. Other findings
This review was generated automatically. Critical issues require attention; other findings are advisory. |
|
@copilot your new code in PR produce flaky test yet |
The CI failure showed a second race window: when the session-merged ctx (user ctx + session lifetime via xcontext.WithDone) is already cancelled before execute() calls c.ExecuteQuery, AfterFunc fires executeCancel immediately, and the balancer's nextConn returns context.Canceled from executeCtx.Err() check at balancer.go:475. The error propagated to execute_query.go:140 as non-retryable, causing pool.With to fail instead of retrying with a fresh session. Fix: apply the same ctx.Done() select check in the c.ExecuteQuery error path (mirroring the existing check at lines 153-160), so that if ctx is done when ExecuteQuery fails, we return xerrors.Retryable(ctx.Err()). Also add SessionDiesBeforeExecuteQuery test to cover the CI failure. Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/44276cc9-3763-4bf2-951d-9e136a57e4ed Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
... Fixed in commit The CI failure was a second race window: The fix applies the same |
🌋 SLO Test Results🔴 8 workload(s) tested — 1 workload(s) exceeded failure thresholds
Threshold violations: native-table-over-query-service:
native-bulk-upsert:
native-table:
database-sql-table:
Generated by ydb-slo-action |
Two race windows in
execute()wherecontext.Canceledwas returned as non-retryable, causingpool.Withto fail rather than retry with a fresh session.Race 1 —
c.ExecuteQueryfails before stream opens:Session.executemerges user context with session lifetime viaxcontext.WithDone(ctx, s.Done()). When a session dies, this merged ctx is immediately cancelled.AfterFunc(ctx, executeCancel)then fires beforec.ExecuteQueryruns, the balancer'snextConnseesexecuteCtx.Err() != niland returnscontext.Canceled, which was propagated non-retryably atexecute_query.go:140.Race 2 —
newResultfails after stream opens: After the non-blockingctx.Done()check passes, the session can die, firingAfterFunc(ctx, executeCancel)which cancelsexecuteCtx. The subsequentnewResult(executeCtx, ...)then fails withcontext.Canceled, which was also returned as non-retryable.Changes
internal/query/execute_query.go:c.ExecuteQueryerror path: addedselect { case <-ctx.Done(): return retryable ... }check (same pattern as the existing check that follows it), so that if ctx is done whenExecuteQueryfails, a retryable error is returned.newResultreturns an error: checkxerrors.Is(err, context.Canceled). If true, wrap asxerrors.Retryable(executeCtx.Err(), ...)(falling back toerrifexecuteCtx.Err()is nil).executeCtx.Err()is used instead ofctx.Err()becauseexecuteCtxis a separate derived context —errcan becontext.Canceledeven whenctx.Err()is still nil (e.g. session death without user cancellation).The outer retry loop runs on the user's context, so if only the session died the retry proceeds; if the user's ctx is also done the loop stops naturally.
internal/query/execute_query_test.go:CancelWhileExecute— now additionally assertsxerrors.IsRetryableError(err).SessionDiesBeforeFirstRecv_ParentCtxAlive— new test:Recv()returnscontext.Canceledwhile the parentctxis still alive, assertingctx.Err() == niland the error is retryable.SessionDiesBeforeExecuteQuery— new test: directly reproduces the CI failure where ctx is cancelled beforeExecuteQueryis called, asserting the returned error is retryable.