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/12] [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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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 c9abe6d8de1f5051eef2d3d0af772f769bca4796 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 16:28:37 +0000 Subject: [PATCH 11/12] 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 | 5 ++ lib/DBSQLLogger.ts | 36 +++++------ lib/contracts/IDBSQLLogger.ts | 5 +- tests/unit/DBSQLLogger.test.ts | 106 +++++++++++++++++++++++++++------ 4 files changed, 112 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3f3dc8b..a53cb9c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Release History +## Unreleased + +- `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 - Upgrade the native kernel backend to 1.4.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 4b5158ccded2a57779cf52b979fc5524df369811 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 20:58:40 +0000 Subject: [PATCH 12/12] 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, '');