[arthurschreiber]

Replaces #1778, which grew far past the change it set out to make. This is the original simplification with only the behaviour Readable.from used to provide added back, and the ownership of the row source made explicit.

Problem

RowTransform pushes COLMETADATA, every row token, every cell's length prefix and every data chunk downstream as its own buffer. A bulk load of small rows reaches the packetizer as many tiny chunks per row, and each row passes through Readable.from(rows), pipe, a Transform with a nextTick callback, that transform's async iterator, and Readable.from(payload) before it gets there.

Change

RowTransform is replaced by BulkLoadPayload, an async iterable that serializes the rows into one WritableTrackingBuffer and yields its contents once it holds a chunk's worth (WritableTrackingBuffer.CHUNK_SIZE, 8 KB), checked after every cell so a wide row goes downstream as it is serialized, and at the end. The bulk load payload reaches the outgoing message the way RPC payloads do. The pipe plumbing in execBulkLoad is gone, and so is streamingMode, which only existed to keep the transform open.

Rows from a synchronous source (an array, a generator) are serialized without an event-loop turn in between. Rows from an asynchronous source are awaited one at a time. There is no flush on a stalled source: the packetizer below only sends whole packets and holds a partial one until the message ends, so bytes stalled in the serializer and bytes stalled in the packetizer are equally invisible to the server, and the server sends nothing back before DONE that a source could be waiting on.

The row source

The payload owns the row source the way a for await loop owns what it iterates, which is also what pipe did on master. It obtains the iterator when it is created and requests the first row right away; the rest are read once the server has accepted the INSERT BULK statement. Requesting the first row is what attaches a Readable's own listeners, so an error the stream emits during the round trip is kept and reported on the next read, with no listener of ours and no instanceof on the source. A synchronous source that throws on that first read is held the same way as a rejected one and reported through the bulk load's callback, as every source error was on master.

Having started the iterator, the payload always closes it, always through return(): when a row fails, when the consumer stops because the bulk load was canceled, and (through close(), called from execBulkLoad's callback) when the INSERT BULK statement is rejected, the bulk load was canceled before the rows were sent, or the connection is no longer logged in. For a Node.js stream, return() destroys the stream. A read still pending in the source cannot be interrupted; the source is closed once it has settled. This is documented on execBulkLoad, with an async generator that opens its file lazily and closes it in its finally as the recommended shape.

master closed the source only on cancel; a bulk load that failed before its rows were sent left the piped stream open.

Kept from what Readable.from used to do: a row given as a promise is settled before it is serialized, and an iterable whose async-iterator property is undefined is read through its synchronous iterator.

Two fixes fall out:

• A bulk load with no rows now works. COLMETADATA is written before the rows are read; the transform wrote it lazily before the first row, so an empty bulk load sent only DONE, which the server rejects.
• A close that fails no longer hides the row error. If the source's return() rejects after a row failed, the row error is still what the bulk load fails with.

Also removed: firstRowWritten, a @private flag left over from the addRow API that nothing ever set. The two guards it fed could not fire, and the check it put around validate meant every row was validated, which every row still is.

The bytes for a bulk load with rows are unchanged. A unit test serializes 300 rows of int, nullable nvarchar and varbinary once per row through the same generate* methods and once through the payload, from an array and from an async source, and asserts the byte streams are identical.

rowToPacketTransform no longer exists. It was @private, and there was no supported way to feed it rows directly. bulk-load.ts is back to columns, COLMETADATA and DONE, and shorter than on master.

The three fake-server cancel tests yielded one 8000-byte row before stalling, relying on the transform passing it straight to the packetizer. That is a few bytes short of a chunk, so they yield two rows now; their intent, that the server has started receiving the message when the stall begins, is unchanged.

Measurements

Measured against master (daa8e77), same machine, same run.

Serializer in isolation, in the pipeline shape makeRequest uses (rows → serializer → Readable.from(payload) → sink), 200,000 rows from an array, median of 5 runs:

rows	master	this PR
int, nvarchar(50), float	~540k rows/s	~1.0M rows/s
40 × int	~93k rows/s	~165k rows/s

End to end against SQL Server 2022, 50 bulk loads of 10,000 rows each:

source	master	this PR
one int column, array	~163k rows/s	~203k rows/s
three columns, array	~138k rows/s	~157k rows/s
three columns, async generator	~131k rows/s	~145k rows/s

Validation

• Unit tests: chunking (at most CHUNK_SIZE per chunk, far fewer chunks than rows, a chunk mid-row, null text cells), byte equivalence with per-row serialization from an array and an async source, the first row requested at creation and no more, a synchronous source serialized without an event-loop turn, the empty bulk load's bytes, promised rows, a stream error before the rows are read, a synchronous source's throw on its first read reported through the iteration rather than the constructor, the undefined async-iterator property, objects vs arrays, text pointers, the source closed when a row fails, when the consumer stops early, and when closed unread (a stream, a generator, and one whose close throws), a close failure neither hiding a row error nor going unreported when there is none, and execBulkLoad closing the source when the connection is not logged in.
• Integration: a bulk load with no rows completes with a row count of zero; a synchronous source's throw reaches the callback; the source is closed when the INSERT BULK statement is rejected, when the bulk load was canceled before it, and when it is canceled right after. Bulk load, pause/resume and cancel suites pass against SQL Server 2022.
• Unit suite 484 passing. Lint and typecheck clean.

🤖 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-05T14:56:21.865212Z	d10126d	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: 0d0a3d81d6

ℹ️ 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

This is a well-executed refactor. Collapsing the RowTransform/Readable.from/pipe chain into a single async generator that writes straight into one WritableTrackingBuffer is a sound way to cut per-row overhead, and the idle-flush mechanism (racing the next iterator.next() against a setImmediate) is a clever way to keep low-latency delivery for async sources without giving up coalescing for fast/sync ones. I traced through the control flow (chunk-size flush, idle flush, promise-row wrapping, error propagation, and iterator.return() cleanup on both error and early-stop paths) and it all checks out against the new tests.

Correctness

• The try { … } finally { if (!done) iterator.return(); } wrapper correctly mirrors native for await…of cleanup semantics, including on a thrown validation/serialization error and on the consumer stopping early (cancel case).
• The switch from callback(error) inside the old Transform._transform to simply letting serializeRows throw is behaviorally equivalent (an async generator throwing rejects the pending next() promise the same way Readable.from(payload).pipe(message) would surface an 'error' from the old transform), and it's simpler.
• Chunk-size is checked after every cell (not only every row), but since every DataType.generateParameterData implementation yields an already-fully-materialized value (a Buffer/string sitting in parameter.value, at most split into 2–3 pieces for PLP), this is equivalent to the old per-chunk this.push() in practice — no new buffering-of-huge-values concern for MAX-length columns.
• The promised-row handling only kicks in for sync sources yielding a Promise (the async-iterator branch is skipped since iterator.next() on an async iterator is already promise-like), which matches spec semantics for async generators (yield auto-awaits) — so there's no double-wrapping gap.

Minor / nice-to-have

• type Row = unknown[] | { [colName: string]: unknown }; is duplicated verbatim in both bulk-load.ts and bulk-load-payload.ts. Worth exporting from one place to avoid the two drifting.
• bulk-load.ts:577 keeps the if (!this.firstRowWritten) { value = c.type.validate(...); } gate from the old RowTransform. firstRowWritten is initialized to false and — on master as well as here — never set to true anywhere, so this branch is effectively always taken and the conditional is dead weight predating this PR. Since this code is being rewritten anyway, it might be worth either wiring up the flag (if the intent was "validate only the first row") or dropping the check. Not a regression from this change, just flagging it since the line is touched here.
• Removing RowTransform, streamingMode, and rowToPacketTransform entirely is a breaking change for any external caller who was piping directly into bulkLoad.rowToPacketTransform (the "streaming mode" escape hatch). Those were marked @private in JSDoc only, so TypeScript wouldn't have stopped a JS consumer from using them — worth a mention in the release notes/changelog if this ships as a minor rather than a major.

Performance

The measurements are credible and match the architectural change (removing several stream/pipe/nextTick hops per row). The ~1µs/row cost for the idle-flush race on async sources is a reasonable tradeoff for not stalling low-throughput producers, and it's called out transparently in the PR description.

Security

Nothing of concern — this is internal wire-format serialization with no new external input handling or injection surface.

Test coverage

Excellent. The unit suite covers chunk-boundary flushing, byte-for-byte equivalence against per-row serialization (both from an array and an async source), the empty-bulk-load fix, promised rows (including rejection), stream error retention before rows are read (plus listener cleanup verified against a plain Readable.from's baseline listener count), early-stop/cancel cleanup, the synchronous-source-no-event-loop-turn guarantee, and object-vs-array rows. The new integration test for a zero-row bulk load nicely closes the loop on the bug fix mentioned in the description.

Nothing here blocks merging — just the small dead-code/type-duplication notes above for consideration.

[codecov]

Codecov Report

❌ Patch coverage is 98.63014% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.27%. Comparing base (daa8e77) to head (89ca107).

Files with missing lines	Patch %	Lines
src/connection.ts	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1779      +/-   ##
==========================================
+ Coverage   81.89%   82.27%   +0.37%     
==========================================
  Files          92       92              
  Lines        4950     4969      +19     
  Branches      933      940       +7     
==========================================
+ Hits         4054     4088      +34     
+ Misses        601      593       -8     
+ Partials      295      288       -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27265c568a

ℹ️ 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

This is a well-scoped, well-tested refactor. The rationale in the description is clear and the byte-equivalence testing strategy (serializing the same rows per-row vs. through the generator and diffing the output) gives good confidence the wire format is unchanged.

Correctness

I traced through BulkLoad.serializeRows (src/bulk-load.ts:516-640) in detail:

• The IDLE/setImmediate race correctly reuses a single pending idle promise across iterations (idle ??= ...) so only one immediate is armed per stall, and it's only rearmed after actually firing — matches the "coalesce a synchronous burst, flush on stall" description.
• The try { … } catch { closed = true; iterator.return() … throw err } finally { if (!done && !closed) iterator.return() } structure correctly distinguishes "closed due to error" vs. "closed due to consumer stopping early" vs. "iterator ran to completion," and a failing return() never masks the original row error — the dedicated tests for this (keeps the row error when the source fails to close, reports a source that fails to close when the consumer stops early) are a nice touch.
• The text-pointer branch (isTextType && value == null vs. the else) preserves the original RowTransform's continue-based semantics exactly (null text cells get only the null pointer byte, everything else gets length+data, text types additionally get the timestamp prefix).
• Chunk-size flushing is checked after every cell rather than every row, which is what makes the "wide row of large cells" case (test: hands a chunk downstream in the middle of a wide row) work.

I didn't find a correctness bug in the new serialization path.

Minor / nit-level observations

1. type Row is duplicated verbatim in src/bulk-load.ts:136 and src/bulk-load-payload.ts:4. Since bulk-load-payload.ts already imports from ./bulk-load, exporting Row from one and importing it in the other would remove the duplication (small, not blocking).
2. firstRowWritten is effectively dead as an optimization gate. serializeRows still guards the c.type.validate(...) call with if (!this.firstRowWritten) (carried over unchanged from RowTransform), but nothing in src/ ever sets firstRowWritten = true — it's only ever declared and initialized to false. So every row is always validated, not just the first. This predates this PR (the check was moved, not introduced) and isn't a regression, but since you were already in this code, it might be worth a follow-up: either the guard is stale and should be removed, or there's a missing this.firstRowWritten = true somewhere that used to exist. Worth a quick look, not a blocker for this PR.
3. The ignoreError listener attached to an EventEmitter row source in the BulkLoadPayload constructor is only removed once the async iterator actually starts (bulk-load-payload.ts:26-31). If a source is never read at all (e.g., the INSERT BULK statement is rejected or the load is canceled before any row is pulled — a case the docs on execBulkLoad explicitly call out as "the caller's to close"), that listener stays attached indefinitely, and a later 'error' on that stream will be silently swallowed rather than surfacing anywhere. This matches the tradeoff described in the PR (avoiding an unhandled 'error' during the in-flight window) and isn't new behavior in spirit, but might be worth a one-line callout in the doc comment on execBulkLoad since it's a slightly different failure mode (silent swallow vs. previously-undocumented behavior) than "the caller must close it."

Performance

The design (accumulate into one long-lived WritableTrackingBuffer, flush at CHUNK_SIZE or on idle) is a sound way to cut down on the enormous number of small allocations/pushes the old Readable.from → pipe → Transform → Readable.from chain produced per row. The measurements in the description (roughly 1.6-2x rows/s for array sources) are consistent with removing that plumbing. The microtask-vs-macrotask distinction used for detecting "idle" (setImmediate only fires once the source stops producing back-to-back) is a clever, low-overhead way to avoid adding a full event-loop turn of latency per row for synchronous/tightly-pipelined sources while still flushing promptly for real async sources.

One thing I couldn't verify without running the fake-server cancel/pause-resume suites myself: the PR description says these already pass against SQL Server 2022, so I'll trust that over re-deriving it from the diff.

Security

No concerns — this is purely a transport-serialization refactor with no new external input parsing, and the byte layout is asserted to be identical to the previous implementation.

Test coverage

Very thorough for the new code path: chunk coalescing, mid-row chunk boundaries, idle-triggered flushes, synchronous-vs-asynchronous sources, byte-equivalence against per-row serialization, empty bulk loads, promise-valued rows, sync iterables with undefined async-iterator property, stream error handling before/after read, and source-closing semantics on both error and early consumer stop. The one integration test added (bulk load with zero rows) directly exercises the bug fix called out in the description. I don't see any gaps I'd ask to be filled in before merging.

---

Review generated by Claude Code.

[claude]

Review

This is a well-scoped and well-documented change. Replacing the Readable.from → pipe → Transform chain with a single async generator (BulkLoad.serializeRows) removes several layers of indirection and per-row event-loop overhead, and coalescing writes into WritableTrackingBuffer.CHUNK_SIZE chunks should meaningfully reduce the number of tiny buffers a bulk load of small rows used to push through the packetizer. The execBulkLoad doc additions (row-source contract, close semantics, worked examples) are a real improvement — and fix a pre-existing inconsistency in the example (date_of_birth vs day_of_birth).

The empty-bulk-load fix and the removal of the dead firstRowWritten guard are correct: firstRowWritten was never set to true anywhere in the old code, so validate was already running unconditionally for every row — removing it is a no-op behavior-wise, just dead code cleanup, as the PR description says. streamingMode/rowToPacketTransform aren't referenced anywhere else in src/tests/docs, so removing them isn't an externally-visible API break.

Possible issue: stray error listener if the payload is never iterated

In bulk-load-payload.ts:

constructor(bulkLoad: BulkLoad, rows: Iterable<Row> | AsyncIterable<Row>) {
  ...
  if (rows instanceof EventEmitter) {
    rows.on('error', ignoreError);
  }
}

async *[Symbol.asyncIterator]() {
  const rows = this.rows;
  if (rows instanceof EventEmitter) {
    rows.removeListener('error', ignoreError);
  }
  yield* this.bulkLoad.serializeRows(rows);
}

The ignoreError listener added in the constructor is only removed when [Symbol.asyncIterator]() actually runs. If execBulkLoad fails before the payload is ever iterated (e.g. the server rejects INSERT BULK immediately, or the request is canceled before the message is sent), that listener stays attached to the caller's stream indefinitely — not just "while the statement is in flight" as the comment states. For a caller who doesn't have their own 'error' listener on that stream and doesn't proactively destroy() it in the callback (the docs recommend this, but don't enforce it), any later error the stream emits would be silently swallowed rather than crashing/surfacing as it would have otherwise. It's a narrow edge case, but worth either clearing the listener from execBulkLoad's failure/callback path too, or noting it as a known caveat.

Concurrency logic (serializeRows)

The idle/Promise.race mechanism for flushing on an async-source stall looks correct after tracing through it carefully: idle is only left set (not reset to undefined) in the branch where result won the race, which by construction means the idle promise hadn't settled yet at that point — so racing the next pending row against the same idle promise instead of arming a fresh one is intentional and correctly implements "flush once per real event-loop turn" rather than restarting the timer on every microtask-resolved row. The try/catch/finally around the iterator (closing on error, closing on early consumer stop, preserving the row error over a return() failure) matches the documented contract and is exercised well by the unit tests (keeps the row error when the source fails to close, reports a source that fails to close when the consumer stops early, etc.).

One minor note: the per-cell chunk-size check only happens after a whole cell's generateParameterData output has been appended, not after each yielded piece of a single large cell. In practice this is equivalent to the old RowTransform behavior (which also pushed all of one cell's chunks synchronously within a single _transform call before yielding to backpressure), so it's not a regression — just worth knowing it's not a hard per-8000-byte-chunk bound.

Test coverage

The new unit test suite in test/unit/bulk-load-test.ts is thorough — it covers chunk coalescing, large-cell flush-before-next-row, empty bulk loads, promise-valued rows, sync-iterable-with-undefined-async-iterator, stream error handling both before and after consumption starts, close-failure precedence, and byte-for-byte equivalence against a naive per-row serialization. The one added integration test (bulk load with zero rows) is a good regression check for the fixed bug. I don't see an integration or unit test that exercises the new "ignoreError listener never removed" scenario above, which would be worth adding if that's confirmed to be a real gap rather than intentional.

Nit

The three-line "flush the buffer" sequence (for (const chunk of buffer.getBuffers()) yield chunk; buffer.consume(buffer.length);) is duplicated three times in serializeRows. It can't trivially be pulled into a helper since it needs to yield from the enclosing generator, so the duplication is understandable, but a comment or small local generator-helper could reduce the repetition if desired — purely optional.

Overall this looks solid: clear rationale, careful edge-case handling, and strong test coverage. The listener-leak edge case above is the only thing I'd want addressed or explicitly accepted before merging.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 899fca7580

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d10126d438

ℹ️ 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

Went through all four commits and the final state of src/bulk-load.ts, src/bulk-load-payload.ts, and src/connection.ts, plus the unit/integration tests. This is a well-executed refactor — the reasoning in the PR description matches the code, and I traced through the trickier bits (chunk-boundary handling, row-source closing on error/cancel, the stream 'error'-listener dance) rather than just trusting the description.

Correctness

• The chunk-boundary check (buffer.length >= WritableTrackingBuffer.CHUNK_SIZE) sits after the if (isTextType && value == null) {...} else {...} block, so it fires for both the null-text-pointer path and the normal path. That's the detail that would've been easy to get wrong (only checking inside the else), and it's correct — confirmed by the "null text cells" test.
• The error-handling split between catch (row failed → close source, swallow a secondary close failure, rethrow the row error) and finally (consumer stopped pulling → close source, let a close failure surface) correctly mirrors the language's own IteratorClose semantics for for await...of (an abrupt completion from the loop body wins over a return() rejection). Good attention to detail, and it's pinned by tests on both branches.
• Verified against Connection.makeRequest: on cancel, payloadStream.destroy() is called on the Readable.from(payload) wrapping the async iterable, which is what actually drives iterator.return() on the row source. So the "closed the way a for await loop closes it" contract described in the new execBulkLoad doc comment is backed by real behavior, not just wishful documentation.
• The BulkLoadPayload constructor attaching a no-op 'error' listener immediately, removed synchronously (no await in between) right before delegating into serializeRows, closes the gap where an early stream error could otherwise become an unhandled 'error' event. No async window where an error could slip through unhandled.
• Chunks from getBuffers() aren't capped at CHUNK_SIZE when a single written value is itself >= CHUNK_SIZE (e.g., a large varbinary/text cell is referenced, not copied, per WritableTrackingBuffer's existing contract). That's intentional and pre-existing behavior of the buffer class, not a regression — worth calling out only because a casual reader of serializeRows's doc comment ("yielded once it holds a chunk's worth") could assume a hard cap. The tests correctly don't assert a hard cap in the large-cell case.

Minor/nitpick (perf, since that's the whole point of the PR)

• Array.isArray(row) and the c.type.name === 'Text' || ... text-type check are both re-evaluated per cell, per row, inside the hot loop. Array.isArray(row) could be hoisted to once per row, and isTextType is a property of the column, not the row, so it could be precomputed once per column (e.g., alongside this.columns) instead of recomputed on every row. Neither is likely to show up in the benchmarks you posted, but given the PR's explicit goal is squeezing out per-row/per-cell overhead, these are easy wins if you want them.

Test coverage

Thorough — byte-equivalence against the old per-row serialization (from both an array and an async source), chunk-boundary edge cases (mid-cell, mid-row, null text cells), the empty-bulk-load fix, promised rows, stream error timing, close-failure-doesn't-hide-row-error (and the reverse), and the synchronous-source-no-event-loop-turn guarantee. The fake-server cancel tests were correctly updated (2×8000-byte rows instead of 1, since a single row no longer forces a flush without the idle-flush logic removed in the last commit) with a comment explaining why.

Nothing else stood out

No security concerns (this is TDS wire serialization, not string-built SQL). No leftover references to the removed streamingMode/rowToPacketTransform/firstRowWritten anywhere in src/ or test/ — the removal is clean.

Nice work — the row-source-closing contract in particular is a genuinely subtle area (promises vs. sync/async iterables vs. streams, plus who owns closing an unstarted source) and it's handled carefully and documented well enough that a caller can actually reason about it.

[claude]

Review

Nicely scoped rewrite — replacing RowTransform/pipe plumbing with an async-generator-backed BulkLoadPayload that coalesces rows into CHUNK_SIZE buffers is a clear win, the execBulkLoad row-source contract is now actually documented, and the byte-identity test against per-row serialization is a great regression guard. Also good to see genuinely dead code (firstRowWritten, streamingMode, rowToPacketTransform) removed rather than left around.

One correctness concern and a couple of minor nits below.

Correctness: a synchronous row source can now throw out of execBulkLoad itself

BulkLoadPayload's constructor eagerly calls iterator.next() to observe stream errors early:

// src/bulk-load-payload.ts
const first = this.iterator.next();

connection.ts doesn't wrap this:

// src/connection.ts
const payload = new BulkLoadPayload(bulkLoad, rows ?? []);

For an AsyncIterable/Promise-returning source this is safe (a synchronous throw inside an async generator becomes a rejected promise, and the rejection is swallowed with .then(undefined, ignoreError) until it's awaited later). But for a synchronous iterable — e.g. a plain generator that validates its input before the first yield — a throw from that first .next() call propagates synchronously out of the BulkLoadPayload constructor and out of execBulkLoad() itself.

That's a behavior change from master: Readable.from(rows) never touched the iterable synchronously during execBulkLoad, so any row-source error — sync or async — used to surface only through bulkLoad.callback(err), on a later tick. Tedious's whole API is callback/event based specifically so callers don't need to wrap calls in try/catch; this path now can throw synchronously for a plausible, easy-to-write row source:

function* rows(data) {
  if (!Array.isArray(data)) throw new TypeError('bad data');
  for (const d of data) yield d;
}
connection.execBulkLoad(bulkLoad, rows(notAnArray)); // now throws synchronously

None of the added tests exercise this exact case (the error-handling tests use an async stream 'error' event or a rejected row promise, not a synchronous throw from the very first next()). Worth either documenting this as an intentional exception to the "always via callback" contract, or catching a synchronous throw from the eager first read and routing it to bulkLoad.callback instead.

Minor

• Duplicated row-shape type. bulk-load-payload.ts now exports Row = unknown[] | { [colName: string]: unknown }, but connection.ts's execBulkLoad signature still spells the same shape inline ({ [columnName: string]: unknown }, note the different property name). Worth having execBulkLoad reuse the exported Row type so the two can't drift.
• isTextType recomputed per cell. c.type.name === 'Text' || c.type.name === 'Image' || c.type.name === 'NText' runs for every cell of every row in the new hot loop (carried over unchanged from RowTransform, so not a regression this PR introduces, but since the loop is being rewritten anyway and the PR's whole point is throughput, it'd be easy to compute this once per column up front).
• PR description slightly stale in one spot. The "Kept from what Readable.from used to do" section says "The payload holds a no-op listener from creation until the rows are read," which describes an earlier commit's design; the final implementation (the last commit) instead reads the first row eagerly on construction. The in-code doc comment on BulkLoadPayload already reflects the final behavior correctly — just the top-level PR body text lags behind.
• Small unrelated-looking rename in the doc example: bulkLoad.addColumn('date_of_birth', ...) → addColumn('day_of_birth', ...), presumably to match the new example below it, but "day_of_birth" reads a little oddly compared to the previous "date_of_birth".

Tests

Coverage is thorough — chunk-boundary behavior, byte-for-byte equivalence vs. per-row serialization (array and async source), empty bulk loads, promised rows, sync-source event-loop-turn check, and the various close/cancel/error-ordering cases are all exercised. The one gap I'd add is the synchronous-throw-from-first-next() case above.

Security

Nothing notable — this is TDS binary serialization from internally-typed values, no string-built SQL or new external input surface.

[claude]

Review

I read through bulk-load-payload.ts, bulk-load.ts, connection.ts, and the test changes in detail, and traced the interaction with Connection.makeRequest (canceled/not-logged-in short-circuit happens before Readable.from(payload) is ever constructed, so payload.close() in the two branches in execBulkLoad's request callback never races with the async generator's own finally-based cleanup — good, these two closing paths are properly mutually exclusive).

Code quality

• The design is clean: moving ownership of the row-source iterator into BulkLoadPayload and closing it consistently through return() (whether via the generator's finally/catch or via close()) removes a real class of leaks (master never closed the source on a rejected INSERT BULK).
• Hoisting isTextType to a per-column array and validating once per cell (previously gated by a flag that could never be false) are worthwhile, low-risk simplifications.
• Dropping RowTransform, streamingMode, and the dead firstRowWritten flag removes a good chunk of incidental complexity along with the pipe/Transform/nextTick hops. The doc comments on BulkLoadPayload and execBulkLoad (including the async-generator-with-finally example) do a good job of making the new "who closes what, when" contract explicit — this was implicit and easy to get wrong before.

Correctness

I didn't find a concrete bug. A few edge cases I specifically checked and believe are handled correctly:

• Eager first-row read in the constructor, combined with the Promise.reject(err) wrapping for a synchronous throw — the rejection handler (ignoreError) is attached immediately, so there's no unhandled-rejection window even if the row is never read (e.g., INSERT BULK rejected before any row is pulled).
• close() on a generator that already completed/threw is a safe no-op per the iterator protocol, so calling it after a synchronous first-read throw doesn't double-report.
• The finally block's !done && !closed guard correctly avoids calling return() on an already-exhausted or already-closed iterator.
• Chunking via buffer.getBuffers()/consume(buffer.length) on a shared WritableTrackingBuffer mid-generator looks correct against the existing _seal()/consume() implementation, including the case where a large writeBuffer argument is referenced rather than copied.

One behavioral tradeoff worth confirming is intentional

The chunk threshold is WritableTrackingBuffer.CHUNK_SIZE (8 KB), checked after every cell. For a slow/trickling async source (e.g., a generator that occasionally awaits an external call between small rows), no bytes reach the packetizer at all until either 8 KB accumulates or the source finishes — there's no idle-flush anymore (removed on purpose per the PR description). This is a reasonable tradeoff and is well-argued in the description, and the connection-cancel-test.ts updates (needing 2× 8000-byte rows instead of 1 to "fill a chunk") confirm the authors hit this exact seam. Just flagging it explicitly in case anyone relies on a trickling bulk load starting to show up server-side sooner (this effectively doubles the old ~4 KB TDS-packet-driven threshold to 8 KB).

Minor nit

BulkLoadPayload shares mutable iterator/pending state across the instance, and [Symbol.asyncIterator]() is an async generator that assumes it's invoked exactly once per instance (nothing guards against a second call, which would race on this.pending). In practice this holds today (only execBulkLoad/makeRequest ever consumes it), but a one-line comment on the class noting "consume at most once" would make that invariant explicit for future maintainers, similar to how the closing contract is already spelled out.

Performance

The measurements in the description (roughly 2× on the pure serializer, and consistent double-digit percentage gains end-to-end against SQL Server) are convincing, and the byte-equivalence test against the old per-row serialization approach is a good way to pin behavior while changing the mechanism.

Security

No concerns — no new external input parsing, buffer bounds are unchanged from the prior per-row serialization, and closing row sources more consistently is a (minor) improvement for resource hygiene (e.g., a rejected INSERT BULK no longer leaves an open file/stream dangling).

Test coverage

This is a strong point of the PR: chunking behavior, byte-for-byte equivalence (sync array vs. async source), first-row eager read timing, synchronous-source throw semantics, promise-settling, empty bulk loads, text-pointer/null encoding, and the various source-closing scenarios (row failure, early consumer stop, close failure not masking a row error, close failure being reported when there's no row error) are all covered at the unit level, with integration tests for the empty-bulk-load and cancel/rejection paths. I'd call this thorough.

Overall this is a well-reasoned and well-tested simplification with a measured performance win; I didn't find correctness issues, just the one tradeoff worth a sentence of documentation and the minor single-consumption nit above.

[github-actions]

🎉 This PR is included in version 20.2.3 🎉

The release is available on:

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

Your semantic-release bot 📦🚀