Skip to content

fix: drain in-flight commands before close frees the handle - #283

Draft
Aryex wants to merge 1 commit into
release-1.0from
alexl/fix-212-close-drain
Draft

fix: drain in-flight commands before close frees the handle#283
Aryex wants to merge 1 commit into
release-1.0from
alexl/fix-212-close-drain

Conversation

@Aryex

@Aryex Aryex commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

Close-vs-close was already serialized via @close_lock (#224), but close-vs-in-flight-command was not. A thread inside Bindings.command / Bindings.batch / Bindings.invoke_script — all blocking: true, so they release the GVL and run in native code — could still hold the raw client handle when another thread's close freed it via Bindings.close_client, tearing down the connection under a live dispatch.

Observed in CI as a malloc(): unaligned tcache chunk detected SIGABRT. This is issue #212, race #2 (distinct from the close/close double-free already fixed).

Fix

A drain protocol guarded by @close_lock:

  • @inflight counter + @drain_cv ConditionVariable, initialized alongside @close_lock in connection!.
  • send_command, send_batch_commands, invoke_script bracket their FFI dispatch with acquire_connection_slot / release_connection_slot. The lock is held only around the counter bump/decrement — never across the FFI call — so concurrent in-flight commands are not serialized (glide-core supports concurrent in-flight commands per client).
  • close is two-phase: phase 1 sets @closing under try_lock (trap-safe); phase 2 waits on @drain_cv until @inflight reaches zero, then nulls the handle and calls close_client. Degrades to a lock-free best-effort close on ThreadError so Signal.trap("TERM") { client.close } shutdown still works.

acquire_connection_slot refuses new slots once @closing is set — this is what makes the drain finite. connection! is retained for the fast fail-fast path.

Testing

  • New test_close_drains_in_flight_commands_no_crash: many worker threads race commands against a concurrent close; asserts the client ends cleanly closed (the client is closed) with a balanced @inflight counter.
  • bundle exec rubocop — clean on all 3 files.
  • All 14 close/lifecycle standalone tests pass, including the pre-existing test_close_racing_in_flight_commands_does_not_crash.

Note: same underlying race would affect Go / Python-sync (shared FFI stack); a close_client-drains-in-flight guarantee in libglide_ffi would be the more complete fix. This PR addresses the Ruby side.

Closes #284

#2)

Close-vs-close was already serialized via `@close_lock`, but
close-vs-in-flight-command was not. A thread inside `Bindings.command` /
`Bindings.batch` / `Bindings.invoke_script` (all `blocking: true`, so they
release the GVL and run in native code) could still hold the raw client
handle when another thread's `close` freed it via `Bindings.close_client`,
tearing down the connection under a live dispatch. Observed in CI as a
`malloc(): unaligned tcache chunk detected` SIGABRT.

Add a drain protocol guarded by `@close_lock`:

- `@inflight` counter + `@drain_cv` ConditionVariable, initialized alongside
  `@close_lock` in `connection!`.
- `send_command`, `send_batch_commands`, and `invoke_script` bracket their
  FFI dispatch with `acquire_connection_slot` / `release_connection_slot`.
  The lock is held only around the counter bump/decrement, never across the
  FFI call, so concurrent in-flight commands are not serialized.
- `close` is two-phase: phase 1 sets `@closing` under `try_lock` (trap-safe),
  phase 2 waits on `@drain_cv` until `@inflight` reaches zero, then nulls the
  handle and calls `close_client`. Degrades to a lock-free best-effort close
  on `ThreadError` so `Signal.trap` shutdown still works.

`acquire_connection_slot` refuses new slots once `@closing` is set, which is
what makes the drain finite. `connection!` is retained for the fast
fail-fast path.

Add `test_close_drains_in_flight_commands_no_crash`, which races many worker
threads issuing commands against a concurrent `close` and asserts the client
ends up cleanly closed with a balanced in-flight counter.

Signed-off-by: Alex Le <alex.le@improving.com>
@Aryex Aryex changed the title fix: drain in-flight commands before close frees the handle (#212, race #2) fix: drain in-flight commands before close frees the handle Aug 15, 2026
@Aryex Aryex added estimate: 2 Relative effort: medium (1–2 days) and removed estimate: 2 Relative effort: medium (1–2 days) labels Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant