Database not found and other errors emitted when they shouldn't be

Environment details OS: MacOS Sonomia 14.6.1 Node.js version: 20.9.0 npm version: 10.1.0 @google-cloud/spanner version: 7.14.0

Steps to reproduce

Note: I was somewhat reluctant to file this because I couldn't easily create a repro case. But I think it's clear from looking at the code that this is broken.

In a process that will handle and complain about unprocessed errors (such as a Jest test), get a Spanner database object representing a database that does not exist (for example, to test for existence with .exist()). Exit the process. There will be an unprocessed error, which in the case of Jest, will cause your test run to fail.

There is an attempt in the session pool code to prevent this exact scenario:

https://github.com/googleapis/nodejs-spanner/blob/992baaa5e7c953938225a0d9f16bc2e5a5c0a6f8/src/session-pool.ts#L571

    this._fill().catch(err => {
      // Ignore `Database not found` error. This allows a user to call instance.database('db-name')
      // for a database that does not yet exist with SessionPoolOptions.min > 0.
      if (
        isDatabaseNotFoundError(err) ||
        isInstanceNotFoundError(err) ||
        isCreateSessionPermissionError(err) ||
        isDefaultCredentialsNotSetError(err) ||
        isProjectIdNotSetInEnvironmentError(err)
      ) {
        return;
      }
      this.emit('error', err);
    });

The problem is that _fill() doesn't throw exceptions, it emits them as errors. So those errors are emitted as 'error' events by _fill() and the code in the this._fill().catch() is never hit.

It should be noted that there will be other session pool listeners attached, in particular any database objects will have a bound emitter that forwards these errors, so simply handling the errors as the exceptions are handled in open() won't work. I'm having trouble following the logic and understanding the significant entanglement of the various objects involved, so there is no PR immediately obvious to me. Hopefully the maintainers will be able to see that there was a clear intention to this code (well intentioned - I definitely don't need an error event for non-existence, often significantly after the fact, when using exist()) that is no longer being satisfied.

The only workaround I could find was to handle all events on all database objects (since they seem to be variously recycled and share the underlying session pool) and ignore the database not found errors (whether they were related to my original attempt to test for existence or not), which is not ideal. The fact that this event will be sent to some future database object at an unpredictable time is also not ideal (for example, after creating a database, which then does exist).

googleapis / nodejs-spanner

Database not found and other errors emitted when they shouldn't be #2106