Skip to content

Share one session assembly between query() and Client - #169

Merged
ya-luotao merged 14 commits into
mainfrom
refactor/session-assembly
Oct 6, 2026
Merged

Share one session assembly between query() and Client#169
ya-luotao merged 14 commits into
mainfrom
refactor/session-assembly

Conversation

@ya-luotao

@ya-luotao ya-luotao commented Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

query() and Client are two lifecycles over one protocol, but the session lifecycle was implemented twice in lib/claude_agent_sdk.rb: two Query.new sites fed by the same six extractors, three copies of the receive loop (parse → observers → user block → Break), two copies of the store-backed-resume wiring and of the preserve-or-cleanup decision, and the (observers, scheduling:, wrapper:) triple threaded by hand through every notification call. The duplication itself is the motivation: a lifecycle fix has to be written twice (4d650d2 changed Client's connect/disconnect teardown only; 7d8cb81 changed the shared notification helper; AUDIT-2026-06-12 H4 was an omission shared by both copies).

This PR introduces two @api private classes in lib/claude_agent_sdk/session_assembly.rb:

  • SessionAssembly — shared implementation and resource ownership for both entry points: a resource-free constructor; #connect acquires in the existing order (materialize → repoint → transport → Query.new → mirror → start → handshake), recording each resource before the next fallible step and never rolling back on its own; #write_prompt / #deliver / #close_resources(always_close_transport:) with an explicit switch that preserves today's two disposal shapes. The adapters keep their own rescue/notification boundaries and exception precedence.
  • Dispatch — binds the observer triple once; Client keeps one stable instance whose observer set is replaced on connect, so a receive already running keeps notifying the current connection's observers as on main.

query() becomes the one-shot adapter over the same object. Client#connect_inner, #install_transcript_mirror, #materialize_resume, #notify_error, #writeln, #write are gone. lib/claude_agent_sdk/query.rb, sig/ and docs/ are untouched; the root file shrinks from 1567 to about 1340 lines.

No change to the public lifecycle. Every documented difference between the entry points is preserved and now named in a spec (L1–L8): query()'s own reactor, String prompt then stdin closed at run end, session_id: '' vs 'default', no follow-ups, ask, injected-transport close, Enumerator return, and both failure-path observer sequences (query() → on_error then on_close; Client#connect → on_error alone). verbatim_prompts keeps both of its read timings and reads the effective (materialized) options. Retrying an interrupted materialized-directory removal behaves as on main. One disclosed cleanup difference: if a reconnect happens while the old disconnect is still removing its resume directory, finishing the old disconnect no longer loses ownership of the new session's resume directory, so the new session's disconnect removes it (main left it behind).

How it was verified

  • Commit 0 (e5b2e73) characterizes the lifecycle through the public API only (65 examples, spec/unit/entry_point_lifecycle_spec.rb + harness): both failure-path observer sequences, exception precedence on both paths, wire session ids, partial-setup cleanup, reconnect, disconnect from a callback, the nested double-on_error case, and the verbatim_prompts capture timings including the store-backed-resume variant. Byte-identical and green at every later commit.
  • spec/unit/entry_point_parity_spec.rb (28 examples): query() vs Client#receive_messages over the same fake streaming transport in both scheduling modes with a wrapper; identical observer sequences and teardown decisions except the explicit difference matrix. It also passes against main's implementation.
  • spec/unit/session_assembly_spec.rb at the new interface; spec/unit/client_teardown_interleavings_spec.rb (10 examples) pins the four interleavings the code review reproduced or probed (racing calls during a parked removal, interrupted cleanup then a second disconnect, parked receive across reconnect, reconnect during a parked removal).
  • At head: full suite 3740 examples / 0 failures (baseline 3574); RuboCop clean; rake rbs:validate rbs:test green; floor gemfile (async 2.10.0) and Rails 8 suites green; keyless real-CLI smoke green.
  • Review process (context, in the branch's gitignored .fleet/arch/): four adversarial design rounds with Codex before implementation; two Codex code-review rounds (three blocking findings on round 1, each reproduced against main and fixed in 84d8f82, 34d409f, 616450a; round 2 APPROVE with 96 public traces identical to main).

Notes for review

  • Interaction with Move Hash-form option interpretation into one private module (OptionForms) #167 (refactor/options-hash-normalization): that branch edits the bodies of the three root extractors; this branch deletes their call sites in favour of SessionAssembly#connect. Whichever lands second rebases. Interaction with Extract the run-end decision into Query::RunLifecycle #168 (refactor/run-end-state-machine): Ruby changes do not overlap, but both edit CLAUDE.md and CHANGELOG.md.
  • Follow-up (not done here, drafted in the branch notes): query()'s close path is weaker than Client's (no independent transport-close ensure); adopting Client's shape would be a behaviour fix.
  • Two commit messages (f75684b, 1e84553) say SessionAssembly "notifies nothing"; precisely, it dispatches on_message inside #deliver while the on_error / on_close policy stays in the adapters (CLAUDE.md states it correctly). Commit messages are left as history.
  • Provenance: the commits were written by a Claude Opus 5.5 implementer session working from a settled spec under this orchestrating session's direction; the commit trailers name the orchestrating session.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm

ya-luotao and others added 10 commits October 6, 2026 03:31
@ya-luotao @claude
Characterize the entry-point lifecycle through the public API
e5b2e73 
query() and Client run one lifecycle whose implementation is written
twice. Before any of it moves, pin what both do today, through the
public API only, over an in-memory CLI and a recording observer:

- the lifecycle differences L1-L8 (own reactor, String and Enumerable
  prompts, one run, ask, injected transports, the block-less Enumerator,
  what observers are told when the session cannot be opened);
- which error wins when the cleanup after a failed connect fails too;
- the session ids on the wire;
- the two moments verbatim_prompts is read, including that a materialized
  store-backed resume reads the repointed copy of the options;
- what is disposed of after a setup that fails part-way;
- which transports get a materialized resume, and what happens to the
  temp dir on disconnect;
- reconnecting, disconnecting from inside a callback, the double
  on_error of a query nested in a receive block;
- a caller's deadline expiring while on_error runs, per scheduling mode.

The deadline is delivered once the observer signals that it is running
with the handshake rejection, the way an expired task.with_timeout
delivers it (the scheduler raising into the waiting fiber), so no private
method has to be wrapped to arm it and slow setup cannot use it up.

None of these examples reads an instance variable or calls a private
method, so they stay as they are while the implementation is extracted.
No lib change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Bind the observer dispatch triple once in Dispatch
e21086f 
Every notification and every user-block invocation in query() and Client
passed (observers, scheduling:, wrapper:) by hand: eleven notify_observers
calls, two observing_prompt_stream calls and three invoke_iteration calls,
with Client holding the triple as three instance variables and query() as
three locals. A call site that forgot one of the two keywords would run a
callback on the wrong execution context without failing anything.

Dispatch holds the triple and offers the three calls. It is a context
holder: each method is the module function it names, called with the
triple, so notify_observers' containment and the Break bridging are
untouched. Client builds an empty one in its constructor (scheduling and
wrapper are captured there, as before) and swaps in the observers it
resolves on each connect.

Client#notify_error stays for now as a one-line delegate; a spec still
wraps it, and both go together with the ownership change.

No behaviour change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Move Client's session resources into SessionAssembly
f75684b 
Client acquired its transport, built its Query, installed the transcript
mirror, wrote prompts, ran the receive loop (twice) and tore everything
down through six private helpers and four instance variables. query() does
the same things with its own copy of each, and every lifecycle fix so far
had to be made twice or was made once.

SessionAssembly now owns the transport, the Query and the materialized
resume dir of one session, and what is done with them:

- #connect acquires in the documented order, recording each resource
  before the next step that can fail, with no rollback of its own;
- #write_prompt / #write_message serialize and write user messages,
  stamped with the verbatim_prompts value #connect captured (after the
  transport connected, before the Query is built: Client's timing);
- #stream_prompt_in_background spawns Query#stream_input;
- #deliver is the receive loop, with until_result: for receive_response;
- #close_resources(always_close_transport:) closes the Query, the
  transport and decides the temp dir's fate under nested ensures.

It is shared implementation, not a lifecycle: it notifies nothing and
rescues nothing. Client keeps every rescue, every notification and its
connect-error precedence, and delegates inside them. Client#query_handler
becomes a delegating method with the same nil-until-connect meaning.

Everything moves in one commit so that there is no state in which
disconnect cannot find a resource or a prompt has no writer:
connect_inner, install_transcript_mirror, materialize_resume,
notify_error, writeln and write are deleted here, together with the last
caller of each.

The specs that reached the old state move with it, through the public API:

- client_teardown_under_deadline_spec delivers the deadline once on_error
  is running, instead of wrapping notify_error;
- close_from_callback_spec takes the handler from Client#query_handler
  before disconnecting and asserts the outcome through the recording
  transport and the public API;
- session_resume_spec, session_resume_nil_options_spec and client_spec
  drive materialization and its disposal through #connect / #disconnect
  with a recording transport, instead of calling materialize_resume or
  planting instance variables;
- client_spec reads the options a Client runs on from the transport it
  constructs.

Client is now under the class-length limit, so its Metrics/ClassLength
exemption goes (RuboCop reports it as redundant).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Rebuild query() on SessionAssembly
bf8bb81 
query() still carried its own copy of everything Client now gets from
SessionAssembly: resume materialization and repointing, transport
construction, the fifteen-keyword Query.new, the transcript mirror
install, the receive loop and the preserve-or-clean-up decision. A new
Query keyword or a teardown fix had to be written in both places.

query() is now the one-shot adapter over the same object. What makes it
one-shot stays in it, inline and in the same order:

- prompt validation, option configuration and observer resolution before
  the reactor task, and the Async(finished: false) task itself;
- on_user_prompt, then write_prompt(..., session_id: '', verbatim:
  :current): the option is read after the notification, from the options
  the session runs on (the repointed copy once a store-backed resume was
  materialized), exactly where the old code read it;
- the background wait_for_result_and_end_input for a String prompt;
- rescue StandardError -> on_error, and an ensure that fires on_close for
  connect-phase failures too, then close_resources(always_close_transport:
  false): the transport is closed on its own only when no Query was built,
  and a raising close still replaces the original error. Giving query()
  Client's stronger close is a behaviour change for a follow-up.

An injected transport is passed as { instance: } and gets no
materialization; without one the session constructs SubprocessCLITransport
after materializing, as before.

The characterization spec passes unchanged. There is now one Query.new,
one receive loop, one mirror install and one temp-dir decision in lib/.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Test the session assembly at its interface, and both entry points sid…
7b3b856 
…e by side

The mapping from options to the Query, the order of acquisition and the
disposal rules were each tested twice, once through query() and once
through Client, by capturing Query.new keywords behind doubles: twenty-six
examples in two files that had to be kept in step by hand, and that said
nothing about whether the two entry points agree.

session_assembly_spec.rb is now the one place where

- every one of the fifteen Query keywords is derived from options (hook
  normalization, SDK MCP instances, exclude_dynamic_sections and snapshot
  with false told from nil, the run-end ceiling, the dispatch pair);
- each acquisition step is failed in turn and what was acquired before it
  is shown to be disposed of;
- the eligibility rule for a materialized resume is spelled out per
  transport source;
- the three verbatim modes of write_prompt, the receive loop and both
  shapes of close_resources are pinned.

It calls only SessionAssembly's own methods.

entry_point_parity_spec.rb runs one scenario through query() and through
Client.open + query + receive_messages, against the same in-memory CLI and
store-backed resume, under both scheduling modes with a wrapper, and
compares what the caller got, what the block was given, what the observer
was told and what became of the temp dir: a completed run, several
results, a stream that dies, a block that raises or breaks, a mirror that
dropped batches, a deadline during on_message. The differences that are
meant to exist (session_id, stdin close, on_close after a failed connect,
which error wins on a raising close) are listed as such, and nowhere else.
It also passes against the lib of the commit before the extraction.

The duplicated keyword examples go from query_api_spec and client_spec;
each keeps one example that runs a real Query and reads the initialize
request and the prompt off the wire. query_api_spec's inline streaming
transport is replaced by the shared FakeCLI.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Document SessionAssembly as the shared session implementation
1e84553 
CLAUDE.md's "Two API Entry Points" said the difference between query()
and Client is lifecycle, not protocol, but nothing said where the part
they share lives. One sentence now names SessionAssembly (and Dispatch)
as the single implementation of acquisition, prompt writes, delivery and
disposal, states that it notifies and rescues nothing, and points at the
two specs that pin the differences and the sameness. The FiberBoundary
section's pointer to the loop that keeps `break` on the dequeue fiber
follows the loop to SessionAssembly#deliver.

CHANGELOG: an Unreleased "Changed" entry marked internal. No public API,
signature or behaviour changed; sig/ is untouched.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Mark Client disconnected before the materialized dir is removed
84d8f82 
Client#disconnect used to reset its connected state after the transport
close and before it dealt with the materialized resume dir. When the
teardown moved into SessionAssembly#close_resources, the reset moved to
after the whole of it. Removing the directory can take a while (it
retries, sleeping, when the directory is busy) and the reactor runs other
tasks meanwhile, so in that window the client still claimed to be
connected while its handler and transport were already gone:

- query, receive_messages, receive_response and the control methods got
  past the "Not connected" guard and raised NoMethodError on nil, where
  they raised CLIConnectionError before, and the first three notified
  on_user_prompt / on_error for a session that was closed;
- a second disconnect notified on_close a second time.

close_resources now takes a block and calls it once both closes are
behind, before the directory is dealt with, also when a close raised.
Client marks itself disconnected there: the old boundary. query() passes
no block and is unchanged.

The reproduction from review round 1 is pinned through the public API
(client_teardown_interleavings_spec): a real Query and a really
materialized resume, the directory's removal parked, the calls made from
another task, under both scheduling modes with a wrapper.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Keep a session whose teardown raised, so the next disconnect finishes it
34d409f 
Client#disconnect dropped its SessionAssembly in an unconditional ensure.
The assembly clears its materialized resume dir only after disposing of
it, precisely so that a disposal that was cut short can be retried; but
with the owner gone, nothing could reach it any more. When the removal of
the directory was interrupted (the directory busy, and the caller's
deadline expiring or its task being stopped during the wait before the
retry), the next disconnect returned normally and left the directory, and
the copy of the credentials and settings in it, behind. Before the
extraction the client kept @materialized in that case and the second
disconnect removed it.

The session is now let go of only when close_resources returned. When it
raised, the session stays: what was closed is not closed again (the
handler and the transport are nil-ed inside), and what was left is
disposed of by the next disconnect. The disconnected state is still reset
inside the teardown, whatever it raises, and the error that propagates is
the same. A session that a reconnect has replaced in the meantime is not
cleared.

Pinned through the public API with the reproduction from review round 1:
a really materialized directory whose first removal fails with EBUSY, an
interruption delivered during the retry sleep (an expired deadline, and a
cancellation that is not a StandardError), then a second disconnect that
removes the directory; both scheduling modes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Let a running receive notify the observers of the current connection
616450a 
Client resolves its observers on each connect, and its receive loop used
to ask the client for them message by message. SessionAssembly was handed
a Dispatch per connection instead (Client replaced its own on connect),
so a receive that was still running when the client was disconnected and
reconnected kept notifying the observers it had started with. With
observer factories that is visible: the first connection's observer got
an on_message after its on_close, and the new connection's observer never
heard of the message.

Client now keeps one Dispatch for its lifetime and puts the observers it
resolves in on each connect; scheduling and wrapper stay as captured by
the constructor. Each notification reads the observers there are at that
moment, as an Array that is replaced and never changed in place, so a
notification under way keeps iterating the one it began with. The session
holds that same Dispatch. query() builds its own and never replaces its
observers. Dispatch#with_observers, which had no other caller, is gone.

Pinned through the public API with the reproduction from review round 1:
a message block parked across disconnect and connect, observers from a
factory, both scheduling modes with a wrapper.

CLAUDE.md's sentence on SessionAssembly now says what stays with Client
across a teardown, and names the spec.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Rescue StandardError in the parity spec's outcome helper
e7248ea 
The helper rescued Exception under a Lint/RescueException exemption "in
case a deadline is a bare Exception", but every scenario of the parity
spec fails with a StandardError, the deadline's Async::TimeoutError
included. Rescuing what the scenarios raise needs no exemption, and one
of the five RuboCop directives the branch added to spec/ goes with it.

The other four are in entry_point_lifecycle_spec.rb, which rescues
Async::Stop and builds a cancellation that is not a StandardError; that
file is the characterization that has to stay byte-identical, so they
stay.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
This was referenced Oct 5, 2026
Merged
Merged
ya-luotao and others added 4 commits October 6, 2026 05:25
@ya-luotao @claude
Pin a reconnect while the old resume dir is still being removed
c5a8c1a 
Client is disconnected from the moment the removal of its materialized
resume dir starts, so it can be connected again while that removal is
still going. The disconnect that finishes afterwards owns the old session
only: Client#disconnect clears @session only if it is still the session
it tore down. Without that check the late disconnect let go of the new
session — the client then claimed to be connected with nothing behind it,
and the new session's own resume dir, with its credentials copy, was out
of reach of the disconnect that ended it.

Nothing pinned that check: review round 2 found the interleaving with a
probe, not in the committed examples. It is now a public-API example next
to the other teardown interleavings, under both scheduling modes: park
the old directory's removal, reconnect, let the old disconnect finish,
use the new session, disconnect it; the messages, the observer's
notifications and both transport closes are as expected and the new
resume dir is removed. It fails when the check is taken out.

Test only.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao @claude
Qualify the CHANGELOG's behaviour claim for the session assembly
ad8808f 
The Unreleased entry said "No public API, signature or behavior change".
The reviews established one difference from main, and an absolute claim
hides it: when a Client is reconnected while its previous disconnect is
still removing the materialized directory of a store-backed resume, the
old disconnect used to clear the new connection's hold on its own resume
directory when it finished, and that directory was then left behind. The
old disconnect now leaves the new session alone, so the new session's
directory is removed by its own disconnect
(client_teardown_interleavings_spec pins it).

The entry now says what is preserved — public signatures and the
documented differences between the two entry points — and names that one
cleanup difference. It makes no claim about retrying an interrupted
removal: a later disconnect finishing it is what main already did.

Two wordings the PR-level review flagged as imprecise are tightened with
it. The entry said the observers are "bound once"; scheduling and wrapper
are captured by Client's constructor, the observers are resolved on each
connect. CLAUDE.md said SessionAssembly "notifies nothing"; it never
notifies on_error or on_close, and it does dispatch on_message. CLAUDE.md
also no longer says the parity spec pins "everything else" as identical:
it pins the scenarios it runs.

Docs only.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
@ya-luotao
Merge remote-tracking branch 'origin/main' into refactor/session-asse…
8320df7 
…mbly

# Conflicts:
#	CHANGELOG.md
#	lib/claude_agent_sdk.rb
@ya-luotao
Merge remote-tracking branch 'origin/main' into refactor/session-asse…
2ee99a6 
…mbly

# Conflicts:
#	CLAUDE.md
@ya-luotao
ya-luotao merged commit d8215fb into main Oct 6, 2026
12 checks passed
ya-luotao added a commit that referenced this pull request Oct 6, 2026
@ya-luotao @claude
Bump version to 1.2.1 and cut CHANGELOG
184ea6c 
Internal release: OptionForms (#167), Query::RunLifecycle (#168) and
SessionAssembly (#169) each move duplicated or scattered logic into one
private module; no public API or signature change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ya-luotao