[peco-engineer-bot]

Summary

Addresses #494 (LOGGING-002). A filepath that names an existing directory now writes to <dir>/databricks-sql-nodejs.log instead of being opened as a file, in line with JDBC and ODBC LogPath and the kernel.

The directory case never crashed. winston's File transport swallows errors opening its write stream, so the file never opened, and once its buffer filled the logger stalled for every transport, including the console. Any unwritable path did the same.

DBSQLLogger now checks the file once before adding the transport, creating the file and its directory as winston would. If the check fails, it logs to the console only, leaves transports.file undefined, and emits a process warning with code DBSQL_LOG_FILE_UNAVAILABLE. That warning doesn't depend on the logger level and isn't an error-level log line. Paths that exist but aren't regular files or directories (FIFOs, devices) skip the check and are left to winston, because opening a FIFO synchronously blocks until it has a reader.

A trailing separator is not treated as a directory unless the path already is one. In 3.0.0, filepath: 'logs/' created a file named logs. Resolving that to logs/databricks-sql-nodejs.log crashed the process with ENOTDIR on upgrade, so the 3.0.0 behaviour is kept.

There is a new ## Unreleased CHANGELOG section covering the directory behaviour and the fallback. The fallback is a visible change: in 3.0.0, a log directory that couldn't be created made the constructor throw.

Testing

Unit tests cover:

• directory resolution and read-back from the resolved file
• trailing separators: an existing directory, an existing file, and a missing path
• the fallback when stat fails and when open fails (EISDIR)
• console delivery of 1000 lines after the fallback (with the probe removed, only 252 got through)
• no synchronous open of a FIFO
• no mkdir of an existing directory, since Windows throws EPERM for a drive root (stubbed, because CI has no Windows runner)

The live-warehouse e2e spec is removed because the unit tests cover the same behaviour. npm test (1494 passing), lint and prettier pass. npm run type-check only reports the existing examples/tokenFederation errors.

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Medium · 2 Nit

Looks good overall: the directory-to-file resolution is small and correct. The main gap is that there's no unit test for it, even though the PR description lists one; the e2e spec also leaves the transport open and its listeners attached.

Other findings

• ⚪ Nit — runWorkload only removes the uncaughtException listener in finally. The error listeners added to logger.logger and logger.transports.file are never removed, and the winston logger/file stream is never closed (no logger.logger.close()). afterEach then rmSyncs the directory while the file descriptor may still be open. On Linux this leaks the fd. On Windows it makes rmSync fail with EBUSY/EPERM. Close the logger (and wait for its finish event) before removing the temp dir.
• ⚪ Nit — LoggerOptions.filepath now has a new public behavior: an existing directory resolves to <dir>/databricks-sql-nodejs.log. Nothing documents this, and the PR author says they picked the file name themselves. This is a public API, so add a JSDoc comment on filepath (and/or a README note) describing the directory behavior and the default file name so callers can find their logs.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

The fix is small and correct, with a few low/medium concerns. Checking for an existing directory before creating the winston File transport resolves the EISDIR problem, and the unit and e2e tests cover both the directory and the file-path forms. The concerns are a possible teardown race in the new unit tests and a few edge-case and documentation gaps.

[peco-review-bot]

Verdict: 2 Low

Looks good overall. The directory-to-default-file resolution is correct, safely falls back to the old behavior when stat fails, and has unit and e2e tests. Two low concerns: the e2e spec doesn't clean up the logger it creates, and the default file name plus trailing-separator behavior need a maintainer's sign-off. Also, the PR description's "Files changed" list leaves out lib/contracts/IDBSQLLogger.ts and tests/unit/DBSQLLogger.test.ts, which the diff does touch.

[peco-review-bot]

Verdict: 1 Nit

Looks good. The directory check is small and correct: statSync follows symlinks, and errors from missing or inaccessible paths fall back to using the path as given. The new doc comment on the contract matches what the code does. I have one low test-precision note. Separately, the PR description's "Files changed" list leaves out lib/contracts/IDBSQLLogger.ts and tests/unit/DBSQLLogger.test.ts, which this diff also modifies.

Other findings

• ⚪ Nit — The directory-case e2e test only checks that some regular file with content shows up in the directory. It doesn't check that the file is databricks-sql-nodejs.log, which is the name IDBSQLLogger.ts now documents as the contract. Compare the file-path case at line 105, which does assert the exact list of files. Asserting deep.equal([path.join(dir, 'databricks-sql-nodejs.log')]) here would catch an accidental rename or a stray extra file. The unit test covers the name, but only through winston's internal filename/dirname fields, not the file on disk.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks reasonable overall, with one medium issue: the console-only fallback warning can be filtered out by the configured log level. Separately, the PR description and "Files changed" list name tests/e2e/logging.test.ts and describe EISDIR/uncaughtException e2e assertions, but that file isn't in the diff. The coverage that did land is unit-only, and the diff also changes lib/contracts/IDBSQLLogger.ts, which the list doesn't include, so the description should be reconciled with the actual change.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low concern. The directory resolution and the writability probe before attaching the file transport are correct, and they keep the 3.0.0 trailing-separator behaviour for existing files. Unit tests cover the resolution branches and the console-only fallback.

[peco-review-bot]

Verdict: 2 Low

Looks good, with two low-severity design notes. The directory resolution and writability check are correct, the 3.0.0 trailing-separator behaviour is kept, and the unit tests cover the new branches well.

Nits:

• The default file name databricks-sql-nodejs.log and the CHANGELOG entry are still open questions for maintainers.
• path.join(path.dirname(p), path.basename(p)) is a roundabout way to strip a trailing separator. A short helper name or comment would make it clearer.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low test-robustness note. Directory resolution, keeping the 3.0.0 trailing-separator behaviour, and the up-front writability check with a console-only fallback are all correct and covered by unit tests. Remaining nit: the PR description says the logger "logs a warning", but the code logs at error level (deliberately, and documented in the code comment).

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.