fix(spannerlib): prevent connection leaks and hangs in FFI execution

[surbhigarg92]

Overview

This PR hardens resource management and shutdown lifecycles inside the spannerlib core API and dynamic language wrappers to prevent connection leaks and runner hangs.

Key Changes

1. Dialect-Aware Multi-Statement Detection (isMulti)

• Problem: Multi-statement execution batches (separated by semicolons) must keep connections open for subsequent result sets, whereas single-statement queries should release connections to the pool immediately upon reaching EOF.
• Solution: Instantiates a dialect-aware StatementParser during pool initialization to dynamically split SQL. If !isMulti, connections auto-close on EOF; if isMulti, the connection is kept open for NextResultSet().

2. Auto-Closing Iterators & Metadata Preservation

• Safe EOF Close: Automatically reads final stream metadata (ResultSetStats) before closing the backend on single-statement EOF, avoiding false-positive errors in PROFILE mode.
• Error Leak Prevention: Hardened error paths in NextResultSet and Next to immediately cancel the iterator context and close the backend on mid-stream failures.

3. Concurrency Safety & Bounded Cleanup Timeout

• Non-Blocking Close: Enforces a strict 2-second timeout on CGO cleanups (ClosePool, CloseConnection) to prevent process termination hangs.
• Safe Goroutine Cleanups: Moves the connection range loop in ClosePool into the background goroutine alongside database closing to respect context cancellation.
• Defensive Merging: Prevents panic/blocks on nil or non-cancellable parent contexts in mergeContexts.

4. Node.js Wrapper Hardening

• Process Exit Safety: Replaces global process.exit overrides with standard beforeExit event hooks to preserve Node's synchronous exit contract.
• Tests: Added mock server verification tests ensuring closed connections release resources cleanly.

[gemini-code-assist]

Code Review

This pull request implements context cancellation, asynchronous connection/pool closing, and automatic resource cleanup on EOF or process exit, along with corresponding unit tests. The reviewer identified several critical issues: overriding process.exit asynchronously in Node.js breaks its synchronous contract; ClosePool can still hang synchronously if connection closing blocks; potential resource leaks exist in NextResultSet and Next error paths; redundant SQL parsing is performed in execute; and mergeContexts can panic or block indefinitely if the second context is nil or has a nil Done channel. Additionally, some test assertions violate the repository's style guide regarding aligned got/want formatting.

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

spannerlib/wrappers/spannerlib-node/src/lib/spannerlib.ts (51-61)

Overriding process.exit to be asynchronous breaks the synchronous contract of process.exit() in Node.js. Any synchronous code following process.exit() in the calling application will continue to execute before the process actually exits, which can lead to severe race conditions, unexpected behavior, or security vulnerabilities. It is highly recommended to avoid overriding process.exit globally in library code and instead rely on beforeExit or explicit cleanup methods.

spannerlib/api/pool.go (91-103)

The pool.connections.Range loop (lines 86-90) is executed synchronously before the goroutine for pool.db.Close(). If any connection's close() call hangs (e.g., due to a blocked rollback or network issue), ClosePool will hang synchronously and completely ignore the ctx timeout. To prevent hangs and respect the context timeout, the connection cleanup loop should be run inside the background goroutine along with pool.db.Close().

Example fix:

go func() {
	pool.connections.Range(func(key, value interface{}) bool {
		conn := value.(*Connection)
		_ = conn.close(ctx)
		return true
	})
	ch <- pool.db.Close()
}()

spannerlib/api/rows.go (213-223)

In NextResultSet, if rows.readMetadata(ctx) or rows.readStats(ctx) fails earlier in the function (lines 202-209), the function returns the error immediately without calling rows.cancel() or rows.backend.Close(). This will leak the rows iterator and the underlying connection. Please ensure that any error returned from readMetadata or readStats also closes the backend and cancels the context.

spannerlib/api/rows.go (250-266)

In Next, if rows.backend.Next() returns false and rows.backend.Err() != nil (lines 247-248), the function returns the error immediately without calling rows.cancel() or rows.backend.Close(). This leaks the rows iterator and the connection on mid-stream query failures. Please ensure that the error path for rows.backend.Err() also closes the backend and cancels the context.

spannerlib/api/connection.go (379-390)

The SQL statement is parsed again to determine isMulti inside the !hasFields block. This is completely redundant because isMulti was already calculated at lines 357-362 and is available in the outer scope of the execute function. Since parsing SQL can be relatively expensive, reusing the outer isMulti variable avoids unnecessary overhead.

		if !isMulti {
			_ = it.Close()
			if cancel != nil {
				cancel()
			}
		}

spannerlib/api/connection.go (587-597)

If ctx2 is nil, calling ctx2.Done() will panic. Additionally, if ctx2 is context.Background() or context.TODO(), its Done() channel is nil, meaning the goroutine will block indefinitely on mergedCtx.Done(). We can defensively check if ctx2 is nil or has a nil Done() channel, and return early without spawning a goroutine.

func mergeContexts(ctx1, ctx2 context.Context) (context.Context, context.CancelFunc) {
	if ctx2 == nil || ctx2.Done() == nil {
		return context.WithCancel(ctx1)
	}
	mergedCtx, cancel := context.WithCancel(ctx1)
	go func() {
		select {
		case <-ctx2.Done():
			cancel()
		case <-mergedCtx.Done():
		}
	}()
	return mergedCtx, cancel
}

References

1. Do not use panic in library code. Errors should be handled or returned to the caller rather than causing a panic.

spannerlib/api/pool_test.go (177-179)

This test assertion does not follow the preferred aligned layout using g (got) and w (want) variables as specified in the Repository style guide (Rule 5).

		if g, w := closeErr, context.DeadlineExceeded; g != w {
			t.Fatalf("ClosePool error mismatch\nGot:  %v\nWant: %v", g, w)
		}

References

1. When writing test assertions, strongly prefer using variable names g (got) and w (want) for comparison, and format error messages using the aligned layout with two spaces after Got:. ^(link)

spannerlib/api/connection_test.go (616-618)

This test assertion does not follow the preferred aligned layout using g (got) and w (want) variables as specified in the Repository style guide (Rule 5).

		if g, w := spanner.ErrCode(err), codes.Canceled; g != w {
			t.Fatalf("error code mismatch\nGot:  %v\nWant: %v", g, w)
		}

References

1. When writing test assertions, strongly prefer using variable names g (got) and w (want) for comparison, and format error messages using the aligned layout with two spaces after Got:. ^(link)

[surbhigarg92]

/gemini review

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request enhances the Go driver and its Node.js wrapper by introducing dialect-aware statement parsing, improving context propagation, and implementing automatic resource cleanup to prevent connection and context leaks. Key updates include tracking connection-level contexts, auto-closing rows on EOF, and adding process exit hooks in the Node.js wrapper. The review feedback highlights critical issues in the Node.js wrapper, specifically a memory leak caused by using a strong Map instead of a WeakMap for resource tracking, a potential infinite loop with process.on('beforeExit'), and missing cleanup logic in the Go driver's nextBatch method during batch-based row consumption. Additionally, defensive checks are recommended in mergeContexts to prevent nil context panics and optimize goroutine spawning.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces context cancellation and timeout handling across connection, pool, and row operations to prevent resource leaks and hangs, including dialect-aware statement parsing and Node.js process exit cleanup. The reviewer feedback highlights a critical issue where passing a potentially cancelled or timed-out context to conn.close(ctx) in both CloseConnection and ClosePool can cause transaction rollbacks to fail, potentially leaking Spanner sessions; using context.Background() is suggested instead.