googleapis / googleapis/google-cloud-node

logging-winston: failed write crashes the process under default wiring; auth-time failure never invokes the completion callback (logger.end() hangs)

Open
#9,086 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
3.2k
Forks
712
Avg merge
2d 3h
Merged PRs (30d)
99

Description

## Environment

- `@google-cloud/logging-winston` 6.0.2 (latest at time of writing)
- `@google-cloud/logging` 11.3.0
- `winston` 3.x, Node.js v22.x

## Summary

Two failure-path defects that together make the transport unsafe for production services. Both were reproduced with real `winston.createLogger` + real `LoggingWinston` wiring (nothing manually `.emit()`'d); the write RPC was stubbed where noted so the failure mode is deterministic.

### 1. A single failed write crashes the host process with default wiring

With an API-level write rejection (e.g. `PERMISSION_DENIED` from `writeLogEntries`) and the transport attached the way the README shows (no logger-level `'error'` listener):

```
[transport "error" event fired] simulated PERMISSION_DENIED from writeLogEntries
Error: simulated PERMISSION_DENIED from writeLogEntries
at LoggingWinston._write (winston-transport/modern.js:82)
at doWrite (readable-stream/lib/_stream_writable.js:390)
Emitted 'error' event on DerivedLogger instance at:
at DerivedLogger.transportEvent (winston/lib/winston/logger.js:663:12)
Node.js v22.22.3
EXIT_CODE=1
```

Attaching `transport.on('error')` is **not** sufficient — winston's `Logger` re-emits the transport error on itself, and with no logger-level handler Node's default unhandled-`'error'` behavior kills the process. A logging transport whose ordinary failure mode (transient quota/permission/API errors) crashes the service it observes is a serious footgun; at minimum the README should require a `logger.on('error')` handler, but better would be for the transport to never invoke the winston callback with an error for fire-and-forget writes (report through its own `'error'` event only).

Reproduced matrix: with BOTH `transport.on('error')` and `logger.on('error')` attached the same rejection is handled cleanly (exit 0) — confirming the crash is exactly the missing-logger-listener path.

### 2. Auth-time failures never invoke the completion callback — `logger.end()` hangs forever

With `GOOGLE_APPLICATION_CREDENTIALS` pointing at a nonexistent file (a misconfiguration you'd want to fail loudly but catchably):

```
[harness] logger.end() callback did NOT fire within 5000ms
RESULT_JSON={"transportErrorFired":false,"loggerErrorFired":false,"endCallbackFired":false,
"unhandledRejectionSeen":null,"uncaughtExceptionSeen":"...ENOENT..."}
```

The process gets an `uncaughtException` from lazy stub creation (that part is an upstream gax defect — filed separately as https://github.com/googleapis/google-cloud-node/issues/9085), but the transport-level consequence stands on its own: the winston completion callback for the in-flight write never fires, so `logger.end()` (and any graceful-shutdown flush built on it) hangs indefinitely. The success path is fine (callback fires via the promisify chain) — the gap is specifically the failure path.

## Expected

- A rejected write must never crash the host process under the documented default wiring.
- The winston completion callback must fire exactly once for every write, success **or** failure, so `logger.end()` semantics hold.

## Workaround

We replaced the transport with a ~180-line custom `winston-transport` that calls `Log.write(entry).catch(note).then(() => callback())` — the callback always fires and never carries an error, so the crash-prone `'error'`-forwarding path is unreachable by construction. Happy to share details.

Contributor guide

Open the contributing guide

Research direction

Start at LoggingWinston._write and trace its winston-transport callback and error handling, then compare the transport behavior with winston's Logger.transportEvent and logger.end() flow. Reproduce both cases using real winston.createLogger and LoggingWinston wiring, including the stubbed write rejection and missing credentials. Done means rejected writes do not crash the default setup and every write invokes the completion callback exactly once.

Written by the indexing model from the issue text.

Assessment

Tech stack
nodejs, typescript
Domain
backend, observability-sre
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.