[aakash-saravanan-db]

Feature or gap

Kernel-backed Go connections need to complete Volume operations with existing staging authorization and error handling.

What this PR addresses

Delegate transfers to shared native completion and preserve cancellation causes. Apply the existing fatal-session policy to SQL instruction errors while preserving sessions on storage HTTP/body failures, including errors racing context cancellation, without returning ErrBadConn or replaying the statement.

Dependencies

Builds select databricks/databricks-sql-kernel#465 at de28034ec75aa6773a5e4e904d77f85511e1aa78 and its matching C header. Merge and release require compatible binding versions, updated modules/sums, and clean installed-package validation. Published 1.4.0 archives lack the new exports.

Tests and validation

Default and native-tagged suites pass, including five actual-native failure-origin/cancellation controls and a negative control that fails with the former invalidation ordering. The source-built named file lifecycle verifies initial bytes, overwrite and removal; retained performance comparisons remain qualified by their recorded build. Clean installed-package and scheduled activation remain open.

[github-actions]

Go integration tests triggered (sea / passthrough). View workflow runs.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Mostly sound, with two low-severity items. The native staging dispatch, cancellation watcher, and defer teardown order look correct. REMOVE not-found=success matches Thrift's 404 handling. The kernel-side behavior depends on the pinned kernel revision, which isn't in this checkout, so I couldn't check it here. Nit: the error strings "Volume transfer: ..." and "Volume operation requires..." start with a capital letter, which is against Go convention (staticcheck ST1005).

Other findings

• ⚪ Nit — nativeStagingStub has an err field, but no table case ever sets it. The dispatch path that wraps a CompleteStaging failure in NewDriverError(..., "error performing staging operation", err) is never tested. No test checks that the returned error is non-nil in that case, or that the cause can still be reached with errors.Is/errors.As (e.g. context.Canceled, *os.PathError). That cause chain is the point of the new KernelError.Unwrap and the ctx-wrapping in CompleteStaging. A case with err: context.Canceled that asserts errors.Is(err, context.Canceled) would cover it.

[peco-review-bot]

🔵 Low — CompleteStaging returns toStatementError(err) for failures from get_result_stream, canceller_new, and complete_volume_operation, but it never calls o.backend.evictIfSessionFatal(err). Both Results (L373-382) and execute make that call on the same kinds of kernel errors. Suppose the token expires or the session is dropped while a Volume transfer completes. The error still reaches the caller, but the conn stays marked valid, so the pool keeps handing out a dead connection. The non-staging read path doesn't have this problem. IsStaging's error path has the same gap. Calling o.backend.evictIfSessionFatal(err) before returning in these branches would match Results.

[aakash-saravanan-db]

I checked the reachable error sources before adding eviction. kernel_executed_statement_get_result_stream only validates/takes the existing synchronous stream (src/c_abi/statement.rs:733–772); marker and detached-canceller creation are local ABI operations. They cannot observe an expired token or dropped remote session.

Completion can encounter both result transport and presigned-storage failures. Their shared kernel error codes do not establish that the SQL session is dead; applying evictIfSessionFatal indiscriminately could discard a healthy SQL connection on a storage authentication/permission failure. I have not found a demonstrated session-origin failure requiring a change here. No message-prefix classifier or new error-origin framework was added for this hypothetical case.

The native source pin is updated to 22f96fac for the required shared retries. Native-enabled package tests and the final named two-backend Volume workflow pass with exact bytes/deletion proof. A concrete session-origin reproduction would be useful evidence to reassess selective eviction.

[peco-review-bot]

You're right about get_result_stream and the canceller: they're local ABI operations, so they can't report a dead session, and that part of my finding was wrong. The complete_volume_operation branch still looks inconsistent to me, though. isSessionFatal only matches Unauthenticated, Unavailable and NetworkError. Results/rows.go already call evictIfSessionFatal on those codes even when the error comes from CloudFetch downloads to presigned storage, which carry the same "storage, not session" ambiguity you describe. So the trade-off you're avoiding here is one the read path already makes. Accepting it in one place and not the other is reasonable, but it's a call for a maintainer. The worst case is a dead conn going back to the pool after a Volume completion fails with an auth or transport error. That's low severity, so I'll leave the thread open for a human to decide instead of pushing further.

[github-actions]

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-kernel were removed for security.

A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.)

Latest commit: 26f142a

[peco-review-bot]

Verdict: 1 Medium · 2 Low

The dispatch is mostly sound, with one medium lint concern in the new kernel CompleteStaging path. ExecContext now routes kernel Volume operations to native completion, and Thrift keeps its existing row-based path. The rest are low-severity issues: missing session-fatal eviction and thin test coverage.

[peco-review-bot]

🔵 Low — CompleteStaging converts kernel failures with toStatementError but never calls o.backend.evictIfSessionFatal(err). That applies to get_result_stream, canceller_new and complete_volume_operation. Every other kernel path that runs after execute does call it, including Results on the same get_result_stream call and every failure branch in execute. A completion that runs long enough for the token to expire, or for the session to be dropped, will return the error but leave the conn marked valid in the pool. The next statement on that conn then fails as well. Call o.backend.evictIfSessionFatal(err) on these failure branches, the same way Results does.

[github-actions]

Go integration tests triggered (sea / passthrough). View workflow runs.

[github-actions]

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-kernel were removed for security.

A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.)

Latest commit: f0c6f41

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

Mostly sound. One medium issue: when native staging completion fails with a session-fatal error, the session is not marked dead, unlike every other kernel result path. Kernel-side test coverage for the new paths is also thin. Nit: the new CHANGELOG entry is followed by a stray blank line inside the bullet list.

Other findings

• ⚪ Nit — None of the new kernel-side behaviour has unit coverage. Untested paths:
• kernelOp.IsStaging with a nil exec.
• The CompleteStaging outcome mapping (outcome != 2 returns an error).
• Wrapping of the cancelled-context error.
• The new KernelError.Unwrap / cause path, where errors.Is(err, fs.ErrNotExist) should work through a *KernelError carrying an os.PathError.

The only new test (TestConnNativeStagingDispatch) uses a Go stub at the conn layer. The removed TestExecuteRejectsStaging has no replacement, and driver_e2e_test.go has no kernel staging test. Unwrap could be pinned in the non-cgo errors_classify_test.go. kernel_test.go could cover IsStaging with a nil exec and the outcome mapping.

[github-actions]

Go integration tests triggered (sea / passthrough). View workflow runs.

[github-actions]

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-kernel were removed for security.

A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.)

Latest commit: 3629843

[peco-review-bot]

Verdict: 1 Medium · 2 Low

The PR looks correct overall. There is one medium concern: CompleteStaging skips evicting a dead session when a session-fatal error races context cancellation, which execute() deliberately avoids. There are also two low gaps: no eviction on stream-acquisition failure, and no test for the cancellation path.

[peco-review-bot]

Verdict: 2 Low

The dispatch and cancellation design look sound, with two low-severity concerns: two early-failure paths in CompleteStaging skip session-fatal eviction, and the native path drops two telemetry tags. As the PR description already says, the pinned KERNEL_REV and header point at an unmerged kernel commit and need exports that the published binding modules don't have, so databricks_kernel-tagged builds against published bindings will fail to link until those modules and the module/sum updates ship.

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 1 Nit

Mostly solid, with one medium gap: the session-fatal eviction this PR adds is skipped on the early get_result_stream failure path in CompleteStaging. Also worth noting for release: KERNEL_REV pins an unmerged kernel PR commit, and the new C symbols need a kernel release later than 1.4.0 before this can ship. The cancellation test relies on the exact number of ctx.Err() calls (errCalls == 2), which is fragile (nit).

Other findings

• 🔵 Low — Removing the isStagingStatement pre-check means staging detection now relies only on the server manifest's is_volume_operation flag, reported through kernel_executed_statement_is_volume_operation. If a warehouse or result shape leaves that flag out (or the kernel misses it), IsStaging returns false, and ExecContext on a PUT/GET reports success with 0 rows while no file moves. That is the silent-data-loss case the removed guard existed to block. Consider keeping the SQL-text detector as a fallback: if the text looks like PUT/GET/REMOVE but the operation is not marked, return an error instead of success. Alternatively, record that the server is guaranteed to always set the flag.
• ⚪ Nit — The native branch returns before the telemetryUpdate callback is wired, so kernel Volume transfers never set TagChunkCount / TagBytesDownloaded. The Thrift staging path does set them. This is a minor telemetry gap compared with Thrift. If the C ABI can't report byte counts, a short comment noting the omission would help.

[peco-review-bot]

Verdict: 1 Low · 2 Nit

Mostly sound, with 3 low concerns: the session-eviction policy is applied inconsistently on the early error paths of CompleteStaging, one error message is misleading for status 0, and the new cancellation watcher has no test. Follow-up outside this PR: per the description, the published 1.4.0 binding archives lack the new kernel_get_last_error_* and volume exports, so a -tags databricks_kernel build against the published bindings will fail to link until a matching bindings release ships.

Other findings

• ⚪ Nit — The outcome != 2 branch handles two different cases: status 0 ("ordinary result": the stream turned out not to be a Volume operation) and status 1 ("input bytes required"). Both get the same error, volume operation requires a local file instruction, and that message only makes sense for status 1. A status-0 result after IsStaging returned true points to a kernel or server inconsistency, and this message would send whoever debugs it in the wrong direction. Neither case is tested. staging_kernel_test.go only covers error returns from completion, and TestConnNativeStagingDispatch stubs out CompleteStaging. Consider using a distinct message for each status (including the numeric status helps) and adding a test for it.
• ⚪ Nit — The *Cancelled subtests do not exercise the cancellation path this PR adds. cancelOnCompletionErrorContext cancels on the second Err() call, which comes after kernel_result_stream_complete_volume_operation has already returned an error. So the watcher goroutine never calls kernel_result_stream_canceller_cancel while a transfer is running. These cases only check how the error is wrapped after the fact. They also depend on how many times CompleteStaging happens to call ctx.Err(), so adding or removing one check would silently change what they test. A test that cancels while the storage request is in flight (for example, a handler that blocks until the ctx is cancelled) would cover the documented promise that "context cancellation" interrupts the transfer.