From b06527c4419e45155ef7a168716e31a658f30b96 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/13] [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 4e0cb192d6c5d62a3e8533f87e628b02783b578d Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:11:37 +0000 Subject: [PATCH 02/13] 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 abd2c840258cfe3f58b55114f71762726bd556cb Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:15:54 +0000 Subject: [PATCH 03/13] 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 8d8c685b0d1fa06b6907ef07a8142dded3073907 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:18:19 +0000 Subject: [PATCH 04/13] 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 eb3dab1bbcb01056885b948e739590c65f3852b8 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 07:02:28 +0000 Subject: [PATCH 05/13] 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 161d95fe55b6a73c67faffebcc15009ae5d875db Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:07:03 +0000 Subject: [PATCH 06/13] 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 bb9dd1a6528fbea37ff4a7b27702e0b83459adfa Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:10:35 +0000 Subject: [PATCH 07/13] 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 cf0127d889db1e7bfe82a51e30be0e1df76ff1ed Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:13:04 +0000 Subject: [PATCH 08/13] 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 890ef049066c4e220a9dc04e8cb36a038fe76c59 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:15:50 +0000 Subject: [PATCH 09/13] 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 75be644ec460304da67f7941c5e71d7821bce491 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 07:18:46 +0000 Subject: [PATCH 10/13] 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); From 928f15096d3181c6b88982c9bb249a1147d06c18 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 16:28:37 +0000 Subject: [PATCH 11/13] Report log file fallback via process warning; don't probe FIFOs - Emit the console-only fallback with process.emitWarning (code DBSQL_LOG_FILE_UNAVAILABLE) instead of an error-level log line, so it doesn't trip error alerting and is visible at any logger level. - Only probe regular files and directories (and missing paths). openSync on a FIFO blocked the constructor until a reader appeared. - Correct the probe comment: winston emits stat errors (ENOTDIR) and only swallows stream open errors, so a runtime error handler on the File transport wouldn't help. Drop that follow-up note. - Test the EISDIR open failure, console delivery after the fallback (the stall), the non-existent trailing-separator path, and the FIFO. - Add an Unreleased CHANGELOG section. Co-authored-by: Isaac Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 2 + lib/DBSQLLogger.ts | 36 +++++------ lib/contracts/IDBSQLLogger.ts | 5 +- tests/unit/DBSQLLogger.test.ts | 106 +++++++++++++++++++++++++++------ 4 files changed, 109 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04174c6e..be2bf390 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) - Reject non-scalar query parameter values at bind time with a `ParameterError` on both the Thrift and kernel backends, instead of stringifying them (which could leave a Thrift operation stuck before FINISHED). Newly rejected on the Thrift backend: arrays, plain objects, boxed primitives (e.g. `new String('x')`, `new Number(1)`), functions, symbols, and a `DBSQLParameter` that wraps another `DBSQLParameter`. Primitives, `Date`, `Int64`, and `DBSQLParameter` are still accepted. +- `DBSQLLogger`: a `filepath` naming an existing directory now writes to `databricks-sql-nodejs.log` inside it, instead of silently producing no logs. (#565) +- `DBSQLLogger`: if the log file can't be opened (including when its directory can't be created, which used to throw from the constructor), the logger now logs to the console only, leaves `transports.file` undefined, and emits a process warning with code `DBSQL_LOG_FILE_UNAVAILABLE`. Previously an unwritable file could silently stall all logging, including the console. (#565) ## 3.0.0 diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 9b369773..38e505ea 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -5,6 +5,8 @@ import IDBSQLLogger, { LoggerOptions, LogLevel } from './contracts/IDBSQLLogger' const DEFAULT_LOG_FILE_NAME = 'databricks-sql-nodejs.log'; +export const LOG_FILE_UNAVAILABLE_WARNING_CODE = 'DBSQL_LOG_FILE_UNAVAILABLE'; + // 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 { @@ -18,20 +20,19 @@ function resolveLogFilePath(filepath: string): string { 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. -// 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. Follow-up (separate change): attach a runtime `error` handler to the -// File transport that removes it and falls back to console-only logging. +// winston's File transport only `debug`-logs errors opening its write stream +// (EISDIR, EACCES, ...), and the logger then stalls for every transport once +// its buffer fills. Its `stat` errors (e.g. ENOTDIR) are emitted instead and, +// with no listener, crash the process. So probe the file once up front, +// creating it and its directory as winston would. Later failures (e.g. disk +// full) aren't covered. Paths that exist but aren't regular files or +// directories are left to winston: opening a FIFO blocks until it has a reader. function checkLogFileWritable(filename: string): Error | undefined { try { + const stat = fs.statSync(filename, { throwIfNoEntry: false }); + if (stat && !stat.isFile() && !stat.isDirectory()) { + return undefined; + } fs.mkdirSync(path.dirname(filename), { recursive: true }); fs.closeSync(fs.openSync(filename, 'a')); return undefined; @@ -63,15 +64,8 @@ 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`). - // 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}`, + process.emitWarning(`Cannot write to log file ${filename}, logging to console only: ${error.message}`, { + code: LOG_FILE_UNAVAILABLE_WARNING_CODE, }); } else { this.transports.file = new winston.transports.File({ filename, handleExceptions: true, level }); diff --git a/lib/contracts/IDBSQLLogger.ts b/lib/contracts/IDBSQLLogger.ts index 80f61469..cc3291c2 100644 --- a/lib/contracts/IDBSQLLogger.ts +++ b/lib/contracts/IDBSQLLogger.ts @@ -2,8 +2,9 @@ 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 reports it on the console (at - * `error` level, so it is shown regardless of `level`) and logs to the console only. + * file can't be opened for writing, the logger logs to the console only + * (`DBSQLLogger.transports.file` stays undefined) and emits a process warning + * with code `DBSQL_LOG_FILE_UNAVAILABLE`. */ filepath?: string; level?: LogLevel; diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index 788e606c..4c52c0fe 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -12,13 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. +import { execFileSync } from 'child_process'; 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 DBSQLLogger, { LOG_FILE_UNAVAILABLE_WARNING_CODE } from '../../lib/DBSQLLogger'; import { LogLevel } from '../../lib/contracts/IDBSQLLogger'; async function waitForFileContent(file: string, text: string, timeoutMs = 2000): Promise { @@ -145,36 +146,107 @@ describe('DBSQLLogger file transport path resolution', () => { expect(logger.transports.file?.filename).to.equal('logs'); }); - 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 }> = []; + it('treats a non-existent filepath with a trailing separator as a file, as in 3.0.0', () => { + const filepath = path.join(tmpDir, 'logs'); + logger = createLogger(`${filepath}${path.sep}`); + + expect(logger.transports.file?.dirname).to.equal(tmpDir); + expect(logger.transports.file?.filename).to.equal('logs'); + expect(fs.statSync(filepath).isFile()).to.be.true; + }); + + it('does not open a FIFO synchronously', async function fifoTest() { + if (process.platform === 'win32') { + this.skip(); + } + const fifo = path.join(tmpDir, 'pipe.log'); + execFileSync('mkfifo', [fifo]); + // A non-blocking reader lets any writer open the FIFO without waiting. Keep + // it open until winston's own open completes, or that open (and the test + // process) would hang. + const readerFd = fs.openSync(fifo, fs.constants.O_RDONLY | fs.constants.O_NONBLOCK); + const openSync = sinon.spy(fs, 'openSync'); + try { + logger = createLogger(fifo); + } finally { + openSync.restore(); + } + await fileSettled; + fs.closeSync(readerFd); + + expect(openSync.called).to.be.false; + expect(logger.transports.file?.filename).to.equal('pipe.log'); + }); + + describe('when the log file cannot be opened', () => { + let emitWarning: sinon.SinonStub; + + beforeEach(() => { + emitWarning = sinon.stub(process, 'emitWarning'); + }); + + afterEach(() => { + emitWarning.restore(); + }); + + const cases = [ + { + name: 'a parent path is a regular file', + filepath: (dir: string) => { + fs.writeFileSync(path.join(dir, 'not-a-dir'), ''); + return path.join(dir, 'not-a-dir', 'driver.log'); + }, + }, + { + name: 'the resolved log file is a directory (EISDIR)', + filepath: (dir: string) => { + fs.mkdirSync(path.join(dir, 'databricks-sql-nodejs.log')); + return dir; + }, + }, + ]; + + for (const { name, filepath } of cases) { + it(`falls back to console only with a process warning when ${name}`, () => { + logger = createLogger(filepath(tmpDir), LogLevel.error); + + expect(logger.transports.file).to.be.undefined; + expect(emitWarning.calledOnce).to.be.true; + expect(emitWarning.firstCall.args[0]).to.contain('logging to console only'); + expect(emitWarning.firstCall.args[1]).to.deep.equal({ code: LOG_FILE_UNAVAILABLE_WARNING_CODE }); + }); + } + + it('keeps delivering console logs after the fallback', async () => { + fs.mkdirSync(path.join(tmpDir, 'databricks-sql-nodejs.log')); + const seen: string[] = []; const consoleLog = sinon.stub(winston.transports.Console.prototype, 'log').callsFake((...args: unknown[]) => { - logged.push(args[0] as { level: string; message: string }); + seen.push((args[0] as { message: string }).message); (args[1] as () => void)(); }); try { - logger = createLogger(path.join(notADir, 'driver.log'), level); - // 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) { + logger = createLogger(tmpDir); + // Far more than the logger's stream buffers hold; an unopened file + // transport used to stall delivery to every transport at that point. + const lines = 1000; + for (let i = 0; i < lines; i += 1) { + logger.logger.info(`line ${i} ${'x'.repeat(100)}`); + } + const deadline = Date.now() + 1500; + while (seen.length < lines && 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); - expect(logged[0].level).to.equal(LogLevel.error); - expect(logged[0].message).to.contain('logging to console only'); + expect(seen).to.have.length(lines); + expect(seen[lines - 1]).to.match(new RegExp(`^line ${lines - 1} `)); } finally { consoleLog.restore(); } }); - } + }); it('uses a non-existent filepath unchanged', () => { const filepath = path.join(tmpDir, 'custom.log'); From deaca2032b0830f5225ba2e73389bbf178994d5f Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 20:58:40 +0000 Subject: [PATCH 12/13] Only mkdir a missing log directory, like winston On Windows, mkdirSync of a drive root throws EPERM even with `recursive`, so `filepath: 'D:\\dbsql.log'` (which worked in 3.0.0) fell back to console only. Guard with existsSync as winston's File transport does; real permission problems still fail at openSync. Co-authored-by: Isaac Signed-off-by: Vu Anh Phung --- lib/DBSQLLogger.ts | 7 ++++++- tests/unit/DBSQLLogger.test.ts | 13 +++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/DBSQLLogger.ts b/lib/DBSQLLogger.ts index 38e505ea..02dff91a 100644 --- a/lib/DBSQLLogger.ts +++ b/lib/DBSQLLogger.ts @@ -33,7 +33,12 @@ function checkLogFileWritable(filename: string): Error | undefined { if (stat && !stat.isFile() && !stat.isDirectory()) { return undefined; } - fs.mkdirSync(path.dirname(filename), { recursive: true }); + const dir = path.dirname(filename); + // Like winston, only create a missing directory: on Windows, mkdir of a + // drive root throws EPERM even with `recursive`. + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true }); + } fs.closeSync(fs.openSync(filename, 'a')); return undefined; } catch (error) { diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index 4c52c0fe..d6f39a64 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -269,6 +269,19 @@ describe('DBSQLLogger file transport path resolution', () => { expect(fs.existsSync(filepath)).to.be.true; }); + it('does not mkdir an existing directory (mkdir of a Windows drive root throws EPERM)', () => { + const mkdirSync = sinon.stub(fs, 'mkdirSync').throws(Object.assign(new Error('EPERM'), { code: 'EPERM' })); + try { + logger = createLogger(path.join(tmpDir, 'driver.log')); + } finally { + mkdirSync.restore(); + } + + expect(mkdirSync.called).to.be.false; + expect(logger.transports.file?.dirname).to.equal(tmpDir); + expect(logger.transports.file?.filename).to.equal('driver.log'); + }); + it('uses an existing file path unchanged', () => { const filepath = path.join(tmpDir, 'existing.log'); fs.writeFileSync(filepath, ''); From 65c8cfc4c6c2608646a5be7bf39605ec15326348 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Tue, 10 Nov 2026 03:55:01 +0000 Subject: [PATCH 13/13] Export logger warning code and test read-only log files Signed-off-by: Vu Anh Phung --- lib/index.ts | 1 + tests/unit/DBSQLLogger.test.ts | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/lib/index.ts b/lib/index.ts index 6b6d42b9..a2b90c9c 100644 --- a/lib/index.ts +++ b/lib/index.ts @@ -23,6 +23,7 @@ import { LogLevel } from './contracts/IDBSQLLogger'; // Re-export types for TypeScript users export type { default as ITokenProvider } from './connection/auth/tokenProvider/ITokenProvider'; +export { LOG_FILE_UNAVAILABLE_WARNING_CODE } from './DBSQLLogger'; export { CircuitBreakerOpenError, CIRCUIT_BREAKER_OPEN_CODE } from './telemetry/CircuitBreaker'; export { TelemetryTerminalError } from './telemetry/DatabricksTelemetryExporter'; diff --git a/tests/unit/DBSQLLogger.test.ts b/tests/unit/DBSQLLogger.test.ts index d6f39a64..0d53d2b8 100644 --- a/tests/unit/DBSQLLogger.test.ts +++ b/tests/unit/DBSQLLogger.test.ts @@ -217,6 +217,26 @@ describe('DBSQLLogger file transport path resolution', () => { }); } + it('falls back to console only for a read-only log file (EACCES)', function readOnlyLogFileTest() { + if (process.platform === 'win32' || process.getuid?.() === 0) { + this.skip(); + } + const filepath = path.join(tmpDir, 'readonly.log'); + fs.writeFileSync(filepath, ''); + fs.chmodSync(filepath, 0o444); + try { + logger = createLogger(filepath, LogLevel.error); + + expect(logger.transports.file).to.be.undefined; + expect(emitWarning.calledOnce).to.be.true; + expect(emitWarning.firstCall.args[0]).to.contain('EACCES'); + expect(emitWarning.firstCall.args[0]).to.contain('logging to console only'); + expect(emitWarning.firstCall.args[1]).to.deep.equal({ code: LOG_FILE_UNAVAILABLE_WARNING_CODE }); + } finally { + fs.chmodSync(filepath, 0o600); + } + }); + it('keeps delivering console logs after the fallback', async () => { fs.mkdirSync(path.join(tmpDir, 'databricks-sql-nodejs.log')); const seen: string[] = [];