325: MSSQL: stream runner with row cap, cancel and timeout #349

Merged

david merged 2 commits from feature/issue-325/mssql-stream-runner-with-row-cap-cancel-and-timeou into main

2026-11-11 00:17:35 +00:00

david commented

2026-11-11 00:17:33 +00:00

Owner

325: MSSQL: stream runner with row cap, cancel and timeout

Implements 325.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 1m 00s.

Review findings

Review Findings — 325

Summary

I fixed all four findings. Post-truncation errors now resolve only when they look like a cancellation (ECANCEL or a message matching /cancel/i), and any other error rejects. maxRows and timeoutMs are validated as positive integers and reject with RangeError. The JSDoc now documents the multi-result-set merge. The timeout test now uses mock.timers. node --test extensions/mssql/runner.test.ts passes all 14 tests. The repo has no build or lint script, so neither gate ran. One part of the first finding is not covered: a timer firing after truncation still resolves as truncated. No error is involved there, because the cancel was already sent, so I left that behaviour as it was.

Critical

High

Medium

extensions/mssql/runner.ts:80 — After truncation, any driver error is treated as the expected cancel error and resolved as a truncated success. A genuine failure such as a connection drop is therefore reported as a normal truncated result. The same applies at line 89 when the timer fires after truncation. Suggested fix: when truncated is true, resolve only if the error looks like a cancellation (for example err.code === "ECANCEL" or a message matching /cancel/i). Otherwise reject. Add a test for a non-cancel error after truncation.

Low

extensions/mssql/runner.ts:45 — maxRows and timeoutMs are not validated. A maxRows of 0 or a negative value truncates on the first row. A timeoutMs that is NaN, 0 or negative fires the timer almost immediately. Suggested fix: throw a RangeError (or reject) unless both values are positive finite integers, and add tests.
extensions/mssql/runner.ts:65 — With multiple result sets, rows are merged into one array and row events don't carry recordset boundaries. Suggested fix: document this in the JSDoc, or handle the recordset event if multi-statement batches are allowed through the guard.
extensions/mssql/runner.test.ts:84 — The timeout test relies on a real 10 ms timer, which could be flaky on a slow CI runner. Suggested fix: use mock.timers from node:test.

## 325: MSSQL: stream runner with row cap, cancel and timeout Implements [325](https://git.excelera.net/david/pi-extensions-and-skills/issues/325). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 1m 00s. ## Review findings # Review Findings — 325 ## Summary I fixed all four findings. Post-truncation errors now resolve only when they look like a cancellation (`ECANCEL` or a message matching `/cancel/i`), and any other error rejects. `maxRows` and `timeoutMs` are validated as positive integers and reject with `RangeError`. The JSDoc now documents the multi-result-set merge. The timeout test now uses `mock.timers`. `node --test extensions/mssql/runner.test.ts` passes all 14 tests. The repo has no build or lint script, so neither gate ran. One part of the first finding is not covered: a timer firing after truncation still resolves as truncated. No error is involved there, because the cancel was already sent, so I left that behaviour as it was. ## Critical ## High ## Medium - [x] extensions/mssql/runner.ts:80 — After truncation, any driver `error` is treated as the expected cancel error and resolved as a truncated success. A genuine failure such as a connection drop is therefore reported as a normal truncated result. The same applies at line 89 when the timer fires after truncation. Suggested fix: when `truncated` is true, resolve only if the error looks like a cancellation (for example `err.code === "ECANCEL"` or a message matching `/cancel/i`). Otherwise reject. Add a test for a non-cancel error after truncation. ## Low - [x] extensions/mssql/runner.ts:45 — `maxRows` and `timeoutMs` are not validated. A `maxRows` of 0 or a negative value truncates on the first row. A `timeoutMs` that is NaN, 0 or negative fires the timer almost immediately. Suggested fix: throw a `RangeError` (or reject) unless both values are positive finite integers, and add tests. - [x] extensions/mssql/runner.ts:65 — With multiple result sets, rows are merged into one array and `row` events don't carry recordset boundaries. Suggested fix: document this in the JSDoc, or handle the `recordset` event if multi-statement batches are allowed through the guard. - [x] extensions/mssql/runner.test.ts:84 — The timeout test relies on a real 10 ms timer, which could be flaky on a slow CI runner. Suggested fix: use `mock.timers` from `node:test`.

david added 2 commits

2026-11-11 00:17:34 +00:00