Skip to content

Comments

Fix resource leak: close staging Rows in execStagingOperation#325

Merged
vikrantpuppala merged 2 commits intodatabricks:mainfrom
vikrantpuppala:fix/close-staging-rows
Feb 24, 2026
Merged

Fix resource leak: close staging Rows in execStagingOperation#325
vikrantpuppala merged 2 commits intodatabricks:mainfrom
vikrantpuppala:fix/close-staging-rows

Conversation

@vikrantpuppala
Copy link
Collaborator

Summary

  • execStagingOperation creates a Rows object via rows.NewRows() to read staging operation metadata (presigned URL, headers, local file path) but never calls row.Close()
  • This leaks the Rows object and its RowScanner resources until GC collects them
  • Add defer row.Close() to ensure proper cleanup after reading the staging metadata

Note: the server-side operation is already closed by ExecContext (lines 122-133), so this is a client-side resource leak rather than a server-side operation leak.

Test plan

  • Existing TestConn_execStagingOperation tests pass
  • TestWorkflowExample e2e test passes
  • go build ./... compiles cleanly

Related: #275

🤖 Generated with Claude Code

execStagingOperation creates a Rows object via rows.NewRows() to read
staging operation metadata but never closes it. This leaks the Rows
object and its RowScanner resources until GC collects them. Add
defer row.Close() to ensure proper cleanup.

Related: databricks#275

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 22, 2026 08:39
Copy link

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

This PR addresses a client-side resource leak in execStagingOperation by ensuring the staging Rows iterator is closed after reading staging metadata, reducing the chance of retained RowScanner/page-iterator resources during concurrent PUT/GET/REMOVE workflows (related to #275).

Changes:

  • Add defer row.Close() after successfully creating staging Rows in execStagingOperation.
Comments suppressed due to low confidence (1)

connection.go:594

  • rows.NewRows can return a non-nil driver.Rows along with a non-nil error (it returns r, err when makeRowScanner fails). In that case this branch returns without closing the partially-initialized rows, so some client-side resources may still leak. Consider explicitly closing row when err != nil (if non-nil) before returning, or adjust NewRows to return nil on error.
		row, err = rows.NewRows(ctx, exStmtResp.OperationHandle, c.client, c.cfg, exStmtResp.DirectResults)
		if err != nil {
			return dbsqlerrint.NewDriverError(ctx, "error reading row.", err)
		}

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

@vikrantpuppala vikrantpuppala merged commit d125096 into databricks:main Feb 24, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants