Prevent crash when req.logger is undefined in error handler

[zackcl]

Problem

The UpGrade instance used by the demo app was crashing with the following error:

uncaughtException: Cannot read properties of undefined (reading 'error')
at ErrorHandlerMiddleware.ts:126:16

This occurred when the ErrorHandlerMiddleware tried to call req.logger.error() without checking if req.logger exists. In certain scenarios (e.g., errors occurring before the LogMiddleware runs or during early request processing), req.logger may be undefined.

Solution

1. Added null check before accessing req.logger: Prevents the crash by checking if req.logger exists before calling req.logger.error(experimentError)
2. Fixed TypeScript warning: Removed async keyword and changed return type from Promise<void> to void to properly implement the ExpressErrorMiddlewareInterface contract (no await calls exist in the method)

Changes

• Added defensive null check: if (req.logger) { req.logger.error(experimentError); }
• Changed method signature from async error(...): Promise<void> to error(...): void

[Copilot]

Pull request overview

This PR fixes a crash in the ErrorHandlerMiddleware when req.logger is undefined. The crash occurred when errors happened before the LogMiddleware set up the logger, causing a "Cannot read properties of undefined" error at runtime.

• Added defensive null check before accessing req.logger.error()
• Removed unnecessary async keyword and changed return type from Promise<void> to void (method contains no async operations)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bcb37]

This makes sense to me. I wonder though: Are we going to be ignoring errors that we could or should be logging?

[danoswaltCL]

@zackcl @bcb37 hey I found the actual reproducible issue here, and it's not really an edge-case so it's good that we waited on merging this. the fix ends up being similar, but we want to make sure the error gets logged because there's a legit situation where this would happen.

if you send in a request that triggers a CORS error (domain not on whitelist), it will trigger this "req.logger is undefined" error. The reason is that CORS check happens before the logger middleware. I can see why this might have happened while working on the Demo server if something was not quite configured right at the time.

if you look at app/index.ts in backend, you can see that CORS is set up in a standard Express config (createExpressServer()). So it is not anything we've put out of order, it's just how this works in Express.

So architecturally, there's not really a good way to try and reorder anything middleware-wise, but that would be overkill to solve this, it should be fine to just create a fallback logger in these cases, as we're done with the request at this point anyway.

Suggested change:

// Use req.logger if available, otherwise create a fallback logger
// This handles cases where errors occur before LogMiddleware runs (e.g., CORS errors)
    if (req.logger) {
      req.logger.error(experimentError);
    } else {
      const fallbackLogger = new UpgradeLogger();
      fallbackLogger.error({
        ...experimentError,
        warning: 'req.logger was undefined so custom logger used - error occurred before LogMiddleware loaded, likely a CORS issue',
      });
    }

[danoswaltCL]

if (req.logger) {
      req.logger.error(experimentError);
    } else {
      const fallbackLogger = new UpgradeLogger();
      fallbackLogger.error({
        ...experimentError,
        warning: 'req.logger was undefined so custom logger used - error occurred before LogMiddleware loaded, likely a CORS issue',
      });
    }
 

[zackcl]

@danoswaltCL I added the fallback logger as you suggested.