feat: assembly-owned codec routing (clean port of #320) - #386
Conversation
Selectively port the final assembly-owned Codec routing design from PR #320 (fix/311-codec-scope-routing) onto dev. Per-Contract/API-5 transitional machinery (ContractCodecSets, per-Contract publication, same-assembly per-Contract policy splits) is deliberately not carried over; the PR body's final ownership contract is the spec. - Public API: assembly-only RpcCodecRoute/RpcCodecScope; singular ContractCodecs/ContractDependencies on the generated assembly manifest; IRpcContractCodecProviderResolver; RpcGeneratedCodecFactoryKind; codec-aware IRpcClientStreamSink overload. - Generator: assembly-owned final Codec graph (explicit > route > default), frozen graph published in the assembly manifest, proxy/stub construction-time binding, and implicit final-selection identities (Native/UnsafeBlit) materialized into format-2 compatibility manifests so UnsafeBlit <-> Adapter transitions are wire breaks. - Runtime: RpcManifestCodecProvider as the per-assembly owner provider, RpcGeneratedCodecResolver, assembly-level manifest structure validation, and the request-level stream drain barrier. - Client/Server: proxy/stub registration binds the owning assembly provider; multi-cluster dependency closure includes ContractDependencies; builder UseCodec<T> removed; dynamic replacement validates assembly-owned dependency closure. - Tests: assembly-owned routing/compatibility/ownership suites ported; per-instance codec-independence and same-assembly split tests removed; fixture projects are real generated contract assemblies. Local validation (net10.0, Release): Unit 1223/1223, Generator 171/171, LoadTest 60/60, Integration 344/344.
SunSi12138
left a comment
There was a problem hiding this comment.
Found two CI/reproducibility blockers around the newly introduced generated fixture projects. I’m leaving these as COMMENT findings rather than REQUEST_CHANGES because this is the author account.
…hots The generated contract fixtures register their manifests into the global catalogs from a module initializer. Lazy mid-suite loads raced the weak-catalog tests that snapshot and restore catalog counts, so force the fixture assemblies to load before any test executes.
SunSi12138
left a comment
There was a problem hiding this comment.
I found three correctness gaps on the exact review head 668ad51:
[P1] Generated [RpcCodec] factories are now classified as Native and rejected at manifest preparation. IRpcGeneratedCodecFactory.Kind defaults to Native whenever AdapterId == null, but AppendCustomCodecFactory does not override Kind. A generated custom Codec therefore reports Kind == Native while carrying its required custom WireFormatId; SharpLinkGeneratedManifestStructureValidator rejects every Native factory whose wire id is not sharplink-native/v1. Existing [RpcCodec] / [RpcCodecImplementation(...)] paths can compile and then fail when the manifest is prepared. The custom factory needs an acquisition kind that matches direct adapter-free construction (likely Direct, unless Custom remains a distinct runtime kind) and corresponding regression coverage.
[P1] [RpcCodec] still bypasses the new Contract-assembly ownership split and is published through the context-global Type -> Codec table. TrySelectCustomCodec executes before TrySelectContractCodecOverride in both the Contract-default and Contract-policy analyses, so a Contract-reachable custom binding is definition-identical in both passes and never becomes a ContractOwnedPolicyRoot. AnalyzeGeneratedCodecsWithPolicyOwnership consequently seeds it into global Codecs, while SharpLinkRuntimeContext merges those registrations from every manifest by CLR Type. Two Contract assemblies that explicitly bind the same external closed T to different custom Codecs will conflict (or collapse into one global registration when identity happens to match), contrary to the assembly-owned invariant (Contract assembly, T) -> one final binding. Treat Contract-reachable [RpcCodec] provenance as assembly-owned policy just like explicit RpcCodecAdapter/route, while keeping standalone [RpcSerializable] behavior separate.
[P1] Generated manifests never publish ContractDependencies; Contract-only dependencies are folded into normal Dependencies. The new runtime/multi-cluster code explicitly distinguishes Dependencies from ContractDependencies so a dependency-only manifest view can hide RPC policy closure, but GenerateAssemblyManifest currently concatenates contractCodecs.SelectMany(AssemblyDependencies) into __dependencies and emits no ContractDependencies override at all. Therefore includeContractPolicyDependencies: false cannot actually prevent generated Contract-policy-only dependencies from being expanded. A valid multi-cluster dependency view can still require/load (or fail on) a dependency that exists only for a hidden Contract Codec graph. Emit separate normal and Contract-policy dependency arrays, and add a generator-backed regression rather than only a hand-authored DependencyManifestView test.
The assembly-level route/public API, owner canonicalization (ContractType -> ContractType.Assembly), and proxy/stub/stream construction-time binding otherwise look aligned with the updated #167/#311 ownership model.
Address the review findings on #386: - classify adapter-free non-native generated factories as direct/custom construction - keep Contract-only [RpcCodec] bindings out of the global graph while freezing them into ContractCodecs - publish ContractDependencies as the RPC-only dependency delta - add generator regressions covering custom ownership, standalone behavior, and dependency separation
Keep Generator.Tests independent from the real Abstractions project. The generator-only ownership/dependency regressions remain there, while the real IRpcGeneratedCodecFactory default-kind behavior is covered by UnitTests.
SunSi12138
left a comment
There was a problem hiding this comment.
[P1] Preserve or bump the API-4 generated descriptor ABI
This PR still publishes SharpLinkGeneratedManifestVersions.Api == 4 and AbiIdentity == "sharplink-2.0-api4-rpcchannel-metadata-v2", and runtime compatibility accepts that exact locator identity. However, SharpLinkGeneratedContractDescriptor no longer exposes the constructor signature that exists on the dev base and that API-4 generated binaries can reference:
(...,
Func<IRpcChannel, object> proxyFactory,
Func<IRpcCodecProvider, IRpcStub> stubFactory)The new primary constructor is (Func<IRpcChannel, IRpcCodecProvider, object>, Func<IRpcCodecProvider, IRpcStub>), while the added compatibility overload is (Func<IRpcChannel, object>, Func<IRpcStub>). That overload does not satisfy an existing binary newobj reference to the old API-4 signature.
As a result, a stale/generated assembly compiled against the PR base can carry the unchanged API-4 ABI locator, pass SharpLinkGeneratedManifestCompatibility, and then fail with MissingMethodException / type-initializer failure when the manifest initializes instead of being rejected at the ABI boundary.
Please either retain the exact old constructor overload (with the old provider-aware stub factory signature) or bump AbiIdentity and regenerate/validate against the new ABI. A binary/stale-generated-artifact regression would be more valuable here than another source-recompile test.
SunSi12138
left a comment
There was a problem hiding this comment.
[P1] Do not turn ordinary custom-Codec implementation binaries into generated Contract dependencies
The new ContractDependencies split is now emitted correctly, but the custom [RpcCodec] path still computes its generated-module dependency closure from both the payload and customCodec.CodecType (GetAssemblyDependencies([type, customCodec.CodecType])). The new regression explicitly expects a normal ReviewPayloadCodecs helper assembly to appear in ContractDependencies.
That is not just metadata: client/server dynamic registration treats every Dependencies / ContractDependencies identity as a SharpLink generated module that must already be present in _staticManifests or _dynamicModules, and multi-cluster AddManifestClosure likewise requires a manifest for every identity. A normal helper DLL containing only a public IRpcCodec<T> + [RpcCodecImplementation] has no generated Contract/service/DTO root, so the generator emits no ISharpLinkGeneratedAssemblyManifest for that assembly. A Contract assembly that references that helper can therefore compile, but dynamic registration or multi-cluster routing fails with MissingDependency / missing generated dependency even though the CLR dependency is present and loadable.
This is also inconsistent with the Adapter/direct path, which deliberately excludes implementation binaries from generated-module dependencies and records only payload ownership. The generated custom factory already has a normal static CLR reference to customCodec.CodecType; assembly loading / ALC handles that binary reference without requiring a SharpLink manifest.
Please keep ordinary custom-Codec implementation assemblies out of Dependencies/ContractDependencies (unless the referenced payload itself is owned by a real generated SharpLink module that needs graph registration), and add a runtime/multi-cluster regression using an external Codec helper assembly with no generated manifest.
SunSi12138
left a comment
There was a problem hiding this comment.
[P1] Client unregister still ignores ContractDependencies
The replacement path was updated to use ManifestDependsOn(...), and the server's EnsureNoDynamicDependants also uses that helper, so both normal and Contract-policy dependency edges are protected there. The client unregister path is still on the old check:
candidate.Manifest.Dependencies.Contains(identity, StringComparer.Ordinal)in SharpLinkClient.AssemblyDrain.cs.
That means UnregisterAssemblyAsync on the client can remove/release a dynamic module that is still referenced only through another manifest's ContractDependencies. ReleaseModule then removes the dependency's generated registrations and disposes its manifest registration / Adapter scopes while the dependant module remains running. A dependant owner provider can subsequently fail to resolve the Codec or, if it already cached an Adapter-created Codec tied to that scope, retain a Codec whose generation-owned scope has been disposed.
Please make the client guard use the same ManifestDependsOn(candidate.Manifest, identity) logic as replacement/server, and add a client unregister regression where A has a Contract-only dependency on B: unregistering B must be rejected until A is removed.
SunSi12138
left a comment
There was a problem hiding this comment.
Continuing review on 46e66ad; the previous three findings are fixed. I found two additional generator/runtime boundary gaps:
[P1] Generated-module dependencies are still inferred from ordinary CLR payload ownership, so SDK-using payload libraries without a generated manifest can make valid Contracts impossible to register dynamically. GetArtifactAssemblyDependencies adds every external payload assembly for which ReferencesSharpLinkSdk(assembly) is true, and DTO GetAssemblyDependencies similarly adds any assembly in _allowedAssemblyNames. GenerateAssemblyManifest publishes those identities through Dependencies / ContractDependencies, while client/server dynamic validation treats every such identity as a generated SharpLink module that must already have an adopted manifest. The new regression avoids this only because ReviewPayloads deliberately does not reference SharpLink.Sdk. A shared DTO assembly that references the SDK merely for [RpcMember], a type-level [RpcCodec], or another annotation but has no [RpcSerializable] / Contract / Service root emits no ISharpLinkGeneratedAssemblyManifest; a Contract using one of its types nevertheless records that assembly as a generated dependency and fails with MissingDependency. More generally, explicit owner-side Adapter/custom bindings do not require the payload owner's generated module just because T.Assembly references the SDK. Dependency edges need to represent actual borrowed generated artifacts/Codecs (at minimum require a real generated manifest; ideally derive them from the finalized Codec graph), not normal CLR type ownership. Please add a regression where the external payload DLL references SharpLink.Sdk but intentionally has no generated manifest.
[P1] Native route classification does not consider explicit custom [RpcCodec] as a resolvable dependency, so it can classify a DTO differently from the actual final selector. Visit() selects TrySelectCustomCodec before default/unsupported analysis, so a generated DTO may validly depend on a child that is only serializable because it has a custom Codec. CanResolveContractCodecDependency, used by CanGenerateNativeDto / IsNativeCodecType, checks HasResolvableExplicitAdapter but has no corresponding custom-Codec check. For example, let Envelope be a sealed DTO containing a non-sealed Child, give Child a valid [RpcCodec(typeof(ChildCodec))], and configure only [assembly: RpcCodecRoute(RpcCodecScope.Native, typeof(AdapterN))]. Normal generation can produce Envelope because Visit(Child) stops at the custom Codec, so Envelope's default path is a generated DTO and must classify as Native. The classifier instead treats Child as unresolved, marks Envelope non-Native/Managed, and the Native route never applies. This violates the scope definition (generated DTO -> Native) and makes classification disagree with the real precedence path (custom > Adapter/selector > route > default). Please make the classification probe recognize a valid custom Codec without mutating diagnostics/state, and add a nested custom-Codec + Native-route regression.
SunSi12138
left a comment
There was a problem hiding this comment.
[P1] Assembly-level [RpcCodec(target, codec)] bindings are still imported from referenced assemblies, so one Contract owner can silently inherit another assembly's serializer policy.
CollectAssemblyCustomCodecBindings() starts at the current compilation assembly, recursively walks every allowed referenced assembly, and collects each assembly's two-argument [RpcCodec] attribute into the single _customCodecBindings table. That is different from RpcCodecAdapter target bindings and RpcCodecRoute, which are correctly collected only from _compilation.Assembly.
This violates the owner-compilation rule in #167/#311. An assembly-level target binding is policy for the assembly that declares it; it is not an ambient registration. Example: assembly A declares [assembly: RpcCodec(typeof(SharedT), typeof(CodecA))]. Contract assembly B references A (or any SDK-using library containing that attribute), defines its own [RpcContract] using SharedT, and declares no binding. B's generator currently imports A's attribute and publishes CodecA as B's own final Contract policy. If B explicitly declares CodecB, the two assembly attributes are treated as competing explicit selections and B can fail with CustomCodecSelectionConflict instead of its own owner policy being authoritative.
Type-level [RpcCodec(typeof(...))] on SharedT can remain discoverable through type.GetAttributes() if that is intended as type-intrinsic metadata, but the two-argument assembly-level form should be read only from the current Contract/standalone compilation assembly. This is the same ownership distinction already implemented for assembly-level RpcCodecAdapter and RpcCodecRoute. Please add a two-assembly regression proving B neither inherits nor conflicts with A's assembly-level custom binding.
SunSi12138
left a comment
There was a problem hiding this comment.
[P1] A Native/All route can override builtin primitives, but the higher-precedence explicit binding layer cannot override that route for those same types.
RpcCodecScope.Native is defined as every payload with a deterministic SharpLink native Codec path, and TrySelectRouteAdapter runs before the builtin fallback, so [assembly: RpcCodecRoute(RpcCodecScope.Native, ...)] legitimately routes values such as int, string, Guid, etc. However, CollectAssemblyBindingsWithEnumSupport() still rejects RpcCodecAdapter for every IsNonOverridableBuiltin except enum/nullable-enum, and the assembly-level custom [RpcCodec] path likewise rejects non-overridable builtins. Therefore there is no per-type explicit binding capable of winning over the route for a primitive/native leaf.
That contradicts the advertised selection invariant explicit custom > explicit RpcCodecAdapter/selector > assembly RpcCodecRoute > default and the #311 rule that explicit + matching route is not a conflict because explicit wins. A concrete configuration such as RpcCodecRoute(Native, AdapterA) plus an explicit RpcCodecAdapter(typeof(int), CodecB) is rejected instead of selecting CodecB; there is also no way to express “route all Native values except this builtin, which keeps/uses another final Codec”.
Either Native routes must not be allowed to replace the protected builtin set, or explicit owner bindings for types that a route is allowed to replace must be legal and take precedence. Given RpcCodecScope.All/Native are explicitly intended to cover the native domain, the latter is the consistent model. Please add primitive + Native-route precedence coverage (including fixed request framing, since the explicit/native-route selection changes inline encoding).
Closes #311.
Supersedes #320.
This PR is the clean implementation of assembly-owned final Codec selection/routing on the current
devline.Its scope is intentionally narrow:
The follow-up compatibility/identity model is tracked separately in #396.
Ownership model
The RPC Contract remains the addressing surface, but the Contract assembly is the Codec policy, ownership, publication, and freeze boundary.
All
[RpcContract]interfaces emitted from the same Contract assembly share the same immutable final Codec graph. Two Contracts in the same assembly do not independently select different codecs for the same closedT. Different Contract assemblies may select different bindings for the same configurable CLR type.Generated proxies/stubs bind the owner provider / final Codec graph at construction or registration time. Runtime endpoint configuration cannot rewrite the published RPC wire representation.
Public policy surface
Framework wire primitives have fixed SharpLink wire semantics and are not configurable policy nodes. This includes supported primitive scalars,
string, enum, fixed SharpLink scalar types such asGuid/time identifiers,byte[], and nullable wrappers whose underlying type is itself a framework wire primitive.Ordinary arrays other than
byte[], collections, tuples, DTOs/records, and user structs/classes remain configurable payload types.The configurable surface is:
Selection precedence is:
There is no
Nativeroute scope and no Direct Codec alias throughRpcCodecAdapter.Runtime / generated graph retained in this PR
ContractCodecsgraph;dev.Compatibility / identity boundary
This PR does not define the long-term RPC compatibility identity model.
Existing
SchemaId,WireFormatId, and current generated registration identity mechanisms remain only as pre-existing infrastructure needed bydev. #386 does not extend them into a new per-type compatibility graph.Specifically, #386 removes/does not own:
Code generation that needs to know whether a payload was policy-overridden uses an internal final-selection result, not compatibility metadata.
#396 owns the replacement identity model:
#396 will replace legacy
SchemaId + WireFormatIdidentity plumbing with fixed-width deterministic hashes. NoCodecHash,RpcAssemblyHash, or remote equality implementation is included here.Clean-port / simplification notes
RpcCodecScopeisManaged | Unmanaged | All; noNativescope;IsFrameworkWirePrimitive(T)is the policy boundary, separate from implementation fast-path/native/blit classification;RpcCodecis the only exact handwrittenIRpcCodec<T>binding API;RpcCodecAdapterandRpcCodecRouteare Adapter-only;Validation
Validation will be updated against the final exact review head after the cleanup is green.
Tracked follow-up: #396 — deterministic
CodecHash/RpcAssemblyHashidentity and assembly-level exact-match compatibility.