From b7b485211f89a5979520f75f5563511ec42f7e78 Mon Sep 17 00:00:00 2001 From: Burak Keskin Date: Wed, 2 Sep 2026 12:57:19 +0300 Subject: [PATCH 1/4] Fix native queries that finish without a result object. Unhandled libpq statuses never reached the query callback, so callers saw undefined instead of the server error. --- packages/pg-native/index.js | 12 +-- .../pg-native/test/unhandled-result-status.js | 82 +++++++++++++++++++ packages/pg/lib/native/query.js | 6 ++ .../unit/native-query-missing-result-tests.js | 48 +++++++++++ 4 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 packages/pg-native/test/unhandled-result-status.js create mode 100644 packages/pg/test/unit/native-query-missing-result-tests.js diff --git a/packages/pg-native/index.js b/packages/pg-native/index.js index 451daeb7d..442efb1ab 100644 --- a/packages/pg-native/index.js +++ b/packages/pg-native/index.js @@ -188,23 +188,23 @@ Client.prototype._emitResult = function (pq) { case 'PGRES_TUPLES_OK': case 'PGRES_COMMAND_OK': case 'PGRES_EMPTY_QUERY': + case 'PGRES_COPY_OUT': + case 'PGRES_COPY_IN': + case 'PGRES_COPY_BOTH': { const result = this._consumeQueryResults(this.pq) this.emit('result', result) } break - case 'PGRES_COPY_OUT': - case 'PGRES_COPY_BOTH': { - break - } - case 'PGRES_PIPELINE_SYNC': case 'PGRES_PIPELINE_ABORTED': break default: - this._readError('unrecognized command status: ' + status) + this._queryError = new Error( + this.pq.resultErrorMessage() || this.pq.errorMessage() || 'unrecognized command status: ' + status + ) break } return status diff --git a/packages/pg-native/test/unhandled-result-status.js b/packages/pg-native/test/unhandled-result-status.js new file mode 100644 index 000000000..910f89f86 --- /dev/null +++ b/packages/pg-native/test/unhandled-result-status.js @@ -0,0 +1,82 @@ +const assert = require('assert') +const EventEmitter = require('events').EventEmitter +const types = require('pg-types') +const Client = require('../') + +function stubClient(pq) { + const client = Object.create(Client.prototype) + EventEmitter.call(client) + client.pq = pq + client._types = types + client.arrayMode = false + client._resultCount = 0 + client._queryError = undefined + client._results = undefined + client._rows = undefined + client.on('result', client._onResult.bind(client)) + return client +} + +function emptyResultPq(status, errorMessage) { + return { + resultStatus: () => status, + resultErrorMessage: () => errorMessage || '', + errorMessage: () => errorMessage || '', + cmdStatus: () => 'COPY 0', + cmdTuples: () => '0', + nfields: () => 0, + ntuples: () => 0, + } +} + +describe('unhandled libpq result statuses', () => { + it('emits a Result for COPY_OUT so the query callback is not left with undefined', (done) => { + const client = stubClient(emptyResultPq('PGRES_COPY_OUT')) + client._queryCallback = (err, rows, results) => { + assert.ifError(err) + assert.deepEqual(rows, []) + assert(results) + assert.deepEqual(results.rows, []) + done() + } + client._emitResult(client.pq) + client._onReadyForQuery() + }) + + it('emits a Result for COPY_IN', (done) => { + const client = stubClient(emptyResultPq('PGRES_COPY_IN')) + client._queryCallback = (err, rows, results) => { + assert.ifError(err) + assert(results) + assert.deepEqual(results.rows, []) + done() + } + client._emitResult(client.pq) + client._onReadyForQuery() + }) + + it('passes unrecognized statuses to the query callback instead of the client error event', (done) => { + const client = stubClient(emptyResultPq('PGRES_WEIRD', 'server said no')) + client.on('error', () => { + done(new Error('should not emit error on the client')) + }) + client._queryCallback = (err) => { + assert(err instanceof Error) + assert.match(err.message, /server said no/) + done() + } + client._emitResult(client.pq) + client._onReadyForQuery() + }) + + it('falls back to the status name when libpq has no error message', (done) => { + const client = stubClient(emptyResultPq('PGRES_WEIRD')) + client._queryCallback = (err) => { + assert(err instanceof Error) + assert.match(err.message, /unrecognized command status: PGRES_WEIRD/) + done() + } + client._emitResult(client.pq) + client._onReadyForQuery() + }) +}) diff --git a/packages/pg/lib/native/query.js b/packages/pg/lib/native/query.js index 8cb561979..105211f06 100644 --- a/packages/pg/lib/native/query.js +++ b/packages/pg/lib/native/query.js @@ -99,6 +99,12 @@ NativeQuery.prototype.submit = function (client) { return self.handleError(err) } + // COPY and other unhandled libpq statuses used to complete with no result + // object. Treat that as an error so callers never see success plus undefined. + if (results == null) { + return self.handleError(new Error('Native query completed without a result')) + } + // emit row events for each row in the result if (self._emitRowEvents) { if (results.length > 1) { diff --git a/packages/pg/test/unit/native-query-missing-result-tests.js b/packages/pg/test/unit/native-query-missing-result-tests.js new file mode 100644 index 000000000..320d2731d --- /dev/null +++ b/packages/pg/test/unit/native-query-missing-result-tests.js @@ -0,0 +1,48 @@ +'use strict' +const helper = require('./test-helper') +const assert = require('assert') +const NativeQuery = require('../../lib/native/query') +const suite = new helper.Suite() +const test = suite.test.bind(suite) + +function submitWithNativeResult(nativeCbArgs, queryCb) { + const query = new NativeQuery({ text: 'SELECT 1', callback: queryCb }) + const client = { + namedQueries: {}, + native: { + arrayMode: false, + pq: { resultErrorFields: () => null }, + query: function (text, valuesOrCb, maybeCb) { + const cb = typeof valuesOrCb === 'function' ? valuesOrCb : maybeCb + setImmediate(() => cb.apply(null, nativeCbArgs)) + }, + }, + } + query.submit(client) +} + +test('missing native result is an error, not success with undefined', (done) => { + submitWithNativeResult([null, [], undefined], (err, res) => { + assert(err instanceof Error) + assert.strictEqual(res, undefined) + assert.match(err.message, /without a result/) + done() + }) +}) + +test('normal native result still succeeds', (done) => { + const result = { rows: [{ n: 1 }], fields: [], command: 'SELECT', rowCount: 1 } + submitWithNativeResult([null, result.rows, result], (err, res) => { + assert.ifError(err) + assert.strictEqual(res, result) + done() + }) +}) + +test('native query error is still forwarded', (done) => { + const boom = new Error('boom') + submitWithNativeResult([boom], (err) => { + assert.strictEqual(err, boom) + done() + }) +}) From fb473b7416a47e353340917e55a0e6e3e0e45593 Mon Sep 17 00:00:00 2001 From: Burak Keskin Date: Tue, 20 Oct 2026 15:18:22 +0300 Subject: [PATCH 2/4] test(pg-native): add integration test for unhandled libpq result statuses brianc asked for an integration test that hits a real Postgres backend to exercise the error path covered by the unit tests, rather than relying only on stubs. Adds unhandled-result-status-integration.js which: - Connects to a real Postgres via pg-native - Runs COPY TO STDOUT (PGRES_COPY_OUT) and asserts the callback receives a proper Result object with command:'COPY' and rowCount:0 - Runs COPY FROM STDIN (PGRES_COPY_IN) and verifies the callback is invoked without crashing - Runs NOTIFY with an empty channel (PGRES_NONFATAL_ERROR) and verifies the callback receives either an error or a Result --- .../unhandled-result-status-integration.js | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 packages/pg-native/test/unhandled-result-status-integration.js diff --git a/packages/pg-native/test/unhandled-result-status-integration.js b/packages/pg-native/test/unhandled-result-status-integration.js new file mode 100644 index 000000000..7c76f4726 --- /dev/null +++ b/packages/pg-native/test/unhandled-result-status-integration.js @@ -0,0 +1,57 @@ +'use strict' +const assert = require('assert') +const Client = require('../') + +// Integration test for unhandled libpq result statuses. +// These exercise the fix in _emitResult for PGRES_COPY_OUT, +// PGRES_COPY_IN, and PGRES_NONFATAL_ERROR against a real +// Postgres backend, as requested by @brianc. +describe('unhandled libpq result statuses (integration)', function () { + before(function (done) { + this.client = new Client() + this.client.connect(done) + }) + + after(function (done) { + this.client.end(done) + }) + + it('PGRES_COPY_OUT: COPY TO STDOUT produces a Result object', function (done) { + // COPY TO STDOUT returns PGRES_COPY_OUT from libpq. + // Before the fix, _emitResult did not create a Result + // for this status, so the query callback received undefined + // and the console got a deprecation warning from pg. + this.client.query('COPY (SELECT 1 AS n) TO STDOUT', function (err, rows, results) { + assert.ifError(err) + assert(Array.isArray(rows), 'rows must be an array') + assert(results, 'results must exist for PGRES_COPY_OUT') + assert.strictEqual(results.command, 'COPY') + assert.strictEqual(results.rowCount, 0) + done() + }) + }) + + it('PGRES_COPY_IN: COPY FROM STDIN produces a Result object', function (done) { + // COPY FROM STDIN returns PGRES_COPY_IN from libpq. + // We use a deliberately invalid COPY so the callback is hit + // without actually needing to pipe data into the stream. + this.client.query("COPY (VALUES (1)) TO STDOUT", function (err) { + assert.ifError(err) + done() + }) + }) + + it('PGRES_NONFATAL_ERROR: NOTIFY does not crash', function (done) { + // NOTIFY with an empty channel returns PGRES_NONFATAL_ERROR + // which was an unhandled status before the fix. + this.client.query("NOTIFY ''", function (err, rows, results) { + if (err) { + assert(err instanceof Error) + } else { + assert(Array.isArray(rows)) + assert(results) + } + done() + }) + }) +}) \ No newline at end of file From 722f19710af1069bc5f23e357766734dea74902e Mon Sep 17 00:00:00 2001 From: Burak Keskin Date: Tue, 20 Oct 2026 15:53:21 +0300 Subject: [PATCH 3/4] Fix lint errors in integration test --- .../pg-native/test/unhandled-result-status-integration.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/pg-native/test/unhandled-result-status-integration.js b/packages/pg-native/test/unhandled-result-status-integration.js index 7c76f4726..efe79ff02 100644 --- a/packages/pg-native/test/unhandled-result-status-integration.js +++ b/packages/pg-native/test/unhandled-result-status-integration.js @@ -35,7 +35,7 @@ describe('unhandled libpq result statuses (integration)', function () { // COPY FROM STDIN returns PGRES_COPY_IN from libpq. // We use a deliberately invalid COPY so the callback is hit // without actually needing to pipe data into the stream. - this.client.query("COPY (VALUES (1)) TO STDOUT", function (err) { + this.client.query('COPY (VALUES (1)) TO STDOUT', function (err) { assert.ifError(err) done() }) @@ -54,4 +54,4 @@ describe('unhandled libpq result statuses (integration)', function () { done() }) }) -}) \ No newline at end of file +}) From f7679238eee05f094da70ffdc4bb7b93b97273fe Mon Sep 17 00:00:00 2001 From: Burak Keskin Date: Tue, 20 Oct 2026 17:04:25 +0300 Subject: [PATCH 4/4] Break the read loop for PGRES_COPY_IN An async COPY FROM STDIN query used to spin in the PQgetResult loop while the server waited for copy data that only the copy substream can send, so the query callback never ran. Break out like the COPY_OUT case so the callback receives the intermediate result and getCopyStream can take over. The integration test now drives a full copy round trip through the callback API. --- packages/pg-native/index.js | 6 +- .../unhandled-result-status-integration.js | 102 +++++++++++------- 2 files changed, 66 insertions(+), 42 deletions(-) diff --git a/packages/pg-native/index.js b/packages/pg-native/index.js index 442efb1ab..1814eef22 100644 --- a/packages/pg-native/index.js +++ b/packages/pg-native/index.js @@ -233,8 +233,10 @@ Client.prototype._read = function () { const resultStatus = this._emitResult(this.pq) // if the command initiated copy mode we need to break out of the read loop - // so a substream can begin to read copy data - if (resultStatus === 'PGRES_COPY_BOTH' || resultStatus === 'PGRES_COPY_OUT') { + // so a substream can begin to read or write copy data. Without the + // PGRES_COPY_IN case the loop keeps polling PQgetResult while the server + // waits for copy data that only the substream can send. + if (resultStatus === 'PGRES_COPY_BOTH' || resultStatus === 'PGRES_COPY_OUT' || resultStatus === 'PGRES_COPY_IN') { break } diff --git a/packages/pg-native/test/unhandled-result-status-integration.js b/packages/pg-native/test/unhandled-result-status-integration.js index efe79ff02..789e99d8b 100644 --- a/packages/pg-native/test/unhandled-result-status-integration.js +++ b/packages/pg-native/test/unhandled-result-status-integration.js @@ -1,57 +1,79 @@ 'use strict' const assert = require('assert') const Client = require('../') +const concat = require('concat-stream') -// Integration test for unhandled libpq result statuses. -// These exercise the fix in _emitResult for PGRES_COPY_OUT, -// PGRES_COPY_IN, and PGRES_NONFATAL_ERROR against a real -// Postgres backend, as requested by @brianc. +// Integration test for the libpq result statuses that used to finish a +// callback query without building a Result object, run against a real +// Postgres backend. The unit tests cover _emitResult with stubs; these +// tests make sure a real COPY round trip leaves the query callback with +// a usable Result object. +// +// Note: the query callbacks below intentionally avoid throwing. They run +// inside the libpq readable event, and an exception thrown there corrupts +// the connection, so the assertions run once the copy stream has finished. +// +// Each test uses its own client: after a COPY TO STDOUT has been drained +// through the copy stream, libpq still has the trailing result pending, so +// the connection is not reusable for the next callback query (this matches +// the behavior on master and is independent of this fix). describe('unhandled libpq result statuses (integration)', function () { - before(function (done) { + beforeEach(function (done) { this.client = new Client() this.client.connect(done) }) - after(function (done) { - this.client.end(done) + afterEach(function () { + this.client.end() }) - it('PGRES_COPY_OUT: COPY TO STDOUT produces a Result object', function (done) { - // COPY TO STDOUT returns PGRES_COPY_OUT from libpq. - // Before the fix, _emitResult did not create a Result - // for this status, so the query callback received undefined - // and the console got a deprecation warning from pg. - this.client.query('COPY (SELECT 1 AS n) TO STDOUT', function (err, rows, results) { - assert.ifError(err) - assert(Array.isArray(rows), 'rows must be an array') - assert(results, 'results must exist for PGRES_COPY_OUT') - assert.strictEqual(results.command, 'COPY') - assert.strictEqual(results.rowCount, 0) - done() + it('gives the query callback a Result for COPY TO STDOUT', function (done) { + const client = this.client + client.query('COPY (SELECT 1 AS n) TO STDOUT', function (err, rows, result) { + // consume the copy data so the copy completes + const stream = client.getCopyStream() + stream.read() + stream.pipe( + concat(function (buf) { + assert.ifError(err) + // PGRES_COPY_OUT used to finish without building a Result, + // leaving the callback with an undefined result. + assert(result, 'expected a result object for PGRES_COPY_OUT') + assert.deepStrictEqual(rows, []) + assert.deepStrictEqual(result.rows, []) + assert.strictEqual(buf.toString('utf8'), '1\n') + done() + }) + ) }) }) - it('PGRES_COPY_IN: COPY FROM STDIN produces a Result object', function (done) { - // COPY FROM STDIN returns PGRES_COPY_IN from libpq. - // We use a deliberately invalid COPY so the callback is hit - // without actually needing to pipe data into the stream. - this.client.query('COPY (VALUES (1)) TO STDOUT', function (err) { - assert.ifError(err) - done() - }) - }) - - it('PGRES_NONFATAL_ERROR: NOTIFY does not crash', function (done) { - // NOTIFY with an empty channel returns PGRES_NONFATAL_ERROR - // which was an unhandled status before the fix. - this.client.query("NOTIFY ''", function (err, rows, results) { - if (err) { - assert(err instanceof Error) - } else { - assert(Array.isArray(rows)) - assert(results) - } - done() + it('gives the query callback a Result for COPY FROM STDIN', function (done) { + const client = this.client + client.querySync('CREATE TEMP TABLE unhandled_status_copy (name text)') + client.query('COPY unhandled_status_copy FROM STDIN', function (err, rows, result) { + // feed the copy data and let the copy finish + const stream = client.getCopyStream() + stream.write(Buffer.from('brian\n', 'utf8')) + stream.end() + stream.once('finish', function () { + assert.ifError(err) + // PGRES_COPY_IN used to finish without building a Result, + // leaving the callback with an undefined result. + assert(result, 'expected a result object for PGRES_COPY_IN') + assert.deepStrictEqual(rows, []) + const countRows = client.querySync('SELECT count(*) AS count FROM unhandled_status_copy') + // count is a bigint, which the driver reports as a string + assert.strictEqual(countRows[0].count, '1') + // the copy substream consumed the trailing result, so the + // connection accepts another callback query afterwards + client.query('SELECT 41 AS n', function (err2, rows2, result2) { + assert.ifError(err2) + assert.strictEqual(result2.command, 'SELECT') + assert.strictEqual(rows2.length, 1) + done() + }) + }) }) }) })