[aakash-saravanan-db]

Feature or gap

Kernel-backed sessions need to complete server-marked Volume PUT, GET and REMOVE operations through the existing staging API.

What this PR addresses

Delegate authorized transfers to native completion and preserve raw instructions otherwise. Keep deferred SQL instruction HTTP errors and their metadata intact, map tagged storage HTTP and local policy failures to the established staging error type, and preserve portable file-error codes when the host does not recognize a native errno.

Dependencies

Builds select databricks/databricks-sql-kernel#465 at 855847bdca71e07285cfc60332fb2f33f8e47786. Coordinated native and connector release acceptance requires compatible platform versions, updated dependency pins/locks, and clean installed-package validation. Published native 1.4.0 lacks completion APIs.

Tests and validation

63 backend/error-mapping tests, TypeScript, lint and formatting checks pass. Source-built sync/async staging comparisons pass with initial and changed bytes verified; retained performance comparisons remain qualified by their recorded build. Clean installed-package and scheduled activation remain open.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly sound, with one medium issue: on the kernel path, the new StagingError wrapping can also catch the statement's own execution errors, not just transfer errors. Nits: there is a stray blank line in the CHANGELOG entry, and the constructor comment in KernelOperationBackend still says cancel() "always routes to the cancellable execution". Follow-up (not caused by this PR): executeStatement waits for the transfer before it returns the operation, so a caller can only interrupt an in-flight transfer by closing the session.

[peco-review-bot]

🔵 Low — The new tests cover only the completed and not_volume outcomes and the cancel routing. These new branches in completeStagingOperation have no unit test:

• the needs_input_stream → StagingError path
• reclassifying ParameterError and httpStatus errors as StagingError
• passing other errors through unchanged (for example local fs errors decoded with code/path)

No test checks that DBSQLSession closes the operation when completion fails either. The error-classification branch is where F1 lives, so a test there would have caught it.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

The change looks sound overall. I have one medium concern: transfer errors are rewrapped as StagingError from the raw message, which drops the kernel metadata and may expose storage error text to callers. Follow-up work: an e2e test that runs GET/PUT/REMOVE through the kernel backend would confirm the end-to-end behaviour the PR description reports. Nits: the CHANGELOG has an extra blank line after the new entry, and the edited KernelSessionBackend.executeStatement JSDoc sentence ('Still rejected … while stagingAllowedLocalPath is applied by the facade') now reads awkwardly.

[peco-review-bot]

🟡 Medium — When a transfer fails with a ParameterError or an error that has kernelMetadata.httpStatus, this code rethrows it as new StagingError(mapped.message). That has two problems:

1. Kernel diagnostics are lost. The new error has no sqlState, kernelMetadata (HTTP status, queryId, retryable) or cause. The thrown error is the only failure signal callers get, and nothing else exposes these fields.
2. The storage error's message reaches the caller unchanged. Presigned Volume URLs carry credentials in the query string. I could not check the kernel PR (Add disableRowMaterialization option to skip per-cell decode #465) to confirm whether its storage HTTP error messages include the request URL. The Thrift path avoids this by building its own message (HTTP error ${response.status} ${response.statusText}, lib/DBSQLSession.ts handleStagingGet/Put/Remove).

Possible fix: for HTTP failures, build the message from kernelMetadata.httpStatus instead of the kernel's text. In either case, copy kernelMetadata/sqlState/cause onto the StagingError, or set the mapped error as its cause.

[aakash-saravanan-db]

Checked against 8e93f23a and native 08a92137. The storage URL-exposure premise does not hold for these sources: non-success HTTP messages contain only the status, and reqwest transport/body failures use without_url() before formatting. No raw presigned URL is passed through this native path.

Preserving additional diagnostics on legacy StagingError remains the optional enhancement identified in the earlier same-source follow-up; it is not a new failure of the staging contract. No message rewrite or new diagnostic layer is needed for this finding.

The portable local-error branch now has four cases in the existing test: raw errno, absent/zero errno with PermissionDenied→EACCES, and unknown kind→EIO. They check negative errno, path, metadata/cause and absent SQLSTATE. The four cases, broader affected tests and typecheck pass.

[peco-review-bot]

Thanks for checking the native source at 08a92137. If non-success messages carry only the status and reqwest errors go through without_url(), then the kernel doesn't leak today. I can't confirm that from here, though, because the native code isn't in this repo. Line 329 is unchanged and still passes the kernel's message straight through, so this path's safety depends on how a separate codebase formats its messages. .claude/knowledge/driver-pitfalls.md is explicit: "Never copy a network error's message into an error the caller sees" (#567). The Thrift handlers follow that by building HTTP error ${status}. Building the message from kernelMetadata.httpStatus here would be a one-line change, and it would keep this path safe if the kernel's formatting changes later. Separately, the StagingError still has no cause or kernelMetadata, and that was the first point in the finding. Those two fixes make one small change, so I'm leaving this thread open for a maintainer to decide whether to accept the reliance on the native code.

[peco-review-bot]

🔵 Low — The new local-error branch has two code paths: the POSIX getSystemErrorName(-raw) path, and the kindCodes / 'EIO' fallback, which runs on Windows, when localOsError is missing or 0, or when the kind is unknown. The one new test (error-mapping.test.ts, localOsError: 2 on Linux) only covers the first. Nothing tests that a missing or zero localOsError maps by kind (e.g. PermissionDenied → EACCES), or that an unknown kind gives EIO with a sensible negative errno. This branch also swaps the typed driver error for a plain Error. A test that pins the result (no sqlState, but kernelMetadata and cause still attached) would protect that contract.

[peco-review-bot]

Verdict: 1 Low

Looks good overall. I found one low-severity issue: when a transfer error is re-wrapped as StagingError, its kernel diagnostics are dropped. Nits: (1) a stray blank line in CHANGELOG.md splits the Unreleased list; (2) the KernelSessionBackend.executeStatement JSDoc now reads awkwardly ("Still rejected: … and … , while stagingAllowedLocalPath is applied by the facade"); (3) there is no unit test for completion going through the directResults Running arm (AsyncResultHandle).

[peco-review-bot]

🔵 Low — When a transfer fails with ParameterError or an HTTP status, the error is replaced by new StagingError(mapped.message). The new error has no kernelMetadata (including httpStatus and queryId), no sqlState and no cause, even though decodeNapiKernelError just attached them.

That means a caller can't tell a storage 403 from a 404 or other failure except by parsing the message. It also can't correlate the failure with a query id. The local-filesystem branch keeps that metadata, so transfer failures end up less diagnosable than the other error classes this PR separates out.

Fix: copy the metadata onto the wrapper, e.g. Object.assign(new StagingError(mapped.message), { kernelMetadata: mapped.kernelMetadata }), or set cause: mapped. The class change still happens, but the diagnostics survive.

[aakash-saravanan-db]

8f800ba narrows conversion to the tagged VOLUME_STORAGE_HTTP_ERROR case and local ParameterError without HTTP metadata. Deferred instruction-fetch 401 and 400 now retain their mapped error identity, SQLSTATE and metadata; the existing backend fixture asserts those controls. Extra structured metadata on legacy storage StagingError remains an optional API enhancement: its existing Thrift surface is the staging class/message, which this adapter preserves.

[peco-review-bot]

Thanks. Narrowing the conversion in 8f800ba fixes part of this: instruction-fetch 400 and 401 errors now keep their class, SQLSTATE and metadata. But line 330 still throws new StagingError(mapped.message) for the VOLUME_STORAGE_HTTP_ERROR path. Those are exactly the storage failures where httpStatus and queryId matter most, so callers still can't tell a 403 from a 404 without parsing the message. Matching the legacy Thrift class and message makes sense, but adding { cause: mapped } (or copying kernelMetadata onto the wrapper) keeps that and only adds information. It's a small additive change, so I'd still like to see it here rather than deferred. Severity stays low.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with 2 low-severity concerns: the errno-name fallback for local file errors, and no test for the needs_input_stream path. The staging dispatch, the close-on-failure cleanup, the forced-sync execution and the routing of cancel to the resolved statement all check out against the surrounding code. Nits: the new completeStagingOperation? method in IOperationBackend has no JSDoc; the local-error tests in error-mapping.test.ts sit outside any describe; and the CHANGELOG entry adds a stray blank line inside the Unreleased list.

[peco-review-bot]

🔵 Low — No test covers the needs_input_stream outcome. The suite only exercises completed, not_volume and thrown errors. This branch throws inside the try, gets passed through decodeNapiKernelError, and must still come out as a StagingError. A single test case pinning that would stop a later refactor of the catch-side classification from silently turning it into a different error class.

[aakash-saravanan-db]

This is a useful optional branch test, but source inspection does not establish a classification defect: the needs_input_stream branch creates a StagingError, and decodeNapiKernelError returns a nonsentinel Error unchanged (KernelErrorMapping.ts:270–274). The class therefore survives this catch. Keeping the existing focused tests and recording the uncovered branch; no behavior change is needed for this finding.

[peco-review-bot]

You're right that today's code keeps the class: decodeNapiKernelError returns a non-sentinel Error unchanged, so the StagingError gets through. I wasn't claiming a current defect, though. This finding is only about missing test coverage. Nothing pins that pass-through, so if someone later changes the catch-side mapping (for example, wrapping or reclassifying every non-sentinel error), the needs_input_stream path could start returning a different error class and no test would fail. The fix is one test that mocks completeVolumeOperation to resolve 'needs_input_stream' and asserts StagingError. Since there's still no test, I'm leaving this open as a low-priority item for a human to decide.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity concerns. The kernel completion dispatch, the forced inline execution and the error mapping are consistent with the Thrift staging behaviour. The remaining gap is the unpublished native dependency, which the PR description already flags.

[peco-review-bot]

🔵 Low — Nothing checks that the loaded native binding actually has completeVolumeOperation. The PR description says published native 1.4.0 doesn't have it. On that binding, any kernel executeStatement that passes stagingAllowedLocalPath will reject with TypeError: handle.completeVolumeOperation is not a function. That includes plain SELECTs, because DBSQLSession now calls completeStagingOperation for every statement with the option set. decodeNapiKernelError passes the TypeError through unchanged. Before this PR the same call logged a warning and continued.

If the binding can't be pinned to a compatible release when this merges, add a feature-detect: when typeof handle.completeVolumeOperation !== 'function', throw a clear StagingError/HiveDriverError stating that the native binding doesn't support Volume operations. That would replace the opaque TypeError.

[aakash-saravanan-db]

Agreed: compatible published native packages are a merge prerequisite, including ordinary statements with stagingAllowedLocalPath. Source KERNEL_REV does not update installed platform dependencies. The matching release must supply completion, all platform pins/locks must advance, and clean installed queries plus Volume workflows must pass before merge. The accessible proxies currently expose 1.4.0, whose API is incompatible. Keeping the gate open; no old-native fallback or unavailable version is being introduced.

[peco-review-bot]

Thanks for confirming. I agree that pinning a compatible published native release is the right fix, and that it has to land before merge. Until then, the code at this line still calls handle.completeVolumeOperation without checking it exists, so on 1.4.0 it would still fail with the opaque TypeError. I'm leaving this thread open as the merge gate. It can be resolved once the platform pins and lockfiles move to a release that includes completeVolumeOperation. If that release won't be ready by merge time, the typeof check plus a clear StagingError is still a cheap safeguard.

[aakash-saravanan-db]

The release plan is coordinated native/connector version updates. Published 1.4.0 remains unsupported for these calls: before connector release, compatible platform pins and locks plus clean installed ordinary-query and Volume validation are required. Source validation selects kernel 855847bd with completeVolumeOperation. We are continuing review of that source pair while keeping package integration as a release prerequisite; no fallback, invented version or completed package proof is claimed.

[peco-review-bot]

Thanks for laying out the release plan. Your reply doesn't challenge the finding. The code at line 318 still calls handle.completeVolumeOperation without checking that it exists, and the installed pins still resolve to 1.4.0. My last reply already said this thread stays open as the merge gate. It can be resolved once the platform pins and lockfiles move to a native release that has completeVolumeOperation, or once a typeof guard that throws a clear StagingError lands. Until one of those is pushed, I'm keeping it open.

[peco-review-bot]

🔵 Low — The test preserves SQL execution errors with HTTP metadata doesn't exercise the classification branch it is named for. result() throws, so getFetchHandle() rejects before the try block, and no error is mapped at all. The test would still pass if the error had no httpStatus, or if the ParameterError → StagingError rule were wrong.

To cover the ordering this test is meant to protect, add a case where result() throws a bare ParameterError with no httpStatus. It should still come back unchanged, not wrapped in StagingError. That pins down that only errors raised during completion get reclassified.

[aakash-saravanan-db]

The existing execution-error case proves passthrough before completion; getFetchHandle remains outside the try. The separate deferred instruction401/400 cases throw from completeVolumeOperation and exercise classification inside that catch. A bare ParameterError variant would strengthen the ordering guard, as an optional test improvement; no current classification defect is established. The missing needs_input_stream case and review-body test grouping/JSDoc/blank-line suggestions remain optional as well.

[peco-review-bot]

Agreed that this isn't a classification defect, and that the deferred 401/400 cases cover the mapping inside the catch. The gap is narrower than that. This test's error already carries httpStatus: 400, so it can't tell "errors from getFetchHandle() are never classified" apart from "errors with HTTP metadata are kept as they are". If someone later moved getFetchHandle() inside the try, this test would still pass. A bare ParameterError with no httpStatus thrown from result() would catch that move. It's a low-severity test-hardening item, so it's fine to defer, but I'll leave the thread open so it stays visible.