[peco-engineer-bot]

Summary

Automated fix for #472 — [coverage] Conformance findings: STATEMENT-022.

DBSQLOperation.cancelInternal() now keeps the in-flight cancel promise and hands it to every concurrent cancel() call, so three simultaneous cancels on Thrift send exactly one CancelOperation RPC. Once the cancel succeeds, the cancelled flag stops any further RPCs; if it fails, the promise is cleared so a later cancel() can try again. The new e2e spec passes against the live warehouse, the two new unit tests pass, and npm test (1483 passing) and lint are clean; npm run type-check fails only on five examples/tokenFederation/*.ts files that can't resolve the @databricks/sql import, which my change doesn't touch.

Root cause & plan

Root cause: DBSQLOperation.cancelInternal() only short-circuits once this.cancelled is set, and it sets that flag only after await this.backend.cancel() resolves. Concurrent cancel() calls all pass the guard before the first RPC returns, so each one calls ThriftOperationBackend.cancel() → driver.cancelOperation(). Three simultaneous cancels therefore send three CancelOperation RPCs instead of one. The reference Go driver (databricks-sql-go#419) and the shared spec expect exactly one on Thrift.
Files: lib/DBSQLOperation.ts, tests/e2e/cancel_concurrent.test.ts, tests/unit/DBSQLOperation.test.ts
Planned coverage:

• New e2e spec tests/e2e/cancel_concurrent.test.ts. Open a session with the config/sinon pattern from iterators.test.ts and spy on session.context.driver.cancelOperation. Start the long-running query from the issue (or a range CROSS JOIN fallback) with runAsync. Call operation.cancel() three times at once with Promise.all. Assert all three resolve without throwing (each returns a Status) and that cancelOperation was called exactly once. Today the spy shows callCount 3, so the test is red. Also check that a later fetchChunk is rejected as cancelled, and close the session. (concurrent cancel dedup on Thrift (live warehouse))
• Optional unit test in tests/unit/DBSQLOperation.test.ts. With a stubbed driver whose cancelOperation resolves on a delay, three concurrent cancel() calls make one driver call and all get the same result. After success, a further cancel() makes no RPC. If the in-flight cancel rejects, every concurrent caller gets the rejection and a later cancel() can try again. (unit edge cases for dedup)

Files changed

• tests/unit/DBSQLOperation.test.ts
• tests/e2e/cancel_concurrent.test.ts
• lib/DBSQLOperation.ts

Test plan

• tests/e2e/cancel_concurrent.test.ts: Concurrent cancel (STATEMENT-022) three simultaneous cancel() calls: no deadlock — fails (red) against the original code, passes (green) after the fix
• single CancelOperation [thrift] — fails (red) against the original code, passes (green) after the fix
• tests/unit/DBSQLOperation.test.ts: DBSQLOperation cancel should send a single CancelOperation for concurrent cancel() calls (STATEMENT-022) — fails (red) against the original code, passes (green) after the fix
• tests/unit/DBSQLOperation.test.ts: DBSQLOperation cancel should propagate in-flight cancel failure to all concurrent callers and allow retry — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 2 Low

Looks good. The cancel dedup is correct: each cancel() caller gets the same shared promise, the reset after the promise settles allows a retry after a failure, and promise.then(reset, reset) prevents an unhandled rejection. The unit tests cover the success path and the retry-after-failure path. I have two low-severity notes, about close() running during a cancel and about duplicated error telemetry.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low concern. Concurrent cancel() calls are correctly collapsed into one shared promise, which is set synchronously so the unit tests are meaningful. A failed cancel is cleared so a later call can retry, and error telemetry now fires once per failed RPC. The new close-waits-for-cancel guard covers only one ordering: a cancel() started while close() is in flight can still overlap with it.

[peco-review-bot]

Verdict: 2 Low

The cancel dedup looks correct, and I traced the cancel/close ordering with no deadlock cycle: close only waits on a cancel already in flight when it starts, and cancel only waits on close when no cancel is in flight. The two low findings are that concurrent close() calls still aren't deduplicated, and that close() now waits on an in-flight cancel RPC, which could stall cleanup.

[peco-review-bot]

Verdict: 2 Low

Looks good. I traced the cancel/close dedup state machine through these cases: cancel→close, close→cancel, failure-then-retry, and multiple waiters resuming after a failed close. I found no deadlocks, no duplicate RPCs and no double onClose. My two low-severity notes are a weak e2e assertion and an undocumented change to how close() behaves.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity documentation gap. The shared-promise dedup for cancel/close is correct: reset is registered before any caller awaits, so a retry after a failure sees a cleared promise. Errors are reported to telemetry once per shared request. Each wait (cancel on close, close on cancel) only starts when the other promise isn't already shared, so the two can't deadlock. The unit tests do exercise the concurrent paths, because the first call sets the shared promise synchronously.

[peco-review-bot]

✅ No issues identified by the review bot.

[vuanhphung]

Closing; not needed. See #472 (comment)