feat: add support for Node wrapper with TypeScript and dual-publishing (ESM/CJS)

[surbhigarg92]

This PR introduces a Node.js wrapper (spannerlib-node) for the Spanner shared library. It uses TypeScript and Node-API to provide a dual-published package (ESM and CommonJS) .

Key Highlights

1. Dual Publishing: Successfully ships targeted builds inside build/esm/ and build/cjs/ mapped through an exports map.
2. executeSql Support: Implemented the core method to execute SQL statements against the database in Node wrapper. Transactions, sessions, and other administrative methods will be built out in a follow-up PR.
3. Raw Protobuf Rows: rows.next() returns raw ListValue objects, matching Java and .NET behaviors.
4. Automated Native Build: Added a custom shell script to handle Go binary compilation on the fly, chained directly into the standard npm run build lifecycle.

Current Limitations
Windows CI: The Windows CI currently fails on compile-time static linking. We have deferred Windows support and platform-specific packaging to a dedicated task after this merges.

[gemini-code-assist]

Code Review

This pull request introduces a new Node.js wrapper for the Spanner shared library, providing three implementation approaches (N-API, Koffi, and IPC) along with a benchmarking suite. The review identified several critical issues, including hardcoded paths in build scripts, synchronous execution of potentially blocking operations, missing memory management for pinned objects, and incomplete type handling in the result parser. I recommend addressing these portability and performance concerns before merging.

I am having trouble creating individual review comments. Click here to see my feedback.

spannerlib/grpc-server/build-protos.sh (31)

Hardcoded absolute path to the Java gRPC plugin. This will fail on any machine other than the author's. Consider using an environment variable or ensuring the plugin is available in the system PATH.

spannerlib/grpc-server/build-protos.sh (40)

Hardcoded absolute path to the C# gRPC plugin. This breaks the portability of the build script and will cause CI/CD pipelines to fail.

spannerlib/wrappers/spannerlib-node/src/cpp/addon.cc (303-321)

CloseRowsWrapper is implemented synchronously. Closing rows in a database driver often involves network communication to signal the end of a stream or release server-side resources. Calling this on the Node.js main thread can block the event loop, leading to performance degradation. It should be implemented using Napi::AsyncWorker.

spannerlib/wrappers/spannerlib-node/src/ffi/utils.ts (15-43)

The invokeAsync function accepts constructor1 and constructor2 (which should be renamed to refInstance and pinManager) but never uses them to register the pinner ID (result.r0) with the memory manager. This will lead to memory leaks in the Go shared library because pinned objects are never released when JavaScript objects are garbage collected.

spannerlib/wrappers/spannerlib-node/src/lib/connection.ts (43)

The session ID is hardcoded to "poc/dummy". Cloud Spanner requires a valid session ID for executing SQL. This will cause errors when interacting with a real Spanner instance. The session should be dynamically obtained from the pool.

spannerlib/wrappers/spannerlib-node/src/lib/rows.ts (24-39)

The parseRowToObject function only handles a subset of Spanner types. It is missing support for bytesValue, structValue, and listValue. This could lead to data loss or undefined values being returned without warning for those types.

spannerlib/wrappers/spannerlib-node/src/lib/rows.ts (72)

Fetching rows one by one (numRows: 1) is highly inefficient as it incurs a network round-trip for every single row. Consider using a larger batch size or making it configurable to improve throughput.

spannerlib/wrappers/spannerlib-nodejs-poc/benchmark/benchmark.js (31)

The database path is hardcoded to a specific testing instance. This should be configurable via environment variables to allow running benchmarks in different environments.

const dbPath = process.env.SPANNER_DB_PATH || `projects/span-cloud-testing/instances/gargsurbhi-testing/databases/${dbName}`;

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing an object-oriented TypeScript interface for managing database pools, connections, and row iterators. The implementation includes a C++ bridge and a memory management system using a FinalizationRegistry. However, several critical issues were identified regarding memory management and performance. Specifically, memory leaks occur in Rows.next() and CloseRowsWrapper because native pinner IDs are not released, and the Pool, Connection, and Rows classes are not correctly registered with the cleanup registry. Additionally, CloseRowsWrapper should be implemented asynchronously to avoid blocking the Node.js event loop, and hardcoded session values in SQL execution must be addressed for production use.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, featuring a C++ addon to bridge Node.js and Go, along with TypeScript classes for managing pools, connections, and row iteration. The implementation includes automatic memory management via a FinalizationRegistry and comprehensive build scripts. Feedback highlights a critical race condition in the C++ addon where memory might be released before being copied, as well as potential issues with promises hanging due to insufficient argument validation. Other suggestions include improving type safety in the TypeScript layer and addressing hardcoded session identifiers.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing a high-performance bridge between Node.js and Go. It includes a C++ addon for handling native calls, TypeScript classes for managing pools, connections, and row iteration, and a memory management system using a FinalizationRegistry. The review identifies several critical memory management issues, specifically regarding the failure to release pinner IDs in error paths and after data copying in the C++ layer. Additionally, the FinalizationRegistry currently only releases pinner IDs without cleaning up the underlying Go objects. Other feedback suggests adding argument length checks in the C++ wrappers for better stability and replacing hardcoded session IDs with dynamic management.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing a bridge between Node.js and the Go-based library. Feedback focuses on critical memory management issues, including the incorrect use of FinalizationRegistry for short-lived identifiers and missing or redundant pinner ID releases in the native and TypeScript layers. Additionally, improvements are needed for C++ argument validation and the removal of hardcoded session identifiers to ensure compatibility with real Spanner environments.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, featuring C++ bindings and TypeScript abstractions for database management. The review identifies several high-severity issues, including native memory leaks in the C++ worker classes and an architectural flaw where Go objects are unpinned but not properly closed. Additional feedback suggests improving safety through argument validation, removing unused parameters, and avoiding tight coupling to internal package paths.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing a high-performance bridge between Node.js and the Go-based library. The implementation includes a C++ addon, TypeScript classes for managing database resources, and a comprehensive build and test suite. However, the review highlights several critical issues: multiple argument mismatches between the TypeScript FFI layer and the C++ wrapper functions will likely cause runtime crashes. Furthermore, native memory leaks were identified in the C++ workers due to unreleased pinner IDs, and the FinalizationRegistry logic for automatic resource cleanup is incorrectly implemented. Finally, the use of internal package paths for protobuf imports should be addressed to ensure long-term stability.

[gemini-code-assist]

There is a critical argument mismatch between the TypeScript call and the C++ wrapper. CreatePoolWrapper in addon.cc expects 2 string arguments (userAgent, connStr) followed by a callback. However, this call passes 4 arguments (p, spannerLib, userAgentSuffix, connectionString). This will cause the C++ side to attempt to cast the Pool instance (p) to a string, resulting in a crash or exception.

    const handled = await ffi.invokeAsync(
      'CreatePool',
      userAgentSuffix,
      connectionString
    );

[gemini-code-assist]

Argument mismatch: CreateConnectionWrapper in C++ expects 1 number argument (poolId) and a callback. The TypeScript code is passing 3 arguments (c, spannerLib, pool.oid). The first argument c (the Connection instance) will be incorrectly interpreted as the poolId number.

    const handled = await ffi.invokeAsync(
      'CreateConnection',
      pool.oid
    );

[gemini-code-assist]

Argument mismatch: ExecuteWrapper in C++ expects 3 arguments (poolId, connId, payload) and a callback. The TypeScript code passes 5 arguments, including two leading null values. This will cause info[0] and info[1] to be null instead of the expected IDs, and info[2] to be a number instead of a Buffer, leading to a crash.

    const rowsResult = await ffi.invokeAsync(
      'Execute',
      this.pool.oid,
      this.oid,
      serializedPb
    );

[gemini-code-assist]

Argument mismatch: NextWrapper in C++ expects 5 arguments (poolId, connId, rowsId, numRows, encodeOtp) and a callback. The TypeScript code passes 7 arguments, including two leading null values. This will shift all arguments and cause type errors in the native layer.

    const handled = await ffi.invokeAsync(
      'Next',
      this.connection.pool.oid,
      this.connection.oid,
      this.oid,
      1,
      ENCODING_PROTOBUF
    );

[gemini-code-assist]

The pinner ID (result_.r0) returned by the Go library for the response message is not being released in CreatePoolWorker. This will lead to a native memory leak as the Go runtime will keep the memory pinned indefinitely. It should be released immediately after the buffer is copied into the Node.js environment.

        if (result_.r4 != nullptr && result_.r3 > 0) {
            obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
        } else {
            obj.Set("r4", env.Null());
        }
        // Release the pinner ID of the response message to prevent native leak!
        if (result_.r0 > 0) {
            ::Release(result_.r0);
        }
        Callback().Call({env.Null(), obj});

[gemini-code-assist]

Similar to CreatePoolWorker, CreateConnectionWorker is missing the call to ::Release(result_.r0). This causes a native memory leak for every connection created.

        if (result_.r4 != nullptr && result_.r3 > 0) {
            obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
        } else {
            obj.Set("r4", env.Null());
        }
        // Release the pinner ID of the response message to prevent native leak!
        if (result_.r0 > 0) {
            ::Release(result_.r0);
        }
        Callback().Call({env.Null(), obj});

[gemini-code-assist]

The FinalizationRegistry logic is fundamentally flawed. It is registering the pinnerId of the creation response message. As noted in other comments, this ID should be released immediately after the creation call finishes. Furthermore, releasing this pinner ID does NOT close the underlying Go object (Pool/Connection/Rows). To prevent native resource leaks, the registry should track the objectId and trigger the appropriate Close method (e.g., ClosePool), although this is complex because those methods are asynchronous and the registry callback is synchronous.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a high-performance Node-API (N-API) wrapper for the Go-based Spanner shared library, providing core classes like Pool, Connection, and Rows along with memory management via a FinalizationRegistry. The review feedback highlights critical memory management and stability issues, specifically pointing out several potential double-release bugs of Go pinner IDs in the C++ worker classes and the SpannerLib registry, as well as premature object unpinning in the Execute worker. Additionally, the reviewer recommends implementing defensive null checks for nullable object IDs in Connection and Rows before invoking native methods to prevent crashes.

[gemini-code-assist]

The Execute function returns a long-lived Rows object whose pinner ID (result_.r0) must be kept alive until the Rows object is closed or garbage collected. Releasing it immediately in ExecuteWorker::OnOK() unpins the Rows object in Go prematurely, which can lead to crashes or undefined behavior when Next is called. Additionally, since JS registers this pinner ID in spannerLib to be released later, releasing it here causes a double-release bug.

Suggested change

	// Release the pinner ID of the response buffer immediately after copy!
	if (result_.r0 > 0) {
	::Release(result_.r0);
	}
	Callback().Call({env.Null(), obj});
	Callback().Call({env.Null(), obj});

[gemini-code-assist]

Since NextWorker already releases result_.r0 in C++ to prevent native leaks of the temporary response buffer, passing result_.r0 to JS can lead to a double-release bug in utils.ts if the operation fails (as utils.ts will attempt to release it again). Setting r0 to 0 in the returned object prevents this double-release.

Suggested change

	obj.Set("r0", Napi::Number::New(env, result_.r0));
	obj.Set("r0", Napi::Number::New(env, 0));

[gemini-code-assist]

Since MetadataWorker already releases result_.r0 in C++ to prevent native leaks of the temporary response buffer, passing result_.r0 to JS can lead to a double-release bug in utils.ts if the operation fails. Setting r0 to 0 in the returned object prevents this double-release.

Suggested change

	obj.Set("r0", Napi::Number::New(env, result_.r0));
	obj.Set("r0", Napi::Number::New(env, 0));

[gemini-code-assist]

Since CloseRowsWorker already releases result_.r0 in C++ to prevent native leaks of the temporary response buffer, passing result_.r0 to JS can lead to a double-release bug in utils.ts if the operation fails. Setting r0 to 0 in the returned object prevents this double-release.

Suggested change

	obj.Set("r0", Napi::Number::New(env, result_.r0));
	obj.Set("r0", Napi::Number::New(env, 0));

[gemini-code-assist]

If releaseAll() is called (e.g., during cleanup or shutdown), all active pinners are released and cleared from activePinners. However, the JS objects themselves are not unregistered from the FinalizationRegistry. When those JS objects are eventually garbage collected, the registry callback will run and attempt to release the pinnerId again, causing a double-release. Checking this.activePinners.has(pinnerId) in the callback prevents this.

Suggested change

	this.registry = new FinalizationRegistry((pinnerId: number) => {
	if (pinnerId && pinnerId > 0) {
	ffi.Release(pinnerId);
	this.activePinners.delete(pinnerId);
	}
	});
	this.registry = new FinalizationRegistry((pinnerId: number) => {
	if (pinnerId && pinnerId > 0 && this.activePinners.has(pinnerId)) {
	ffi.Release(pinnerId);
	this.activePinners.delete(pinnerId);
	}
	});

[gemini-code-assist]

To enforce defensive programming, we should check if this.oid or this.pool.oid are null before passing them to the native wrapper, as they are nullable in TypeScript and passing null can cause crashes or type errors in the native layer.

Suggested change

	async executeSql(sqlString: string): Promise<Rows> {
	if (this.closed) throw new Error('Connection is already closed');
	if (!this.pool) throw new Error('Connection is not bound to a Pool');
	async executeSql(sqlString: string): Promise<Rows> {
	if (this.closed) throw new Error('Connection is already closed');
	if (!this.pool) throw new Error('Connection is not bound to a Pool');
	if (this.oid === null) throw new Error('Connection is not initialized');
	if (this.pool.oid === null) throw new Error('Pool is not initialized');

[gemini-code-assist]

To enforce defensive programming, we should check if pool.oid is null before attempting to create a connection, as passing null to the native wrapper can cause crashes or type errors.

  static async create(pool: Pool): Promise<Connection> {
    if (pool.oid === null) {
      throw new Error('Cannot create connection: Pool is not initialized');
    }
    const c = new Connection();
    c.pool = pool;

[gemini-code-assist]

To enforce defensive programming, we should check if this.connection.oid or this.connection.pool.oid are null before passing them to the native wrapper, as they are nullable in TypeScript and passing null can cause crashes or type errors.

  async next(): Promise<unknown> {
    if (this.closed) throw new Error('Rows are already closed');

    if (!this.connection.pool) {
      throw new Error('Connection must be bound to a Pool to fetch rows');
    }
    if (this.connection.oid === null) {
      throw new Error('Connection is not initialized');
    }
    if (this.connection.pool.oid === null) {
      throw new Error('Pool is not initialized');
    }

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a high-performance Node-API (N-API) wrapper for the Go-based Spanner shared library, featuring automatic native memory management via a FinalizationRegistry. The review feedback highlights critical issues in the C++ addon where native resources are released prematurely on worker threads, potentially causing crashes or losing error messages during pool and connection closures. Additionally, the reviewer recommends removing redundant release calls in the FFI utility layer, adding defensive null checks for object IDs in Connection and Rows to prevent passing invalid handles, and addressing the misleading, currently no-op cleanup() export.

[gemini-code-assist]

In ClosePoolWorker, the pinner ID (result_.r0) is released in Execute() (which runs on a worker thread) before OnOK() runs. If ClosePool fails, the error message in result_.r4 is pinned by result_.r0. Releasing it early makes it unsafe to access result_.r4 in OnOK(). Furthermore, OnOK() currently does not copy result_.r4 or set r0/r3/r4 on the returned object, meaning any error message returned by Go is lost in the JS layer.

To fix this, do not release result_.r0 in Execute(). Instead, copy result_.r4 to a JS Buffer in OnOK() (just like CreatePoolWorker does) and release result_.r0 at the end of OnOK() to prevent native memory leaks and ensure error messages are correctly propagated.

    void Execute() override {
        result_ = ClosePool(poolId_);
    }

    void OnOK() override {
        Napi::Env env = Env();
        Napi::Object obj = Napi::Object::New(env);
        obj.Set("r0", Napi::Number::New(env, result_.r0));
        obj.Set("r1", Napi::Number::New(env, result_.r1));
        obj.Set("r2", Napi::Number::New(env, result_.r2));
        obj.Set("r3", Napi::Number::New(env, result_.r3));
        if (result_.r4 != nullptr && result_.r3 > 0) {
            obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
        } else {
            obj.Set("r4", env.Null());
        }
        if (result_.r0 > 0) {
            ::Release(result_.r0);
        }
        Callback().Call({env.Null(), obj});
    }

[gemini-code-assist]

Similarly to ClosePoolWorker, CloseConnectionWorker releases result_.r0 in Execute() and does not populate r0/r3/r4 in OnOK(). This causes any error messages from CloseConnection to be lost in the JS layer.

To fix this, defer the release of result_.r0 to OnOK() and copy result_.r4 to the returned JS object.

    void Execute() override {
        result_ = CloseConnection(poolId_, connId_);
    }

    void OnOK() override {
        Napi::Env env = Env();
        Napi::Object obj = Napi::Object::New(env);
        obj.Set("r0", Napi::Number::New(env, result_.r0));
        obj.Set("r1", Napi::Number::New(env, result_.r1));
        obj.Set("r2", Napi::Number::New(env, result_.r2));
        obj.Set("r3", Napi::Number::New(env, result_.r3));
        if (result_.r4 != nullptr && result_.r3 > 0) {
            obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
        } else {
            obj.Set("r4", env.Null());
        }
        if (result_.r0 > 0) {
            ::Release(result_.r0);
        }
        Callback().Call({env.Null(), obj});
    }

[gemini-code-assist]

The result.r0 pinner ID is already released in the C++ layer (addon.cc) immediately after copying the buffer in OnOK(). Calling ffi.Release(result.r0) here is redundant and results in an unnecessary native call. This block can be safely removed.

Suggested change

	if (result.r1 !== 0) {
	if (result.r0 && result.r0 > 0) {
	ffi.Release(result.r0);
	}
	if (result.r4 && result.r3 > 0) {
	if (result.r1 !== 0) {
	if (result.r4 && result.r3 > 0) {

[gemini-code-assist]

To enforce defensive programming and prevent potential runtime errors or crashes in the native addon, we should explicitly check that both this.pool.oid and this.oid are not null before executing the SQL statement.

Suggested change

	if (this.closed) throw new Error('Connection is already closed');
	if (!this.pool) throw new Error('Connection is not bound to a Pool');
	if (this.closed) throw new Error('Connection is already closed');
	if (!this.pool || this.pool.oid === null || this.oid === null) {
	throw new Error('Connection is not bound to an active Pool');
	}

[gemini-code-assist]

Defensively check that this.connection.pool.oid and this.connection.oid are not null before calling ffi.invokeAsync to prevent passing invalid handles to the native layer.

Suggested change

	if (this.closed) throw new Error('Rows are already closed');
	if (!this.connection.pool) {
	throw new Error('Connection must be bound to a Pool to fetch rows');
	}
	if (this.closed) throw new Error('Rows are already closed');
	if (!this.connection.pool || this.connection.pool.oid === null || this.connection.oid === null) {
	throw new Error('Connection must be bound to an active Pool to fetch rows');
	}

[gemini-code-assist]

releaseAll() is currently a no-op, but it is exported as cleanup() in index.ts with documentation claiming it 'Releases all pinned resources and handles managed by the library... to prevent native memory leaks.' This is misleading because it does not actually close any active pools, connections, or rows. We should either implement tracking of active resources (e.g., using a Set of WeakRefs) to actually close them in releaseAll(), or remove the misleading cleanup() export entirely.