fix(pool): eliminate abandon monitor race for blocking commands (#6971)

[affonsov]

Summary

This PR fixes a race condition in the ClientPool abandon monitor (#6971) and migrates the Node.js pool to use glide-core::ClientPool directly (#6887), removing the TypeScript-side state duplication that existed since the pool was first introduced.

Bug (#6971): The abandon monitor could silently reclaim a pooled client executing a blocking command (BLPOP, XREAD BLOCK, etc.), causing the in-flight future to fail with a ClosingException. Root cause: mark_client_blocking() used try_lock() on the pools TokioMutex`. Under lock contention the flag was never set, and the monitor treated the client as idle-abandoned.

Refactor (#6887): Since the pool was introduced in #6338, Nodes ClientPoolmaintained its own TypeScript-side idle stack, active map, waiter queue, and abandon monitor — duplicating state thatglide-core::ClientPoolalready tracks for Java, Go, and Python. This PR removes that duplication by wiringClientPool.ts` to delegate all pool state to Rust.

Issue link

Closes #6971 — ClientPool abandon monitor can reclaim a client parked on a blocking command
Closes #6887 — Node: Refactor pool state management into Rust core

Features / Behaviour Changes

• The ClientPool abandon monitor now correctly skips clients executing blocking commands under all conditions, including high pool mutex contention, for Java, Go, Python, and Node.
• mark_client_blocking() (which used try_lock) is removed. All bindings use the lock-free get_blocking_flag(client_id) API from glide-core.
• Node.js ClientPool.ts now delegates all pool state to glide-core::ClientPool via the standard Rust pool APIs (createPool, poolTryAcquire, poolAcquireBlocking, poolRelease, poolMetrics, poolDestroy). The TypeScript-side idle stack, active map, waiter queue, and abandon monitor are removed.
• create_handle_for_client() extracted from create_direct_client() in node/rust-client/src/lib.rs. Pool-managed Rust connections are wrapped as full GlideClientHandle N-API objects at acquire time, preserving the complete command API surface for TypeScript callers.
• The fix is available to all future language clients (C#, PHP, Ruby) — calling glide_core::pool::get_blocking_flag(client_id) in the dispatch path is sufficient.
• Public API of ClientPool is unchanged.

Implementation

Root cause and fix (glide-core/src/pool.rs)

mark_client_blocking() acquired the pools TokioMutexviatry_lock(). If the lock was held at the moment a blocking command was dispatched, try_lock()returnedErrand theis_blocking flag was silently never set. The abandon monitors next scan saw is_blocking = false and discarded the client.

Fix: a BLOCKING_FLAG_REGISTRY: DashMap<u64, Arc<AtomicBool>> is added to glide-core/src/pool.rs. The Arc is created when a client enters the pool (add_client) and is shared between PooledClient.is_blocking (read by the monitor) and the registry (read by bindings via get_blocking_flag(client_id)). Each binding calls arc.store(true, Ordering::Release) synchronously on the calling thread, before any spawn() or channel send. An UnmarkOnDrop RAII guard inside the async task clears the flag on all exit paths.

Node pool migration (node/rust-client/src/pool.rs, node/src/ClientPool.ts)

create_handle_for_client(client, push_rx, wake_tsfn, inflight_limit, provided_client_id) is extracted from create_direct_client(). It accepts a provided_client_id: Option<u64> — when Some(id) is given (pool path), the handles client_id matches the pools tracking key; when None (standalone path), a new ID is allocated from NEXT_CLIENT_ID. This ensures poolRelease(pool_id, client.getClientId()) finds the correct in_use entry.

pool_build_handle(client_id, wake_callback) retrieves the existing Rust Client from the glide-core scope registry and wraps it as a full GlideClientHandle with a fresh worker thread and response buffer — no new connection is opened.

close_for_pool_release() stops the worker thread without unregistering from the scope registry, so the same underlying Rust connection can be reused across pool borrow cycles.

On-demand connection creation is triggered in both pool_try_acquire and pool_acquire_blocking via a shared maybe_spawn_on_demand_creation() helper, preserving the maxSize pool growth behaviour.

Key attention areas for reviewers

• glide-core/src/pool.rs — BLOCKING_FLAG_REGISTRY and CLIENT_TO_POOL lifecycle (add on creation, reset-to-false on return-to-idle, remove on permanent discard and destroy)
• java/src/lib.rs — pre_blocking_arc set before get_runtime().spawn(); UnmarkOnDrop guard handles all exit paths including routing errors via ? and task cancellation
• ffi/src/lib.rs — same pre-spawn pattern; _arc_guard for single-command path
• node/rust-client/src/lib.rs — create_handle_for_client extraction; close_for_pool_release
• node/rust-client/src/pool.rs — maybe_spawn_on_demand_creation; pool_build_handle; on-demand creation in both acquire paths; discard draining
• node/src/ClientPool.ts — all TS-side state removed; delegates to Rust pool APIs

Limitations

• Go/Python fix has no dedicated integration test; the two new glide-core unit tests (abandon_monitor_skips_blocking_clients, abandon_monitor_evicts_non_blocking_abandoned_clients) cover the core invariant without a Valkey server.
• pool_acquire_blocking uses a 5 ms polling loop rather than a condvar wait (pre-existing limitation, not introduced by this PR).
• All batch/script executions are conservatively marked as blocking for the full dispatch duration (pre-existing behavior, consistent with prior semantics).
• Node pool integration tests are not run as part of this PR`s CI (Node integration tests require a full native addon build); existing unit tests and the TypeScript build are verified clean.

Testing

New unit tests added (glide-core/src/pool.rs — no Valkey server required):

• abandon_monitor_skips_blocking_clients — regression test for Java: ClientPool abandon monitor can reclaim a client parked on a blocking command (best-effort is_blocking marking race) #6971: inserts a client into pool.in_use with backdated borrowed_at, sets is_blocking = true under pool mutex contention, runs the monitor for 3× the abandon timeout, asserts the client is not evicted.
• abandon_monitor_evicts_non_blocking_abandoned_clients — companion: same setup with is_blocking = false, asserts the client is evicted, confirming the monitor is active.

Verified:

• 190/190 glide-core unit tests pass
• Java pool integration tests pass
• cargo clippy clean across all 4 crates (glide-core, java/glide-rs, ffi, node/rust-client)
• Java spotless lint clean
• Node TypeScript build (npm run build:ts) clean
• CHANGELOG.md updated

Checklist

• This Pull Request is related to one issue.
• Commit message has a detailed description of what changed and why.
• Tests are added or updated.
• CHANGELOG.md and documentation files are updated.
• Linters have been run and Prettier has been run.
• Destination branch is correct — main.
• Create merge commit if merging release branch into main, squash otherwise.
• Make sure to update the documentation in the valkey-glide-docs repository if necessary.

[valkey-review-bot]

The lock-free flag removes the original pool-mutex race, but the new registry/lifecycle code introduces correctness problems in Node and on overlapping dispatches.

[valkey-review-bot]

pool_allocate_id() is exposed to TypeScript as a number, but 2^62 is above JavaScript's Number.MAX_SAFE_INTEGER (2^53 - 1). At this magnitude adjacent fetch_add(1) results round to the same JS value (the ULP is 1024), so many ClientPool instances receive an indistinguishable poolId; their CLIENT_TO_POOL, monitor-handle, and discard-queue entries then overwrite or drain one another. Keep this allocator within the safe integer range (or expose the ID as a BigInt) and separate the two pool kinds without relying on an unsafe numeric offset.

[affonsov]

Resolved by the Node pool migration. pool_allocate_id() no longer exists — ClientPool.ts now calls createPool() which delegates to glide-core::register_pool(), same as Java, Go, and Python. No separate TS-side pool ID is needed.

[valkey-review-bot]

If the pool is destroyed while this client is being created, this branch returns without releasing adapter_ptr. create_pool_client() has already transferred an Arc<ClientAdapter> into the raw pointer and get_pool_clients() is populated only below, so glide_pool_destroy() has no way to find/drop it. The same early-return shape exists in the two on-demand creation paths below. Reconstruct and drop the raw Arc before returning when POOL_RUNNING is false.

[affonsov]

Fixed. All three early-return paths after create_pool_client() now reconstruct and drop the Arc before returning:

unsafe { drop(Arc::from_raw(adapter_ptr as *const ClientAdapter)); }

[valkey-review-bot]

A single AtomicBool plus one UnmarkOnDrop per request is not safe when requests overlap on the same multiplexed pooled client. For example, two concurrent BLPOPs both store true; when the first future completes, its guard stores false while the second is still blocking, allowing the abandon monitor to reclaim the client. Node and FFI have the same last-writer-wins behavior. Track an active-blocking count (increment before dispatch, decrement on drop, and let the monitor test count > 0) rather than independently clearing a shared boolean.

[affonsov]

Safe by design: the pool enforces exclusive borrow — only one caller holds a pooled client at a time via try_acquire/release_client_async. Concurrent blocking commands on the same pooled client are not possible through the pool API. This invariant is documented in the BLOCKING_FLAG_REGISTRY doc comment.