refactor(server): simplify resource ownership and package boundaries - #11
Merged
Merged
Conversation
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
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.
The server previously coordinated each resource through global registries, owner indexes, operation managers, and a cross-resource lifecycle manager. This cleanup scopes resource storage to each logical Connect connection and puts creation and teardown on the resource owners. Multiple logical connections may continue sharing one transport.
Ownership and removed abstractions
Adapter and failure boundaries
Keep all five gRPC service adapters. Replace core.Context/operationContextFactory with ordinary contexts and prepareOperation; preserve values, deadlines, cancellation causes, and callback detachment. Pass canonical sources and API options into core, remove input envelopes and identity-bearing snapshots, and group conversions by responsibility within the adapter.
Resource owners construct terminal failures once. Share canonical diagnostic extraction and category serialization while retaining typed immediate errors, joined cleanup errors, cancellation/deadline distinctions, and host-error redaction. panicboundary remains under server/internal and wraps only hosted API calls; option preparation and Wire bookkeeping stay outside it. RPC recovery and detached cleanup retain distinct policies. Constructor panics preserve reusable parents; stateful session/debugger panics prevent reuse.
The shared packages remain pkg/execution (Wire asynchronous states, snapshots, events), pkg/debugger (Wire lifecycle snapshots and ordered events), and pkg/failure (sanitized failures and categories). Client production code and its four exports, cleanup escalation, and borrowed transport policies are unchanged.
Intentional Go API migrations
Replace server.Runtime with api.Runtime and execution.Identity with server.RuntimeIdentity. Both old types are deleted without aliases. Nil and typed-nil runtime rejection remains covered. Handshake-only metadata now belongs to transport configuration. Runtime/listener ownership, protobuf, Universal API behavior, limits, cancellation, and failure semantics are preserved.
Tests and review
Preserve integration/protocol assertions and migrate lifecycle coverage to the new owners. Remove obsolete topology/reflection and controller tests; retain their behavioral coverage in owner, protocol, and public integration tests. Added tests cover shared-transport connection isolation and independent limits, closing connection capacity, parent release racing session publication and abandoned cleanup, every hosted debugger panic boundary, concurrent retained debugger close, detached inspection slices, and operation-context causes/callback cleanup.
Updated README migration examples, architecture/protocol documentation, AGENTS.md, and API comments. Completed the full-diff self-review. It removed leftover duplicate normal-session/execution close coordination, placed registration/reclamation in the store, preserved Execute's existing snapshot behavior, and centralized repeated adapter conversions. Affected validation was rerun after corrections.
Validation
Passed locally on macOS arm64 with Go 1.26.5 (the module's Go 1.25 minimum is unchanged):
Fetched origin/main and verified local main matched 3388848 before the breaking check. Buf required network-enabled execution for its pinned Go tools. Protobuf sources, generated bindings, dependencies, and client implementation are unchanged. No required local gate was skipped. Hosted CI remains responsible for the configured Go 1.25 and Linux/Windows coverage; those environments were not run locally.
Benchmarks
Apple M2 Max, darwin/arm64; medians of three samples, original captured baseline to final code:
Core durable-session samples reported 32–33 allocations. Cancellation's final median is 2.75 ns higher (+4.4%); lookup now checks retained release state. Earlier runs showed a 5–7 ns increase, with unchanged allocation cost. An initial public round-trip slowdown was investigated with an unchanged baseline checkout and five-sample back-to-back runs: median 86,835 → 79,675 ns/op. It did not reproduce in the final run. These small samples are local measurements, not cross-platform performance guarantees.
The baseline's cancellation and core durable-session benchmarks used the old BenchmarkExecutorCancelExecution and BenchmarkExecutorRunDurableSession names. No protocol features, dependency upgrades, compatibility shims, native-runtime integration, or new framework machinery are included.