test: cover startResponseReader serialization against concurrent buffer reads

[David Levy (dlevy-msft-sql)]

Summary

Adds the regression test for startResponseReader serialization. The production fix landed separately in #359, so this PR is test-only.

Background

startReading() spawns a processSingleResponse goroutine that reads from sess.buf. If the caller does not fully drain the rows before executing the next statement, a second goroutine reads the same buffer concurrently. Both write tdsBuffer.rpos and tdsBuffer.rsize without synchronisation, which go test -race reports as a data race.

The fix is the readDone chan struct{} field on tdsSession: each processSingleResponse goroutine closes its channel on exit, and startResponseReader waits on the previous one before spawning a new goroutine. That shipped in 65e137f (#359, "make readCancelConfirmation respect context cancellation") and is already on main — it is the only commit on main that touches readDone. This PR adds the coverage that was missing.

What the test does

TestStartResponseReaderSerializes holds the first reader inside Read using a blocking transport, starts a second startResponseReader in a goroutine, and asserts it has not returned. It then unblocks the first reader and asserts the second proceeds.

Verification

Mutation-tested rather than assumed. With the wait disabled in token.go:

if sess.readDone != nil && false {
	<-sess.readDone
}

the test fails as intended:

    token_test.go:537: second startResponseReader returned before first completed
--- FAIL: TestStartResponseReaderSerializes (0.11s)

Restoring the wait returns it to green over -count=5.

On the time.Sleep(100 * time.Millisecond): it guards a negative assertion — proving the second call has not returned — which cannot be made fully deterministic without a hook into the blocked receive. I measured how much of it is load-bearing. With the wait disabled and the sleep cut to 1 * time.Microsecond, the regression is still caught. The 100ms is headroom, not a threshold the result depends on.

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.18%. Comparing base (1f52296) to head (dd73ddf).

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #357       +/-   ##
===========================================
+ Coverage   77.91%   97.18%   +19.26%     
===========================================
  Files          35       93       +58     
  Lines        7222    74687    +67465     
===========================================
+ Hits         5627    72582    +66955     
- Misses       1332     2068      +736     
+ Partials      263       37      -226     

Flag	Coverage Δ
unittests	97.14% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 74 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR addresses a go test -race data race in the TDS response-reading path by ensuring only one processSingleResponse goroutine can read from a session’s shared tdsBuffer at a time (fixes #171).

Changes:

• Add readDone chan struct{} to tdsSession to track completion of the active response-reader goroutine.
• Introduce (*tdsSession).startResponseReader(...) to wait for the previous reader to exit before starting the next one, and use it from both startReading() and the cancellation retry path in nextToken().
• Add a unit test intended to validate the new serialization behavior.

Reviewed changes

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

File	Description
tds.go	Adds tdsSession.readDone used as a completion gate for response readers.
token.go	Centralizes spawning of processSingleResponse via startResponseReader, serializing readers to prevent concurrent buffer reads.
token_test.go	Adds a test for response-reader serialization behavior.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

🟢 Approval recommended

The change is test-only, compiles cleanly in-context, and the new test’s synchronization strategy is consistent with the intended serialization guarantee.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The diff is test-only, matches the PR description, and the added test appears safe and self-contained without weakening existing coverage.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The focused test-only change adds coverage with no unresolved issues.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The test does not directly prove that the second transport read waits, leaving concurrent buffer access insufficiently guarded.

Review details

Suppressed comments (1)

token_test.go:527

• This assertion only observes when the second startResponseReader call returns; it never observes when its processSingleResponse goroutine enters Read. An implementation that launches the second reader before waiting on the previous readDone, but delays returning until after that wait, would still pass this test while allowing concurrent tdsBuffer reads. Use the existing readEntered signal (the first signal has already been consumed) to fail if a second transport read starts before closeUnblock().

	go func() {
		close(goroutineStarted)
		sess.startResponseReader(context.Background(), ch2, outputs{})
		secondStarted.Store(1)

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The focused test changes provide the intended concurrency regression coverage.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

Focused test-only changes with no unresolved blocking issues.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite