From 815149f439cfee8b6ce40c85abad3723047e9fd9 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 16:01:21 +0800 Subject: [PATCH] Keep the stderr drain thread off rspec doubles in connect specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine Open3.popen3 stubs handed #connect an instance_double(IO) as stderr. #connect starts a thread that drains stderr, and these examples never close the transport, so the thread outlived them: run during its own example, #each_line was an unexpected message; run during a later one, the double had expired. Both errors are Exceptions, not StandardErrors, so the thread died and Ruby printed the report on $stderr in whichever example was running — which failed main's CI once on Ruby 4.0, inside an `output.to_stderr` matcher of an unrelated example (spec/unit/subprocess_cli_transport_spec.rb:921, run at 07ad108). Ruby 4.0 runs printed the report 26 times in a green run; Ruby 3.x runs printed none. The stubs now return StringIOs for stdout and stderr, as the #read_messages examples in the same file already do, and connect through connect_draining_stderr, which joins the drain (bounded): a double put back fails its own example (21 examples did, with the join and the old doubles) instead of an unrelated later one. spec/README.md gets the convention. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01CNgMt2RjeBLyRP3CWFWNy2 --- spec/README.md | 3 +- spec/support/test_helpers.rb | 13 +++++++ spec/unit/query_run_end_spec.rb | 5 ++- spec/unit/subprocess_cli_transport_spec.rb | 40 +++++++++------------- 4 files changed, 33 insertions(+), 28 deletions(-) diff --git a/spec/README.md b/spec/README.md index 89874211..afbc2cca 100644 --- a/spec/README.md +++ b/spec/README.md @@ -63,7 +63,7 @@ The SessionStore reference adapters under `examples/session_stores/`. The S3 one `spec_helper.rb` requires every file here. -- **`test_helpers.rb`** (included in every example group): `sample_user_message`, `sample_assistant_message`, `sample_assistant_message_with_tool_use`, `sample_result_message` and `sample_system_message` return small message Hashes with Symbol keys, the key style the transport yields (they are trimmed, not copies of real frames); `mock_transport` takes no arguments and returns a double whose `connect`, `close`, `write`, `ready?` and `end_input` are stubbed. An example that needs `read_messages` stubs it on that double itself (`query_run_end_spec.rb` feeds it from a queue). +- **`test_helpers.rb`** (included in every example group): `sample_user_message`, `sample_assistant_message`, `sample_assistant_message_with_tool_use`, `sample_result_message` and `sample_system_message` return small message Hashes with Symbol keys, the key style the transport yields (they are trimmed, not copies of real frames); `mock_transport` takes no arguments and returns a double whose `connect`, `close`, `write`, `ready?` and `end_input` are stubbed. An example that needs `read_messages` stubs it on that double itself (`query_run_end_spec.rb` feeds it from a queue). `connect_draining_stderr(transport)` runs `#connect` over a stubbed `Open3.popen3` and waits for the stderr drain thread it starts. - **`scripted_cli.rb`** — `ScriptedCLI`, the CLI's end of the control protocol in process. `ScriptedCLI.session(options) { |cli, client| ... }` opens a real `Client` on it; `cli.request(...)` sends a control request (`hook_callback`, `can_use_tool`, `mcp_message`) and returns the control response the SDK wrote; `initialize` is answered the way CLI 2.1.286 answers it. - **`fake_claude.rb`** — `FakeClaude`, a stand-in for the `claude` executable: a plain-Ruby child process that speaks the stream-JSON protocol. `FakeClaude.install(dir)` writes the executable and returns its absolute path for `cli_path:`; `FakeClaude.env(scenario:, log:)` selects what the child does (a conversation, a slow exit after stdin EOF, a process that ignores EOF and SIGTERM, stderr lines followed by a failure, an immediate exit); `FakeClaude.events(log)` reads back what the child received and did. - **`callback_exit_harness.rb`** — `CallbackExitHarness`, the child-process side of `callback_process_exit_spec.rb`. @@ -72,6 +72,7 @@ The SessionStore reference adapters under `examples/session_stores/`. The S3 one - `expect` syntax only (`disable_monkey_patching!`), random order, and status persisted to `.rspec_status` for `--only-failures` / `--next-failure`. - An example must not leave anything in `SubprocessCLITransport`'s at-exit process registry: a check in `spec_helper.rb` fails the example that does, and empties the registry for the next one. +- No background thread may read an rspec double. A double expires with its example; a thread that outlives the example dies on its next call, and Ruby prints the report on `$stderr` in whichever example is running then, which fails an `output.to_stderr` matcher there. `#connect` starts such a thread (the stderr drain): an example that stubs `Open3.popen3` returns `StringIO`s for stdout and stderr and connects through `connect_draining_stderr`. - Synchronize on events, never on the clock: a queue, a barrier, the child's own output. `sleep` to "let things settle" makes an example pass or fail with the machine's load. - Bound every wait that a regression could turn into a hang (`task.with_timeout`, `Thread#join(seconds)`, `Queue#pop(timeout:)`), and stop or kill whatever is left waiting, so a hang fails the example instead of the run. CI jobs also have a time limit. - Fixtures should look like what the real CLI sends: same keys, same nesting. Several bugs hid behind hand-simplified frames. diff --git a/spec/support/test_helpers.rb b/spec/support/test_helpers.rb index b7851f6b..f5f652a2 100644 --- a/spec/support/test_helpers.rb +++ b/spec/support/test_helpers.rb @@ -72,6 +72,19 @@ def sample_system_message } end + # #connect over a stubbed Open3.popen3, then wait for the stderr drain it + # starts. An example that never closes its transport leaves that thread + # running: given an rspec double as stderr, it reads it after the example, + # when the double has expired, dies, and Ruby prints the report on $stderr in + # whichever example is running then — inside an `output.to_stderr` matcher, a + # failure (seen on Ruby 4.0). Stub stdout and stderr with StringIOs; the join + # turns a double that comes back into a failure of its own example. + def connect_draining_stderr(transport) + transport.connect + drain = transport.instance_variable_get(:@stderr_task) + raise 'the stderr drain did not finish' if drain && !drain.join(5) + end + # Helper to create a mock transport def mock_transport double('Transport').tap do |transport| diff --git a/spec/unit/query_run_end_spec.rb b/spec/unit/query_run_end_spec.rb index 815c26e5..95f0e730 100644 --- a/spec/unit/query_run_end_spec.rb +++ b/spec/unit/query_run_end_spec.rb @@ -650,10 +650,9 @@ def connect_and_capture_env(options) allow(stdin).to receive(:close) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end - transport.connect + connect_draining_stderr(transport) captured_env end diff --git a/spec/unit/subprocess_cli_transport_spec.rb b/spec/unit/subprocess_cli_transport_spec.rb index e4cfbdf0..43c840b7 100644 --- a/spec/unit/subprocess_cli_transport_spec.rb +++ b/spec/unit/subprocess_cli_transport_spec.rb @@ -773,10 +773,9 @@ def connect_capturing_opts(options) stdin = instance_double(IO, close: nil) allow(Open3).to receive(:popen3) do |_env, *rest| captured_opts = rest.last.is_a?(Hash) ? rest.last : {} - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end - transport.connect + connect_draining_stderr(transport) captured_opts end @@ -812,10 +811,9 @@ def connect_and_capture_env(options) allow(stdin).to receive(:close) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end - transport.connect + connect_draining_stderr(transport) captured_env end @@ -1012,12 +1010,11 @@ def fake_version_probe(output) allow(transport).to receive(:check_claude_version) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end allow(stdin).to receive(:close) - transport.connect + connect_draining_stderr(transport) expect(captured_env['SYMBOL_KEY']).to eq('value') expect(captured_env['STRING_KEY']).to eq('value2') @@ -1035,11 +1032,10 @@ def fake_version_probe(output) allow(transport).to receive(:check_claude_version) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end - transport.connect + connect_draining_stderr(transport) # Pre-fix ||= let the inherited 'cli' win, mis-attributing telemetry. expect(captured_env['CLAUDE_CODE_ENTRYPOINT']).to eq('sdk-rb') @@ -1062,11 +1058,10 @@ def fake_version_probe(output) allow(transport).to receive(:check_claude_version) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end - transport.connect + connect_draining_stderr(transport) expect(captured_env['CLAUDE_AGENT_SDK_VERSION']).to eq(ClaudeAgentSDK::VERSION) end @@ -1083,12 +1078,11 @@ def fake_version_probe(output) allow(transport).to receive(:check_claude_version) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end allow(stdin).to receive(:close) - transport.connect + connect_draining_stderr(transport) expect(captured_env['CLAUDE_CODE_ENTRYPOINT']).to eq('custom-entrypoint') end @@ -1105,12 +1099,11 @@ def fake_version_probe(output) allow(transport).to receive(:check_claude_version) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end allow(stdin).to receive(:close) - transport.connect + connect_draining_stderr(transport) expect(captured_env['CLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING']).to eq('true') end @@ -1127,12 +1120,11 @@ def fake_version_probe(output) allow(transport).to receive(:check_claude_version) allow(Open3).to receive(:popen3) do |env, *_args| captured_env = env - [stdin, instance_double(IO, set_encoding: nil), instance_double(IO, set_encoding: nil), - instance_double(Process::Waiter)] + [stdin, StringIO.new, StringIO.new, instance_double(Process::Waiter)] end allow(stdin).to receive(:close) - transport.connect + connect_draining_stderr(transport) # FGTS env var was reverted in Python SDK v0.1.48 due to 400 errors on proxies/Bedrock/Vertex expect(captured_env).not_to have_key('CLAUDE_CODE_ENABLE_FINE_GRAINED_TOOL_STREAMING')