GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: treat a failed close of a finished operation as best effort by vuanhphung · Pull Request #592 · databricks/databricks-sql-nodejs

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

DBSQLOperation.close() rethrew any failure of the close request. A teardown CloseOperation that failed after the results were fully read (e.g. a 404) therefore escaped close(), and the rest of the teardown was skipped: the operation stayed open locally (reads were not blocked), stayed registered on its session, and never emitted statement-complete telemetry. The same happened to autoClose iterators and streams at the end of the data, and on the kernel backend whenever its CloseStatement fails.

The fix is in the DBSQLOperation facade, so it covers both backends. Once the backend has reported the statement finished, a failed close is logged at warn, emitted as error telemetry (as before), and reported in the returned Status's info. The operation is then closed locally as usual. The server-side result is also freed when the session closes. Before the statement has finished, a close failure still throws and the operation stays open so the close can be retried, because there the close is what stops a running statement.

Verification: new unit tests cover both backends with a mocked failing close (HTTP 404 and an error status): drained results, finished(), an autoClose iterator, the session-close cascade, and the still-throwing case before the statement finishes. All of them failed on main. npm test (1535 passing), lint, type-check, and build pass. A live script against a warehouse, run on Thrift and useKernel: true, checks three cases: a normal close after draining 10k rows; a close failure injected after draining (close() resolves, a warning is logged, reads are refused, and SELECT 1 on the same session still works); and a close failure on a running query (still throws, then the retry succeeds). The injected-failure case fails on origin/main on both backends.

Performance: the only addition to the default path is one flag store per wait. An in-memory micro-benchmark (200k chunks, 15 alternating fresh-process runs) gave a median of 202.3 ms on main and 204.7 ms on this branch, within the 0.9% A/A noise. Live execute+drain+close of range(100000), alternating builds: Thrift 594.1 vs 600.3 ms (8 runs each), kernel 300.0 vs 304.6 ms (4 runs each), with overlapping ranges.

Fixes #480

This pull request and its description were written by Isaac.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vuanhphung Isaac
fix: treat a failed close of a finished operation as best effort
0970af6
Once the backend has reported the statement finished, a failed close
request (e.g. a 404 on CloseOperation / CloseStatement) is logged as a
warning, emitted as error telemetry and reported in the returned status,
and the operation is closed locally. Before the statement finishes, close
failures still throw, since closing is what stops a running statement.

Fixes #480

Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:16 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:16 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:16 — with GitHub Actions Inactive
peco-review-bot[bot]
peco-review-bot Bot approved these changes Nov 11, 2026
View reviewed changes

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

✅ No issues identified by the review bot.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

This branch was previously deployed

1 inactive deployment
azure-prod — 0970af69 Deployed Nov 11, 2026 by vuanhphung via e2e-test (24.22.1, 24_22_1) #1877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peco-review-bot peco-review-bot[bot] peco-review-bot[bot] approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[coverage] Conformance findings: STATEMENT-023

Uh oh!

There was an error while loading. Please reload this page.

1 participant

@vuanhphung
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.