msnodesqlv8: resuming a stream paused before its first result set can fail with a spurious 'Function sequence error' (HY010) #1924

Summary

With the msnodesqlv8 driver, a stream that is paused before its query reaches a result set can fail with a spurious [Microsoft][ODBC Driver Manager] Function sequence error (HY010) if it is resumed while the query is still executing. toReadableStream() pauses the request this way before it runs it, and so does calling request.pause() before query().

Observed

Seen in a live probe on Windows CI against SQL Server 2022 with msnodesqlv8 5.6.0. The request was a stream paused before it ran, for select * from dbo.probe_no_such_table, resumed 8 ms after it started. These are the driver events:

call:pauseQuery
drv:submitted  select * from dbo.probe_no_such_table
call:resumeQuery   (query still running, no result yet)
drv:error HY010  [Microsoft][ODBC Driver Manager] Function sequence error  more=false
drv:error 42S02  Invalid object name 'dbo.probe_no_such_table'.            more=false
drv:free
drv:free

The stream emitted two EREQUEST errors instead of one, and the statement was freed twice. The next query on the connection still ran. In other runs at other timings the resume landed before submitted or after the error, and only the real error was emitted. So this depends on timing, and that one run is the only time it has been seen live so far.

Cause

msnodesqlv8's Query.resume() does not check whether a native call is still under way. With no batch fetched yet, it ends in dispatch(), which starts a fetch at once. If the query's begin() (its SQLExecute) or a nextResult() is still running, ODBC rejects that fetch with HY010. When begin() resolves it calls dispatch() again, so a successful query can also have its first fetch issued twice.

In 5.6.0 (lib/reader.js), resume() falls through to this.dispatch() when batchData is null, and dispatch() calls nativeGetRows() unless the query is paused or no longer running. 2.7 to 5.2 behave the same way: resume() there is dispatchRows(batchData) followed by dispatch().

Since #1919, the adapter pauses the driver at only two points. One is the end of a full 50-row batch, where the driver has nothing of its own under way. The other is before the query's first result set, in _pauseStream(), which is called just after queryRaw() and so while begin() is usually in flight. _resumeStream() resumes the driver whenever the stream is resumed. A stream paused before its first result set and resumed before the driver reaches one can therefore resume it in the middle of begin() or of a nextResult() chain.

#1215 reported the same error from mid-stream pause and resume on msnodesqlv8 2.0. #1919 removed most of that window by only pausing the driver at batch ends. This is the window that is left.

Proposed fix

Keep the early pause, because a cancel needs it to reach a stream paused before it runs. Hold back the matching resume until the driver has stopped:

• If the query reaches a result set: resume it a tick after its first meta, once begin() or nextResult() has handed over and dispatch() has returned because the query is paused.
• If the query ends first: don't resume it at all. It needs no resume to finish, and fix: stop msnodesqlv8 streams resumed after they end from hanging the connection #1921 already stops pause and resume reaching a query that has ended.

The underlying gap could also be raised upstream: msnodesqlv8's Query.resume() could wait for an in-flight native call. The adapter fix is needed either way, for the versions already released.

I'll work on a fix, with a deterministic reproduction across msnodesqlv8 2.7 to 5.6. A slow first statement such as waitfor delay should widen the window enough to hit it every time.

Related: #1215, #1919, #1921.