engine: fix race in concatenate

[vmg]

Description

Race in concatenate spotted by @GuptaManan100.

W0308 15:06:30.305024   15472 log.go:39] Failed to read in config : Config File "vtconfig" Not Found in "[/Users/manangupta/vitess/go/test/endtoend/vtgate/queries/union]". This is optional, and can be ignored if you are not using config files. For a detailed explanation, see https://github.com/vitessio/vitess/blob/main/doc/viper/viper.md#config-files.
panic: runtime error: index out of range [0] with length 0

goroutine 396 [running]:
vitess.io/vitess/go/vt/vtgate/engine.(*Concatenate).parallelStreamExec.func1(0x14000f29110, 0x30?)
	vitess.io/vitess/go/vt/vtgate/engine/concatenate.go:266 +0x2a8
vitess.io/vitess/go/vt/vtgate/engine.(*Concatenate).parallelStreamExec.func2.1(0x14000f29110)
	vitess.io/vitess/go/vt/vtgate/engine/concatenate.go:323 +0x18c
vitess.io/vitess/go/vt/vtgate/engine.(*Route).streamExecuteShards.func1(0x0?)
	vitess.io/vitess/go/vt/vtgate/engine/route.go:307 +0x150
vitess.io/vitess/go/vt/vttablet/queryservice.(*wrappedService).StreamExecute.func1.1(0x14000b20ec8?)
	vitess.io/vitess/go/vt/vttablet/queryservice/wrapped.go:198 +0x34
vitess.io/vitess/go/vt/vttablet/grpctabletconn.(*gRPCQueryClient).StreamExecute(0x14000271d00, {0x10639f388?, 0x14000f8b540?}, 0x14000ce13e0, {0x14000055a00, 0x28}, 0x14000ce3ad0, 0x0, 0x0, 0x14000d2f5f0, ...)
	vitess.io/vitess/go/vt/vttablet/grpctabletconn/conn.go:190 +0x168
vitess.io/vitess/go/vt/vttablet/queryservice.(*wrappedService).StreamExecute.func1({0x10639f388, 0x14000f8b540}, 0x14000ce13e0, {0x1063bf1d8, 0x14000271d00})
	vitess.io/vitess/go/vt/vttablet/queryservice/wrapped.go:196 +0xfc
vitess.io/vitess/go/vt/vtgate.(*TabletGateway).withRetry(0x140002b6690, {0x10639f388, 0x14000f8b540}, 0x14000ce13e0, {0x14000a04a98?, 0x10506a5c8?}, {0x40?, 0x10628f120?}, 0x0, 0x14000ce8940)
	vitess.io/vitess/go/vt/vtgate/tabletgateway.go:339 +0x420
vitess.io/vitess/go/vt/vttablet/queryservice.(*wrappedService).StreamExecute(0x1400079afd8, {0x10639f388, 0x14000f8b540}, 0x14000ce13e0, {0x14000055a00, 0x28}, 0x14000ce3ad0, 0x0, 0x0, 0x14000d2f5f0, ...)
	vitess.io/vitess/go/vt/vttablet/queryservice/wrapped.go:194 +0x118
vitess.io/vitess/go/vt/vtgate.(*ScatterConn).StreamExecuteMulti.func1(0x14000cd7f08, 0x1, 0x14000cdb1a0)
	vitess.io/vitess/go/vt/vtgate/scatter_conn.go:408 +0x1ec
vitess.io/vitess/go/vt/vtgate.(*ScatterConn).multiGoTransaction.func1(0x14000cd7f08, 0x1)
	vitess.io/vitess/go/vt/vtgate/scatter_conn.go:640 +0x138
vitess.io/vitess/go/vt/vtgate.(*ScatterConn).multiGoTransaction.func2(0x0?, 0x1400059cea0?)
	vitess.io/vitess/go/vt/vtgate/scatter_conn.go:668 +0x58
created by vitess.io/vitess/go/vt/vtgate.(*ScatterConn).multiGoTransaction in goroutine 451
	vitess.io/vitess/go/vt/vtgate/scatter_conn.go:666 +0x188

Upon review, we can see that there are two races in this helper. The first one is not locking the muFields mutex early enough: we want to lock as soon as we enter the function because a pause between the first if and the lock can cause a race. The second one is more obvious: the fieldTypes field needs to be written to with the lock being held.

The fixed code is slightly more linear but I don't think that's gonna be a performance regression, and either way it needs to be linear for correctness.

Related Issue(s)

• Fixes Bug Report: Panic in Concatenate Engine #15434

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

[vitess-bot]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.65%. Comparing base (d5bd597) to head (7cab524).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #15454   +/-   ##
=======================================
  Coverage   65.64%   65.65%           
=======================================
  Files        1563     1563           
  Lines      194389   194395    +6     
=======================================
+ Hits       127602   127622   +20     
+ Misses      66787    66773   -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[deepthi]

Why didn't y'all use the lock ... defer unlock pattern? That way you call unlock once at line 289 instead of 3 times in various places. And it's future-proof.
Nice work, btw 👍

[vmg]

That wouldn't be correct. The lock needs to be released before we call callback or we'd deadlock, so using a defer doesn't work in 2 of the cases.