From fe9d2bfce9955c34d5045b15641d9952fd1e69a1 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 05:07:12 +0000 Subject: [PATCH 01/10] [coverage] Conformance findings: LOGGING-002 (#494) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com> --- lib/DBSQLLogger.ts | 24 ++++++++- tests/e2e/logging.test.ts | 101 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/logging.test.ts diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 3d298b9b..19c0a372 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -1,6 +1,24 @@ +import fs from 'fs'; +import path from 'path'; import winston, { Logger } from 'winston'; import IDBSQLLogger, { LoggerOptions, LogLevel } from './contracts/IDBSQLLogger'; +const DEFAULT_LOG_FILE_NAME = 'databricks-sql-nodejs.log'; + +// If `filepath` names an existing directory, write logs to a file inside it +// instead of trying to open the directory itself as a file (which fails with an +// asynchronous EISDIR on the transport stream). Any other value is used as-is. +function resolveLogFilePath(filepath: string): string { + try { + if (fs.statSync(filepath).isDirectory()) { + return path.join(filepath, DEFAULT_LOG_FILE_NAME); + } + } catch { + // path does not exist (or is inaccessible) — treat it as a file path + } + return filepath; +} + export default class DBSQLLogger implements IDBSQLLogger { logger: Logger; @@ -21,7 +39,11 @@ export default class DBSQLLogger implements IDBSQLLogger { transports: [this.transports.console], }); if (filepath) { - this.transports.file = new winston.transports.File({ filename: filepath, handleExceptions: true, level }); + this.transports.file = new winston.transports.File({ + filename: resolveLogFilePath(filepath), + handleExceptions: true, + level, + }); this.logger.add(this.transports.file); } } diff --git a/tests/e2e/logging.test.ts b/tests/e2e/logging.test.ts new file mode 100644 index 00000000..95c35bca --- /dev/null +++ b/tests/e2e/logging.test.ts @@ -0,0 +1,101 @@ +import { expect } from 'chai'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { DBSQLClient } from '../../lib'; +import DBSQLLogger from '../../lib/DBSQLLogger'; +import { LogLevel } from '../../lib/contracts/IDBSQLLogger'; +import config from './utils/config'; + +const delay = (ms: number) => + new Promise((resolve) => { + setTimeout(resolve, ms); + }); + +// Runs a small Thrift workload with the given logger, collecting any errors emitted +// by the logger / its file transport or thrown as uncaught exceptions meanwhile. +async function runWorkload(logger: DBSQLLogger): Promise> { + const errors: Array = []; + const onError = (error: Error) => { + errors.push(error); + }; + process.on('uncaughtException', onError); + logger.logger.on('error', onError); + logger.transports.file?.on('error', onError); + + try { + const client = new DBSQLClient({ logger }); + await client.connect({ + host: config.host, + path: config.path, + token: config.token, + }); + const session = await client.openSession({ + initialCatalog: config.catalog, + initialSchema: config.schema, + }); + const operation = await session.executeStatement('SELECT 1'); + await operation.fetchAll(); + await operation.close(); + await session.close(); + await client.close(); + + // winston's file transport writes asynchronously + await delay(1_500); + } finally { + process.removeListener('uncaughtException', onError); + } + + return errors; +} + +function listRegularFiles(dir: string): Array { + return fs + .readdirSync(dir) + .map((name) => path.join(dir, name)) + .filter((p) => fs.statSync(p).isFile()); +} + +describe('DBSQLLogger file destination (LOGGING-002)', function loggingSuite() { + this.timeout(120_000); + + const tempDirs: Array = []; + + afterEach(() => { + while (tempDirs.length > 0) { + const dir = tempDirs.pop() as string; + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + function makeTempDir() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'dbsql-log-')); + tempDirs.push(dir); + return dir; + } + + it('directory log destination resolves to a log file inside it', async () => { + const dir = makeTempDir(); + const logger = new DBSQLLogger({ level: LogLevel.debug, filepath: dir }); + + const errors = await runWorkload(logger); + + expect(errors, `unexpected logger errors: ${errors.map((e) => e.message).join('; ')}`).to.be.empty; + const files = listRegularFiles(dir); + expect(files.length, 'expected a log file to be created inside the directory').to.be.greaterThan(0); + const contents = files.map((f) => fs.readFileSync(f, 'utf8')).join(''); + expect(contents.trim().length, 'expected driver log lines inside the directory').to.be.greaterThan(0); + }); + + it('file log destination writes logs to exactly that file', async () => { + const dir = makeTempDir(); + const logFile = path.join(dir, 'explicit.log'); + const logger = new DBSQLLogger({ level: LogLevel.debug, filepath: logFile }); + + const errors = await runWorkload(logger); + + expect(errors).to.be.empty; + expect(listRegularFiles(dir)).to.deep.equal([logFile]); + expect(fs.readFileSync(logFile, 'utf8').trim().length).to.be.greaterThan(0); + }); +}); From c5d60e373a26fb33e1ea6da97d2e7b96e7f03146 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:11:37 +0000 Subject: [PATCH 02/10] ai: apply changes for #565 (1 review thread) Addresses: - #4476824300 at lib/DBSQLLogger.ts:11 Signed-off-by: peco-engineer-bot[bot] --- tests/unit/DBSQLLogger.test.ts | 44 ++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index 05cb60b6..470f62e1 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -12,6 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import { expect } from 'chai'; import DBSQLLogger from '../../lib/DBSQLLogger'; import { LogLevel } from '../../lib/contracts/IDBSQLLogger'; @@ -60,3 +63,44 @@ describe('DBSQLLogger level subscription', () => { expect(logger.getLevel()).to.equal(LogLevel.debug); }); }); + +describe('DBSQLLogger file transport path resolution', () => { + let tmpDir: string; + let logger: DBSQLLogger | undefined; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'dbsql-logger-')); + }); + + afterEach(() => { + logger?.logger.close(); + logger = undefined; + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('writes to a default file inside the directory when filepath is an existing directory', () => { + logger = new DBSQLLogger({ filepath: tmpDir }); + + expect(logger.transports.file).to.not.be.undefined; + expect(logger.transports.file?.dirname).to.equal(tmpDir); + expect(logger.transports.file?.filename).to.equal('databricks-sql-nodejs.log'); + }); + + it('uses a non-existent filepath unchanged', () => { + const filepath = path.join(tmpDir, 'custom.log'); + logger = new DBSQLLogger({ filepath }); + + expect(logger.transports.file).to.not.be.undefined; + expect(logger.transports.file?.dirname).to.equal(tmpDir); + expect(logger.transports.file?.filename).to.equal('custom.log'); + }); + + it('uses an existing file path unchanged', () => { + const filepath = path.join(tmpDir, 'existing.log'); + fs.writeFileSync(filepath, ''); + logger = new DBSQLLogger({ filepath }); + + expect(logger.transports.file?.dirname).to.equal(tmpDir); + expect(logger.transports.file?.filename).to.equal('existing.log'); + }); +}); From 522968518a439d7b9a040d51c84b52e5e454a3ee Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:15:54 +0000 Subject: [PATCH 03/10] ai: apply changes for #565 (3 review threads) Addresses: - #4476845245 at tests/unit/DBSQLLogger.test.ts:76 - #4476845255 at lib/DBSQLLogger.ts:13 - #4476845259 at lib/DBSQLLogger.ts:7 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLLogger.ts | 12 ++++++++--- lib/contracts/IDBSQLLogger.ts | 6 ++++++ tests/unit/DBSQLLogger.test.ts | 39 ++++++++++++++++++++++++++++++---- 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 19c0a372..83c1721a 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -5,10 +5,16 @@ import IDBSQLLogger, { LoggerOptions, LogLevel } from './contracts/IDBSQLLogger' const DEFAULT_LOG_FILE_NAME = 'databricks-sql-nodejs.log'; -// If `filepath` names an existing directory, write logs to a file inside it -// instead of trying to open the directory itself as a file (which fails with an -// asynchronous EISDIR on the transport stream). Any other value is used as-is. +// If `filepath` names an existing directory, or ends with a path separator +// (meaning "directory", even if it doesn't exist yet), write logs to a default +// file inside it instead of trying to open the directory itself as a file +// (which fails with an asynchronous EISDIR on the transport stream). A missing +// directory is created by the winston File transport. Any other value is used +// as-is. function resolveLogFilePath(filepath: string): string { + if (filepath.endsWith('/') || filepath.endsWith(path.sep)) { + return path.join(filepath, DEFAULT_LOG_FILE_NAME); + } try { if (fs.statSync(filepath).isDirectory()) { return path.join(filepath, DEFAULT_LOG_FILE_NAME); diff --git a/lib/contracts/IDBSQLLogger.ts b/lib/contracts/IDBSQLLogger.ts index cbf73b6b..52bc2001 100644 --- a/lib/contracts/IDBSQLLogger.ts +++ b/lib/contracts/IDBSQLLogger.ts @@ -1,4 +1,10 @@ export interface LoggerOptions { + /** + * Optional: where to write logs, in addition to the console. If it names an + * existing directory, or ends with a path separator (a directory that doesn't + * exist yet will be created), logs go to `databricks-sql-nodejs.log` inside + * that directory. Any other value is used as the log file path. + */ filepath?: string; level?: LogLevel; } diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index 470f62e1..20fe35e9 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -67,28 +67,59 @@ describe('DBSQLLogger level subscription', () => { describe('DBSQLLogger file transport path resolution', () => { let tmpDir: string; let logger: DBSQLLogger | undefined; + let fileSettled: Promise | undefined; + + // The winston File transport opens its write stream asynchronously after the + // constructor returns. Subscribe to `open`/`error` right away so teardown can + // wait for it to settle before deleting the directory — otherwise a deferred + // open could fail with ENOENT and surface as an uncaught error later. + function createLogger(filepath: string): DBSQLLogger { + const instance = new DBSQLLogger({ filepath }); + const file = instance.transports.file; + fileSettled = file + ? new Promise((resolve) => { + file.once('open', () => resolve()); + file.once('error', () => resolve()); + }) + : undefined; + return instance; + } beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'dbsql-logger-')); }); - afterEach(() => { + afterEach(async () => { + if (fileSettled) { + await fileSettled; + } logger?.logger.close(); logger = undefined; + fileSettled = undefined; fs.rmSync(tmpDir, { recursive: true, force: true }); }); it('writes to a default file inside the directory when filepath is an existing directory', () => { - logger = new DBSQLLogger({ filepath: tmpDir }); + logger = createLogger(tmpDir); expect(logger.transports.file).to.not.be.undefined; expect(logger.transports.file?.dirname).to.equal(tmpDir); expect(logger.transports.file?.filename).to.equal('databricks-sql-nodejs.log'); }); + it('treats a non-existent filepath with a trailing separator as a directory', () => { + const dir = path.join(tmpDir, 'nested', 'logs'); + logger = createLogger(`${dir}${path.sep}`); + + expect(logger.transports.file).to.not.be.undefined; + expect(logger.transports.file?.dirname).to.equal(dir); + expect(logger.transports.file?.filename).to.equal('databricks-sql-nodejs.log'); + expect(fs.statSync(dir).isDirectory()).to.be.true; + }); + it('uses a non-existent filepath unchanged', () => { const filepath = path.join(tmpDir, 'custom.log'); - logger = new DBSQLLogger({ filepath }); + logger = createLogger(filepath); expect(logger.transports.file).to.not.be.undefined; expect(logger.transports.file?.dirname).to.equal(tmpDir); @@ -98,7 +129,7 @@ describe('DBSQLLogger file transport path resolution', () => { it('uses an existing file path unchanged', () => { const filepath = path.join(tmpDir, 'existing.log'); fs.writeFileSync(filepath, ''); - logger = new DBSQLLogger({ filepath }); + logger = createLogger(filepath); expect(logger.transports.file?.dirname).to.equal(tmpDir); expect(logger.transports.file?.filename).to.equal('existing.log'); From 016d6ab99b99d11cefdbe88d7df101cb8b1a5858 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:18:19 +0000 Subject: [PATCH 04/10] ai: apply changes for #565 (1 review thread) Addresses: - #4476880210 at tests/e2e/logging.test.ts:23 Signed-off-by: peco-engineer-bot[bot] --- tests/e2e/logging.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/e2e/logging.test.ts b/tests/e2e/logging.test.ts index 95c35bca..6e3fc043 100644 --- a/tests/e2e/logging.test.ts +++ b/tests/e2e/logging.test.ts @@ -43,6 +43,13 @@ async function runWorkload(logger: DBSQLLogger): Promise> { // winston's file transport writes asynchronously await delay(1_500); } finally { + // Close the logger (and its file transport's write stream) before the temp + // directory is removed in `afterEach`; give the stream a moment to finish + // while the error listeners are still attached, then detach them. + logger.logger.close(); + await delay(200); + logger.transports.file?.removeListener('error', onError); + logger.logger.removeListener('error', onError); process.removeListener('uncaughtException', onError); } From f9ddaaacd18d758c5f148617dd7c17e9b5af5839 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 07:02:28 +0000 Subject: [PATCH 05/10] Fall back to console when the log file can't be opened Drop the trailing-separator rule. On an existing file (which is what `filepath: 'logs/'` created in 3.0.0) it resolved to a path under a regular file and crashed the process with ENOTDIR. A trailing separator now behaves as in 3.0.0 unless the path is an existing directory. winston's File transport swallows stream open errors and then stalls every transport, so check the file is writable before adding it, and otherwise warn and log to the console only. Replace the live-warehouse e2e spec with unit tests that read the log file back and cover both cases. Co-authored-by: Isaac Signed-off-by: Vu Anh Phung --- lib/DBSQLLogger.ts | 45 ++++++++------ lib/contracts/IDBSQLLogger.ts | 7 +-- tests/e2e/logging.test.ts | 108 --------------------------------- tests/unit/DBSQLLogger.test.ts | 71 +++++++++++++++++++--- 4 files changed, 92 insertions(+), 139 deletions(-) delete mode 100644 tests/e2e/logging.test.ts diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 83c1721a..7e795f03 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -5,24 +5,30 @@ import IDBSQLLogger, { LoggerOptions, LogLevel } from './contracts/IDBSQLLogger' const DEFAULT_LOG_FILE_NAME = 'databricks-sql-nodejs.log'; -// If `filepath` names an existing directory, or ends with a path separator -// (meaning "directory", even if it doesn't exist yet), write logs to a default -// file inside it instead of trying to open the directory itself as a file -// (which fails with an asynchronous EISDIR on the transport stream). A missing -// directory is created by the winston File transport. Any other value is used -// as-is. +// An existing directory resolves to a default file inside it. Anything else is +// returned as the path winston will open (which drops a trailing separator). function resolveLogFilePath(filepath: string): string { - if (filepath.endsWith('/') || filepath.endsWith(path.sep)) { - return path.join(filepath, DEFAULT_LOG_FILE_NAME); - } try { if (fs.statSync(filepath).isDirectory()) { return path.join(filepath, DEFAULT_LOG_FILE_NAME); } } catch { - // path does not exist (or is inaccessible) — treat it as a file path + // doesn't exist or isn't accessible: treat it as a file path + } + return path.join(path.dirname(filepath), path.basename(filepath)); +} + +// winston's File transport swallows errors opening its stream (EISDIR, EACCES, +// ENOTDIR, ...), after which the logger stalls for all transports once its +// buffer fills. Check up front that the file can be opened for appending. +function checkLogFileWritable(filename: string): Error | undefined { + try { + fs.mkdirSync(path.dirname(filename), { recursive: true }); + fs.closeSync(fs.openSync(filename, 'a')); + return undefined; + } catch (error) { + return error instanceof Error ? error : new Error(String(error)); } - return filepath; } export default class DBSQLLogger implements IDBSQLLogger { @@ -45,12 +51,17 @@ export default class DBSQLLogger implements IDBSQLLogger { transports: [this.transports.console], }); if (filepath) { - this.transports.file = new winston.transports.File({ - filename: resolveLogFilePath(filepath), - handleExceptions: true, - level, - }); - this.logger.add(this.transports.file); + const filename = resolveLogFilePath(filepath); + const error = checkLogFileWritable(filename); + if (error) { + this.logger.log({ + level: LogLevel.warn, + message: `Cannot write to log file ${filename}, logging to console only: ${error.message}`, + }); + } else { + this.transports.file = new winston.transports.File({ filename, handleExceptions: true, level }); + this.logger.add(this.transports.file); + } } } diff --git a/lib/contracts/IDBSQLLogger.ts b/lib/contracts/IDBSQLLogger.ts index 52bc2001..7348dbb4 100644 --- a/lib/contracts/IDBSQLLogger.ts +++ b/lib/contracts/IDBSQLLogger.ts @@ -1,9 +1,8 @@ export interface LoggerOptions { /** - * Optional: where to write logs, in addition to the console. If it names an - * existing directory, or ends with a path separator (a directory that doesn't - * exist yet will be created), logs go to `databricks-sql-nodejs.log` inside - * that directory. Any other value is used as the log file path. + * Optional: a log file to write to, in addition to the console. If it names an + * existing directory, logs go to `databricks-sql-nodejs.log` inside it. If the + * file can't be opened for writing, the logger warns and logs to the console only. */ filepath?: string; level?: LogLevel; diff --git a/tests/e2e/logging.test.ts b/tests/e2e/logging.test.ts deleted file mode 100644 index 6e3fc043..00000000 --- a/tests/e2e/logging.test.ts +++ /dev/null @@ -1,108 +0,0 @@ -import { expect } from 'chai'; -import fs from 'fs'; -import os from 'os'; -import path from 'path'; -import { DBSQLClient } from '../../lib'; -import DBSQLLogger from '../../lib/DBSQLLogger'; -import { LogLevel } from '../../lib/contracts/IDBSQLLogger'; -import config from './utils/config'; - -const delay = (ms: number) => - new Promise((resolve) => { - setTimeout(resolve, ms); - }); - -// Runs a small Thrift workload with the given logger, collecting any errors emitted -// by the logger / its file transport or thrown as uncaught exceptions meanwhile. -async function runWorkload(logger: DBSQLLogger): Promise> { - const errors: Array = []; - const onError = (error: Error) => { - errors.push(error); - }; - process.on('uncaughtException', onError); - logger.logger.on('error', onError); - logger.transports.file?.on('error', onError); - - try { - const client = new DBSQLClient({ logger }); - await client.connect({ - host: config.host, - path: config.path, - token: config.token, - }); - const session = await client.openSession({ - initialCatalog: config.catalog, - initialSchema: config.schema, - }); - const operation = await session.executeStatement('SELECT 1'); - await operation.fetchAll(); - await operation.close(); - await session.close(); - await client.close(); - - // winston's file transport writes asynchronously - await delay(1_500); - } finally { - // Close the logger (and its file transport's write stream) before the temp - // directory is removed in `afterEach`; give the stream a moment to finish - // while the error listeners are still attached, then detach them. - logger.logger.close(); - await delay(200); - logger.transports.file?.removeListener('error', onError); - logger.logger.removeListener('error', onError); - process.removeListener('uncaughtException', onError); - } - - return errors; -} - -function listRegularFiles(dir: string): Array { - return fs - .readdirSync(dir) - .map((name) => path.join(dir, name)) - .filter((p) => fs.statSync(p).isFile()); -} - -describe('DBSQLLogger file destination (LOGGING-002)', function loggingSuite() { - this.timeout(120_000); - - const tempDirs: Array = []; - - afterEach(() => { - while (tempDirs.length > 0) { - const dir = tempDirs.pop() as string; - fs.rmSync(dir, { recursive: true, force: true }); - } - }); - - function makeTempDir() { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'dbsql-log-')); - tempDirs.push(dir); - return dir; - } - - it('directory log destination resolves to a log file inside it', async () => { - const dir = makeTempDir(); - const logger = new DBSQLLogger({ level: LogLevel.debug, filepath: dir }); - - const errors = await runWorkload(logger); - - expect(errors, `unexpected logger errors: ${errors.map((e) => e.message).join('; ')}`).to.be.empty; - const files = listRegularFiles(dir); - expect(files.length, 'expected a log file to be created inside the directory').to.be.greaterThan(0); - const contents = files.map((f) => fs.readFileSync(f, 'utf8')).join(''); - expect(contents.trim().length, 'expected driver log lines inside the directory').to.be.greaterThan(0); - }); - - it('file log destination writes logs to exactly that file', async () => { - const dir = makeTempDir(); - const logFile = path.join(dir, 'explicit.log'); - const logger = new DBSQLLogger({ level: LogLevel.debug, filepath: logFile }); - - const errors = await runWorkload(logger); - - expect(errors).to.be.empty; - expect(listRegularFiles(dir)).to.deep.equal([logFile]); - expect(fs.readFileSync(logFile, 'utf8').trim().length).to.be.greaterThan(0); - }); -}); diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index 20fe35e9..618c8e65 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -16,9 +16,23 @@ import fs from 'fs'; import os from 'os'; import path from 'path'; import { expect } from 'chai'; +import sinon from 'sinon'; +import winston from 'winston'; import DBSQLLogger from '../../lib/DBSQLLogger'; import { LogLevel } from '../../lib/contracts/IDBSQLLogger'; +async function waitForFileContent(file: string, text: string, timeoutMs = 2000): Promise { + const deadline = Date.now() + timeoutMs; + const read = () => (fs.existsSync(file) ? fs.readFileSync(file, 'utf8') : ''); + while (!read().includes(text) && Date.now() < deadline) { + // eslint-disable-next-line no-await-in-loop + await new Promise((resolve) => { + setTimeout(resolve, 20); + }); + } + return read(); +} + describe('DBSQLLogger level subscription', () => { it('getLevel reflects the configured level', () => { const logger = new DBSQLLogger({ level: LogLevel.warn }); @@ -69,10 +83,8 @@ describe('DBSQLLogger file transport path resolution', () => { let logger: DBSQLLogger | undefined; let fileSettled: Promise | undefined; - // The winston File transport opens its write stream asynchronously after the - // constructor returns. Subscribe to `open`/`error` right away so teardown can - // wait for it to settle before deleting the directory — otherwise a deferred - // open could fail with ENOENT and surface as an uncaught error later. + // The File transport opens its stream asynchronously; teardown waits for that + // before closing the logger and deleting the directory. function createLogger(filepath: string): DBSQLLogger { const instance = new DBSQLLogger({ filepath }); const file = instance.transports.file; @@ -107,14 +119,53 @@ describe('DBSQLLogger file transport path resolution', () => { expect(logger.transports.file?.filename).to.equal('databricks-sql-nodejs.log'); }); - it('treats a non-existent filepath with a trailing separator as a directory', () => { - const dir = path.join(tmpDir, 'nested', 'logs'); - logger = createLogger(`${dir}${path.sep}`); + it('writes log lines to the default file inside the directory', async () => { + logger = createLogger(tmpDir); + logger.transports.console.silent = true; - expect(logger.transports.file).to.not.be.undefined; - expect(logger.transports.file?.dirname).to.equal(dir); + await logger.log(LogLevel.info, 'hello from DBSQLLogger test'); + + const content = await waitForFileContent(path.join(tmpDir, 'databricks-sql-nodejs.log'), 'hello from'); + expect(content).to.contain('hello from DBSQLLogger test'); + }); + + it('resolves an existing directory with a trailing separator to the default file inside it', () => { + logger = createLogger(`${tmpDir}${path.sep}`); + + expect(logger.transports.file?.dirname).to.equal(tmpDir); expect(logger.transports.file?.filename).to.equal('databricks-sql-nodejs.log'); - expect(fs.statSync(dir).isDirectory()).to.be.true; + }); + + it('keeps writing to an existing file when filepath has a trailing separator', () => { + const filepath = path.join(tmpDir, 'logs'); + fs.writeFileSync(filepath, ''); + logger = createLogger(`${filepath}${path.sep}`); + + expect(logger.transports.file?.dirname).to.equal(tmpDir); + expect(logger.transports.file?.filename).to.equal('logs'); + }); + + it('falls back to console only, with a warning, when the log file cannot be opened', async () => { + const notADir = path.join(tmpDir, 'not-a-dir'); + fs.writeFileSync(notADir, ''); + const logged: Array<{ level: string; message: string }> = []; + const consoleLog = sinon.stub(winston.transports.Console.prototype, 'log').callsFake((...args: unknown[]) => { + logged.push(args[0] as { level: string; message: string }); + (args[1] as () => void)(); + }); + try { + logger = createLogger(path.join(notADir, 'driver.log')); + await new Promise((resolve) => { + setImmediate(resolve); + }); + + expect(logger.transports.file).to.be.undefined; + expect(logged).to.have.length(1); + expect(logged[0].level).to.equal(LogLevel.warn); + expect(logged[0].message).to.contain('logging to console only'); + } finally { + consoleLog.restore(); + } }); it('uses a non-existent filepath unchanged', () => { From 324d725c029aa2cdc279deaef5aacd134f9e4144 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:07:03 +0000 Subject: [PATCH 06/10] ai: apply changes for #565 (2 review threads) Addresses: - #4477535158 at lib/DBSQLLogger.ts:57 - #4477535166 at lib/DBSQLLogger.ts:26 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLLogger.ts | 7 ++++- lib/contracts/IDBSQLLogger.ts | 3 ++- tests/unit/DBSQLLogger.test.ts | 48 ++++++++++++++++++---------------- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 7e795f03..a4877cb4 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -21,6 +21,9 @@ function resolveLogFilePath(filepath: string): string { // winston's File transport swallows errors opening its stream (EISDIR, EACCES, // ENOTDIR, ...), after which the logger stalls for all transports once its // buffer fills. Check up front that the file can be opened for appending. +// Note: this probe is intentionally not side-effect free. It creates missing +// parent directories and an empty log file (as winston itself would when the +// transport opens), so the check exercises the exact same open path. function checkLogFileWritable(filename: string): Error | undefined { try { fs.mkdirSync(path.dirname(filename), { recursive: true }); @@ -54,8 +57,10 @@ export default class DBSQLLogger implements IDBSQLLogger { const filename = resolveLogFilePath(filepath); const error = checkLogFileWritable(filename); if (error) { + // Logged at `error` so the console transport's level filter can never + // drop it (e.g. when the logger is configured with `level: error`). this.logger.log({ - level: LogLevel.warn, + level: LogLevel.error, message: `Cannot write to log file ${filename}, logging to console only: ${error.message}`, }); } else { diff --git a/lib/contracts/IDBSQLLogger.ts b/lib/contracts/IDBSQLLogger.ts index 7348dbb4..80f61469 100644 --- a/lib/contracts/IDBSQLLogger.ts +++ b/lib/contracts/IDBSQLLogger.ts @@ -2,7 +2,8 @@ export interface LoggerOptions { /** * Optional: a log file to write to, in addition to the console. If it names an * existing directory, logs go to `databricks-sql-nodejs.log` inside it. If the - * file can't be opened for writing, the logger warns and logs to the console only. + * file can't be opened for writing, the logger reports it on the console (at + * `error` level, so it is shown regardless of `level`) and logs to the console only. */ filepath?: string; level?: LogLevel; diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index 618c8e65..df2bd1c5 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -85,8 +85,8 @@ describe('DBSQLLogger file transport path resolution', () => { // The File transport opens its stream asynchronously; teardown waits for that // before closing the logger and deleting the directory. - function createLogger(filepath: string): DBSQLLogger { - const instance = new DBSQLLogger({ filepath }); + function createLogger(filepath: string, level?: LogLevel): DBSQLLogger { + const instance = new DBSQLLogger({ filepath, level }); const file = instance.transports.file; fileSettled = file ? new Promise((resolve) => { @@ -145,28 +145,30 @@ describe('DBSQLLogger file transport path resolution', () => { expect(logger.transports.file?.filename).to.equal('logs'); }); - it('falls back to console only, with a warning, when the log file cannot be opened', async () => { - const notADir = path.join(tmpDir, 'not-a-dir'); - fs.writeFileSync(notADir, ''); - const logged: Array<{ level: string; message: string }> = []; - const consoleLog = sinon.stub(winston.transports.Console.prototype, 'log').callsFake((...args: unknown[]) => { - logged.push(args[0] as { level: string; message: string }); - (args[1] as () => void)(); - }); - try { - logger = createLogger(path.join(notADir, 'driver.log')); - await new Promise((resolve) => { - setImmediate(resolve); + for (const level of [LogLevel.info, LogLevel.error]) { + it(`falls back to console only, with a notice, when the log file cannot be opened (level: ${level})`, async () => { + const notADir = path.join(tmpDir, 'not-a-dir'); + fs.writeFileSync(notADir, ''); + const logged: Array<{ level: string; message: string }> = []; + const consoleLog = sinon.stub(winston.transports.Console.prototype, 'log').callsFake((...args: unknown[]) => { + logged.push(args[0] as { level: string; message: string }); + (args[1] as () => void)(); }); - - expect(logger.transports.file).to.be.undefined; - expect(logged).to.have.length(1); - expect(logged[0].level).to.equal(LogLevel.warn); - expect(logged[0].message).to.contain('logging to console only'); - } finally { - consoleLog.restore(); - } - }); + try { + logger = createLogger(path.join(notADir, 'driver.log'), level); + await new Promise((resolve) => { + setImmediate(resolve); + }); + + expect(logger.transports.file).to.be.undefined; + expect(logged).to.have.length(1); + expect(logged[0].level).to.equal(LogLevel.error); + expect(logged[0].message).to.contain('logging to console only'); + } finally { + consoleLog.restore(); + } + }); + } it('uses a non-existent filepath unchanged', () => { const filepath = path.join(tmpDir, 'custom.log'); From 1c1d1ea88aa6497d43cc320b974789c7db79b5f2 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:10:35 +0000 Subject: [PATCH 07/10] ai: apply changes for #565 (2 review threads) Addresses: - #4477563176 at lib/DBSQLLogger.ts:61 - #4477563184 at lib/DBSQLLogger.ts:29 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLLogger.ts | 4 +++- tests/unit/DBSQLLogger.test.ts | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index a4877cb4..1851c49c 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -59,9 +59,11 @@ export default class DBSQLLogger implements IDBSQLLogger { if (error) { // Logged at `error` so the console transport's level filter can never // drop it (e.g. when the logger is configured with `level: error`). + // The message is explicitly marked non-fatal so log-based alerting / + // readers don't mistake this config fallback for a driver failure. this.logger.log({ level: LogLevel.error, - message: `Cannot write to log file ${filename}, logging to console only: ${error.message}`, + message: `[warn] Cannot write to log file ${filename}, logging to console only (non-fatal, driver continues normally): ${error.message}`, }); } else { this.transports.file = new winston.transports.File({ filename, handleExceptions: true, level }); diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index df2bd1c5..a1765fcd 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -179,6 +179,18 @@ describe('DBSQLLogger file transport path resolution', () => { expect(logger.transports.file?.filename).to.equal('custom.log'); }); + it('creates missing parent directories and still attaches the file transport', () => { + const nestedDir = path.join(tmpDir, 'a', 'b'); + const filepath = path.join(nestedDir, 'driver.log'); + logger = createLogger(filepath); + + expect(logger.transports.file).to.not.be.undefined; + expect(logger.transports.file?.dirname).to.equal(nestedDir); + expect(logger.transports.file?.filename).to.equal('driver.log'); + expect(fs.statSync(nestedDir).isDirectory()).to.be.true; + expect(fs.existsSync(filepath)).to.be.true; + }); + it('uses an existing file path unchanged', () => { const filepath = path.join(tmpDir, 'existing.log'); fs.writeFileSync(filepath, ''); From c1e837b5e478a014fc8a322c3de9af7f62f8aca3 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:13:04 +0000 Subject: [PATCH 08/10] ai: apply changes for #565 (1 review thread) Addresses: - #4477590259 at lib/DBSQLLogger.ts:64 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLLogger.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 1851c49c..17080396 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -59,8 +59,10 @@ export default class DBSQLLogger implements IDBSQLLogger { if (error) { // Logged at `error` so the console transport's level filter can never // drop it (e.g. when the logger is configured with `level: error`). - // The message is explicitly marked non-fatal so log-based alerting / - // readers don't mistake this config fallback for a driver failure. + // Trade-off: consumers filtering on the winston `level` field (e.g. + // error-level alerting) WILL see this as an error; the "[warn] ... + // non-fatal" wording only helps human readers tell this config + // fallback apart from a driver failure. this.logger.log({ level: LogLevel.error, message: `[warn] Cannot write to log file ${filename}, logging to console only (non-fatal, driver continues normally): ${error.message}`, From f157c89d44bc8c9e063847dc339126e2f83e87ba Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:15:50 +0000 Subject: [PATCH 09/10] ai: apply changes for #565 (1 review thread) Addresses: - #4477606208 at lib/DBSQLLogger.ts:29 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLLogger.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 17080396..d2d05749 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -24,6 +24,11 @@ function resolveLogFilePath(filepath: string): string { // Note: this probe is intentionally not side-effect free. It creates missing // parent directories and an empty log file (as winston itself would when the // transport opens), so the check exercises the exact same open path. +// Limitations: this is a one-time, synchronous check (statSync/mkdirSync/ +// openSync) run each time a DBSQLLogger is constructed with a `filepath`. +// Failures that start later (file/dir deleted, permissions changed, disk +// full, a Windows file lock) are not detected here and can still stall the +// logger. function checkLogFileWritable(filename: string): Error | undefined { try { fs.mkdirSync(path.dirname(filename), { recursive: true }); From 2700d952f0c30782d9793e4d983b3796282a4209 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:18:46 +0000 Subject: [PATCH 10/10] ai: apply changes for #565 (2 review threads) Addresses: - #4477606208 at lib/DBSQLLogger.ts:34 - #4477625054 at tests/unit/DBSQLLogger.test.ts:157 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLLogger.ts | 3 ++- tests/unit/DBSQLLogger.test.ts | 12 +++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index d2d05749..9b369773 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -28,7 +28,8 @@ function resolveLogFilePath(filepath: string): string { // openSync) run each time a DBSQLLogger is constructed with a `filepath`. // Failures that start later (file/dir deleted, permissions changed, disk // full, a Windows file lock) are not detected here and can still stall the -// logger. +// logger. Follow-up (separate change): attach a runtime `error` handler to the +// File transport that removes it and falls back to console-only logging. function checkLogFileWritable(filename: string): Error | undefined { try { fs.mkdirSync(path.dirname(filename), { recursive: true }); diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index a1765fcd..788e606c 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -156,9 +156,15 @@ describe('DBSQLLogger file transport path resolution', () => { }); try { logger = createLogger(path.join(notADir, 'driver.log'), level); - await new Promise((resolve) => { - setImmediate(resolve); - }); + // Delivery to the Console transport goes through winston's stream piping, + // so poll rather than assuming it completes within a single tick. + const deadline = Date.now() + 2000; + while (logged.length === 0 && Date.now() < deadline) { + // eslint-disable-next-line no-await-in-loop + await new Promise((resolve) => { + setTimeout(resolve, 20); + }); + } expect(logger.transports.file).to.be.undefined; expect(logged).to.have.length(1);