Skip Attention packet when in rollback

[alhails]

We have noticed an issue when the context is cancelled during an open transaction. In this case, the driver would be requested to rollback (via code in database/sql)...

func (tx *Tx) awaitDone() {
	// Wait for either the transaction to be committed or rolled
	// back, or for the associated context to be closed.
	<-tx.ctx.Done()

	// Discard and close the connection used to ensure the
	// transaction is closed and the resources are released.  This
	// rollback does nothing if the transaction has already been
	// committed or rolled back.
	// Do not discard the connection if the connection knows
	// how to reset the session.
	discardConnection := !tx.keepConnOnRollback
	tx.rollback(discardConnection)
}

The driver will request the rollback and during the processing of the rollback response, it will currently send an Attention packet. If the rollback request is not yet complete (maybe the server is slow), then this may lead to the rollback being cancelled.

func (t tokenProcessor) nextToken() (tokenStruct, error) {
	...
	select {
	...
	case <-t.ctx.Done():
		if t.noAttn {
			return nil, t.ctx.Err()
		}
		t.sess.LogF(t.ctx, msdsn.LogDebug, "Sending attention to the server")
		if err := sendAttention(t.sess.buf); err != nil {
			// unable to send attention, current connection is bad
			// notify caller and close channel
			return nil, err
		}

This leads to an error from SQL...

The request failed to run because the batch is aborted, this can be caused by abort signal sent from client, or another request is running in the same session, which makes the session busy.

This means the transaction is left open in SQL leading to blocked queries etc.
If the transaction is subsequently requested to rollback, this will result in ErrTxDone as the transaction has already been marked as complete.

[alhails]

@microsoft-github-policy-service agree company="Datasoft Computing"

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.15%. Comparing base (8c35947) to head (e993cf0).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
- Coverage   75.32%   75.15%   -0.17%     
==========================================
  Files          33       33              
  Lines        6500     6501       +1     
==========================================
- Hits         4896     4886      -10     
- Misses       1321     1328       +7     
- Partials      283      287       +4     

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

🚀 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 fixes a concurrency issue where Attention packets sent during transaction rollbacks could cause the rollback to be cancelled, leaving transactions open in SQL Server. The solution introduces a boolean parameter to prevent sending Attention packets during rollback operations.

Key changes:

• Added isRollback parameter to simpleProcessResp() to control Attention packet behavior
• Modified rollback flow to suppress Attention packets by setting reader.noAttn = true
• Updated all callers to pass appropriate boolean values

Reviewed Changes

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

File	Description
mssql.go	Added isRollback parameter to simpleProcessResp() and updated all call sites to pass appropriate boolean values
queries_test.go	Updated test calls to simpleProcessResp() to include the new isRollback parameter

[shueybubbles]

thx for the PR!
Please add a specific test that fails without your fix.

[alhails]

Hi, we've written this PR partly to highlight the bug we think we've discovered in the driver. I'm not 100% that this is the best solution although it does appear to have solved the problem after manual testing against a real server. It also makes no sense to us that you would want to send an Attention packet when handling a rollback, since you would always want the rollback to complete regardless of whether the context has cancelled.

With regards to writing a unit test, I'm not sure how I would go about testing the exact issue when a large portion of it relies on SQL Server behaviour. How would you go about simulating this accurately? I could write a test that confirms that the driver no longer sends Attention packets when performing a rollback - however this only confirms that the change is doing what I designed it to do and not really a proper test of the original issue.

Any advice you could provide on this would be appreciated

[shueybubbles]

You can have two tests - a unit test that validates no attention packet is sent during rollback, and a functional test that runs against with a real SQL connection. The validation pipeline will run your test against multiple versions of SQL.

[alhails]

Ok, re the functional test, can you point me at any existing examples I can look at. Thanks

[shueybubbles]

any test with open(t) in it

[Copilot]

Pull Request Overview

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

[shueybubbles]

If

did you try running this locally with your fix commented out to see how often it hits?

[alhails]

Yes I ran against main and it generally fails on the 1st or 2nd attempt

[alhails]

To clarify, since it wasn't 100%, I added the loop in the test which attempts it 10 times. The test appears to reliably show the issue now in main (on the 1st or 2nd time through the loop). With the fix applied, it runs through 10 times without issue

[alhails]

If you like, I can rebase and switch the order of commits so that we add the test first, this way you could see the failing test. And then it would go green with the code change

[shueybubbles]

i think it's fine as is

[alhails]

Actually I beat you to it and I've already reorganised the commits. The CI process hasn't built the commit with just the test tho

[shueybubbles]