Skip to content

VTGate: fix warming reads timeout context#19674

Merged
timvaillancourt merged 11 commits into
vitessio:mainfrom
timvaillancourt:fix-warming-reads-timeout
Mar 27, 2026
Merged

VTGate: fix warming reads timeout context#19674
timvaillancourt merged 11 commits into
vitessio:mainfrom
timvaillancourt:fix-warming-reads-timeout

Conversation

@timvaillancourt
Copy link
Copy Markdown
Contributor

@timvaillancourt timvaillancourt commented Mar 19, 2026

Description

Fixes the warming reads timeout context so it actually works as intended.

CloneForReplicaWarming() was creating a context.WithTimeout using the --warming-reads-query-timeout flag value, but:

  1. The cancel func was discarded (suppressed with //nolint), leaking the timeout goroutine
  2. The timeout started from clone creation, not goroutine execution — the warming read could time out before it even started if the goroutine was delayed
  3. The warming goroutine passed the original request ctx to findRoute() and ExecuteMultiShard(), so when the primary query completed and its context was cancelled, the warming read got a KILL QUERY

The dedicated --warming-reads-query-timeout was effectively unused since the introduction of warming reads in v16.

This PR introduces a WarmingReadsContext() method on the VCursor interface that creates the timeout context inside the warming goroutine using context.WithoutCancel(ctx) as the base (preserves tracing spans and caller IDs while detaching from parent cancellation). The clone is also moved inside the select case to avoid unnecessary work when the channel is full.

Test coverage includes:

  • Warming read context has its own deadline, independent of parent
  • Parent context cancellation does not cancel the warming read
  • findRoute() receives the warming context (via ResolveDestinations hook)
  • Warming read is dropped when the channel is full
  • Warming goroutine exits on context timeout with DeadlineExceeded

The justification for backports is this timeout is not working as designed, since the introduction of warming reads in v16. cc @olyazavr

Related Issue(s)

Resolves: #19673
Related: #19656

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

N/A

AI Disclosure

Claude Code assisted with development and testing. Claude Code prepared this PR summary

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@timvaillancourt timvaillancourt self-assigned this Mar 19, 2026
@github-actions github-actions Bot added this to the v24.0.0 milestone Mar 19, 2026
@vitess-bot vitess-bot Bot added NeedsWebsiteDocsUpdate What it says NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Mar 19, 2026
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot Bot commented Mar 19, 2026

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@timvaillancourt timvaillancourt changed the title VTTablet: fix warming read timeout context VTGate: fix warming reads timeout context Mar 19, 2026
@timvaillancourt timvaillancourt removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Mar 19, 2026
@timvaillancourt
Copy link
Copy Markdown
Contributor Author

@copilot please provide a review of this draft

@timvaillancourt timvaillancourt marked this pull request as ready for review March 19, 2026 13:41
Copilot AI review requested due to automatic review settings March 19, 2026 13:41
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes VTGate replica warming reads so --warming-reads-query-timeout actually bounds the warming queries by propagating the timeout context into the warming execution path.

Changes:

  • Updated VCursor.CloneForReplicaWarming() to return both a cloned VCursor and the context to use for the warming query.
  • Switched the warming read goroutine to use the returned context for routing and execution.
  • Added/updated unit coverage to assert the warming read executes with a deadline-bearing context.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
go/vt/vtgate/executorcontext/vcursor_impl.go Changes CloneForReplicaWarming to return the timeout context alongside the cloned VCursor.
go/vt/vtgate/engine/route.go Uses the returned warming context for findRoute and ExecuteMultiShard.
go/vt/vtgate/engine/primitive.go Updates VCursor interface signature for CloneForReplicaWarming.
go/vt/vtgate/engine/route_warming_reads_test.go Extends test VCursor to return a timeout context and asserts a deadline is present.
go/vt/vtgate/engine/fake_vcursor_test.go Adjusts test VCursor implementations to match the new interface signature.
Comments suppressed due to low confidence (1)

go/vt/vtgate/executorcontext/vcursor_impl.go:306

  • CloneForReplicaWarming creates a context.WithTimeout but discards the returned cancel func. Now that this context is used to run the actual warming query, failing to call cancel will keep the timer/resources around until the timeout fires (and if the warming read is dropped, it still allocates a timer). Consider returning a context.CancelFunc (or encapsulating cancellation in the returned VCursor) and ensuring callers defer cancel() when done; also guard WarmingReadsTimeout <= 0 if "no timeout" is intended (since WithTimeout(..., 0) is immediately expired).
func (vc *VCursorImpl) CloneForReplicaWarming(ctx context.Context) (engine.VCursor, context.Context) {
	callerId := callerid.EffectiveCallerIDFromContext(ctx)
	immediateCallerId := callerid.ImmediateCallerIDFromContext(ctx)

	timedCtx, _ := context.WithTimeout(context.Background(), vc.config.WarmingReadsTimeout) //nolint
	clonedCtx := callerid.NewContext(timedCtx, callerId, immediateCallerId)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread go/vt/vtgate/engine/route.go Outdated
Comment thread go/vt/vtgate/engine/primitive.go Outdated
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copilot AI review requested due to automatic review settings March 27, 2026 14:02
@timvaillancourt timvaillancourt enabled auto-merge (squash) March 27, 2026 14:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/vt/vtgate/engine/route_warming_reads_test.go Outdated
Comment thread go/vt/vtgate/engine/route_warming_reads_test.go
Comment thread go/vt/vtgate/executorcontext/vcursor_impl.go Outdated
Comment thread go/vt/vtgate/engine/primitive.go
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copilot AI review requested due to automatic review settings March 27, 2026 14:08
timvaillancourt and others added 2 commits March 27, 2026 15:08
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/vt/vtgate/engine/primitive.go
Comment thread go/vt/vtgate/executorcontext/vcursor_impl.go
Comment thread go/vt/vtgate/engine/route_warming_reads_test.go
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/vt/vtgate/engine/route_warming_reads_test.go Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copilot AI review requested due to automatic review settings March 27, 2026 14:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 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.

@timvaillancourt timvaillancourt merged commit 8c937df into vitessio:main Mar 27, 2026
172 of 174 checks passed
@timvaillancourt timvaillancourt deleted the fix-warming-reads-timeout branch March 27, 2026 15:42
timvaillancourt added a commit that referenced this pull request Mar 27, 2026
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
timvaillancourt added a commit that referenced this pull request Mar 27, 2026
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
timvaillancourt added a commit that referenced this pull request Mar 28, 2026
…9727)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com>
timvaillancourt added a commit that referenced this pull request Mar 28, 2026
…9728)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com>
mhamza15 pushed a commit to planetscale/vitess that referenced this pull request Apr 9, 2026
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
timvaillancourt added a commit to timvaillancourt/vitess that referenced this pull request May 12, 2026
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug Report: warming-reads-query-timeout does not bound warming read queries

6 participants