From 92e7387f0a4cb4e3c7790b2f8cad7071de141121 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:04:51 +0000 Subject: [PATCH 01/15] [coverage] Conformance findings: CLOUDFETCH-017 (#471) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 52 ++++++++++-- tests/e2e/cloudfetch.test.ts | 82 +++++++++++++++++++ .../result/CloudFetchResultHandler.test.ts | 60 ++++++++++++++ 3 files changed, 185 insertions(+), 9 deletions(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 755f2a24..a220a390 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -1,6 +1,7 @@ import fetch, { RequestInfo, RequestInit, Request, Response } from 'node-fetch'; import { TGetResultSetMetadataResp, TRowSet, TSparkArrowResultLink } from '../../thrift/TCLIService_types'; import HiveDriverError from '../errors/HiveDriverError'; +import RetryError from '../errors/RetryError'; import IClientContext from '../contracts/IClientContext'; import IResultsProvider, { ResultsProviderFetchNextOptions } from './IResultsProvider'; import { ArrowBatch } from './utils'; @@ -157,15 +158,20 @@ export default class CloudFetchResultHandler implements IResultsProvider { - downloaded = undefined; - const request = new Request(url, requestConfig); - const response = await fetch(request); - if (response.ok) { - downloaded = await response.arrayBuffer(); - } - return { request, response }; - }); + let result: { request: Request; response: Response }; + try { + result = await retryPolicy.invokeWithRetry(async () => { + downloaded = undefined; + const request = new Request(url, requestConfig); + const response = await fetch(request); + if (response.ok) { + downloaded = await response.arrayBuffer(); + } + return { request, response }; + }); + } catch (error) { + throw CloudFetchResultHandler.describeDownloadError(error); + } // Fall back to reading the body here if the retry policy returned a // response without consuming it via our operation (e.g. unit-test stubs // that hand back a pre-baked Response without invoking the operation @@ -177,6 +183,34 @@ export default class CloudFetchResultHandler implements IResultsProvider { const chunk = await operation.fetchChunk({ maxRows: 100000, disableBuffering: true }); expect(chunk.length).to.be.gt(0); }); + + it('should surface HTTP status and download context when CloudFetch retries are exhausted', async () => { + const retryMaxAttempts = 2; + const session = await openSession({ + cloudFetchConcurrentDownloads: 1, + useLZ4Compression: false, + retryMaxAttempts, + retryDelayMin: 1, + retryDelayMax: 10, + }); + + let serverHits = 0; + const server = http.createServer((req, res) => { + serverHits += 1; + res.writeHead(500, 'Internal Server Error', { 'Content-Type': 'text/plain' }); + res.end('Internal Server Error'); + }); + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => resolve()); + }); + const { port } = server.address() as AddressInfo; + + try { + const queriedRowsCount = 10000000; // result has to be quite big to enable CloudFetch + const operation = await session.executeStatement( + ` + SELECT * + FROM range(0, ${queriedRowsCount}) AS t1 + LEFT JOIN (SELECT 1) AS t2 + `, + { + maxRows: null, // disable DirectResults + useCloudFetch: true, + }, + ); + + await operation.finished(); + + // @ts-expect-error TS2339: Property getResultHandler does not exist on type IOperation + const resultHandler = await operation.getResultHandler(); + expect(resultHandler.source.source).to.be.instanceOf(CloudFetchResultHandler); + const cfResultHandler = resultHandler.source.source; + + // Plain-HTTP local server: let node-fetch pick its default agent for the download + const connectionProvider = await cfResultHandler.context.getConnectionProvider(); + sinon.stub(connectionProvider, 'getAgent').resolves(undefined); + + // Redirect every presigned link to the local server that always responds with HTTP 500, + // keeping the real expiry time and row count returned by the warehouse + const rowSetProvider = cfResultHandler.source; + const originalFetchNext = rowSetProvider.fetchNext.bind(rowSetProvider); + sinon.stub(rowSetProvider, 'fetchNext').callsFake(async (...args: Array) => { + const rowSet = await originalFetchNext(...args); + rowSet?.resultLinks?.forEach((link: any) => { + link.fileLink = `http://127.0.0.1:${port}/result`; + link.httpHeaders = {}; + }); + return rowSet; + }); + + let thrown: unknown; + try { + await operation.fetchChunk({ maxRows: 100000, disableBuffering: true }); + } catch (error) { + thrown = error; + } + + expect(thrown).to.be.instanceOf(Error); + expect(serverHits).to.be.equal(retryMaxAttempts); + const { message } = thrown as Error; + expect(message).to.contain('500'); + expect(message).to.contain('Internal Server Error'); + expect(message).to.match(/cloudfetch|download/i); + } finally { + await new Promise((resolve) => { + server.close(() => resolve()); + }); + await session.close(); + } + }); }); diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index 93ab712a..58d73f78 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -10,6 +10,7 @@ import CloudFetchResultHandler from '../../../lib/result/CloudFetchResultHandler import ResultsProviderStub from '../.stubs/ResultsProviderStub'; import { TRowSet, TSparkArrowResultLink, TStatusCode } from '../../../thrift/TCLIService_types'; import { ArrowBatch } from '../../../lib/result/utils'; +import RetryError, { RetryErrorCode } from '../../../lib/errors/RetryError'; import BaseClientContextStub from '../.stubs/ClientContextStub'; import { ClientConfig } from '../../../lib/contracts/IClientContext'; @@ -414,6 +415,65 @@ describe('CloudFetchResultHandler', () => { } }); + it('should add download context and HTTP status to RetryError when retries are exhausted', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + context.invokeWithRetryStub.callsFake(async () => { + throw new RetryError(RetryErrorCode.AttemptsExceeded, { + request: new Request('http://example.com/result/1'), + response: new Response('', { status: 500, statusText: 'Internal Server Error' }), + }); + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.instanceOf(RetryError); + expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); + expect(thrown.message).to.contain('CloudFetch'); + expect(thrown.message).to.contain('HTTP 500'); + expect(thrown.message).to.contain('Internal Server Error'); + expect(thrown.message).to.contain('Max retry count exceeded'); + }); + + it('should add download context and network error cause to RetryError when retries are exhausted', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + context.invokeWithRetryStub.callsFake(async () => { + throw new RetryError( + RetryErrorCode.AttemptsExceeded, + Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' }), + ); + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.instanceOf(RetryError); + expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); + expect(thrown.message).to.contain('CloudFetch'); + expect(thrown.message).to.contain('socket hang up'); + expect(thrown.message).to.contain('Max retry count exceeded'); + }); + it('should handle expired links', async () => { const context = new ClientContextStub(); const rowSetProvider = new ResultsProviderStub([sampleExpiredRowSet], undefined); From 29b57b9db9b0946c653503c3e7ad87e88df9f2e5 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:08:44 +0000 Subject: [PATCH 02/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498121259 at lib/result/CloudFetchResultHandler.ts:209 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 13 ++++++++++--- tests/unit/result/CloudFetchResultHandler.test.ts | 13 +++++++++---- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index a220a390..45b30631 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -206,9 +206,16 @@ export default class CloudFetchResultHandler implements IResultsProvider { expect(thrown.message).to.contain('HTTP 500'); expect(thrown.message).to.contain('Internal Server Error'); expect(thrown.message).to.contain('Max retry count exceeded'); + expect(thrown.stack.split('\n')[0]).to.be.equal(`Error: ${thrown.message}`); + expect(thrown.stack).to.not.contain('describeDownloadError'); }); it('should add download context and network error cause to RetryError when retries are exhausted', async () => { @@ -453,11 +455,12 @@ describe('CloudFetchResultHandler', () => { status: { statusCode: TStatusCode.SUCCESS_STATUS }, }); + const originalError = new RetryError( + RetryErrorCode.AttemptsExceeded, + Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' }), + ); context.invokeWithRetryStub.callsFake(async () => { - throw new RetryError( - RetryErrorCode.AttemptsExceeded, - Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' }), - ); + throw originalError; }); let thrown: any; @@ -467,6 +470,8 @@ describe('CloudFetchResultHandler', () => { thrown = e; } + expect(thrown).to.be.equal(originalError); + expect(thrown.stack.split('\n')[0]).to.be.equal(`Error: ${thrown.message}`); expect(thrown).to.be.instanceOf(RetryError); expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); expect(thrown.message).to.contain('CloudFetch'); From 060da4540ee54b24b4272e92707b2c193f01a41e Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:11:59 +0000 Subject: [PATCH 03/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498155013 at lib/result/CloudFetchResultHandler.ts:201 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 6 +++- .../result/CloudFetchResultHandler.test.ts | 35 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 45b30631..eea57468 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -198,7 +198,11 @@ export default class CloudFetchResultHandler implements IResultsProvider'); + cause = code ? `${safeMessage} (${code})` : safeMessage; } else if (payload && typeof payload === 'object' && 'response' in payload) { const { response } = payload as { response?: { status?: number; statusText?: string } }; if (response && typeof response.status === 'number') { diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index e1e87404..713f2ecb 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -479,6 +479,41 @@ describe('CloudFetchResultHandler', () => { expect(thrown.message).to.contain('Max retry count exceeded'); }); + it('should not leak presigned URLs from network errors into the RetryError message', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; + const originalError = new RetryError( + RetryErrorCode.AttemptsExceeded, + Object.assign(new Error(`request to ${presignedUrl} failed, reason: socket hang up`), { + code: 'ECONNRESET', + type: 'system', + }), + ); + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(thrown.message).to.contain('socket hang up'); + expect(thrown.message).to.contain('ECONNRESET'); + expect(thrown.message).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.message).to.not.contain('storage.example.com'); + expect(thrown.stack).to.not.contain('SECRET_SIGNATURE'); + }); + it('should handle expired links', async () => { const context = new ClientContextStub(); const rowSetProvider = new ResultsProviderStub([sampleExpiredRowSet], undefined); From af818e9234866f053479a03906afbe790cd21325 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:16:33 +0000 Subject: [PATCH 04/15] ai: apply changes for #567 (2 review threads) Addresses: - #4498177105 at lib/result/CloudFetchResultHandler.ts:204 - #4498177119 at lib/result/CloudFetchResultHandler.ts:191 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 13 ++++++- .../result/CloudFetchResultHandler.test.ts | 38 ++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index eea57468..23217c3c 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -201,7 +201,18 @@ export default class CloudFetchResultHandler implements IResultsProvider'); + // The payload itself stays reachable as `error.payload` (and is printed by `util.inspect` and + // structured-logger error serializers), so scrub its own message / stack in place as well. + const redactUrls = (text: string) => text.replace(/\b[a-z][a-z0-9+.-]*:\/\/\S+/gi, ''); + const safeMessage = redactUrls(payload.message); + try { + payload.message = safeMessage; + if (typeof payload.stack === 'string') { + payload.stack = redactUrls(payload.stack); + } + } catch { + // Best effort: non-writable properties on exotic error objects must not mask the failure. + } cause = code ? `${safeMessage} (${code})` : safeMessage; } else if (payload && typeof payload === 'object' && 'response' in payload) { const { response } = payload as { response?: { status?: number; statusText?: string } }; diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index 713f2ecb..93eb164b 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -1,3 +1,4 @@ +import { inspect } from 'util'; import { expect, AssertionError } from 'chai'; import sinon, { SinonStub } from 'sinon'; import Int64 from 'node-int64'; @@ -447,6 +448,36 @@ describe('CloudFetchResultHandler', () => { expect(thrown.stack).to.not.contain('describeDownloadError'); }); + it('should add download context and HTTP status to RetryError when retry timeout is exceeded', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + context.invokeWithRetryStub.callsFake(async () => { + throw new RetryError(RetryErrorCode.TimeoutExceeded, { + request: new Request('http://example.com/result/1'), + response: new Response('', { status: 503, statusText: 'Service Unavailable' }), + }); + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.instanceOf(RetryError); + expect(thrown.errorCode).to.be.equal(RetryErrorCode.TimeoutExceeded); + expect(thrown.message).to.be.equal( + 'CloudFetch result download failed: Retry timeout exceeded (last error: HTTP 503: Service Unavailable)', + ); + expect(thrown.stack.split('\n')[0]).to.be.equal(`Error: ${thrown.message}`); + }); + it('should add download context and network error cause to RetryError when retries are exhausted', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); @@ -479,7 +510,7 @@ describe('CloudFetchResultHandler', () => { expect(thrown.message).to.contain('Max retry count exceeded'); }); - it('should not leak presigned URLs from network errors into the RetryError message', async () => { + it('should not leak presigned URLs from network errors into the RetryError message or payload', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); @@ -512,6 +543,11 @@ describe('CloudFetchResultHandler', () => { expect(thrown.message).to.not.contain('SECRET_SIGNATURE'); expect(thrown.message).to.not.contain('storage.example.com'); expect(thrown.stack).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.payload.message).to.contain('socket hang up'); + expect(thrown.payload.message).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.payload.stack).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.payload.code).to.be.equal('ECONNRESET'); + expect(inspect(thrown, { depth: 5 })).to.not.contain('SECRET_SIGNATURE'); }); it('should handle expired links', async () => { From d9f19fbc68bfa4625639030fe57e28837704c537 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:20:29 +0000 Subject: [PATCH 05/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498212388 at lib/result/CloudFetchResultHandler.ts:217 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 80 ++++++++++++++----- .../result/CloudFetchResultHandler.test.ts | 65 +++++++++++++++ 2 files changed, 125 insertions(+), 20 deletions(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 23217c3c..dafb71a3 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -183,14 +183,46 @@ export default class CloudFetchResultHandler implements IResultsProvider'); + } + + /** + * Network errors (e.g. node-fetch `FetchError`) embed the request URL in their message and stack. + * For CloudFetch that is a presigned cloud-storage link whose query string carries credentials, + * so scrub any URL in place. Best effort: non-writable properties on exotic error objects must + * not mask the original failure. + */ + private static scrubErrorUrls(error: Error): void { + try { + error.message = CloudFetchResultHandler.redactUrls(error.message); + if (typeof error.stack === 'string') { + error.stack = CloudFetchResultHandler.redactUrls(error.stack); + } + } catch { + // ignore + } + } + /** - * When the retry policy gives up (`RetryError`), its message is a generic "Max retry count - * exceeded" / "Retry timeout exceeded" that hides both the CloudFetch download context and the - * actual cause (last HTTP status or network error). Enrich the message while keeping the same - * error class, error code and payload so existing `instanceof` / `errorCode` checks still work. + * Make a download failure safe to log and easier to diagnose: + * - Any error escaping the retry policy (wrapped in `RetryError` or re-thrown unchanged, e.g. a + * non-retryable TLS `FetchError`) has URLs scrubbed from its message / stack, since the + * presigned CloudFetch link carries credentials in its query string. + * - For `RetryError`, the generic "Max retry count exceeded" / "Retry timeout exceeded" message + * is enriched with the CloudFetch context and the last cause (HTTP status or network error). + * When the payload is an HTTP `{ request, response }` pair, the node-fetch objects (which hold + * the full presigned URL) are replaced with a plain snapshot carrying the status, status text, + * headers and the URL without its query string, so `util.inspect` / structured-logger + * serializers that walk `error.payload` don't print credentials. + * The same error instance, class and error code are kept so `instanceof` / `errorCode` checks + * still work. */ private static describeDownloadError(error: unknown): unknown { if (!(error instanceof RetryError)) { + if (error instanceof Error) { + CloudFetchResultHandler.scrubErrorUrls(error); + } return error; } @@ -198,27 +230,35 @@ export default class CloudFetchResultHandler implements IResultsProvider text.replace(/\b[a-z][a-z0-9+.-]*:\/\/\S+/gi, ''); - const safeMessage = redactUrls(payload.message); - try { - payload.message = safeMessage; - if (typeof payload.stack === 'string') { - payload.stack = redactUrls(payload.stack); - } - } catch { - // Best effort: non-writable properties on exotic error objects must not mask the failure. - } + CloudFetchResultHandler.scrubErrorUrls(payload); + const safeMessage = CloudFetchResultHandler.redactUrls(payload.message); cause = code ? `${safeMessage} (${code})` : safeMessage; } else if (payload && typeof payload === 'object' && 'response' in payload) { - const { response } = payload as { response?: { status?: number; statusText?: string } }; + const { request, response } = payload as { + request?: { method?: string; url?: string }; + response?: { status?: number; statusText?: string; ok?: boolean; headers?: unknown; url?: string }; + }; if (response && typeof response.status === 'number') { cause = `HTTP ${response.status}${response.statusText ? `: ${response.statusText}` : ''}`; } + const stripQuery = (url?: string) => (typeof url === 'string' ? url.split('?')[0] : undefined); + const safePayload = { + request: request ? { method: request.method, url: stripQuery(request.url) } : undefined, + response: response + ? { + status: response.status, + statusText: response.statusText, + ok: response.ok, + headers: response.headers, + url: stripQuery(response.url), + } + : undefined, + }; + try { + Object.defineProperty(error, 'payload', { value: safePayload }); + } catch { + // Best effort: never mask the original failure. + } } // Mutate the original error rather than creating a new one, so the stack trace still points at diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index 93eb164b..de4ab66e 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -550,6 +550,71 @@ describe('CloudFetchResultHandler', () => { expect(inspect(thrown, { depth: 5 })).to.not.contain('SECRET_SIGNATURE'); }); + it('should not leak presigned URLs from the HTTP request/response payload of a RetryError', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; + const originalError = new RetryError(RetryErrorCode.AttemptsExceeded, { + request: new Request(presignedUrl), + response: new Response('', { status: 503, statusText: 'Service Unavailable', url: presignedUrl } as any), + }); + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(thrown).to.be.instanceOf(RetryError); + expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); + expect(thrown.message).to.contain('HTTP 503: Service Unavailable'); + expect(thrown.payload.response.status).to.be.equal(503); + expect(thrown.payload.response.statusText).to.be.equal('Service Unavailable'); + expect(thrown.payload.request.url).to.be.equal('https://storage.example.com/results/chunk-0'); + expect(inspect(thrown, { depth: 10, showHidden: true })).to.not.contain('SECRET_SIGNATURE'); + }); + + it('should not leak presigned URLs from network errors re-thrown without a RetryError wrapper', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; + const originalError = Object.assign( + new Error(`request to ${presignedUrl} failed, reason: unable to verify the first certificate`), + { code: 'UNABLE_TO_VERIFY_LEAF_SIGNATURE', type: 'system' }, + ); + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(thrown.code).to.be.equal('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(thrown.message).to.contain('unable to verify the first certificate'); + expect(thrown.message).to.not.contain('SECRET_SIGNATURE'); + expect(inspect(thrown, { depth: 5 })).to.not.contain('SECRET_SIGNATURE'); + }); + it('should handle expired links', async () => { const context = new ClientContextStub(); const rowSetProvider = new ResultsProviderStub([sampleExpiredRowSet], undefined); From fcc7be4d0ca7f2b1ff093a846cdc9c71d50d845f Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:23:51 +0000 Subject: [PATCH 06/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498240240 at lib/result/CloudFetchResultHandler.ts:258 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 1 + lib/result/CloudFetchResultHandler.ts | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 957853dc..4a7b268d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +- Redact presigned URLs from CloudFetch download errors. For CloudFetch HTTP failures, `RetryError.payload` is now a plain `{ request: { method, url }, response: { status, statusText, ok, headers, url } }` snapshot (URLs without query string) instead of the node-fetch `Request` / `Response` objects, so `payload.response instanceof Response` and `payload.response.text()` / `.json()` no longer apply to these errors. - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) ## 3.0.0 diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index dafb71a3..922a0af5 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -214,7 +214,11 @@ export default class CloudFetchResultHandler implements IResultsProvider Date: Fri, 6 Nov 2026 22:27:54 +0000 Subject: [PATCH 07/15] ai: apply changes for #567 (2 review threads) Addresses: - #4498266048 at lib/result/CloudFetchResultHandler.ts:198 - #4498266060 at tests/e2e/cloudfetch.test.ts:182 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 7 ++++- tests/e2e/cloudfetch.test.ts | 9 ++++-- .../result/CloudFetchResultHandler.test.ts | 28 +++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 922a0af5..77f1c2db 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -197,7 +197,12 @@ export default class CloudFetchResultHandler implements IResultsProvider (/^\s+at /.test(line) ? line : CloudFetchResultHandler.redactUrls(line))) + .join('\n'); } } catch { // ignore diff --git a/tests/e2e/cloudfetch.test.ts b/tests/e2e/cloudfetch.test.ts index faa82d86..2f0c7011 100644 --- a/tests/e2e/cloudfetch.test.ts +++ b/tests/e2e/cloudfetch.test.ts @@ -177,9 +177,12 @@ describe('CloudFetch', () => { expect(resultHandler.source.source).to.be.instanceOf(CloudFetchResultHandler); const cfResultHandler = resultHandler.source.source; - // Plain-HTTP local server: let node-fetch pick its default agent for the download + // Plain-HTTP local server: let node-fetch pick its default agent for the download. + // The stub is scoped to the `fetchChunk` call below and restored right after it, so + // later requests on this connection provider (e.g. `session.close()`) keep the real agent + // (proxy / custom CA / keep-alive config) const connectionProvider = await cfResultHandler.context.getConnectionProvider(); - sinon.stub(connectionProvider, 'getAgent').resolves(undefined); + const getAgentStub = sinon.stub(connectionProvider, 'getAgent').resolves(undefined); // Redirect every presigned link to the local server that always responds with HTTP 500, // keeping the real expiry time and row count returned by the warehouse @@ -199,6 +202,8 @@ describe('CloudFetch', () => { await operation.fetchChunk({ maxRows: 100000, disableBuffering: true }); } catch (error) { thrown = error; + } finally { + getAgentStub.restore(); } expect(thrown).to.be.instanceOf(Error); diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index de4ab66e..c1beba56 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -615,6 +615,34 @@ describe('CloudFetchResultHandler', () => { expect(inspect(thrown, { depth: 5 })).to.not.contain('SECRET_SIGNATURE'); }); + it('should keep file:// stack frame locations when scrubbing URLs from error stacks', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; + const frame = ' at fetchChunk (file:///app/node_modules/pkg/x.mjs:12:3)'; + const originalError = new Error(`request to ${presignedUrl} failed, reason: socket hang up`); + originalError.stack = `Error: ${originalError.message}\n${frame}`; + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(thrown.stack).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.stack.split('\n')[1]).to.be.equal(frame); + }); + it('should handle expired links', async () => { const context = new ClientContextStub(); const rowSetProvider = new ResultsProviderStub([sampleExpiredRowSet], undefined); From 8889dee50f69e4960fbd371a6e8d19d3b7ed68ba Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:31:38 +0000 Subject: [PATCH 08/15] ai: apply changes for #567 (2 review threads) Addresses: - #4498293235 at CHANGELOG.md:5 - #4498293241 at lib/result/CloudFetchResultHandler.ts:265 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 1 + lib/result/CloudFetchResultHandler.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a7b268d..99a63e23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased - Redact presigned URLs from CloudFetch download errors. For CloudFetch HTTP failures, `RetryError.payload` is now a plain `{ request: { method, url }, response: { status, statusText, ok, headers, url } }` snapshot (URLs without query string) instead of the node-fetch `Request` / `Response` objects, so `payload.response instanceof Response` and `payload.response.text()` / `.json()` no longer apply to these errors. +- CloudFetch download error messages changed as part of the same redaction. A `RetryError` thrown from a CloudFetch download no longer has exactly the message `'Max retry count exceeded'` / `'Retry timeout exceeded'`. The message now reads `CloudFetch result download failed: (last error: )`, and the `stack` header is rewritten to match. Network errors from CloudFetch downloads (a non-`RetryError` error re-thrown unchanged, or a network error held in `RetryError.payload`) now have every URL in their `message` and stack header replaced with ``. Code that compares `err.message` against the fixed retry messages will no longer match. Check `err instanceof RetryError` and `err.errorCode` (`RetryErrorCode.AttemptsExceeded` / `RetryErrorCode.TimeoutExceeded`) instead; both are unchanged. - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) ## 3.0.0 diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 77f1c2db..80b1d993 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -263,6 +263,21 @@ export default class CloudFetchResultHandler implements IResultsProvider void) => unknown; resume?: () => unknown } + | undefined; + if (body && typeof body.resume === 'function') { + if (typeof body.on === 'function') { + body.on('error', () => {}); + } + body.resume(); + } + } catch { + // Best effort: never mask the original failure. + } try { Object.defineProperty(error, 'payload', { value: safePayload }); } catch { From d5e99aacf4c28230801f2494861e0d7878272af4 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:38:11 +0000 Subject: [PATCH 09/15] ai: apply changes for #567 (2 review threads) Addresses: - #4498326409 at lib/result/CloudFetchResultHandler.ts:265 - #4498326429 at lib/result/CloudFetchResultHandler.ts:245 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 15 ++++++- .../result/CloudFetchResultHandler.test.ts | 41 +++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 80b1d993..2d4ef287 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -195,7 +195,9 @@ export default class CloudFetchResultHandler implements IResultsProvider { expect(inspect(thrown, { depth: 10, showHidden: true })).to.not.contain('SECRET_SIGNATURE'); }); + it('should drain the body of the final HTTP response when replacing a RetryError payload', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + const body = new Readable({ read() {} }); + body.push('error body'); + body.push(null); + const response = new Response(body as any, { status: 503, statusText: 'Service Unavailable' }); + expect(response.body).to.be.equal(body); + const ended = new Promise((resolve) => { + body.once('end', () => resolve()); + }); + + const originalError = new RetryError(RetryErrorCode.AttemptsExceeded, { + request: new Request('https://storage.example.com/results/chunk-0'), + response, + }); + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(body.readableFlowing).to.be.true; + await ended; + + // A late stream error must not become an unhandled 'error' event (which would throw). + expect(() => body.emit('error', new Error('socket hang up'))).to.not.throw(); + }); + it('should not leak presigned URLs from network errors re-thrown without a RetryError wrapper', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); From 7222abce0367e158c6ee3b7044b4880106efd30d Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:42:17 +0000 Subject: [PATCH 10/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498373579 at lib/result/CloudFetchResultHandler.ts:293 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99a63e23..5f2fe3da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,8 @@ ## Unreleased -- Redact presigned URLs from CloudFetch download errors. For CloudFetch HTTP failures, `RetryError.payload` is now a plain `{ request: { method, url }, response: { status, statusText, ok, headers, url } }` snapshot (URLs without query string) instead of the node-fetch `Request` / `Response` objects, so `payload.response instanceof Response` and `payload.response.text()` / `.json()` no longer apply to these errors. -- CloudFetch download error messages changed as part of the same redaction. A `RetryError` thrown from a CloudFetch download no longer has exactly the message `'Max retry count exceeded'` / `'Retry timeout exceeded'`. The message now reads `CloudFetch result download failed: (last error: )`, and the `stack` header is rewritten to match. Network errors from CloudFetch downloads (a non-`RetryError` error re-thrown unchanged, or a network error held in `RetryError.payload`) now have every URL in their `message` and stack header replaced with ``. Code that compares `err.message` against the fixed retry messages will no longer match. Check `err instanceof RetryError` and `err.errorCode` (`RetryErrorCode.AttemptsExceeded` / `RetryErrorCode.TimeoutExceeded`) instead; both are unchanged. +- **Breaking (security):** Redact presigned URLs from CloudFetch download errors. For CloudFetch HTTP failures, `RetryError.payload` is now a plain `{ request: { method, url }, response: { status, statusText, ok, headers, url } }` snapshot (URLs without query string) instead of the node-fetch `Request` / `Response` objects, so `payload.response instanceof Response` and `payload.response.text()` / `.json()` no longer apply to these errors. `payload.response.headers` is still the original node-fetch `Headers` object (so `headers.get(...)` keeps working), and `status` / `statusText` / `ok` keep their values; only the URL-bearing objects and the response body are no longer exposed. +- **Breaking:** CloudFetch download error messages changed as part of the same redaction. A `RetryError` thrown from a CloudFetch download no longer has exactly the message `'Max retry count exceeded'` / `'Retry timeout exceeded'`. The message now reads `CloudFetch result download failed: (last error: )`, and the `stack` header is rewritten to match. Network errors from CloudFetch downloads (a non-`RetryError` error re-thrown unchanged, or a network error held in `RetryError.payload`) now have every URL in their `message` and stack header replaced with ``. Code that compares `err.message` against the fixed retry messages will no longer match. Check `err instanceof RetryError` and `err.errorCode` (`RetryErrorCode.AttemptsExceeded` / `RetryErrorCode.TimeoutExceeded`) instead; both are unchanged. - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) ## 3.0.0 From d7de1071262f717753465d70c1c44f98f8c00c26 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:45:56 +0000 Subject: [PATCH 11/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498401855 at lib/result/CloudFetchResultHandler.ts:187 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/result/CloudFetchResultHandler.ts | 8 +++++- .../result/CloudFetchResultHandler.test.ts | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 2d4ef287..0f7326b6 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -184,7 +184,13 @@ export default class CloudFetchResultHandler implements IResultsProvider'); + // Match greedily up to whitespace (so a `'` / `)` inside the URL can't leave the credentialed tail + // unredacted), then hand back only trailing closing delimiters / sentence punctuation so quoted or + // parenthesised URLs keep the surrounding message well-formed. + return text.replace(/\b[a-z][a-z0-9+.-]*:\/\/\S+/gi, (url) => { + const trailing = /['")\]>.,;:!?]*$/.exec(url)?.[0] ?? ''; + return `${trailing}`; + }); } /** diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index a9842817..40391be7 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -684,6 +684,31 @@ describe('CloudFetchResultHandler', () => { expect(thrown.stack.split('\n')[1]).to.be.equal(frame); }); + it('should keep closing delimiters around quoted or parenthesised URLs when scrubbing', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); + + const result = new CloudFetchResultHandler(context, rowSetProvider, { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; + const originalError = new Error(`fetch to '${presignedUrl}'. failed (${presignedUrl})`); + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + let thrown: any; + try { + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(thrown.message).to.be.equal(`fetch to ''. failed ()`); + }); + it('should handle expired links', async () => { const context = new ClientContextStub(); const rowSetProvider = new ResultsProviderStub([sampleExpiredRowSet], undefined); From e3f63e86e943a3257a3603cdb60311c19eac90d8 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:51:44 +0000 Subject: [PATCH 12/15] ai: apply changes for #567 (2 review threads) Addresses: - #4498436205 at tests/e2e/cloudfetch.test.ts:209 - #4498436211 at lib/result/CloudFetchResultHandler.ts:242 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 2 +- lib/result/CloudFetchResultHandler.ts | 4 +++- tests/e2e/cloudfetch.test.ts | 25 ++++++++++++++++++++----- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f2fe3da..3c27c431 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## Unreleased - **Breaking (security):** Redact presigned URLs from CloudFetch download errors. For CloudFetch HTTP failures, `RetryError.payload` is now a plain `{ request: { method, url }, response: { status, statusText, ok, headers, url } }` snapshot (URLs without query string) instead of the node-fetch `Request` / `Response` objects, so `payload.response instanceof Response` and `payload.response.text()` / `.json()` no longer apply to these errors. `payload.response.headers` is still the original node-fetch `Headers` object (so `headers.get(...)` keeps working), and `status` / `statusText` / `ok` keep their values; only the URL-bearing objects and the response body are no longer exposed. -- **Breaking:** CloudFetch download error messages changed as part of the same redaction. A `RetryError` thrown from a CloudFetch download no longer has exactly the message `'Max retry count exceeded'` / `'Retry timeout exceeded'`. The message now reads `CloudFetch result download failed: (last error: )`, and the `stack` header is rewritten to match. Network errors from CloudFetch downloads (a non-`RetryError` error re-thrown unchanged, or a network error held in `RetryError.payload`) now have every URL in their `message` and stack header replaced with ``. Code that compares `err.message` against the fixed retry messages will no longer match. Check `err instanceof RetryError` and `err.errorCode` (`RetryErrorCode.AttemptsExceeded` / `RetryErrorCode.TimeoutExceeded`) instead; both are unchanged. +- **Breaking:** CloudFetch download error messages changed as part of the same redaction. A `RetryError` thrown from a CloudFetch download no longer has exactly the message `'Max retry count exceeded'` / `'Retry timeout exceeded'`. The message now reads `CloudFetch result download failed: (last error: )`, and the `stack` header is rewritten to match. Every `Error` escaping a CloudFetch download's retry policy (any non-`RetryError` error re-thrown unchanged, not only node-fetch `FetchError`s, plus any error held in `RetryError.payload`) now has every `scheme://...` URL in its `message` and stack header replaced with ``; this includes unrelated URLs such as a proxy URL in an agent error. Stack frame lines (` at ...`) are left untouched. Code that compares `err.message` against the fixed retry messages will no longer match. Check `err instanceof RetryError` and `err.errorCode` (`RetryErrorCode.AttemptsExceeded` / `RetryErrorCode.TimeoutExceeded`) instead; both are unchanged. - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) ## 3.0.0 diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 0f7326b6..5ae7dc73 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -221,7 +221,9 @@ export default class CloudFetchResultHandler implements IResultsProvider { // keeping the real expiry time and row count returned by the warehouse const rowSetProvider = cfResultHandler.source; const originalFetchNext = rowSetProvider.fetchNext.bind(rowSetProvider); + const originalFileLinks: Array = []; sinon.stub(rowSetProvider, 'fetchNext').callsFake(async (...args: Array) => { const rowSet = await originalFetchNext(...args); rowSet?.resultLinks?.forEach((link: any) => { + originalFileLinks.push(link.fileLink); link.fileLink = `http://127.0.0.1:${port}/result`; link.httpHeaders = {}; }); @@ -206,12 +209,24 @@ describe('CloudFetch', () => { getAgentStub.restore(); } - expect(thrown).to.be.instanceOf(Error); + // Pin the retry-exhaustion path (not the plain `CloudFetch HTTP error ...` thrown by `downloadLink`) + expect(thrown).to.be.instanceOf(RetryError); + expect((thrown as RetryError).errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); expect(serverHits).to.be.equal(retryMaxAttempts); - const { message } = thrown as Error; - expect(message).to.contain('500'); - expect(message).to.contain('Internal Server Error'); - expect(message).to.match(/cloudfetch|download/i); + const { message } = thrown as RetryError; + expect(message).to.contain('CloudFetch result download failed'); + expect(message).to.contain('Max retry count exceeded'); + expect(message).to.contain('HTTP 500: Internal Server Error'); + + // The real presigned link (host and credentialed query string) must not leak into the message + expect(originalFileLinks.length).to.be.gt(0); + originalFileLinks.forEach((fileLink) => { + const { host, search } = new URL(fileLink); + expect(message).to.not.contain(host); + if (search) { + expect(message).to.not.contain(search); + } + }); } finally { await new Promise((resolve) => { server.close(() => resolve()); From 34c1b4f66258f0dac476150928495afaa5e36c7c Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 7 Nov 2026 00:12:42 +0000 Subject: [PATCH 13/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498463556 at lib/result/CloudFetchResultHandler.ts:302 Signed-off-by: peco-engineer-bot[bot] --- CHANGELOG.md | 3 +- lib/result/CloudFetchResultHandler.ts | 122 +++------------- .../result/CloudFetchResultHandler.test.ts | 138 +++++------------- 3 files changed, 51 insertions(+), 212 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c27c431..22e15e66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,7 @@ ## Unreleased -- **Breaking (security):** Redact presigned URLs from CloudFetch download errors. For CloudFetch HTTP failures, `RetryError.payload` is now a plain `{ request: { method, url }, response: { status, statusText, ok, headers, url } }` snapshot (URLs without query string) instead of the node-fetch `Request` / `Response` objects, so `payload.response instanceof Response` and `payload.response.text()` / `.json()` no longer apply to these errors. `payload.response.headers` is still the original node-fetch `Headers` object (so `headers.get(...)` keeps working), and `status` / `statusText` / `ok` keep their values; only the URL-bearing objects and the response body are no longer exposed. -- **Breaking:** CloudFetch download error messages changed as part of the same redaction. A `RetryError` thrown from a CloudFetch download no longer has exactly the message `'Max retry count exceeded'` / `'Retry timeout exceeded'`. The message now reads `CloudFetch result download failed: (last error: )`, and the `stack` header is rewritten to match. Every `Error` escaping a CloudFetch download's retry policy (any non-`RetryError` error re-thrown unchanged, not only node-fetch `FetchError`s, plus any error held in `RetryError.payload`) now has every `scheme://...` URL in its `message` and stack header replaced with ``; this includes unrelated URLs such as a proxy URL in an agent error. Stack frame lines (` at ...`) are left untouched. Code that compares `err.message` against the fixed retry messages will no longer match. Check `err instanceof RetryError` and `err.errorCode` (`RetryErrorCode.AttemptsExceeded` / `RetryErrorCode.TimeoutExceeded`) instead; both are unchanged. +- CloudFetch retry-exhaustion errors now identify the download and the last failure: the `RetryError` message reads `CloudFetch result download failed: (last error: HTTP : )`, or `(last error: network error )` for network failures (the raw network-error message, which can contain the presigned URL, is not copied). The `RetryError` instance, `errorCode` and `payload` (including the node-fetch `Request` / `Response`) are unchanged. (#471) - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) ## 3.0.0 diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index 5ae7dc73..e1c71e73 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -183,124 +183,36 @@ export default class CloudFetchResultHandler implements IResultsProvider { - const trailing = /['")\]>.,;:!?]*$/.exec(url)?.[0] ?? ''; - return `${trailing}`; - }); - } - - /** - * Network errors (e.g. node-fetch `FetchError`) embed the request URL in their message and stack. - * For CloudFetch that is a presigned cloud-storage link whose query string carries credentials, - * so scrub any URL in place. Best effort: non-writable properties on exotic error objects must - * not mask the original failure. - */ - private static scrubErrorUrls(error: Error): void { - try { - if (typeof error.message === 'string') { - error.message = CloudFetchResultHandler.redactUrls(error.message); - } - if (typeof error.stack === 'string') { - // Only the header (message) lines can carry the request URL; leave ` at ...` frames intact so - // `file://` locations of ESM modules stay readable. - error.stack = error.stack - .split('\n') - .map((line) => (/^\s+at /.test(line) ? line : CloudFetchResultHandler.redactUrls(line))) - .join('\n'); - } - } catch { - // ignore - } - } - /** - * Make a download failure safe to log and easier to diagnose: - * - Any error escaping the retry policy (wrapped in `RetryError` or re-thrown unchanged, e.g. a - * non-retryable TLS `FetchError`) has URLs scrubbed from its message / stack, since the - * presigned CloudFetch link carries credentials in its query string. This deliberately applies to - * every `Error`, not only node-fetch `FetchError`s: errors from custom agents / proxies or wrapped - * errors can embed the request URL too, so any `scheme://` text (e.g. a proxy URL) is redacted. - * - For `RetryError`, the generic "Max retry count exceeded" / "Retry timeout exceeded" message - * is enriched with the CloudFetch context and the last cause (HTTP status or network error). - * When the payload is an HTTP `{ request, response }` pair, the node-fetch objects (which hold - * the full presigned URL) are replaced with a plain snapshot carrying the status, status text, - * headers and the URL without its query string, so `util.inspect` / structured-logger - * serializers that walk `error.payload` don't print credentials. Note this changes the observable - * payload shape for CloudFetch HTTP failures: `payload.response` is no longer a node-fetch - * `Response` (so `instanceof Response`, `.text()` / `.json()` do not apply); it is a plain - * `{ status, statusText, ok, headers, url }` object. This is intentional — keeping the original - * `Response` reachable would keep the credentialed URL reachable too. - * The same error instance, class and error code are kept so `instanceof` / `errorCode` checks - * still work. + * Make a CloudFetch retry-exhaustion failure easier to diagnose (#471): the generic + * "Max retry count exceeded" / "Retry timeout exceeded" message of a `RetryError` is enriched with + * the CloudFetch context and the last cause — the HTTP status / status text, or for network errors + * only the error code / type. Raw network-error messages are deliberately NOT copied, because + * node-fetch embeds the presigned (credential-bearing) request URL in them. + * The same error instance, class, error code and payload (including the node-fetch + * `Request` / `Response` objects) are kept. Errors other than `RetryError` are returned unchanged. */ private static describeDownloadError(error: unknown): unknown { if (!(error instanceof RetryError)) { - if (error instanceof Error) { - CloudFetchResultHandler.scrubErrorUrls(error); - } return error; } let cause: string | undefined; const { payload } = error; if (payload instanceof Error) { - const { code } = payload as Error & { code?: unknown }; - CloudFetchResultHandler.scrubErrorUrls(payload); - let safeMessage: string; - try { - // `message` may be non-string on exotic error subclasses; never let that mask the `RetryError`. - const rawMessage: unknown = payload.message; - safeMessage = CloudFetchResultHandler.redactUrls( - typeof rawMessage === 'string' ? rawMessage : String(rawMessage), - ); - } catch { - safeMessage = ''; + const { code, type } = payload as Error & { code?: unknown; type?: unknown }; + if (typeof code === 'string' && code) { + cause = `network error ${code}`; + } else if (typeof type === 'string' && type) { + cause = `network error (${type})`; + } else { + cause = 'network error'; } - cause = code ? `${safeMessage} (${code})` : safeMessage; } else if (payload && typeof payload === 'object' && 'response' in payload) { - const { request, response } = payload as { - request?: { method?: string; url?: string }; - response?: { status?: number; statusText?: string; ok?: boolean; headers?: unknown; url?: string }; - }; + const { response } = payload as { response?: { status?: unknown; statusText?: unknown } }; if (response && typeof response.status === 'number') { - cause = `HTTP ${response.status}${response.statusText ? `: ${response.statusText}` : ''}`; - } - const stripQuery = (url?: string) => (typeof url === 'string' ? url.split('?')[0] : undefined); - const safePayload = { - request: request ? { method: request.method, url: stripQuery(request.url) } : undefined, - response: response - ? { - status: response.status, - statusText: response.statusText, - ok: response.ok, - headers: response.headers, - url: stripQuery(response.url), - } - : undefined, - }; - // The original `Response` becomes unreachable once replaced below, so nobody could drain its body - // afterwards. Drain it now (best effort) so an unread stream doesn't pin a keep-alive socket. - try { - const body = (response as { body?: unknown } | undefined)?.body as - | { on?: (event: string, listener: () => void) => unknown; resume?: () => unknown } - | undefined; - if (body && typeof body.resume === 'function') { - if (typeof body.on === 'function') { - body.on('error', () => {}); - } - body.resume(); - } - } catch { - // Best effort: never mask the original failure. - } - try { - Object.defineProperty(error, 'payload', { value: safePayload }); - } catch { - // Best effort: never mask the original failure. + const { statusText } = response; + cause = `HTTP ${response.status}${typeof statusText === 'string' && statusText ? `: ${statusText}` : ''}`; } } diff --git a/tests/unit/result/CloudFetchResultHandler.test.ts b/tests/unit/result/CloudFetchResultHandler.test.ts index 40391be7..9aa08813 100644 --- a/tests/unit/result/CloudFetchResultHandler.test.ts +++ b/tests/unit/result/CloudFetchResultHandler.test.ts @@ -1,5 +1,3 @@ -import { inspect } from 'util'; -import { Readable } from 'stream'; import { expect, AssertionError } from 'chai'; import sinon, { SinonStub } from 'sinon'; import Int64 from 'node-int64'; @@ -506,12 +504,12 @@ describe('CloudFetchResultHandler', () => { expect(thrown.stack.split('\n')[0]).to.be.equal(`Error: ${thrown.message}`); expect(thrown).to.be.instanceOf(RetryError); expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); - expect(thrown.message).to.contain('CloudFetch'); - expect(thrown.message).to.contain('socket hang up'); - expect(thrown.message).to.contain('Max retry count exceeded'); + expect(thrown.message).to.be.equal( + 'CloudFetch result download failed: Max retry count exceeded (last error: network error ECONNRESET)', + ); }); - it('should not leak presigned URLs from network errors into the RetryError message or payload', async () => { + it('should not leak presigned URLs from network errors into the RetryError message', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); @@ -539,53 +537,13 @@ describe('CloudFetchResultHandler', () => { } expect(thrown).to.be.equal(originalError); - expect(thrown.message).to.contain('socket hang up'); expect(thrown.message).to.contain('ECONNRESET'); expect(thrown.message).to.not.contain('SECRET_SIGNATURE'); expect(thrown.message).to.not.contain('storage.example.com'); - expect(thrown.stack).to.not.contain('SECRET_SIGNATURE'); - expect(thrown.payload.message).to.contain('socket hang up'); - expect(thrown.payload.message).to.not.contain('SECRET_SIGNATURE'); - expect(thrown.payload.stack).to.not.contain('SECRET_SIGNATURE'); - expect(thrown.payload.code).to.be.equal('ECONNRESET'); - expect(inspect(thrown, { depth: 5 })).to.not.contain('SECRET_SIGNATURE'); - }); - - it('should not leak presigned URLs from the HTTP request/response payload of a RetryError', async () => { - const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); - const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); - - const result = new CloudFetchResultHandler(context, rowSetProvider, { - status: { statusCode: TStatusCode.SUCCESS_STATUS }, - }); - - const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; - const originalError = new RetryError(RetryErrorCode.AttemptsExceeded, { - request: new Request(presignedUrl), - response: new Response('', { status: 503, statusText: 'Service Unavailable', url: presignedUrl } as any), - }); - context.invokeWithRetryStub.callsFake(async () => { - throw originalError; - }); - - let thrown: any; - try { - await result.fetchNext({ limit: 10000 }); - } catch (e) { - thrown = e; - } - - expect(thrown).to.be.equal(originalError); - expect(thrown).to.be.instanceOf(RetryError); - expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); - expect(thrown.message).to.contain('HTTP 503: Service Unavailable'); - expect(thrown.payload.response.status).to.be.equal(503); - expect(thrown.payload.response.statusText).to.be.equal('Service Unavailable'); - expect(thrown.payload.request.url).to.be.equal('https://storage.example.com/results/chunk-0'); - expect(inspect(thrown, { depth: 10, showHidden: true })).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.stack.split('\n')[0]).to.not.contain('SECRET_SIGNATURE'); }); - it('should drain the body of the final HTTP response when replacing a RetryError payload', async () => { + it('should use the network error type when no code is available', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); @@ -593,19 +551,10 @@ describe('CloudFetchResultHandler', () => { status: { statusCode: TStatusCode.SUCCESS_STATUS }, }); - const body = new Readable({ read() {} }); - body.push('error body'); - body.push(null); - const response = new Response(body as any, { status: 503, statusText: 'Service Unavailable' }); - expect(response.body).to.be.equal(body); - const ended = new Promise((resolve) => { - body.once('end', () => resolve()); - }); - - const originalError = new RetryError(RetryErrorCode.AttemptsExceeded, { - request: new Request('https://storage.example.com/results/chunk-0'), - response, - }); + const originalError = new RetryError( + RetryErrorCode.AttemptsExceeded, + Object.assign(new Error('request to https://storage.example.com/x?sig=SECRET failed'), { type: 'aborted' }), + ); context.invokeWithRetryStub.callsFake(async () => { throw originalError; }); @@ -618,14 +567,12 @@ describe('CloudFetchResultHandler', () => { } expect(thrown).to.be.equal(originalError); - expect(body.readableFlowing).to.be.true; - await ended; - - // A late stream error must not become an unhandled 'error' event (which would throw). - expect(() => body.emit('error', new Error('socket hang up'))).to.not.throw(); + expect(thrown.message).to.be.equal( + 'CloudFetch result download failed: Max retry count exceeded (last error: network error (aborted))', + ); }); - it('should not leak presigned URLs from network errors re-thrown without a RetryError wrapper', async () => { + it('should preserve the original node-fetch request/response payload of a RetryError', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); @@ -634,10 +581,10 @@ describe('CloudFetchResultHandler', () => { }); const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; - const originalError = Object.assign( - new Error(`request to ${presignedUrl} failed, reason: unable to verify the first certificate`), - { code: 'UNABLE_TO_VERIFY_LEAF_SIGNATURE', type: 'system' }, - ); + const request = new Request(presignedUrl); + const response = new Response('', { status: 503, statusText: 'Service Unavailable', url: presignedUrl } as any); + const payload = { request, response }; + const originalError = new RetryError(RetryErrorCode.AttemptsExceeded, payload); context.invokeWithRetryStub.callsFake(async () => { throw originalError; }); @@ -650,13 +597,17 @@ describe('CloudFetchResultHandler', () => { } expect(thrown).to.be.equal(originalError); - expect(thrown.code).to.be.equal('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); - expect(thrown.message).to.contain('unable to verify the first certificate'); + expect(thrown).to.be.instanceOf(RetryError); + expect(thrown.errorCode).to.be.equal(RetryErrorCode.AttemptsExceeded); + expect(thrown.message).to.contain('HTTP 503: Service Unavailable'); expect(thrown.message).to.not.contain('SECRET_SIGNATURE'); - expect(inspect(thrown, { depth: 5 })).to.not.contain('SECRET_SIGNATURE'); + expect(thrown.payload).to.be.equal(payload); + expect(thrown.payload.request).to.be.equal(request); + expect(thrown.payload.response).to.be.equal(response); + expect(thrown.payload.response).to.be.instanceOf(Response); }); - it('should keep file:// stack frame locations when scrubbing URLs from error stacks', async () => { + it('should re-throw errors other than RetryError unchanged', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); @@ -664,36 +615,11 @@ describe('CloudFetchResultHandler', () => { status: { statusCode: TStatusCode.SUCCESS_STATUS }, }); - const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; - const frame = ' at fetchChunk (file:///app/node_modules/pkg/x.mjs:12:3)'; - const originalError = new Error(`request to ${presignedUrl} failed, reason: socket hang up`); - originalError.stack = `Error: ${originalError.message}\n${frame}`; - context.invokeWithRetryStub.callsFake(async () => { - throw originalError; + const originalError = Object.assign(new Error('unable to verify the first certificate'), { + code: 'UNABLE_TO_VERIFY_LEAF_SIGNATURE', }); - - let thrown: any; - try { - await result.fetchNext({ limit: 10000 }); - } catch (e) { - thrown = e; - } - - expect(thrown).to.be.equal(originalError); - expect(thrown.stack).to.not.contain('SECRET_SIGNATURE'); - expect(thrown.stack.split('\n')[1]).to.be.equal(frame); - }); - - it('should keep closing delimiters around quoted or parenthesised URLs when scrubbing', async () => { - const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); - const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined); - - const result = new CloudFetchResultHandler(context, rowSetProvider, { - status: { statusCode: TStatusCode.SUCCESS_STATUS }, - }); - - const presignedUrl = 'https://storage.example.com/results/chunk-0?sig=SECRET_SIGNATURE&se=2026-11-06'; - const originalError = new Error(`fetch to '${presignedUrl}'. failed (${presignedUrl})`); + const originalMessage = originalError.message; + const originalStack = originalError.stack; context.invokeWithRetryStub.callsFake(async () => { throw originalError; }); @@ -706,7 +632,9 @@ describe('CloudFetchResultHandler', () => { } expect(thrown).to.be.equal(originalError); - expect(thrown.message).to.be.equal(`fetch to ''. failed ()`); + expect(thrown.code).to.be.equal('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(thrown.message).to.be.equal(originalMessage); + expect(thrown.stack).to.be.equal(originalStack); }); it('should handle expired links', async () => { From 99e8f00a11afce6cbcf17cf8e5089a355e483972 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 7 Nov 2026 00:15:36 +0000 Subject: [PATCH 14/15] ai: apply changes for #567 (1 review thread) Addresses: - #4498993031 at CHANGELOG.md:5 Signed-off-by: peco-engineer-bot[bot] --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22e15e66..6e70b188 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- CloudFetch retry-exhaustion errors now identify the download and the last failure: the `RetryError` message reads `CloudFetch result download failed: (last error: HTTP : )`, or `(last error: network error )` for network failures (the raw network-error message, which can contain the presigned URL, is not copied). The `RetryError` instance, `errorCode` and `payload` (including the node-fetch `Request` / `Response`) are unchanged. (#471) +- CloudFetch retry-exhaustion errors now identify the download and the last failure: the `RetryError` message reads `CloudFetch result download failed: (last error: )`. `` is `HTTP : ` (or `HTTP ` when the status text is empty) for HTTP failures, and `network error `, `network error ()` (e.g. `aborted`, when there is no `code`) or a bare `network error` for network failures (the raw network-error message, which can contain the presigned URL, is not copied). If no cause can be determined, the ` (last error: …)` suffix is omitted. The `RetryError` instance, `errorCode` and `payload` (including the node-fetch `Request` / `Response`) are unchanged. (#471) - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) ## 3.0.0 From c6c654cfb7d6360536ee09dfcdb41251aa82f565 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 7 Nov 2026 00:19:06 +0000 Subject: [PATCH 15/15] ai: apply changes for #567 (1 review thread) Addresses: - #4499012658 at lib/result/CloudFetchResultHandler.ts:222 Signed-off-by: peco-engineer-bot[bot] --- lib/result/CloudFetchResultHandler.ts | 9 ++++- .../result/CloudFetchResultHandler.test.ts | 33 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/result/CloudFetchResultHandler.ts b/lib/result/CloudFetchResultHandler.ts index e1c71e73..a64bb4bb 100644 --- a/lib/result/CloudFetchResultHandler.ts +++ b/lib/result/CloudFetchResultHandler.ts @@ -197,6 +197,13 @@ export default class CloudFetchResultHandler implements IResultsProvider { ); }); + it('should not prefix the same RetryError instance twice when it is re-thrown', async () => { + const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); + + const originalError = new RetryError( + RetryErrorCode.AttemptsExceeded, + Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' }), + ); + context.invokeWithRetryStub.callsFake(async () => { + throw originalError; + }); + + const expectedMessage = + 'CloudFetch result download failed: Max retry count exceeded (last error: network error ECONNRESET)'; + + for (let i = 0; i < 2; i += 1) { + const result = new CloudFetchResultHandler(context, new ResultsProviderStub([sampleRowSet1], undefined), { + status: { statusCode: TStatusCode.SUCCESS_STATUS }, + }); + + let thrown: any; + try { + // eslint-disable-next-line no-await-in-loop + await result.fetchNext({ limit: 10000 }); + } catch (e) { + thrown = e; + } + + expect(thrown).to.be.equal(originalError); + expect(thrown.message).to.be.equal(expectedMessage); + expect(thrown.stack.split('\n')[0]).to.be.equal(`Error: ${expectedMessage}`); + } + }); + it('should not leak presigned URLs from network errors into the RetryError message', async () => { const context = new ClientContextStub({ cloudFetchConcurrentDownloads: 1 }); const rowSetProvider = new ResultsProviderStub([sampleRowSet1], undefined);