Skip to content

fix: raise ConnectionError instead of segfaulting on a closed client - #224

Merged
Aryex merged 3 commits into
release-1.0from
jamesx/fix-212-closed-client-guard
Aug 7, 2026
Merged

fix: raise ConnectionError instead of segfaulting on a closed client#224
Aryex merged 3 commits into
release-1.0from
jamesx/fix-212-closed-client-guard

Conversation

@jamesx-improving

@jamesx-improving jamesx-improving commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Calling pipelined, multi, eval, evalsha or invoke_script on a client that has already been closed hard-killed the Ruby VM with SIGSEGV (exit 139) instead of raising a rescuable error. Bindings.batch and Bindings.invoke_script passed @connection straight into the native layer with no nil/null guard — only send_command had one — so the core dereferenced a NULL pointer. This adds a single connection! guard used by every native call site that takes the client handle, and makes close idempotent.

Two things a reviewer should read before commenting — both contradict an earlier revision of the issue:

1. The mutex was rejected deliberately. An earlier revision of #212 (and the #216 Group 6 row for R3-10) prescribed "add a mutex around @connection access". This PR does not do that, on purpose: Mutex#synchronize raises ThreadError: can't be called from trap context, which breaks Signal.trap("TERM") { client.close } — the standard graceful-shutdown idiom — and leaks the native handle when it raises. Verified: that idiom works on release-1.0 and with this fix, and raises ThreadError as soon as a mutex is introduced. test_close_works_in_trap_context locks the behavior in. Idempotency comes from clear-then-free ordering in close, not from a lock.

2. No follow-up is needed for in-flight commands. An earlier revision of the issue claimed a residual window requiring refcounting. That is wrong — glide-ffi already refcounts: every command entry point does Arc::increment_strong_count before Arc::from_raw, and close_client only decrements (ffi/src/lib.rs:2513, whose own upstream comment notes the count reaches zero "once all client requests are done"). Client creation uses Arc::into_raw, so the raw pointer owns exactly one strong ref that close_client consumes. The native ClientAdapter therefore outlives any request still executing. There is no follow-up task here.

Issue link

Closes #212

This closes both halves of #216 Group 6: R3-10b (the sequential close-then-use crash) and R3-10 (the concurrent close-vs-command TOCTOU plus concurrent double-close). One fix covers both, because connection! reads the handle into a local before use — collapsing the race window as well as covering the sequential path. Both halves are now test-backed rather than only probe-verified.

Features / Changes

  • lib/valkey.rb — new private connection! helper: reads @connection into a local and raises Valkey::ConnectionError, "the client is closed" if it is nil or null, otherwise returns the local. Message matches the sibling GLIDE clients (Go ClosingError, Node/Java ClosingException).
  • lib/valkey.rbsend_command and send_batch_commands now go through connection! and pass the returned local to the native call. In send_batch_commands the check happens before any FFI memory is allocated, so a closed client fails fast.
  • lib/valkey/commands/scripting_commands.rbinvoke_script does the same, likewise before allocating FFI buffers. This covers eval, evalsha, and the _ro variants, which all funnel through it.
  • lib/valkey.rbclose is idempotent by construction: @connection is cleared before the handle is freed, so a second (or concurrent) close sees nil and does nothing instead of double-freeing.
  • test/valkey/connection_lifecycle_test.rb — 9 new tests.

Completeness: all 22 attach_function declarations in lib/valkey/bindings.rb were audited. Exactly 5 take the client handle — close_client, command, command_with_route_info, batch, invoke_script — and all 5 are now guarded. Before this change only send_command (command / command_with_route_info) had a guard, leaving batch and invoke_script exposed.

Out of scope: p.eval inside a pipelined block raises NoMethodError, a pre-existing wart unaffected by this change (only the message text differs, build_command_args -> connection!).

Testing

Against a dedicated Valkey 8.1.3 on port 6412, Ruby 4.0.6 arm64-darwin25.

  • bundle exec rubocop — 99 files inspected, 0 offenses.
  • Baseline, pristine fb2fa46: 840 tests, 4155 assertions, 0 failures, 0 errors, 100 skips.
  • With this change: 849 tests, 0 failures, 0 errors, 100 skips, reproduced twice. 840 -> 849 is exactly the 9 new tests; skip count identical.
    CI=1 SKIP_TLS_TESTS=true VALKEY_PORT=6412 bundle exec rake test:standalone
    

New tests: closed-client raise on pipelined, multi, eval, evalsha and invoke_script; close idempotent; concurrent close frees the handle once; close racing in-flight commands does not crash; close works in a trap context.

Negative controls — the tests actually catch the bug:

  • With lib/ reverted and the new tests kept, the guard tests segfault (exit 139) — precisely the crash this PR fixes.
  • test_close_racing_in_flight_commands_does_not_crash fails 3/3 without the fix, clean 3/3 with it.
  • pipelined racing close survives 5/5 with the fix vs. segfaulting 3/3 without.

Deferred: cluster-mode tests were not run (no local cluster available). The new tests are skip-guarded on cluster_mode?, so they are inert there; the change itself is mode-independent. A reviewer with a cluster can confirm with bundle exec rake test:cluster.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • Documentation is updated (if applicable).
  • Linters have been run (bundle exec rubocop) and pass.
  • Destination branch is correct - main

@Aryex
Aryex requested review from Aryex and Sasidharan3094 August 4, 2026 23:29
@Aryex Aryex assigned Aryex and jamesx-improving and unassigned Aryex Aug 4, 2026
@nderraugh
nderraugh self-requested a review August 5, 2026 20:59
Comment thread lib/valkey.rb Outdated
Comment thread lib/valkey.rb Outdated
Comment thread lib/valkey.rb
Comment thread test/valkey/connection_lifecycle_test.rb
jamesx-improving added a commit that referenced this pull request Aug 6, 2026
…ession

Two threads that both read @connection before either nulled it would both
call Bindings.close_client and double-decrement the Arc refcount - UB per
Rust. Serialize close with @close_lock.try_lock: exactly one caller frees
the handle, the rest short-circuit. try_lock (not synchronize) is chosen
so Signal.trap("TERM") { client.close } still works - only lock/synchronize
raise ThreadError from a trap context.

Add a TracePoint-based regression test (contributed by @nderraugh on PR
 #224) that pauses one thread's close between the ivar read and the ivar
clear, runs a second close to completion, then releases the first, and
asserts close_client is invoked exactly once.

Also trim the connection! docblock per reviewer request.

Signed-off-by: James Xin <james.xin@improving.com>
@jamesx-improving
jamesx-improving force-pushed the jamesx/fix-212-closed-client-guard branch from 9beb934 to e627610 Compare August 6, 2026 16:33

@Aryex Aryex left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small nit to reduce the doc strings.

Comment thread lib/valkey.rb Outdated
Comment thread lib/valkey.rb Outdated
`Bindings.batch` and `Bindings.invoke_script` passed `@connection`
straight into the native layer with no nil/null guard, so `pipelined`,
`multi`, `eval`, `evalsha` and `invoke_script` after `close` made the
core dereference a NULL pointer and killed the whole Ruby VM with
SIGSEGV instead of raising something Ruby code could rescue. Only
`send_command` had a guard.

Adds a private `connection!` helper that reads `@connection` into a
local and raises `Valkey::ConnectionError, "the client is closed"` when
the handle is nil or null, and routes every native call site that takes
the client handle through it: `send_command`, `send_batch_commands` and
`invoke_script`. All 22 `attach_function` declarations in
`lib/valkey/bindings.rb` were audited; exactly 5 take the client handle
(`close_client`, `command`, `command_with_route_info`, `batch`,
`invoke_script`) and all 5 are now guarded.

Because `connection!` returns the handle into a local before the native
call, the check-then-use window against a concurrent `close` is closed
too, not just the sequential close-then-use path. `close` is made
idempotent by clearing `@connection` before freeing it, so a second or
concurrent `close` sees nil and does nothing.

Deliberately does not add a Mutex around `@connection`.
`Mutex#synchronize` raises `ThreadError: can't be called from trap
context`, which would break the standard
`Signal.trap("TERM") { client.close }` graceful-shutdown idiom and leak
the native handle when it raised. Idempotency comes from the
clear-then-free ordering instead, and `test_close_works_in_trap_context`
locks that behavior in.

Adds 9 tests in `test/valkey/connection_lifecycle_test.rb` covering the
closed-client raise on each entry point, idempotent and concurrent
`close`, `close` racing in-flight commands, and `close` from a trap
context.

Closes #212

Signed-off-by: James Xin <james.xin@improving.com>
…ession

Two threads that both read @connection before either nulled it would both
call Bindings.close_client and double-decrement the Arc refcount - UB per
Rust. Serialize close with @close_lock.try_lock: exactly one caller frees
the handle, the rest short-circuit. try_lock (not synchronize) is chosen
so Signal.trap("TERM") { client.close } still works - only lock/synchronize
raise ThreadError from a trap context.

Add a TracePoint-based regression test (contributed by @nderraugh on PR
 #224) that pauses one thread's close between the ivar read and the ivar
clear, runs a second close to completion, then releases the first, and
asserts close_client is invoked exactly once.

Also trim the connection! docblock per reviewer request.

Signed-off-by: James Xin <james.xin@improving.com>
@jamesx-improving
jamesx-improving force-pushed the jamesx/fix-212-closed-client-guard branch from e627610 to 8bc4d35 Compare August 6, 2026 22:16
Signed-off-by: James Xin <james.xin@improving.com>
@jamesx-improving
jamesx-improving force-pushed the jamesx/fix-212-closed-client-guard branch from 8bc4d35 to 785b913 Compare August 6, 2026 22:17

@yipin-chen yipin-chen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aryex
Aryex merged commit 35f3c86 into release-1.0 Aug 7, 2026
18 checks passed
@Aryex
Aryex deleted the jamesx/fix-212-closed-client-guard branch August 7, 2026 02:00
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.

4 participants