refactor(sessions): decompose LlmSessionActor into transient-state handlers (steps 1–4)#1496
Merged
Aaronontheweb merged 6 commits intoJun 25, 2026
Conversation
…InFlightTurnDedup Convert reminder/background-job identities from string to the existing ReminderId/BackgroundJobId value objects along the dedup + journaling path (MessageSource, ChannelInput, SessionState dedup ledgers, TurnRecorded event, TurnCompleted output, channel reminder-delivery observer dictionaries). Value objects map to the existing bare-string proto/DTO fields at the serialization boundary, so persisted journals/snapshots and the SignalR wire form are byte-identical (proven by new SerializationRoundTripTests). Extract the in-flight reminder/job dedup state out of LlmSessionActor into a new Sessions/Handlers/InFlightTurnDedup handler, mirroring ActiveExecutionTracker.
Move the transient model-input media buffer out of LlmSessionActor into a new Sessions/Handlers/ModelInputMediaBuffer. Accumulation, drain-on-batch-complete, and reset are behavior-preserving; AddModelInputMediaNudge (which mutates _state) stays in the actor. DrainSnapshot returns the buffered references and clears in one call, collapsing the prior nudge-then-clear pairs.
Move the explicit SessionPhase field and legal-transition validation out of LlmSessionActor into a new Sessions/Handlers/SessionPhaseMachine. TransitionTo now delegates validation to _phase.TryTransition, but the Become() switch, processing-state emission, and observer notification stay in the actor so phase tracking and behavior remain co-located (no drift). Drops the now-redundant IsLegalTransition wrapper.
DiscoveredToolCache now owns the exposed tool list and base-tool count, removing the external-state-passing where LlmSessionActor handed its list to EvictAll/PrepareForNewTurn on every call. The actor seeds base tools once (SeedBaseTools), reads the list via AvailableTools, and adds discovered tools via AddIfMissing (returns whether it added, so the actor keeps its log line). Tool-access-policy orchestration (TryActivateDiscoveredTool, ResolveExposedToolsForCurrentTurn) stays in the actor: dragging the registry, access policy, and trust context into the cache would be plumbing without a win.
Cleanup follow-up to the LlmSessionActor decomposition (no behavior change): 1. Repoint three stale comments that named the deleted _pendingModelInputMediaReferences field at the now-correct ModelInputMediaBuffer / DrainSnapshot mechanism (SessionState.BuildNudgeMessage + two regression-test comments). 2. ModelInputMediaBuffer.DrainSnapshot now hands off its backing list and adopts a fresh one (swap) instead of ToArray-copying; BuildNudgeMessage's existing defensive copy remains the single copy, restoring the pre-refactor 1-copy cost. 3. InFlightTurnDedup collapses its duplicated reminder/job trios into one generic InFlightSet<T>, so the reserve-ignores-empty / complete-doesn't guard asymmetry lives in one place and can't drift. 4. ReminderDeliveryResult.ReminderDeliveryKey is now ReminderId (was string), closing the last stringly-typed seam on the reminder-delivery path: the four binding actors stop unwrapping with .Value, ReminderExecutionActor compares typed keys, and tests drop the .Value.Value double-unwrap. The message is INoSerializationVerificationNeeded (transient, never journaled), so this has zero persistence/wire impact.
Aaronontheweb
added
the
cleanup
Aaronontheweb
commented
Jun 25, 2026
Aaronontheweb
left a comment
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
Goal here is to move a bunch of plumbing out of the LlmSessionActor and introduce value objects for reminder / background job ids so we can avoid primitive confusion.
Aaronontheweb
enabled auto-merge (squash)
June 25, 2026 18:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Incremental, behavior-preserving decomposition of the 4,768-line
LlmSessionActor, moving loose transient state into small owned handler objects (the establishedSessions/Handlers/*idiom) and closing a stringly-typed seam. Net effect on the actor: −43 lines and several loose fields/methods replaced by cohesive handlers.Steps (one commit each)
InFlightTurnDedup— reminder/background-job IDs become the existingReminderId/BackgroundJobIdvalue objects end-to-end (MessageSource,ChannelInput,SessionStatededup ledgers,TurnRecorded,TurnCompleted). They map to the same bare-string proto/DTO fields, so old journals/snapshots still deserialize (manifest strings + proto field numbers unchanged) — proven by newSerializationRoundTripTests. In-flight dedup state moves into a newInFlightTurnDeduphandler.ModelInputMediaBuffer— the transient model-input media buffer moves into its own handler;AddModelInputMediaNudge(which mutates_state) stays in the actor.SessionPhaseMachine— phase + legal-transition validation move into a handler; theBecome()switch stays co-located in the actor so phase and behavior can't drift (illegal transitions still throw the same exception).DiscoveredToolCache— the cache now owns the exposed tool list it already mutated, removing the awkward external-state-passing (EvictAll/PrepareForNewTurnno longer take the list).Plus a code-review fixups commit (stale-comment repointing, a redundant-copy fix in
DrainSnapshot, generic-ifyingInFlightTurnDedup, and typingReminderDeliveryResult.ReminderDeliveryKeyasReminderId).Invariants held
Timers/Context/Self/_state/CTS) and never callPersist/Become.Out of scope (deferred to a later test-first effort): the approval subsystem and the passivation/job-reap/restart-drain handshake.
Testing
Netclaw.Actors.Tests: 2484 passedNetclaw.Daemon.Tests: 742 passed (covers the SignalR reminder-delivery path)dotnet slopwatch analyze: clean; copyright headers verified