feat: stream max and table-valued parameters from async sources

[arthurschreiber]

Problem

Every parameter value has to be fully in memory before a request is sent. A varbinary(max) must be one Buffer, an nvarchar(max) one string, a table-valued parameter an array of rows. So a value larger than memory cannot be sent at all, and a large in-memory TVP is serialized into a second full copy before any of it reaches the socket.

The write contract from #1774 was built for this: parameters are resolved before the request starts, and types write into a buffer. This adds the streaming half.

Change

DataType.writeValue now covers a value read from a source as well as one in memory. A value that is fully in memory is written before it returns, as before. A value read from a source while the request is written (an async iterable) returns the rest of the write instead: an async iterable that yields whenever the buffer holds a chunk's worth (WritableTrackingBuffer.CHUNK_SIZE) so the caller can hand those bytes on before the rest of the value is read, and nothing of that rest runs before its first next(). resolve detects an async source and declares the parameter as a max type, since its length is not known up front. There is no second method and no flag on the resolved parameter: the type decides per value, and the caller decides by the return.

• varbinary(max) / varchar(max) / nvarchar(max) accept an async iterable of chunks (buffers, or strings encoded per the type) and write them as PLP through one shared helper. VarChar gets its in-memory writeValue, ported from its generate* methods, to host its streamed branch.
• TVP accepts an async iterable of rows as well as an array; rows are validated and written as they arrive, cell by cell in one pass, through one cell object per column that is reused for every row. A TVP's writeValue always returns the rest of the write; rows given as an array are written in a synchronous loop inside it, so the only asynchrony is the yield per chunk's worth of rows, not per row.

writeRest(rest, wrap) in data-type.ts drives such a rest for whoever called writeValue: only the rest's own next() is wrapped through wrap (the payload names the parameter), an error thrown into the driver by its consumer keeps its identity, a consumer that stops early closes the rest and with it the value's source as for await would, and a close that fails does not replace a propagating error.

RpcRequestPayload becomes an async iterable, the same shape as BulkLoadPayload (#1779): one generator writes the request header and each parameter's header, TYPE_INFO and value into one WritableTrackingBuffer, and yields the buffer's contents whenever it holds a chunk's worth (checked after every parameter, and at every yield of a streamed value's rest) and at the end. The whole request is one buffer from header to last parameter, with no partial chunks at a streamed value's edges. Bytes are unchanged, a large value written by reference stays by reference, and the first packet can leave before the last parameter is written. Chunks are yielded one by one rather than through yield*, since an array iterator has no throw method and a consumer's thrown error would otherwise become a TypeError on Node 24+. Compared with master's small buffer per parameter, a request of 20 scalar parameters reaches the packetizer as 4 chunks instead of 21 and serializes about 1.45x faster (see Measurements). This folds in #1776, which is closed in favour of this PR.

makeRequest already consumes the payload through Readable.from, which drove the previous synchronous iterator through its own asynchronous read loop anyway, so an always-async payload costs nothing on the scalar path (an earlier revision of this PR kept a synchronous iterator for requests without a streamed value and measured the same).

The TVP's legacy generateTypeInfo, generateParameterLength, generateParameterData and validate, still required by the DataType interface but no longer reached when a TVP is serialized, delegate to the same column, row and type-info writers as the live path (verified byte-identical for a null and an array table), and validate and resolve share one table check.

Streaming flushes at chunk-sized boundaries, so the number of yields is proportional to the byte size, not the row/chunk count, and memory stays bounded regardless of how large the source is. A source that throws mid-stream aborts the request with the same InputError that names the parameter.

Behaviour

• No change for existing (in-memory) scalar/max values: byte-for-byte identical. The 40-case byte-equivalence suite consumes the payload asynchronously, so it also covers the TVP path.
• New: passing an async iterable (e.g. a Readable) as a max value, or as a TVP's rows, streams it. This works through execSql, callProcedure and a prepared statement's execute alike, since all three resolve their parameters the same way. Request.addParameter's doc comment describes the form.
• A Request carrying a streamed value can be sent once — a one-shot source (a consumed Readable, a generator) cannot be replayed the way a buffer/string can.
• Each chunk of a string source is encoded on its own, as Writable.write encodes it: a chunk must not end halfway through a UTF-16 surrogate pair. Node.js core does not stitch pairs across writes anywhere (Writable.write, fs.createWriteStream, crypto.Hash.update all produce two replacement characters for a split pair), and this PR follows that rather than re-chunking strings itself. Text Node decoded from UTF-8 never splits a pair; a string sliced by index can. A Buffer chunk of 8 KB or more is sent by reference, so a source must not reuse or modify it until the request has completed. Both documented on addParameter.
• RpcRequestPayload is internal, but for anyone iterating it directly: it is now an async iterable only. DataType.writeValue's return type widens from void to void | AsyncIterable<void>; a caller that ignored the return keeps working for in-memory values.

Validation

• test/unit/streaming-parameters-test.ts: resolve declaring an async source as max and writeValue returning the rest for it, declaration() returning the max form for an async source, the payload handing out a full chunk before it writes the next parameter, PLP output (including empty-chunk skipping, and a chunk of CHUNK_SIZE or more handed on by reference), InputError propagation from a failing max or TVP source, the value's source being closed when the consumer stops early, an error the consumer throws into the payload keeping its identity even when closing the source fails as well, a TVP row that is not an array or has the wrong length, a TVP whose rows are neither an array nor an async iterable, and a TVP from an async iterable serializing identically to the same rows as an array (including a ~2000-row case past the flush size, and an async iterable that yields no rows).
• test/integration/streaming-parameters-test.ts (against a real server): a streamed varbinary(max), nvarchar(max) and varchar(max) value round-trips unchanged (100 KB, uneven chunks including empty ones, crossing packet and flush boundaries), through execSql and through a prepared statement's execute; a 5,000-row TVP fed from an async iterable arrives intact; a source that throws after data is already on the wire, and a TVP whose streamed row fails validation, both surface as the parameter's InputError and leave the connection usable for the next request. The TVP tests name their type and procedure per test, since the Azure CI jobs share one database.
• Also checked by hand: for each max type, a streamed value and the equivalent in-memory value produce the same server-side HASHBYTES and DATALENGTH (3 MB binary, 400 KB nvarchar, 240 KB varchar), and empty streamed values send as zero-length.
• Unit suite: 551 passing. Streaming, TVP, bulk load and parameterised-statement integration suites pass against SQL Server 2022 (187). Lint and typecheck clean.

Measurements

Serialization only, no server: each request is resolved, serialized and consumed through Readable.from(payload) into a no-op sink, as makeRequest does. master at 563d37b (with #1774) against this branch at 231cc68, same machine and run, Node 22, median of 5–9 runs, each pair measured twice back to back.

Request	master	this PR
20 scalar parameters (int, nvarchar, varbinary, datetime, decimal × 4)	~34–36k req/s, 21 chunks	~50–52k req/s, 4 chunks	1.45x
one in-memory 1 MB varbinary(max)	~140–146k req/s	~160–180k req/s	within noise
TVP, 1,000,000 rows as an array (int, nvarchar, bit)	~0.79–0.86M rows/s	~1.97–2.13M rows/s	2.5x
TVP, 1,000,000 rows from an async iterable	not possible	~1.4M rows/s	new

The scalar gain is the single buffer: one flush per request instead of one small buffer per parameter. The large-max case is a by-reference write on both sides; both spread from ~30k to ~190k req/s across runs depending on GC, so the medians move either way from run to run. The array TVP no longer builds a second full copy of the rows before the first byte goes out and allocates nothing per row or cell; the async source pays for a promise per row and still streams with bounded memory.

Where the remaining TVP time goes: the same 1M-row benchmark with the bit column replaced by a second int runs at ~3.9M rows/s (array) and ~1.9M rows/s (async). Bit still serializes through the legacy generateParameterLength/generateParameterData path, which allocates a buffer and a generator per cell; that is the per-family migration of the remaining types, planned after this PR. #1780, stacked on this one, compiles a writer per column on top of this contract.

The repo's own benchmarks/parameters/scalar-params.js, tvp-rows.js and tvp-rows-async.js measure the same three shapes.

Follow-ups (not in this PR)

• The TVP's legacy generate* methods and validate are dead on the live path and now only wrap the shared writers; deleting them means making the legacy trio optional on DataType, which touches every type's tests and belongs with the end of the per-family migration.
• BulkLoadPayload still allocates a parameter object per cell; feat: compile one writer per parameter or column, and read a row's max cells from a source #1780 replaces that with a compiled writer per column.

#1774 has merged, so the diff here is the streaming work itself (12 files). #1772 is independent of this PR and goes in first; this branch takes master again after it lands.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fxv5h4UMCGJEpjgCKcAxug

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-03T13:15:19.518026Z	ed8fb39	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5265dbf2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review: streaming max and TVP parameters from async sources

Nicely structured PR — the "resolve once, write from resolved data" contract from #1776 extends cleanly into a streaming variant without disturbing the synchronous fast path. A few observations, nothing blocking:

Design / correctness

• TVP is always streamed: true, even for plain-array rows. TVP.resolve (src/data-types/tvp.ts:246) unconditionally sets streamed: true regardless of whether rows is an array or an async iterable. That means every request containing a TVP parameter now goes through RpcRequestPayload.generateDataAsync, not just ones with an async row source — the "no async cost for the common case" claim in the PR description holds for scalar/max params, but not for TVPs. This looks intentional (TVP never had a synchronous writeValue fast path to begin with, and the benchmark shows the array case getting faster, not slower), but it's worth calling out explicitly since it's a bit surprising on first read of resolve() — a reader could easily assume streamed tracks "is this an async source" rather than "does this type use writeValueStream".

• Re-executing a Request with a streamed parameter. Request.validateParameters re-runs resolveParameter (and thus reads parameter.value, the async iterable) on every call to execSql/callProcedure. In-memory values tolerate a request object being reused/retried; a one-shot async iterable (e.g. a Readable already fully consumed, or a generator) does not — a second execution of the same Request would silently send an empty/short PLP or TVP rather than erroring loudly. Probably fine to leave as a documented caveat rather than something to code around, but a line in the addParameter/TYPES doc comments would help users avoid assuming buffer/string reuse semantics.

• parameter.length is silently ignored for streamed max values. resolve() in varbinary.ts/varchar.ts/nvarchar.ts always sets length: MAX when the value is an async iterable, even if the caller explicitly passed length. That's the only sane choice (length truly isn't known up front), but since it diverges from the explicit-length-wins rule used everywhere else in resolveParameter, a short comment noting the override is intentional (already present in nvarchar/varchar's resolve, missing in varbinary.ts) would help future readers.

Test coverage

• Good unit coverage for the new mechanics: streamed-detection, sync-vs-async iterator switch on the payload, PLP framing (including the empty-chunk-as-terminator hazard), error propagation, and TVP array/async equivalence.
• Two gaps that seem worth filling with cheap unit tests rather than relying only on the manual SQL Server 2022 validation mentioned in the description:
  • declaration() returning 'varbinary(max)' / 'varchar(max)' / 'nvarchar(max)' for an async-iterable value — this feeds sp_executesql's parameter list in execSql and isn't exercised by streaming-parameters-test.ts (which only builds RpcRequestPayload directly, i.e. the callProcedure path).
  • A TVP row that fails validation partway through an async source (mirrors the existing InputError test for varbinary, but for writeRowsFrom's validateRow failure path).

Nit

• plp-stream.ts's isAsyncIterable is exported and reused by all three max types plus tvp.ts, which is good; just flagging that it intentionally only recognizes Symbol.asyncIterator sources (not plain sync iterables/generators) for the max types, while TVP additionally accepts arrays. Consistent with the PR's stated scope — just worth a one-line comment at the export site so the restriction doesn't look like an oversight.

Overall: solid, well-tested extension of the existing contract; the points above are refinements rather than blockers.

[arthurschreiber]

Thanks — addressed the review:

• Test gaps (both added, 7274823): declaration() returning the max form for an async-iterable value, for all three max types (the sp_executesql path that the payload-level tests didn't reach); and a TVP whose async row source fails validation partway through surfacing the InputError that names the parameter, mirroring the varbinary case.
• Clarity comments (7274823): noted in TVP.resolve that a TVP is always streamed (it has no synchronous writeValue), and in varbinary.resolve that a streamed value is always sent as max, overriding an explicit length — matching the notes already in nvarchar/varchar.
• Separately, the earlier Codex finding about splitting a UTF-16 surrogate pair across varchar chunks is fixed in db690af.

On re-executing a Request with a streamed parameter: you're right that a one-shot source (a consumed Readable, a generator) can't be replayed, unlike a buffer/string. That's inherent to streaming rather than something to code around — I've left it as-is, and it's worth a line in the addParameter docs; happy to add that here or in a follow-up, whichever you'd prefer.

---

Generated by Claude Code

[claude]

Review: streaming max and TVP parameters

Nice piece of work — the sync/async split in RpcRequestPayload (keeping generateData fully synchronous and only switching to generateDataAsync when a parameter is actually marked streamed) is a clean way to add this without taxing the common case, the surrogate-pair stitching in varchar.ts is a genuinely subtle correctness fix with a good regression test, and error handling consistently wraps failures from the source as InputError naming the parameter, matching existing conventions.

A few things worth a look:

TVP.resolve always sets streamed: true (src/data-types/tvp.ts:246)

const data: ParameterData<TvpValue | null> = { value: null, streamed: true };

This is unconditional — even when rows is a plain in-memory array. Since RpcRequestPayload's constructor treats any streamed parameter as reason to switch the whole payload to the async iteration path (src/rpcrequest-payload.ts:44-50), every request containing a TVP parameter now goes through generateDataAsync, regardless of whether the rows are an array or an async source. That's a reasonable tradeoff given the array-TVP benchmark is itself ~2x faster, but it does mean the PR description's framing — "a request of ordinary in-memory values keeps the fully synchronous serialization path unchanged, so there is no async cost for the common case" — doesn't quite hold for the very common case of a TVP with an in-memory rows array. Worth confirming this is intentional (and maybe calling it out explicitly in the PR description/comments) rather than an oversight.

Dead code left in tvp.ts after the always-streamed change

Because TVP.resolve now always marks the parameter streamed, TVP parameters never go through writeValue()'s generateParameterLength/generateParameterData fallback anymore (src/data-types/tvp.ts:152-221), and TVP.validate (:223-241) is no longer reached via resolveParameter either (it's bypassed since TVP.resolve exists). These are only exercised now by their own direct unit tests (test/unit/data-type.ts:1469-1510, test/unit/validations-test.ts:387-395), not by anything in the live request-serialization path. Given this is a stacked migration series, that might be deliberate (deferred cleanup), but if not, it's ~100 lines that could be removed along with their now-vestigial tests.

No unit test crosses the CHUNK_SIZE flush boundary for the new streaming code

writePlpStream (src/data-types/plp-stream.ts:36-39) and writeRows/writeRowsFrom (src/data-types/tvp.ts:99-102, 117-120) all flush mid-stream once the accumulated buffer reaches WritableTrackingBuffer.CHUNK_SIZE (8 KiB) — that's the core of the memory-bounded streaming story. In test/unit/streaming-parameters-test.ts, the largest streamed-value equivalence check is 'the quick brown fox'.repeat(10) (200 bytes), and the TVP async-rows test uses 3 rows — neither ever exercises the buffer.length >= CHUNK_SIZE branch. The PR description says this was checked manually against a live SQL Server 2022 instance (3 MB binary, 400 KB nvarchar, etc.), which is great, but that isn't part of CI. A single added case per type (e.g. an async source yielding enough chunks to add up to ~20 KB, compared byte-for-byte against the in-memory serialization, and a TVP with enough rows to cross FLUSH_SIZE) would give CI-level coverage of the flush/consume bookkeeping — which is the part of this change most likely to have an off-by-one if anything does.

Minor / non-blocking

• The writeValueStream JSDoc on DataType (src/data-type.ts:124-131) and the module doc on plp-stream.ts are clear, but this is a new, user-facing capability (parameters can now be Readable/async iterables). It's not mentioned anywhere near the public TYPES reference table in data-type.ts, so users are unlikely to discover it without reading source or benchmarks. Worth a short mention/example there.
• stitchSurrogates in varchar.ts only special-cases a lone trailing high surrogate; that's correct (a lone low surrogate doesn't need cross-chunk stitching since it's already invalid on its own either way), but a one-line comment noting why the low-surrogate side doesn't need the same treatment would save the next reader from re-deriving it.

None of these look like correctness bugs in the byte-equivalence sense the PR is targeting — the logic checks out and the equivalence tests that do exist are solid. The TVP always-streamed point and the missing large-payload test are the two I'd actually want resolved (or explicitly acknowledged) before merging; the dead-code and doc points are cleanup-later material.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7274823e95

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

I went through the diff in detail (data-type.ts, plp-stream.ts, varbinary/varchar/nvarchar.ts, tvp.ts, rpcrequest-payload.ts, and the new tests) rather than just skimming. No CLAUDE.md exists in this repo, so I reviewed against the conventions already established by the surrounding code (the resolve/writeTypeInfo/writeValue contract from the stacked parameter-contract series).

Overall

This is a well-scoped, carefully engineered addition. A few things stood out positively:

• The sync/async duality in RpcRequestPayload ([Symbol.asyncIterator] only installed when a parameter is actually streamed) genuinely preserves the fully synchronous fast path for ordinary requests — verified by tracing generateData() vs generateDataAsync(), and covered by the "is synchronous with only in-memory values" / "is asynchronous when a value is streamed" tests.
• writePlpStream's skip of zero-length encoded chunks is correct and important — a 0-length PLP chunk would otherwise be misread by the server as the terminator. Good catch, and it's explicitly tested.
• The WritableTrackingBuffer flush/consume pattern (getBuffers() then consume(buffer.length)) is safe against aliasing — _seal() allocates a fresh _open buffer rather than reusing/mutating chunks already handed out, so there's no risk of a later write corrupting an already-yielded (but not yet drained) chunk.
• InputError propagation from a failing async source works correctly through yield * writeValueStream(...) wrapped in try/catch — this relies on generator-delegation semantics (a throw from the delegated iterator's .next(), even after a prior yield paused the outer generator, still lands in the enclosing try), and it's exercised by both the varbinary and TVP failure tests.
• Nice example of iterative hardening: the surrogate-pair-splitting bug (commit db690af) was caught by an earlier Codex review and fixed with a dedicated regression test before this review even started — the stitchSurrogates fix is correct for the general case (it only special-cases a lone trailing high surrogate, which is exactly the situation that can cause a split across an encode boundary; a lone low surrogate or a complete pair is unaffected).

Minor / non-blocking observations

1. TVP always takes the async path. TVP.resolve unconditionally sets streamed: true, so any parameter list containing a TVP forces RpcRequestPayload onto generateDataAsync() for the whole request — even a TVP with a handful of in-memory rows. That's called out in the code comments as intentional (TVP never had a synchronous writeValue to begin with), and the benchmark shows a clear win at 1K–1M rows. It'd be worth a data point (or just a sanity check) for a very small TVP (say, 1–10 rows) to confirm there's no observable regression from per-request async iterator overhead versus the old fully-synchronous TVP serialization, since that's presumably a common case too (e.g. small lookup-table TVPs).

2. Streaming detection is Symbol.asyncIterator-only. isAsyncIterable (and hence resolve in varbinary/varchar/nvarchar) only recognizes sources with Symbol.asyncIterator (e.g. Readable, async generators). A plain sync generator or a bare array of chunks silently falls through to validate() and fails with the generic "Invalid buffer."/"Invalid string." — accurate, but doesn't hint that only async iterables are accepted for streaming. Minor DX point, not a correctness issue, since users would presumably just concatenate a fully in-memory array anyway.

3. Codepage coverage in tests. The stitchSurrogates doc comment states the output is byte-identical to encoding the whole string at once "for every codepage," verified manually against SQL Server per the PR description. The unit tests, though, only exercise UTF-8 (plus UCS-2 for nvarchar) explicitly. Given how easy it is to get this subtly wrong for double-byte codepages, one additional unit test using a non-UTF-8 multi-byte codepage (e.g. a CJK codepage) split across a surrogate pair would make that "every codepage" guarantee self-verifying rather than resting on the PR description's manual verification.

4. Nit: RpcRequestPayload's [Symbol.asyncIterator]?: () => AsyncGenerator<Buffer, void>; field is declared without declare (unlike its sibling fields procedure, parameters, options, txnDescriptor, streamed, which all use declare). Functionally fine since it needs to be a real, conditionally-assigned own property, but a one-line comment noting why it's the odd one out would help a future reader avoid "fixing" the inconsistency.

Security / correctness

No injection or memory-safety concerns — this is purely wire-format serialization, buffers are written by length-prefix, and streamed sources are read, not eval'd or interpolated. I didn't find a case where a large/adversarial chunk size, an empty stream, or a thrown error from a user-supplied source leaves the connection or buffer state corrupted; errors are consistently surfaced as InputError naming the offending parameter.

Test coverage

Strong — streaming-parameters-test.ts covers resolve detection, the sync/async iterator switch on the payload, PLP correctness (including the empty-chunk skip and equivalence to in-memory serialization), the surrogate-pair fix, InputError propagation for both a raw streamed value and a TVP row validation failure mid-stream, and array-vs-async-iterable TVP equivalence. Combined with the byte-equivalence suite now running through the async path, this gives good confidence the change is behavior-preserving for existing (non-streamed) callers.

Nothing here blocks merging as far as I can tell — the observations above are polish/robustness suggestions rather than bugs.

[arthurschreiber]

Thanks — addressed the two you'd want resolved before merge:

• Flush-boundary coverage (f32e926): added CI tests that cross the CHUNK_SIZE flush/consume path — a 25 KB varbinary and a 30 KB nvarchar source in several chunks whose reassembled PLP must equal the concatenated input, and a ~2000-row TVP (well past the flush size) serialized from an array and an async source, asserted byte-for-byte equal (with a sanity assert that the request really exceeds one chunk). This is the off-by-one-prone bookkeeping you flagged.
• TVP always-streamed: confirmed intentional. It's now noted in TVP.resolve and called out explicitly in the PR description — a request with a TVP always takes the async path (array rows included), and the array case is still ~2x faster because it's no longer buffered whole.

Also added the stitchSurrogates low-surrogate comment, and documented the one-shot-source caveat (a Request with a streamed value can't be replayed) in the description.

On the deferred points: the dead TVP.validate / legacy generate* path and the TYPES-table discoverability note are both listed as follow-ups in the description — I'd rather not remove ~100 lines plus their direct tests inside this feature PR, but happy to do it as a small cleanup PR right after if you'd like.

---

Generated by Claude Code

[claude]

Review

Went through the diff in detail (data-type.ts, plp-stream.ts, varbinary/varchar/nvarchar/tvp.ts, rpcrequest-payload.ts, and both test files). Overall this is a clean, well-scoped extension of the write contract from #1776, and it's unusually well tested for a feature this size. A few notes below, nothing blocking.

Code quality / best practices

• The resolve → writeValueStream split mirrors the existing resolve/writeValue contract nicely, and factoring the shared PLP framing into plp-stream.ts (isAsyncIterable/writePlpStream) avoids repeating the flush/terminator logic across the three max types.
• RpcRequestPayload only installs [Symbol.asyncIterator] when a parameter is actually streamed, so the documented "no async cost for the common case" claim holds up — generateData's synchronous path is untouched other than being split into writeHeader/writeParameterHeader helpers that are now shared with generateDataAsync.
• WritableTrackingBuffer.getBuffers()/consume() are safe to call repeatedly across yield points here: _seal() allocates a fresh _open buffer before returning references to the sealed one, so later writes can't mutate a chunk that was already handed to the consumer. Worth confirming that was already the intended contract from perf: serialize an RPC request into one buffer #1776 rather than something this PR is newly relying on.
• stitchSurrogates is a nice, minimal fix for the UTF-8 surrogate-splitting bug found in review, and the comment explaining why only a trailing high surrogate needs holding back (not a leading low one) is exactly the kind of non-obvious invariant worth documenting.

Potential bugs / issues

• Nothing I could pin down as an actual defect. One design point worth double-checking with the author rather than a bug: TVP.resolve marks every TVP as streamed: true, even a 3-row in-memory array. That means any request containing a TVP always takes the async iteration path in RpcRequestPayload, forgoing the synchronous fast path that's the point of the perf: serialize an RPC request into one buffer #1776 stack for that request. This is clearly intentional (there's a comment and a dedicated test for it), but it's a behavior change worth calling out explicitly in the PR description's "Behaviour" section, since today it only says "No change for existing (in-memory) values" — that's true for varbinary/varchar/nvarchar but not quite for TVP.
• isAsyncIterable only recognizes Symbol.asyncIterator. A caller who passes a synchronous generator/iterable (has Symbol.iterator but not Symbol.asyncIterator) expecting it to stream will instead fall through to validate() and get a generic TypeError('Invalid buffer.')/'Invalid string.' with no hint that sync iterables aren't supported for streaming. Minor DX nit, not a correctness issue.
• Always Encrypted: Parameter carries forceEncrypt/cryptoMetadata/encryptedVal, and I didn't trace far enough to be sure how (or whether) a streamed value interacts with the AE encryption path (which presumably needs the whole plaintext in memory to encrypt it). Might be worth an explicit "not supported with AE" check or a test, if that combination is reachable today.

Performance

• The benchmark methodology (array vs Readable.from vs true async source, same row shape) is sound, and the ~2x number for the array case plausibly comes from avoiding the second full-array copy that the old generateParameterData path implied.
• Flushing at CHUNK_SIZE/FLUSH_SIZE (8 KB) boundaries is well covered by the new "crossing the flush boundary" tests (25/30 KB sources, ~2000-row TVP), which is exactly the part most likely to have off-by-one bugs in a hand-rolled buffer-list implementation, so good call adding those after the fact.

Security

• No new injection surface — TVP rows still go through column.type.validate before serialization, same as before.
• Errors from a failing async source are consistently wrapped as InputError naming the parameter (not leaking raw source data), matching the existing error-reporting convention.

Test coverage

• Strong: streamed-detection for all three max types, the sync/async iterator switch on the payload itself, PLP correctness (including empty-chunk skipping and byte-equivalence to the in-memory path), the surrogate-pair fix, the flush-boundary crossing, and both InputError propagation paths (source failure, TVP row validation failure). The declaration() coverage for the max form was a good catch to add since that's what execSql actually sends to sp_executesql.
• Two additions that might be worth it given the notes above:
  • A test asserting cancellation mid-stream (request.cancel()/payloadStream.destroy() in connection.ts) actually calls .return() on the source iterator, so a Readable handed in as a streamed value doesn't leak. This should already work via Readable.from's generator-destroy behavior, but there's no regression test pinning it down for this new code path.
  • A test for a synchronous (non-async) iterable/generator passed as a max/TVP value, just to lock in the current (fallback-to-validate-with-a-generic-error) behavior as intentional rather than accidental.

Nice work overall — the stacking on #1776, the byte-equivalence testing strategy, and the surrogate-pair fix in particular show careful attention to correctness.

[arthurschreiber]

Thanks for the thorough pass. Applied the two test/robustness items:

• Codepage coverage (042fe42): added a CP932 (non-UTF-8, double-byte) case to the surrogate-splitting test, asserting the stitched PLP equals iconv.encode('a😀b', 'CP932') — so the "byte-identical for every codepage" claim is now self-verified in CI, not just by the manual SQL Server check.
• [Symbol.asyncIterator] field (042fe42): added a comment explaining why it's a conditionally-assigned own property rather than a declared field (so Readable.from picks the sync iterator when nothing streams).

Also, separately, Codex flagged (and I fixed in d3c298a) a mismatched-TVP-row-length case — validateRow now rejects a row whose length doesn't match the columns, on both paths.

On the two notes I left as intentional tradeoffs:

• Small-TVP async overhead: a 1–10 row TVP does now take the async path. The added cost per request is the constructor's parameter scan plus one async iterator; against the actual row serialization it's negligible, and there's no buffering-whole regression. I didn't add a micro-benchmark for it, but happy to if you'd like a number.
• Symbol.asyncIterator-only detection: intentional for this PR's scope — a sync generator or bare chunk array isn't treated as a stream (a user with everything in memory would just pass the concatenated value). Noted as a possible DX follow-up.

519 unit tests passing, lint + typecheck clean. I think that covers the actionable findings from both reviews; the remaining points (dead-code cleanup, TYPES-table docs) are listed as follow-ups in the description.

---

Generated by Claude Code

[arthurschreiber]

Thanks — checked the new points:

• Always Encrypted: not reachable today. getParameterEncryptionMetadata and encryptWithKey have no callers in the send path, and encryptedVal/cryptoMetadata are never read by the serializer — parameter-level AE encryption isn't wired into request sending in the current codebase. So a streamed value can't reach an encrypt step; there's nothing to guard against yet. If/when parameter AE is wired up it will inherently need the whole plaintext in memory, and that's the right place to reject (or buffer) a streamed value — I'd rather not add a dead guard now.
• TVP always-streamed in Behaviour: already reflected — the description's Behaviour bullet is scoped to "existing (in-memory) scalar/max values", and the Change section calls out the TVP exception explicitly, with the code comment + test.

The two test suggestions I'll leave as optional follow-ups rather than push again:

• Cancellation closing the source: as you note, Readable.from's generator-destroy propagates .return() through the delegated yield* to the source's for await, so a cancelled request closes a Readable value — worth a pinning test, but it needs a connection-level harness rather than a payload unit test.
• Sync-iterable-as-stream fallback: locking in the current generic-error behavior is reasonable; goes with the Symbol.asyncIterator-only DX note as a small follow-up.

I've now worked through every actionable finding across the review rounds (surrogate splitting, flush-boundary coverage, TVP row-length mismatch, codepage coverage) and both bots agree nothing blocks merge, so I'll stop iterating here to avoid re-triggering reviews on each push. Remaining items are the follow-ups listed in the description. 519 unit tests passing, lint + typecheck clean.

---

Generated by Claude Code

[claude]

Review

I read through the diff against the stacked base (#1776) end-to-end: data-type.ts, the new plp-stream.ts, the varbinary/varchar/nvarchar/tvp changes, rpcrequest-payload.ts, and the new/updated tests. Overall this is a clean, well-scoped extension of the existing resolve/write-contract split, and the design choice to only go async when a parameter is actually streamed (keeping the synchronous fast path for the common case) is sound.

Correctness

• PLP framing (plp-stream.ts) — unknown-length marker, length-prefixed chunks with empty-chunk skipping, and the terminator all match the existing PLP wire format used elsewhere (varbinary/nvarchar's own PLP writers). Verified against the byte-equivalence tests.
• Surrogate-pair stitching (varchar.ts) — stitchSurrogates correctly holds back only a trailing high surrogate (a lone low surrogate encodes the same whether or not it's split, so it doesn't need special handling), and flushes a dangling high surrogate at end-of-stream the same way a whole-string encode would (as a replacement character). I traced through the edge cases (empty chunks, back-to-back high surrogates, a low surrogate arriving in a later chunk) and the output matches encoding the full string at once. Good catch adding the CP932 case too — that's the scenario most likely to silently regress if this were ever "simplified" back to per-chunk encoding.
• RpcRequestPayload — flushing the accumulated buffer before delegating to writeValueStream and resuming into the same (now-empty) buffer afterward preserves ordering correctly, including when a streamed parameter is followed by more in-memory ones. The [Symbol.asyncIterator] being a conditionally-assigned own property (rather than always present) is a nice touch — Readable.from picks the sync iterator when nothing is streamed, so non-streaming requests don't pay any async overhead.
• TVP — writeRows/writeRowsFrom reproduce the legacy generateParameterData byte layout (column metadata, end token, row token + cells, end token), and the 40-case suite now runs the TVP fixture through this path and diffs it against the legacy generator, which is good coverage for a refactor with this much surface area.
• Error propagation: wrapping both writeTypeInfo and writeValueStream calls in try/catch → InputError in generateDataAsync mirrors the existing synchronous writeParameterData behavior, and the "TVP row validation failure surfaces as InputError" test exercises the cause chain nicely.

Minor observations (non-blocking)

• UNKNOWN_PLP_LEN/PLP_TERMINATOR are now defined in four places (varbinary.ts, varchar.ts, nvarchar.ts, and plp-stream.ts). Since the three type files already import from plp-stream.ts for isAsyncIterable/writePlpStream, it might be worth also importing these two constants from there instead of redeclaring them, next time one of these files is touched — but I get why it wasn't bundled into this diff.
• TVP.validate (and the legacy generateParameterLength/generateParameterData it's paired with) is now dead on the live serialization path per the PR description — good that this is called out explicitly as a follow-up rather than silently left; worth a tracking issue so it doesn't linger.
• The one-shot-source caveat ("a consumed Readable or generator can't be replayed") is documented in the PR description but not anywhere a user would encounter it in code (e.g., near the public TYPES docs or writeValueStream's JSDoc). Also flagged as a follow-up already — agree it'd help discoverability, especially since a retried/resent request with a streamed parameter would fail in a confusing way (empty/partial data rather than an obvious error) if a caller didn't realize the source was already consumed.

Test coverage

Coverage looks thorough: streamed-detection per type, declaration() returning the max form, sync-vs-async iterator selection on the payload, PLP reassembly across the CHUNK_SIZE flush boundary, the surrogate-pair edge cases (UTF-8 and CP932), TVP array-vs-async-iterable equivalence at both small and >flush-size row counts, and InputError propagation from both a failing max source and a failing TVP row validation. I didn't spot a gap worth calling out — the flush-boundary tests in particular are the kind that are easy to skip and would have been the most likely place for a latent bug.

Security / Performance

No security concerns — this doesn't change validation semantics, only how already-validated bytes reach the wire, and the InputError path still names the offending parameter without leaking raw values. Performance-wise the benchmarks show what's claimed (bounded memory for large/streamed sources, ~2x for large array TVPs), and the design correctly avoids adding any async overhead to requests that don't use streaming.

Nice work — this is a substantial, carefully-tested change and I didn't find any correctness issues.

[claude]

Review

I traced the resolve → writeTypeInfo/writeValue/writeValueStream contract end-to-end (src/data-type.ts, src/rpcrequest-payload.ts, the three *(max) types, and tvp.ts), plus how Request/Connection drive it (request.ts:validateParameters, connection.ts:makeRequest). Overall this is a well-designed, carefully-tested piece of work. Highlights:

• The resolve-once / writeValueStream-once lifecycle is sound: Request.validateParameters (request.ts:506) calls resolveParameter exactly once per parameter and caches the result, and declaration() only calls isAsyncIterable() (no iteration) — so a one-shot async source genuinely gets consumed exactly once, matching the "sent once" caveat in the PR description.
• The conditional this[Symbol.asyncIterator] = this.generateDataAsync trick in the constructor (rpcrequest-payload.ts:46-53) is a nice way to keep the fully-synchronous fast path for the common (non-streamed) case, and it correctly relies on Readable.from preferring Symbol.asyncIterator over Symbol.iterator — verified both production call sites (connection.ts:2589, connection.ts:3298) use Readable.from(payload), so this holds today.
• The stitchSurrogates logic in varchar.ts is correct and its asymmetry with nvarchar.ts (which needs no stitching) is well-reasoned: UCS-2 always encodes 2 bytes/UTF-16 code unit regardless of surrogate validity, so only the iconv-based codepages (which may combine or replacement-char a split pair) need the carry-over buffering. The CP932/UTF-8 split-surrogate tests back this up nicely.
• Good regression coverage: the byte-equivalence suite in rpcrequest-payload-test.ts now runs through the async path too, and streaming-parameters-test.ts covers empty chunks, flush-boundary crossing, TVP array-vs-async equivalence, and mid-stream failures surfaced as InputError naming the parameter.

A few things worth a look before merging:

1. Undocumented buffer-aliasing contract for large streamed chunks. WritableTrackingBuffer#writeBuffer (tracking-buffer/writable-tracking-buffer.ts:285-297) stores/yields any buffer ≥ CHUNK_SIZE (8 KB) by reference, not copied. That applies to chunks coming out of a user's own async source for varbinary(max) (writePlpStream passes the chunk straight through in varbinary.ts's encode callback). If someone implements their streaming source with a reused/mutated scratch buffer for chunks ≥ 8 KB (a common pattern to avoid allocations), the request can be silently corrupted, since nothing copies it before it's queued for the socket. DataType.writeValueStream's doc comment (data-type.ts:124-131) doesn't mention this; worth a one-line callout there (and in any future user-facing docs for this feature).

2. No test exercises a single chunk ≥ CHUNK_SIZE. The "crossing the CHUNK_SIZE flush boundary" tests build up the crossing via several 5 KB chunks; none passes a single ≥8 KB chunk from an async source, which is the zero-copy writeBuffer branch this feature most wants to exercise. Worth adding for varbinary at least.

3. Discoverability. Request.addParameter/addOutputParameter (request.ts:448) still document value only in terms of the type's normal in-memory shape. Since value is already typed unknown, this streaming capability is reachable today through the existing public API with no compile-time hint — a short doc note on addParameter (and ideally the README/type docs) would help users actually find this.

4. Minor defensive gap. RpcRequestPayload#generateData ([Symbol.iterator], rpcrequest-payload.ts:56-73) doesn't check this.streamed at all. If a payload containing a streamed parameter were ever consumed via a plain for...of/spread instead of Readable.from/for await, TVP would throw an obscure generateParameterLength is not a function (it no longer implements the legacy generator methods), while varbinary/varchar/nvarchar would silently call .toString() on the async source (e.g. a Readable) and write garbage instead of erroring. Not reachable today (both real call sites use Readable.from), but a cheap guard (throw if this.streamed in generateData) would turn any future misuse into a loud failure instead of corrupted bytes on the wire.

5. Not a bug, just flagging for reviewer awareness: TVP.resolve now marks every TVP parameter streamed: true, even a plain in-memory array of rows, so any request containing a TVP now always goes through the async generateDataAsync path. This is explicitly called out and justified in the PR description (still ~2x faster despite going async), so just flagging it as an intentional behavior change worth a second set of eyes rather than an issue.

Nothing here blocks merging in my view — items 1–3 are documentation/coverage gaps, and 4 is a defensive nice-to-have. Nice work on keeping the non-streaming path byte-for-byte identical while adding this.

[arthurschreiber]

Good set of points — applied all three concrete ones in 29122b7:

1. By-reference aliasing — documented on writeValueStream's JSDoc: a buffer of CHUNK_SIZE or more is referenced, not copied, so a source must not reuse/mutate a yielded buffer until the request is sent. This is the real footgun for a zero-alloc streaming source.
2. Single ≥ CHUNK_SIZE chunk — added a test that streams one 20 KB varbinary chunk and asserts the reassembled PLP equals it, exercising the zero-copy writeBuffer branch directly.
3. Defensive guard — generateData (the synchronous iterator) now throws if the payload contains a streamed parameter, so sync-iterating a streamed payload fails loudly instead of writing garbage. Not reachable through the current call sites (both use Readable.from), but cheap insurance.

Points 3 (an addParameter doc note) and 5 (TVP always-streamed) are already tracked — 5 is documented in the description + code + test; 3 goes with the TYPES-docs follow-up in the description.

520 unit tests passing, lint + typecheck clean. That closes out the actionable review feedback for me — thanks for the careful passes.

---

Generated by Claude Code

[claude]

Review

Went through the full diff (data-type.ts, plp-stream.ts, varbinary.ts, varchar.ts, nvarchar.ts, tvp.ts, rpcrequest-payload.ts, and the new/updated tests) plus the underlying WritableTrackingBuffer it builds on. Overall this is a careful, well-scoped piece of work — the sync/async split is exercised well by the byte-equivalence suite, and the two Codex-flagged bugs (surrogate pair splitting, TVP row/column-count desync) were fixed with targeted, well-explained follow-up commits and regression tests. A few notes below, none blocking.

Strengths

• No cost for the common case is real. RpcRequestPayload only switches to the async iterator when a parameter is actually streamed (this.streamed set in the constructor loop), and generateData() throws loudly if a streamed payload is iterated synchronously instead of silently corrupting bytes — good defensive guard against the misuse the PR itself calls out (rpcrequest-payload.ts:64-66).
• writePlpStream/TVP flushing is correctly bounded. Traced through WritableTrackingBuffer.consume/getBuffers — sealing then consuming the full length correctly resets _bufs/_pos for reuse, and the >= CHUNK_SIZE flush check keeps memory bounded regardless of source size. The "single chunk ≥ CHUNK_SIZE" test exercises the zero-copy writeBuffer branch directly, which is exactly the case worth covering.
• Surrogate-pair stitching (varchar.ts) is correct: only a trailing high surrogate is ever held back (a lone low surrogate is already invalid and encodes the same either way), and it's verified against both a UTF-8 and a double-byte (CP932) collation — good, since it'd be easy to get this subtly wrong for one codepage family but not the other.
• TVP row-length validation (validateRow in tvp.ts) closes a real wire-desync bug: a short/long row would have thrown off the server's column-metadata parsing for every subsequent row. Good catch, and it's applied uniformly to both the array and async-iterable paths.
• Error propagation is consistent: a failing source (for await/yield* inside a try in generateDataAsync) is correctly caught and re-wrapped as InputError naming the parameter, matching the synchronous path's existing contract.

Suggestions (non-blocking)

1. Duplicated resolve() fallback logic across varbinary.ts/varchar.ts/nvarchar.ts. Each of the three files now has its own resolve() that (a) branches on isAsyncIterable and (b) otherwise re-implements the same validate → resolveLength → collation-assignment sequence that resolveParameter() in data-type.ts already does generically for types without a custom resolve. varchar.ts and nvarchar.ts in particular are nearly line-for-line identical here. Since this is the kind of logic that's easy to drift on future edits (e.g. someone fixes a length-resolution edge case in one file and forgets the other two), it might be worth a small shared helper in plp-stream.ts (which already hosts isAsyncIterable/writePlpStream) — something like a resolveMaybeStreamed(parameter, collation, type) that returns either the streamed ParameterData or falls through to the type's own validate/resolveLength.

2. parameter.type.writeValueStream!(...) non-null assertion in rpcrequest-payload.ts:109. This is safe today because every type that sets streamed: true is one of the four in this PR, all of which implement writeValueStream. But DataType is a public interface — if a third-party type sets streamed: true without implementing writeValueStream, the resulting TypeError: ... is not a function gets wrapped into InputError("Input parameter 'x' could not be validated"), which reads like a data problem rather than an implementation contract violation. A cheap if (!parameter.type.writeValueStream) throw new Error(...) (or just relying on the current wrap, if that's judged good enough) would make the failure mode clearer for anyone implementing a custom streamed type.

3. Integration coverage. The PR description mentions manual end-to-end verification against SQL Server 2022 (HASHBYTES/DATALENGTH comparisons, TVP integration tests passing), but that verification isn't captured as an automated test/integration case in this diff. Given the project already has integration tests for other parameter paths, it'd be worth adding at least one (streamed varbinary(max) and/or TVP-from-async-iterable round trip) so this doesn't rely on manual re-verification in the future. Understand this may already exist elsewhere or be planned separately — just flagging since it wasn't in the diff.

4. Minor: the comment on RpcRequestPayload's [Symbol.asyncIterator] field (rpcrequest-payload.ts:22-25) describes it as "installed as an own property... only when a parameter is streamed." With target: esnext (useDefineForClassFields on), an un-declared class field like this is actually defined as an own property with value undefined on every instance as part of field initialization, before the constructor body runs; the constructor's conditional assignment only overwrites the value on the streamed path. This doesn't change behavior (typeof undefined !== 'function', so Readable.from still correctly picks the sync iterator either way), so no functional issue — just the comment is slightly more absolute than the runtime semantics.

Other

• Performance numbers in the description look credible and are backed by the two benchmark scripts.
• Didn't spot any security concerns — this is wire-protocol serialization behind the existing parameterized-query boundary, no new injection surface.
• Didn't independently re-run the suite, but the diff to rpcrequest-payload-test.ts (sync → async collect/chunks helpers) looks like a faithful, minimal adaptation rather than a weakening of the existing byte-equivalence assertions.

Nice work — this is a substantial capability add (unbounded-size params, ~2x/lower-memory TVP serialization) landed with a genuinely thorough test suite.

[arthurschreiber]

Thanks — these are all fair, and none are correctness issues, so I'm going to hold the branch here rather than push another round (each push re-triggers the bot reviews, and the real findings across the earlier rounds — surrogate stitching, TVP row/column desync, flush-boundary coverage, the by-reference contract and sync-path guard — are all fixed and tested). Recording my take on each for whoever does the human review:

1. DRY the three resolve()s — agree they've drifted close to identical; a resolveMaybeStreamed(parameter, collation, type) helper in plp-stream.ts is the right shape. I'd fold this into the cleanup follow-up (with the dead legacy-TVP removal) rather than expand this PR's surface.
2. writeValueStream! assertion — reasonable; a if (!type.writeValueStream) throw with a contract-violation message reads better than the wrapped InputError for a third-party type. Happy to add if preferred, but it's guarding a case no in-tree type can hit.
3. Integration test — the manual verification is a streamed-vs-buffered server-side HASHBYTES/DATALENGTH comparison for all three max types (3 MB binary in uneven+empty chunks, 400 KB nvarchar, 240 KB varchar) plus TVP array-vs-async, but you're right it isn't committed. I can add a test/integration case (streamed varbinary(max) round-trip + TVP-from-async-iterable) — I'd rather do it as a focused follow-up so it can be shaped to the existing integration conventions.
4. Field comment — fair, with useDefineForClassFields the field is undefined on every instance and only reassigned on the streamed path; I'll tighten the wording when the file is next touched.

So: 1 and 3 are the two I'd genuinely want, and I think both belong in a small follow-up rather than more commits here. If you'd prefer any of them in this PR instead, say which and I'll add it. 520 unit tests passing, lint + typecheck clean.

---

Generated by Claude Code

[claude]

Review

This is a well-designed, well-tested addition on top of the #1774/#1779 write-contract refactor. Overall it's in good shape — the commit history shows the PR already went through several rounds of self-review (surrogate-pair splitting, TVP row/column-count mismatch, collation-check timing, dropping a dead guard), and each of those fixes is now backed by a regression test. A few notes below, none blocking.

Code quality / design

• The writeValueStream contract is clean: types opt in, resolve is the single place that decides in-memory vs. streamed, and plp-stream.ts factors out the PLP framing shared by varbinary/varchar/nvarchar. Good separation of concerns.
• RpcRequestPayload writing everything into one shared WritableTrackingBuffer and flushing at CHUNK_SIZE boundaries (instead of yielding a small buffer per parameter) is a nice perf win that doesn't compromise the zero-copy-for-large-values property — and it's covered by a test that asserts the by-reference buffer still shows up unchanged in the output.
• Making RpcRequestPayload async-only (rather than keeping a dual sync/async path) is the right call given every call site already goes through Readable.from; the PR body's own measurements back up that the "fast path" wasn't actually faster.
• Good catch fixing the streamed-varchar collation check to happen in resolve() instead of only failing once writeValueStream starts running — that keeps the "fails before the request starts" behavior consistent between in-memory and streamed values.

Potential edge cases (minor, non-blocking)

• Request.addParameter doesn't guard against combining options.output = true with an async-iterable value. It's a narrow case (an output parameter's "input" value being a stream), but since it isn't semantically obviously meaningful, it might be worth an explicit rejection so misuse fails clearly rather than silently doing something the caller didn't intend.
• writeColumnMetadata in tvp.ts still builds each column's TYPE_INFO without a collation, so a varchar/nvarchar TVP column can't specify one — but this matches the pre-existing (pre-PR) generateTypeInfo(column) call, so it's not a regression from this change, just a pre-existing limitation that this refactor made more visible while it was in the neighborhood.
• The "flush at CHUNK_SIZE" bound for TVP rows (and PLP chunks) is checked between rows/chunks, not within one; a single pathologically large row/chunk can still push memory well past CHUNK_SIZE before the next flush point. That's inherent to any chunked design and is a reasonable tradeoff, just worth knowing it's a soft, not hard, bound.

Performance

Matches expectations: no extra copy for buffer-backed values, bounded memory for streamed sources, and the benchmark numbers in the description (added tvp-rows.js/tvp-rows-async.js) make the win visible and reproducible. Good practice keeping the sync-vs-stream distinction benchmarked separately.

Security

No concerns — parameterization is preserved throughout (values still flow through the same type validators before being written), no new string concatenation into SQL text, and the collation/encoding checks fail closed (throw) rather than silently mis-encoding.

Test coverage

This is thorough: byte-equivalence between streamed and in-memory serialization, empty-chunk skipping, chunks that straddle the CHUNK_SIZE flush boundary, a chunk larger than CHUNK_SIZE (exercising the zero-copy path directly), surrogate-pair handling across multiple codepages, TVP row/column mismatch, mid-stream source failure (both for max types and TVP rows) leaving the connection usable afterward, and the same paths through execSql, callProcedure, and a prepared statement's execute. The integration suite's per-test random naming for TVP types/procedures to survive parallel CI runs against a shared database is a nice touch.

Nothing here blocks merging; the two minor points above could be follow-ups rather than changes to this PR.

[claude]

Review: streaming max and TVP parameters from async sources

This is a well-designed and thoroughly tested addition. The writeValueStream contract is a clean generalization of the existing writeValue, RpcRequestPayload reusing the same one-buffer-with-flush design as BulkLoadPayload keeps the two payload types consistent, and the by-reference behavior for large chunks preserves the zero-copy property the earlier refactor established. The incremental commit history shows several rounds of self-review already caught the trickiest issues (surrogate-pair splitting, TVP row/column length mismatches, checking varchar's collation in resolve instead of only at write time) — nice to see those land as separate, well-explained fixes rather than being squashed away.

A few things worth a look:

Possible bug: silently sending truncated/empty data on request retry

Request.addParameter's doc comment correctly notes that "a request that carries [a streamed value] can be executed only once," but nothing enforces this. If a caller (an ORM, a retry wrapper around execSql/callProcedure) retries the same Request object after a transient connection error, an already-exhausted async iterable will produce zero PLP chunks or zero TVP rows on the second attempt — i.e. the retry silently sends an empty value instead of erroring loudly. Given how common blanket retry-on-connection-error logic is around DB clients, a defensive check, or at least calling this out more prominently, might be worth considering, since a silently-wrong value is worse than a thrown error. This may be outside this PR's scope, but it's a sharp edge that isn't covered by any test (all the tests use a fresh generator per request).

Documentation gap: buffer-reuse contract isn't visible to end users

DataType.writeValueStream's JSDoc (src/data-type.ts) and the internal "harden and document the streamed-value buffer contract" commit note that a Buffer chunk of CHUNK_SIZE (8 KB) or more is referenced rather than copied, so the source must not reuse or mutate a yielded buffer until the request finishes sending. That's the right contract for internal implementers, but it's not mentioned in Request.addParameter's public doc comment (src/request.ts), which only documents the string surrogate-pair rule. A user writing a naive async generator that reuses a single scratch buffer across reads (a common pattern for perceived memory efficiency) would get silently corrupted data with no error, only for values ≥ 8 KB per chunk. Worth a one-line addition to the public docs alongside the surrogate-pair note.

Minor

• RpcRequestPayload's for await (const _ of parameter.type.writeValueStream!(...)) with an eslint-disable for the unused binding works but reads a little awkwardly; a manual .next() loop (as e.g. writeRows/writeRowsFrom do internally via plain generators) would avoid the disable comment, though this is purely stylistic.
• plp-stream.ts's isAsyncIterable duck-types on Symbol.asyncIterator — correct and matches how Readable exposes it, just flagging that any object with a coincidental [Symbol.asyncIterator] method would now be routed to the streaming path instead of failing type validation the way it did before. Not a real-world concern given the types involved (Buffer/string/array vs. an intentional async source), just noting the widened surface.

Test coverage

Coverage is strong: the 40-case byte-equivalence suite exercising both old and new serialization paths, the CHUNK_SIZE-boundary-crossing tests for both PLP and TVP rows, the surrogate-pair/codepage tests, and the integration tests that verify the connection remains usable after a mid-stream failure are all exactly the kind of cases that are easy to skip and easy to get wrong. The TVP row-length mismatch test (guarding against desyncing the server's row parser) is a great catch. I didn't find a test exercising the "retry with an exhausted source" scenario mentioned above — might be worth adding one that at least documents the current (silent) behavior, even if the fix is deferred.

Performance

Measurements are clearly presented and the reasoning about why the scalar/array-TVP cases are unaffected while streamed sources add only a small fixed per-request cost is sound. No concerns here.

Security

No new concerns. TVP type/schema/name serialization and RPC parameter encoding reuse existing, unchanged encoding paths.

[claude]

Review: streaming max/TVP parameters from async sources

This is a well-scoped follow-up to #1774's write contract, and the iteration history shows good responsiveness to earlier review feedback (surrogate-pair handling, the TVP row-length desync fix). I read through data-type.ts, plp-stream.ts, varbinary.ts/varchar.ts/nvarchar.ts, tvp.ts, and rpcrequest-payload.ts against the underlying WritableTrackingBuffer (getBuffers/consume/writeBuffer's by-reference threshold) to check the flush bookkeeping, plus the makeRequest/Readable.from consumers.

Strengths

• writeValueStream writing into the caller's buffer (rather than owning one) is a nice simplification over the earlier revision — one flush point in RpcRequestPayload, no sync/async union to juggle.
• The flush arithmetic checks out: getBuffers() seals and returns a stable snapshot array, consume(buffer.length) only discards after the whole snapshot has already been handed to the consumer via yield*, and buffers >= CHUNK_SIZE are referenced rather than copied end-to-end (writeBuffer), so the "large value stays by reference" claim holds.
• TVP.validateRow's new length check (src/data-types/tvp.ts:43-45) fixes a real latent bug: the pre-existing generateParameterData iterated row.length without checking it against columns.length, so a short/long row would silently desync the server's parse of subsequent rows. Good catch, and it's covered by both unit and integration tests.
• Test coverage is thorough: byte-equivalence between array/async TVP paths, CHUNK_SIZE boundary crossing, empty-chunk skipping, InputError propagation (including "connection still usable afterward" checks), and a real per-codepage surrogate-pair regression test.

Findings

1. Silent truncation on stream reuse (design note, not a specific line) — Request.addParameter's doc (src/request.ts:447) correctly says a request carrying a streamed value "can be executed only once," but if a caller does accidentally reuse one (e.g. calls execSql twice on the same Request, or any future retry path touches this), the already-exhausted async iterable just yields nothing on the second pass. writePlpStream/writeRowsFrom will happily write an empty PLP value or zero rows instead of erroring — i.e. the failure mode is silent data truncation rather than a loud error. Given tedious doesn't currently retry requests internally this may be acceptable, but it might be worth a cheap guard (e.g. a done flag flipped after the first iteration that throws a clear "already sent" error on a second attempt) since the current failure mode is the worst kind — wrong data, not a crash.

2. Dead code left in tvp.ts — generateParameterLength/generateParameterData (src/data-types/tvp.ts:150-219) and the array-only check duplicated in validate (:221-239) are no longer reachable from the live serialization path now that TVP.resolve always sets streamed: true. This is already called out in the PR description as a planned cleanup, just flagging so it doesn't get lost — right now these ~90 lines are untested-by-omission dead code sitting next to the real implementation, which is a minor maintenance/readability tax until the follow-up lands.

3. Duplicated collation guard in varchar.ts (src/data-types/varchar.ts:142-148 vs 164-169) — resolve already checks for a missing collation/codepage before setting streamed: true, collation, so by the time writeValueStream runs, parameter.collation is guaranteed valid (every caller goes through resolve first). The re-check in writeValueStream can't be reached via any current call path — a defensible defensive-coding choice, but per the "don't add error handling for scenarios that can't happen" guideline it could be dropped or reduced to a comment referencing resolve's guarantee.

4. Per-row/per-cell allocation in writeRow (src/data-types/tvp.ts:82-92) — a new ParameterData object is allocated per column per row, and validateRow allocates a new Array per row. For very large TVPs this is real GC pressure (the PR's own measurements attribute the Bit-column slowdown partly to this style of dispatch). Already flagged in the PR body as a follow-up (per-type migration off the legacy generate* path), so no action needed now — just confirming it's the known tradeoff and not something introduced silently.

5. Minor: since RpcRequestPayload dropped its own guard for "streamed but no writeValueStream" (relying on "only tedious's own types set streamed, and they all implement it"), a third-party DataType implementation that sets ParameterData.streamed = true from a custom resolve without also implementing writeValueStream will fail with an opaque TypeError: ... is not a function wrapped in the parameter's InputError. Worth a one-line note on the streamed field or writeValueStream JSDoc in data-type.ts making that pairing an explicit contract for type authors, since DataType is part of the public typings surface.

Security / correctness

No injection or memory-safety concerns — values are still routed through the same validate/encoding paths as the in-memory case, just incrementally. The by-reference buffer semantics are clearly documented for callers who might otherwise mutate a yielded chunk.

Test coverage

Strong. Unit tests hit the resolve/streamed-detection, flush-boundary, and error-propagation cases; integration tests round-trip real values against a server, including a prepared-statement path and TVP failure mid-stream. I'd only add a small case for validateRow's "row is not an array" branch (currently only the length-mismatch and per-column-validation branches are exercised).

Nice work overall — this is a substantial perf/capability win (streamed max values, 2x+ faster array TVPs) with the byte-for-byte compatibility guarantees carefully preserved and tested.

[claude]

Reviewed the diff (12 files, ~1100 additions). This is a clean, well-scoped addition on top of the #1774 write contract, and the PR description is unusually thorough about invariants and edge cases. Went through writeValueStream, the plp-stream.ts helper, the TVP row-writing paths, and the new RpcRequestPayload async generator line by line, plus the two test suites. A few notes, nothing blocking:

Correctness

• Verified the streaming resolve() overrides in varbinary.ts/varchar.ts/nvarchar.ts reproduce the default (non-resolve) path in resolveParameter (data-type.ts:151) exactly for the non-streamed branch (value → validate, length precedence, collation), so the "byte-for-byte identical" claim for existing in-memory values checks out against the code, not just the 40-case test.
• The double-wrapping of InputError for TVP row validation (validateRow throws an InputError, which RpcRequestPayload's catch wraps in another InputError naming the parameter) is a little unusual but is deliberate and exactly what the unit tests assert (cause chain), so it's fine — just flagging in case the double "InputError" nesting wasn't intentional at the call site.
• MAX (65535) vs maximumLength (8000/4000) — double-checked the streamed-length invariant asserted in streaming-parameters-test.ts:1189 (resolved.data.length! > maximumLength) actually holds; it does, since maximumLength is much smaller than the (1<<16)-1 used for the streamed case.
• The UCS-2 surrogate-pair chunking note on Request.addParameter is correctly scoped to varchar (codepage-based iconv.encode), not nvarchar — ucs2 encodes each UTF-16 code unit independently, so splitting a surrogate pair across chunks there is provably lossless, and the nvarchar integration test correctly does exercise emoji split across chunk boundaries while the varchar test deliberately avoids it. Nice attention to detail.

Minor/non-blocking observations

• The get-buffers-then-consume idiom (yield * buffer.getBuffers(); buffer.consume(buffer.length);) is repeated three times in rpcrequest-payload.ts's [Symbol.asyncIterator]. Could be a tiny private helper (flush()), but it's only 2 lines each and arguably clearer inline — not worth blocking on.
• Test coverage is strong on data correctness (byte-for-byte reassembly, empty-chunk skipping, error propagation, array-vs-async TVP equivalence) but there's no unit test asserting that a streamed Buffer chunk ≥ CHUNK_SIZE is passed through by reference (the scalar large-value test does check object identity via assert.include, but the streamed large-chunk test only checks byte equality). Might be worth an analogous identity check given how much the PR description leans on "large value written by reference stays by reference" as a perf claim.
• Request's doc comment now correctly documents the one-shot nature of a streamed request, but it might be worth a follow-up check (not this PR) on whether any retry path ever re-sends a Request/payload object — I checked connection.ts's transient-retry logic and it only retries the login/connect sequence, not in-flight requests, so no conflict today, but it seems like exactly the kind of invariant that could silently regress later without an explicit assertion/guard.

Overall: solid design, the shared-buffer/async-generator restructuring of RpcRequestPayload is a nice simplification over the old per-parameter-buffer approach, and the streaming contract (writeValueStream yields, caller owns flushing) is consistently applied across all three max types and TVP. No security concerns — this only changes how already-parameterized values are serialized, not how they're interpreted by the server.

[claude]

Review

This is a well-scoped, carefully engineered PR. The split between resolve (detect async source → mark streamed, force the (max) declaration) and writeValueStream (read-as-you-write into the caller's buffer) is clean, and the byte-equivalence tests against the pre-#1774 "legacy" serialization (including the TVP null/array/async cases) give good confidence that behavior for existing in-memory values is unchanged. I traced through WritableTrackingBuffer.getBuffers()/consume(), the chunk-flush logic in RpcRequestPayload, and the TVP NULL_TABLE/column-metadata paths by hand against the pre-PR code and didn't find a byte-level discrepancy.

Correctness

• The mid-stream failure path (writePlpStream/writeRows* throw → wrapped as InputError in RpcRequestPayload) relies on bytes already having been handed to Readable.from(payload)/message before the error surfaces. That's pre-existing infrastructure (the payloadStream.once('error', ...) → message.ignore = true; message.end() path in connection.ts), not new, and it's covered by both a unit test and a real-server integration test ("leaves the connection usable"), so this looks solid rather than newly risky.
• One thing worth a conscious decision (not necessarily a fix): a Request carrying a streamed value is documented as single-use because the source (e.g. an async generator) can't be replayed. If application code ever re-executes the same Request instance (retry logic, etc.), the second run won't error — an already-exhausted async generator's for await loop just completes immediately, so the parameter silently serializes as empty/null instead of failing loudly. Since tedious itself doesn't retry in-flight requests, this only bites intentional re-execution, but a silent empty-TVP-insert instead of a thrown error is a rough edge if it does happen. Might be worth a short callout in the same doc comment, or out of scope for this PR — your call.

Code quality

• UNKNOWN_PLP_LEN/PLP_TERMINATOR/MAX are now defined in four places (varbinary.ts, varchar.ts, nvarchar.ts, and the new plp-stream.ts). It already followed this per-file-constant pattern before the PR, so it's consistent, but since plp-stream.ts is now the shared home for PLP writing, it could also be the single source of these constants for the three *.ts files that import from it.
• test/unit/streaming-parameters-test.ts's plpData() helper locates the PLP value via buffer.lastIndexOf(UNKNOWN_PLP_LEN). It's safe for the fixed/patterned buffers used today, but would be fragile if a future test fed it high-entropy random data that could coincidentally contain that 8-byte marker — worth keeping in mind if the suite grows.

Performance

Matches the PR description's own measurements (single flush buffer per request, by-reference passthrough for large chunks/TVP rows preserved). Nothing to add here — the benchmark additions (tvp-rows.js, tvp-rows-async.js) are a nice touch for keeping the claims reproducible.

Security

No issues spotted. Chunk lengths are always the actual byte count of encoded data (no user-controlled length fields written independently of the data itself), and the streamed cases are still subject to the same validate-equivalent checks (buffer/string type checks, TVP row/column-count checks) as the in-memory path, just performed incrementally instead of up front.

Test coverage

Strong — unit coverage for resolve detection, declaration, chunk-boundary flushing, PLP reassembly/empty-chunk skipping, TVP array-vs-async equivalence (including >CHUNK_SIZE cases), and error propagation (source throw, invalid TVP row shape/values), plus integration tests round-tripping real values through execSql, a prepared statement, and a TVP procedure against a live server, including the "connection stays usable after a mid-stream failure" case. I didn't see a test for the always-encrypted parameter path interacting with a streamed value, but if Always Encrypted isn't wired into resolve()/writeValue at all yet in this codebase (I didn't find forceEncrypt/cryptoMetadata referenced anywhere in data-type.ts's write contract), that's likely just out of scope rather than a gap.

Nice work overall — this is a substantial change handled with a lot of care for byte-level compatibility.

[claude]

Review

This is a well-scoped, carefully documented PR that extends the resolve/write contract from #1774 with a genuinely useful capability (streaming max/TVP values from async sources). Overall quality is high: the design keeps the "one buffer, flush at chunk boundaries" model from BulkLoadPayload, the byte-equivalence test suite gives strong confidence nothing regressed on the in-memory path, and the new unit/integration tests cover the interesting edge cases (empty chunks, flush-boundary crossing, mid-stream failures, prepared statements, shared-DB test isolation for TVP types).

A few things worth a look before merging:

Correctness / robustness

RpcRequestPayload[Symbol.asyncIterator] yields from inside the try block for streamed parameters (src/rpcrequest-payload.ts).

try {
  writeTypeInfo(parameter.type, buffer, parameter.data, this.options);
  if (parameter.data.streamed) {
    for await (const _ of parameter.type.writeValueStream!(buffer, parameter.data, this.options)) {
      yield * buffer.getBuffers();          // <-- yield happens inside the try
      buffer.consume(buffer.length);
    }
  } else {
    writeValue(parameter.type, buffer, parameter.data, this.options);
  }
} catch (error) {
  throw new InputError(`Input parameter '${parameter.name}' could not be validated`, { cause: error });
}

Previously (pre-PR), yield * buffer.getBuffers() always happened outside the try/catch. Now, for a streamed parameter, the yield point sits inside the try. If anything ever resumes this generator via .throw() while it's suspended at that yield (rather than .return()), the resulting error — which has nothing to do with parameter validation — would get wrapped and mislabeled as Input parameter 'x' could not be validated.

In practice this is low-risk today: Readable.from() tears down an async generator via iterator.return() on destroy, not .throw(), and connection.ts's payloadStream.once('error', ...) just unpipes rather than injecting back into the source. But it's a latent trap for future consumers of this async iterable (direct callers, a different stream adapter, etc.). Worth either narrowing the try to just the synchronous write calls (keeping the yields outside, as before) or adding a comment noting the assumption that nothing calls .throw() on this iterator.

Minor / non-blocking

• Output parameters + streamed values aren't guarded. addOutputParameter combined with an async-iterable value isn't explicitly rejected anywhere. Semantically a one-shot streamed source doesn't make sense as an output parameter's "initial" value. Not urgent (probably nobody does this), but a clear InputError at resolve() time would be friendlier than whatever currently happens.
• TVP.generateParameterLength/generateParameterData are now dead on the live path since TVP.resolve always sets streamed: true. The PR already calls this out explicitly as a planned follow-up cleanup, so just flagging that I noticed the same thing and agree with deferring it.
• The writeRows (array) vs writeRowsFrom (async iterable) duplication in tvp.ts is intentional per the PR description (avoids a promise-per-row cost for the array case), and the measurements back that up, so no complaint there — just noting it's a deliberate perf trade-off for anyone re-reading the code later without the PR description in hand.

Test coverage

Coverage is strong: byte-for-byte equivalence for existing scalar/TVP serialization, streamed vs. in-memory equivalence for all three max types, flush-boundary crossing, empty-chunk skipping, mid-stream source failure surfaced as InputError with connection left usable, and TVP row-shape validation (wrong length, non-array row, out-of-range cell) for the async path specifically. The one gap I'd call out is the output-parameter + streamed-value combination mentioned above — not necessarily worth a dedicated test, but worth an explicit decision (support vs. reject).

Performance / security

No concerns. The chunk-size-based flush/by-reference threshold reuse from WritableTrackingBuffer (#1774) is a clean way to avoid extra copies, and the measurements in the description are consistent with the implementation (single shared buffer, PLP chunks written directly, TVP rows validated before being written so a bad cell fails fast without corrupting the wire format). No new input trust boundaries are introduced — values are still validated per-cell/per-chunk before being serialized.

Nice work overall — this is a substantial extension implemented with a lot of care for backward compatibility and test rigor.

[claude]

Review: Streaming parameter values (async iterable support)

This is a well-designed, carefully-scoped addition -- writeValueStream mirrors writeValue's signature plus one yield, so a type author only has to reason about one extra concept. The comments explain why (chunk-size flush boundary, PLP zero-length-chunk ambiguity, surrogate-pair splitting) rather than restating the code, which fits this codebase's style well. Test coverage is genuinely strong: the byte-for-byte parity assertions between the array-based and async-iterable serialization paths (rpcrequest-payload-test.ts, streaming-parameters-test.ts) are exactly the right thing to assert here, and the integration tests exercise real round-trips, prepared statements, mid-stream failures, and connection reuse after a failed streamed request.

Correctness

• I traced whether tedious itself could ever re-consume an already-exhausted async source (e.g. via an internal retry). It can't: execSql/callProcedure/execute each resolve parameters once and hand them to a single makeRequest call, and the only retry loop (performTransientFailureRetry) runs during login, before any Request payload exists. So the "one-shot" caveat documented on Request.addParameter only matters for user-level reuse (e.g. manually retrying the same Request/value object), which is correctly called out in the JSDoc.
• One rough edge: in rpcrequest-payload.ts, the call that obtains the type's async iterator (parameter.type.writeValueStream!(buffer, parameter.data, this.options)[Symbol.asyncIterator]()) sits outside the try/catch that turns failures into InputError. For every built-in type this is safe today (it just returns an already-constructed generator, doing no real work until .next()), but if a custom/third-party DataType ever set data.streamed = true without implementing writeValueStream, the resulting TypeError: ... is not a function would escape as a raw error instead of the usual "Input parameter 'x' could not be validated" InputError. Might be worth a defensive check, or at least a comment noting the invariant is load-bearing for custom types.

Design / code quality

• TVP.generateTypeInfo/generateParameterLength/generateParameterData are kept only to satisfy the (non-optional) legacy DataType interface -- since resolve() now always marks a TVP as streamed, these methods are never called from tedious's own runtime path anymore. That's clearly commented ("still required by the DataType interface"), so it reads as a deliberate, understood trade-off rather than an oversight -- just flagging for reviewers who might otherwise wonder why TVP still carries ~70 lines of code that never executes internally.
• The finally/flushes.return() cleanup in RpcRequestPayload, and the corresponding "closes the source when the consumer stops early" / "does not relabel a consumer's thrown error" tests, are a nice touch -- this is exactly the kind of generator-cleanup edge case that's easy to get wrong and easy to skip testing.
• plp-stream.ts's isAsyncIterable check is shared across varbinary, varchar, nvarchar, and tvp via one helper rather than four copies -- good reuse of the one nontrivial predicate.

Performance

• The core value proposition here (no full second in-memory copy for large TVPs, ability to stream values larger than memory, buffers >= CHUNK_SIZE still passed by reference) is real, and the benchmarks added (tvp-rows.js / tvp-rows-async.js) are a good way to keep it honest over time.
• Rows given as a plain array are still written in a synchronous inner loop with only a periodic yield (not one yield per row), which avoids turning the common case into a slow per-row-async-tick operation -- good attention to detail.

Security

• Nothing concerning. Chunk encoding is validated per-chunk (TypeError on the wrong JS type for the value being streamed), the collation/codepage check for a streamed varchar happens eagerly in resolve() rather than being deferred into the write path, and there's no new user input reaching SQL text or dynamic property access.

Minor nit

• VarBinary.resolve documents "an explicitly specified length is deliberately overridden" for the streamed case; the equivalent comments on VarChar.resolve/NVarChar.resolve don't spell out the same override behavior even though it applies identically. Not a functional issue, just a small documentation consistency gap.

Overall this looks solid and ready modulo the two flagged points above (neither of which looks blocking).

[claude]

Review

This is a well-designed and unusually thoroughly-tested change. A few observations by category:

Architecture / code quality

• The writeValueStream(buffer, parameter, options) contract is a clean extension of the existing writeValue contract from refactor: resolve parameters once and serialize them through a write contract #1774: the type writes into the caller's buffer and yields only when a chunk's worth has accumulated, so the flush/consume bookkeeping lives in exactly one place (RpcRequestPayload) instead of being duplicated per type. plp-stream.ts factoring the PLP framing out of varbinary/varchar/nvarchar is a nice reduction in duplication.
• RpcRequestPayload's async generator correctly keeps the type's writeValueStream iteration (flushes.next()) wrapped in the InputError-relabeling try, while keeping the yield chunk calls to the consumer outside it, with a finally that closes the type's generator (and transitively the value's source) on early consumer stop. That's a subtle bit of generator plumbing to get right, and the two dedicated unit tests for it ("does not relabel an error the consumer throws into it" / "closes the value's source when the consumer stops early") are exactly the right way to pin it down.
• One nit for future maintainers: VarChar.writeValueStream does const codepage = parameter.collation!.codepage!; synchronously, before returning the writePlpStream generator — and that call happens outside the try in RpcRequestPayload (only the generator's .next() calls are wrapped). This is safe today only because VarChar.resolve already validates collation/codepage up front and is the only path that sets streamed: true. If a future type implements writeValueStream and does non-trivial synchronous work before the first yield, an exception there would surface as a raw error instead of an InputError naming the parameter. Might be worth a short comment on the DataType.writeValueStream doc (or a defensive wrap in RpcRequestPayload) noting that synchronous work before the first yield isn't covered by the parameter-error wrapping.

Potential bugs

• Nothing outstanding found — the trickier correctness issues (the UTF-16 surrogate-pair split across streamed varchar chunks, the TVP row/column-count desync, and the yield*-over-an-array .throw() gap on newer Node versions) were already caught and fixed within this same PR's commit history, and each has a regression test.
• Request.addParameter's doc now correctly calls out that a request carrying a streamed value is single-use (a consumed Readable/generator can't be replayed). There's no runtime guard against accidentally executing the same Request twice, though — a caller that retries a failed request naively would get a silently truncated/empty value on the second attempt rather than an obvious error. That's a reasonable trade-off given arbitrary async iterables can't generally be introspected for "already consumed," but it's worth flagging since it's an easy footgun that only a docstring protects against.

Performance

• The measurements in the PR description are convincing and the benchmark additions (tvp-rows.js, tvp-rows-async.js) make the claims reproducible. Consuming through Readable.from either way (confirmed by the earlier revision that kept a sync iterator and measured no improvement) is a good justification for simplifying to async-only rather than maintaining two code paths.
• Buffering into one WritableTrackingBuffer per request/flush and passing large chunks by reference (verified by identity in rpcrequest-payload-test.ts) avoids the extra copy that would otherwise erase the benefit of streaming a large value.

Security

• No concerns. The change is purely about how already-validated parameter bytes are framed and handed to the socket; no new input trust boundaries are introduced. Row/value validation (validateRow, validateTable, per-column validate) still runs before bytes are written, whether the source is an array or an async iterable.

Test coverage

• Excellent — unit tests cover streamed detection, declaration()'s max form, chunk-boundary flushing, by-reference chunk passthrough, TVP row-count/type mismatches, early-consumer-stop cleanup, and error identity preservation; integration tests round-trip real streamed values (including uneven/empty chunk boundaries and a mid-stream failure) against a live server and confirm the connection stays usable afterward. The 40-case byte-equivalence suite being run through the async payload is a nice way to get TVP coverage "for free."
• Minor gap: I didn't see a unit test for a TVP whose rows is neither an array nor an async iterable (e.g. rows: {} or rows: null on a non-null table) — validateTable would throw TypeError('Invalid table.') for that today, presumably wrapped the same way as the other validate failures, but an explicit test would make that path as well-pinned as the others.

Overall this looks like solid, careful work — happy to see this land pending CI.

[claude]

Review

I read through the diff (data-type.ts, plp-stream.ts, tvp.ts, varbinary.ts/varchar.ts/nvarchar.ts, rpcrequest-payload.ts, and the unit/integration tests) end to end, focusing on the trickiest part of this change: the manual async-generator plumbing in RpcRequestPayload's [Symbol.asyncIterator] that drives a type's writeValueStream.

Overall this is a well-designed, well-tested change. A few observations, nothing blocking:

Correctness

• The manual flushes.next()/flushes.return() handling in rpcrequest-payload.ts gets the subtle cases right: a thrown error from the source is wrapped as InputError without double-closing the generator (done = true before rethrow, since a throwing generator is already finished); an early consumer stop (break/return()) closes the source via flushes.return(); and a consumer-injected .throw() propagates untouched because it lands on a bare yield rather than inside the try that wraps InputError. The dedicated unit tests for each of these (source-closes-on-early-stop, error-not-relabeled, InputError-naming) match the implementation, and I couldn't find a path that double-closes or leaks the source's generator.
• Reusing one cell (ParameterData) object per TVP column across rows is safe even for by-reference large buffers: WritableTrackingBuffer.writeBuffer stores the Buffer object itself in its internal chunk list, so reassigning cell.value on the next row never mutates a chunk that's already queued for a previous row.
• The TVP legacy generateTypeInfo/generateParameterData methods (now dead on the live path, kept only to satisfy the DataType interface) are still automatically checked byte-for-byte against the new path — test/unit/rpcrequest-payload-test.ts's parameters() fixture includes both a populated and a null TVP, and the "serializes ... exactly as before" tests exercise legacyPayload (which calls the real generateTypeInfo/generateParameterData) against RpcRequestPayload. Good, since the PR description mentions this was otherwise only checked "by hand."

Minor/consider

• TVP.resolve always marks a TVP as streamed: true, even for a tiny in-memory array (e.g. a handful of rows) or a null table. That's a reasonable simplification (no synchronous writeValue to fall back to) and is already covered by benchmarks, so this is just a note, not a request for change.
• The by-reference caveat documented on Request.addParameter ("A Buffer chunk of 8 KB or more is sent by reference... a source must not reuse or modify it") is phrased around streamed sources. The same by-reference behavior already applied pre-PR to any varbinary(max) value ≥ CHUNK_SIZE, including a value sitting inside a TVP row given as a plain array — worth double-checking the doc comment reads naturally for that case too, though it's pre-existing behavior rather than something new here.
• Nice touch avoiding yield* for both the buffer-chunk hand-off and the type-generator delegation, with the reasoning spelled out in comments (array iterators lack throw, which would otherwise turn a consumer's thrown error into a TypeError on Node 24+). That kind of detail is easy to get subtly wrong and easy to regress later if someone "simplifies" it back to yield* — might be worth a short regression test asserting yield* isn't reintroduced, but the existing does not relabel an error the consumer throws into it test already guards the behavior, so this is optional.

Test coverage

• Very thorough: unit tests cover streamed-detection, PLP framing (including the empty-chunk-skipped-so-it-isn't-mistaken-for-the-terminator case), chunk-size-boundary flushing, by-reference hand-off, TVP array-vs-async-iterable equivalence (including a ~2000-row case past the flush size and an empty async iterable), and all the generator-cleanup/error-propagation edge cases discussed above. Integration tests round-trip real values through execSql, prepared statements, and TVPs against a live server, plus mid-stream failure recovery. I don't see gaps worth calling out.

Performance

• The measurements in the description are informative and the reasoning for the scalar-path speedup (one buffer flush per request vs. one per parameter) and the array-TVP speedup (no second full copy, no per-row allocation) both check out against the code.

No blocking issues found; nice work on a genuinely tricky piece of async-generator plumbing.

[claude]

Review

This is a well-engineered change: the streaming contract is minimal (one optional writeValueStream per type), the RpcRequestPayload refactor keeps byte-for-byte compatibility for the in-memory path (verified by the existing 40-case byte-equivalence suite, now driven asynchronously), and the perf claims are backed by concrete before/after numbers and checked-in benchmarks. Generator cleanup semantics (early break, .throw() from the consumer, a mid-stream source failure) are all covered by targeted unit tests, and WritableTrackingBuffer's chunk-sealing means a large by-reference writeBuffer is never later overwritten by a subsequent write into the same tracking buffer — so the "yield chunks, then keep writing into the same buffer" design is safe.

A few things worth a look, roughly in order of importance:

1. A source's own cleanup failure can mask the real error (low severity)

In rpcrequest-payload.ts, the finally block that closes a streamed value's generator isn't guarded:

} finally {
  if (!done && typeof flushes.return === 'function') {
    await flushes.return();
  }
}

If the consumer throws into the payload (or otherwise causes early exit) while a streamed value is in flight, and the value's own source throws from its finally/cleanup logic when force-closed via .return(), that new exception replaces whatever was already propagating (per JS finally-block semantics) — so a consumer-initiated abort, or the original parameter error, could get silently swapped for an unrelated cleanup error. It's a double-fault edge case, but since the whole point of this code path is "make cleanup robust when a source misbehaves," it might be worth wrapping that call and preferring the original error (or at least not losing it silently). The existing tests cover cleanup that succeeds (sets a flag) but not cleanup that itself throws.

2. Minor asymmetry between the three max types' writeValueStream

varbinary.ts / nvarchar.ts return the writePlpStream(...) generator directly:

writeValueStream(buffer, parameter) {
  return writePlpStream(buffer, parameter.value as AsyncIterable<unknown>, (chunk) => { ... });
}

while varchar.ts wraps it in its own async * and delegates with yield*:

async * writeValueStream(buffer, parameter) {
  const codepage = parameter.collation!.codepage!;
  yield * writePlpStream(buffer, parameter.value as AsyncIterable<unknown>, (chunk) => { ... });
}

Both are correct (an extra yield* layer still forwards .throw()/.return() properly, unlike the array-iterator case called out in the RpcRequestPayload doc comment), but the difference in shape is only there to grab codepage into a local first. Not worth blocking on, just a small consistency nit.

3. Streamed value + output parameter isn't guarded

Nothing stops a caller from passing an async iterable to addOutputParameter. Since output values conceptually flow from the server, this combination is presumably meaningless, but it's not rejected anywhere and there's no test pinning down what should happen (e.g. does resolve/writeValueStream even get reached for an output parameter today?). Possibly a non-issue if it's already unreachable in practice — just flagging in case it's worth an explicit guard or a doc note next to the new addParameter doc comment.

Test coverage

Coverage is genuinely strong here — resolve/declaration detection for all three max types, PLP framing edge cases (empty chunks, by-reference large chunks, CHUNK_SIZE-crossing), TVP array-vs-async equivalence at scale, input-validation failures mid-stream for both max values and TVP rows, and integration round-trips including through prepared statements. The one gap I'd add, if anything, is a case for finding #1 above (a source whose finally throws when force-closed).

Security

Nothing concerning — this is serialization-side plumbing, no new trust boundary. The documented "buffers ≥ 8 KB are sent by reference; don't mutate/reuse them until the request completes" contract is a real footgun for callers using buffer-pooling stream implementations, but it's clearly documented on addParameter and matches pre-existing behavior for large in-memory values, so it's a reasonable tradeoff rather than a defect.

Overall: solid design, byte-compatible fallback path, and thorough validation. The findings above are minor/edge-case polish rather than blockers.

[claude]

Review

This is a very well-executed PR — I read through data-type.ts, plp-stream.ts, varbinary.ts/varchar.ts/nvarchar.ts, tvp.ts, rpcrequest-payload.ts, and both test files in full (not just the diff), and traced the actual consumption paths in connection.ts, bulk-load-payload.ts and the Always Encrypted code rather than trusting the PR description at face value. I didn't find any correctness bugs. Notes below are mostly confirmations plus a couple of low-severity observations for possible follow-up.

Code quality / design

• The writeValueStream contract (write into the caller's buffer, yield when it's chunk-sized, own no buffer state) is a clean, minimal addition to the existing resolve/writeTypeInfo/writeValue split from refactor: resolve parameters once and serialize them through a write contract #1774. Every implementer (varbinary, varchar, nvarchar, tvp) follows it uniformly through the shared writePlpStream helper, so there's no duplicated PLP-framing logic.
• RpcRequestPayload[Symbol.asyncIterator] is the one place doing real work — it manually drives the type's generator with next()/return() instead of for await/yield*, specifically to keep consumer-thrown errors from being mislabeled as the parameter's own InputError, and to dodge the Node 24 "array iterator has no throw" footgun (good catch — and it's nice that this is pinned by a test rather than just a comment). The done flag correctly prevents a double-close of the type's generator across the catch/finally paths. It's intricate for ~80 lines, but the comments carry their weight and the unit tests (early-stop, consumer-throw, close-fails-during-unwind) exercise exactly the paths that would be easy to get wrong.
• TVP.writeValueStream delegates to writeRows/writeRowsFrom via yield* — safe here (unlike the payload's own case) because both delegates are real async generators with working .throw()/.return(), not array iterators.
• Reusing one ParameterData cell per TVP column (cellsFor) and documenting the "no reference kept after writeValue returns" invariant is a nice touch that heads off a future misuse if someone adds an async-capable column type later.

Potential bugs

Nothing I'd block on. A few minor things worth a look, not necessarily in this PR:

• Re-executing a Request with a streamed value fails silently-wrong, not loudly. Request.validateParameters (existing code, request.ts:520) re-resolves parameters on every call, and resolve() for the streamed types just re-wraps whatever parameter.value currently holds — the same (possibly already-consumed) reference. If a Request carrying a spent Readable/generator is ever re-sent (nothing tedious does internally today, but nothing stops a caller from retrying the same Request object), the second attempt would serialize a truncated/empty value instead of throwing a clear "already consumed" error. It's documented on addParameter, which is reasonable, but since the failure mode is silent-wrong-data rather than a thrown error, a one-line mention in validateParameters's doc (or in resolve() itself) might save someone a confusing debugging session later.
• TVP column metadata (writeColumns) never forwards a collation into the per-column TYPE_INFO it writes, so a character-typed TVP column's TYPE_INFO always serializes with a zeroed collation. Traced this back to the pre-PR code (column.type.generateTypeInfo(column) on the bare column descriptor) — unchanged behavior, not a regression, just flagging it since tvp.ts got a full rewrite here and this seemed like a good moment to surface it if it's actually a live gap for non-ASCII TVP string columns.
• NChar/Char's declaration/resolveLength call value.toString().length with no isAsyncIterable guard (unlike VarBinary/VarChar/NVarChar). These types never supported streaming before or after this PR, so it's not a regression, but if a caller mistakenly passes a Readable to one, declaration() will silently compute a nonsense length from "[object Object]" a moment before validate() throws a clear error anyway — cosmetic, not a real risk.

I also specifically checked two adjacent subsystems that the PR body doesn't mention:

• Always Encrypted: not wired into the runtime request path at all currently (getParameterEncryptionMetadata/encryptWithKey have zero callers), so a streamed value can't reach it either way — no interaction, safe by omission rather than by design.
• Bulk load: BulkLoadPayload calls type.validate() directly, never type.resolve(), so a streamed value passed to a bulk-load column fails fast with the existing TypeError: Invalid buffer./Invalid string. rather than being silently mishandled. Bulk load simply doesn't get this streaming capability (consistent with the "follow-ups" section of the PR body around BulkLoadPayload's per-cell allocation); the error message doesn't hint that streaming isn't supported there, but that's a pre-existing rough edge, not something this PR introduces.

Performance

The measurements in the PR body are credible and the design rationale (single buffer per request, cell reuse, streaming flush at CHUNK_SIZE) matches what's in the code. Consuming via Readable.from for all paths (rather than keeping a separate sync fast path) is the right simplification — the note that this costs nothing on the scalar path is something the commit history shows was actually measured, not just assumed.

Security

Nothing concerning. Errors from a failing source are wrapped in InputError and don't leak internal state; the "ignore" bit + message.end() path in makeRequest (pre-existing, not new here) correctly tells the server to discard a partially-sent, aborted RPC message — exactly the scenario a mid-stream source failure now creates. The integration test for this ("fails the request when the source throws, and leaves the connection usable") exercises that real code path rather than just the unit-level abstraction, which is the right thing to verify given streaming makes partial-message-then-abort a normal occurrence instead of a rare edge case.

Test coverage

Excellent. Unit tests cover resolve/streamed-detection, the max-form declaration, chunk-boundary flushing, by-reference hand-off, TVP row validation (wrong length, non-array row, async validation failure), and the generator-cleanup edge cases (early stop, consumer-thrown error, close-failing-during-unwind) that are exactly the ones worth testing given how manual the generator driving is. Integration tests round-trip all three max types and a TVP through execSql, a prepared statement, and a mid-stream failure against a real server. I can't run the integration suite here, but the coverage on paper matches the claims in the PR description and targets the right edge cases rather than just the happy path.

Nice work overall — a substantial, carefully-sequenced change (visible in how many commits are review-driven fixes for exactly the kind of generator-delegation/error-identity subtleties that are easy to get wrong), and the test suite gives real confidence in it.

[claude]

Review

This is a well-scoped, carefully engineered addition on top of #1774's write contract. The design — writeValue returning "the rest of the write" as an async iterable that yields into the caller's buffer, with one shared writeRest driver handling close-on-early-stop and error identity — avoids the duplicated flush bookkeeping that earlier revisions apparently had (per the commit history). The commit log shows real bugs caught and fixed along the way (surrogate-pair splitting, TVP row length mismatch, yield* losing throw, error identity on early close), and the test suite (unit + integration, including early-stop/throw-into-generator/failing-source cases) is thorough.

Bug: stale reference to a removed API name

src/data-types/tvp.ts:200, in the legacy generateParameterData:

if (!Array.isArray(value.rows)) {
  throw new TypeError('A TVP whose rows are an async iterable can only be written through writeValueStream.');
}

writeValueStream was renamed away in the final refactor commit ("let writeValue return the rest of a streamed value's write") — the method no longer exists anywhere in the codebase (confirmed via grep). This code path is effectively dead today (TVP.writeValue is always defined, so writeValue() in data-type.ts never falls through to generateParameterData), but if it's ever hit — e.g. by a third party calling the legacy DataType methods directly — the error message points at a nonexistent method. Worth updating the message (or just removing the now-unreachable branch) to avoid confusing whoever eventually reads it.

Things that look right (called out because they're easy to get wrong)

• writeRest's catch/finally split — closing the source in catch (with its own failure swallowed) rather than in finally — correctly preserves a consumer-thrown error instead of replacing it with a .return() failure. Good catch in the commit history, and it's covered by a dedicated test.
• Chunks are yielded one-by-one instead of via yield* in RpcRequestPayload, avoiding the Node 24 TypeError: iterator does not provide a 'throw' method trap — also directly tested.
• The TVP row-length check (row.length !== columns.length) runs before any bytes for that row are written, so a bad row can't desync the server's parse of subsequent rows.
• Reusing one ParameterData cell object per column across all TVP rows is a reasonable perf win given cells are consumed synchronously by writeValue and never retained.
• Buffers ≥ CHUNK_SIZE are still referenced rather than copied for streamed values, preserving the zero-copy property from refactor: resolve parameters once and serialize them through a write contract #1774; documented on addParameter for callers who might reuse a scratch buffer.

Minor/non-blocking observations

• TVP.writeValue is an async function* that, for a null table, only does a single synchronous buffer.writeBuffer(NULL_TABLE) with no yield. Every TVP write therefore goes through writeRest's async-iterator machinery even when there's nothing to stream. This is called out and measured in the PR description (~6µs/request fixed cost) as an accepted tradeoff rather than an oversight, so just flagging for visibility rather than as an issue.
• The doc comment on DataType.writeValue and the various resolve implementations do a good job of explaining why (e.g., collation must be checked in resolve for a streamed varchar so failure timing matches the in-memory case), which will help future type migrations.
• No behavior change for in-memory values is asserted by reusing the existing 40-case byte-equivalence suite over the now-async payload — good regression coverage for what is otherwise an invasive refactor of the iteration protocol.

Test coverage

Solid. Unit tests cover the streaming contract in isolation (flush boundaries, by-reference buffers, error propagation/identity, early stop, TVP validation edge cases), and the integration suite exercises the real wire path for all three max types plus TVP, through execSql, callProcedure, and prepared statements, including failure-recovery ("connection stays usable after an aborted streamed request"). I don't see gaps worth calling out.

Security / performance

No concerns. This is additive (opt-in via async iterables) and the perf numbers in the PR description (measured, not just claimed) show it's a net win even on the non-streaming path due to the single-buffer serialization.

[claude]

Review

I read through the full diff (12 files, ~1.3k additions) plus the commit history, which already shows several rounds of fixes for issues an earlier Codex review caught (surrogate-pair splitting, TVP row/column-count mismatch, error identity across throw/return, yield* vs plain yield on Node 24). That prior hardening is visible and the remaining code is in good shape. Notes below are things I traced through by hand; I did not find any correctness bugs beyond what's already been fixed.

Design / code quality

• The writeValue contract (in-memory value written synchronously, streamed value returns an AsyncIterable<void> "rest") is a clean way to avoid a second method + flag on ParameterData. writeRest centralizes the close-on-early-stop / error-identity / no-double-close logic once, and both varbinary/nvarchar/varchar (via plp-stream.ts) and tvp.ts reuse it correctly.
• plp-stream.ts's writePlpStream correctly skips zero-length chunks (a zero-length PLP chunk would otherwise be misread as the terminator) — good catch, and it's covered by a test.
• RpcRequestPayload yielding chunks one-by-one instead of yield* (to preserve .throw() semantics across Node versions) is a subtle correctness fix and is well commented.
• tvp.ts's cell-object reuse (one ParameterData per column, mutated per row) is safe here because writing a row is fully synchronous with no await in between — worth calling out that this invariant must hold if writeRow is ever changed to do anything async mid-row, since concurrent reuse of the same cell object would otherwise corrupt data. A short comment already hints at this ("keeps no reference to it"), which is the right level of documentation.
• I checked the TVP per-cell ParameterData objects don't carry collation, but that's fine: validate() already encodes text values with the collation into a Buffer before writeValue sees them, and array-based TVP rows can't stream, so no per-column async path needs the collation at write time. Pre-existing behavior, not something this PR changes.

Potential edge cases considered (no bug found)

• Confirmed all in-tree callers of RpcRequestPayload (connection.ts, always-encrypted/get-parameter-encryption-metadata.ts, and the two older files under benchmarks/request/) already go through Readable.from(...), so dropping the synchronous Symbol.iterator in favor of Symbol.asyncIterator-only doesn't break anything.
• Traced writeRest's done bookkeeping against consumer-thrown errors, early break, and a source whose .return() itself rejects — the "don't replace a propagating error, don't double-close" logic holds together, matching the added unit tests.
• Confirmed TVP.writeValue is an async generator unconditionally (even for null/array-only tables), so rest is never undefined for TVP — intentional per the PR description ("a TVP's writeValue always returns the rest of the write"), just a minor bit of always-on async-iteration overhead for the non-streamed TVP case; not worth optimizing given the design goal of one code path.

Test coverage

Coverage looks strong: unit tests exercise resolve's streamed/max declaration, PLP chunking (including the CHUNK_SIZE boundary and by-reference hand-off), TVP row-count/type mismatches, error propagation/identity, and early-consumer-stop; integration tests round-trip real values through execSql, execute (prepared statements), and TVPs against a live server, including mid-stream failures. The byte-equivalence suite consuming the payload asynchronously is a nice way to keep TVP coverage without a separate byte-diff test.

Minor / non-blocking

• Request.addParameter's doc comment nicely spells out the surrogate-pair and by-reference-buffer caveats for consumers — good, since those are easy footguns for anyone streaming their own chunks.
• Nothing security-sensitive stood out: no new external input parsing, no injection surface: this is purely wire-format serialization of already-validated values.

Overall this looks solid and well-tested. Nice work threading the "no second copy, bounded memory, byte-identical output" needle while keeping the public API additive (existing in-memory callers are unaffected).

[github-actions]

🎉 This PR is included in version 20.3.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀