build(om): wire messaging package CI

[egil]

What changed

• Added package documentation for Egil.Orleans.Messaging and listed it in the repository README.
• Added the om Conventional Commit scope for the Messaging package.
• Included the Messaging README in the NuGet package.
• Added the egil-orleans-messaging GitHub Actions workflow.
• Switched Messaging tests from a sibling Egil.Orleans.Testing project reference to the published NuGet package dependency.

Why

The Messaging package should build and validate as an independent NuGet package boundary. Its tests should consume Egil.Orleans.Testing the same way external consumers do, and CI should validate that boundary on GitHub.

Validation

• dotnet test Egil.Orleans.Messaging\Egil.Orleans.Messaging.slnx -c Release passed: 123 tests.
• dotnet pack Egil.Orleans.Messaging\src\Egil.Orleans.Messaging\Egil.Orleans.Messaging.csproj -c Release --no-restore -p:ContinuousIntegrationBuild=true --output Egil.Orleans.Messaging\artifacts succeeded.
• dotnet meziantou.validate-nuget-package (Get-ChildItem "Egil.Orleans.Messaging\artifacts\*.nupkg") --excluded-rules IconMustBeSet returned IsValid: true.

[egil]

PR review findings

I reviewed the full PR diff against origin/main, including the new package, tests, workflow, docs, and the compiled public API surface. I also checked current Orleans streaming docs/API shape for implicit vs explicit subscriptions: Orleans implicit subscriptions are selected by [ImplicitStreamSubscription] + grain identity/namespace, but the grain still attaches observer logic; explicit subscriptions are durable handles that must be resumed to avoid duplicate subscriptions. See Microsoft docs: https://learn.microsoft.com/en-us/dotnet/orleans/streaming/streams-programming-apis.

Local validation during review: dotnet build Egil.Orleans.Messaging\Egil.Orleans.Messaging.slnx -c Release --no-restore passes.

Findings

P1 - Stream handler exceptions are swallowed, so failed messages can be accepted/lost

StreamManager.OnNextAsync catches every handler exception, records telemetry, invokes/logs onError, and then returns a successfully completed task (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Streams/StreamManager.cs:343, :362, :370). Since StreamObserver<T>.OnNextAsync just returns that task (:523), Orleans sees the stream delivery as completed even when the user handler failed. That is surprising compared to Orleans' IAsyncObserver<T>.OnNextAsync contract, where the returned task is the natural way to surface a failed delivery. It also means a grain can fail before updating MessageTracker, while the stream provider still advances past the event.

The existing test Default_error_handler_does_not_break_subscription_when_handler_throws (Egil.Orleans.Messaging/test/Egil.Orleans.Messaging.Tests/Streams/StreamManagerTests.cs:126) locks in the swallow-and-continue behavior, but I think this should be an explicit error policy, not the default. Suggested shape: default to rethrow after telemetry/logging, and add an opt-in StreamErrorPolicy/disposition callback for users who intentionally want "log and continue" semantics. If onError remains, consider making it async and able to choose whether the exception is handled.

P1 - Missing implicit subscription configuration only logs and returns

If a grain has [ImplicitStreamSubscription] but forgets to call ConfigureImplicitSubscription(...), or configures a different namespace, IStreamManagerComponent.OnSubscribedAsync logs a warning and returns successfully (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Streams/StreamManager.cs:207-:218). That makes a namespace mismatch a runtime log-only problem, and the stream activation can proceed without attaching any observer. This is the same class of misconfiguration as a missing StreamManager component, which already throws in IImplicitStreamGrain (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Streams/IImplicitStreamGrain.cs:13-:20).

I would fail fast here with an InvalidOperationException that includes the provider and stream id/namespace. A warning is too easy to miss for a message-delivery path.

P2 - ConfigureExplicitSubscription looks like Orleans explicit streams, but only supports grain-keyed streams

The public API takes only (streamProviderName, streamNamespace, handler) (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Streams/StreamManager.cs:124-:148). Internally, it always derives the stream id from the owning grain (:317-:323, :439-:452). That is narrower than Orleans explicit subscriptions, where the caller can subscribe to any StreamId from an IStreamProvider. It also makes the API hard to use for common explicit-stream cases: subscribing to a stream keyed by an order/customer id, fan-in from multiple streams, or using an explicit stream id unrelated to the grain id.

Two concrete API options:

• Add overloads that accept StreamId or Func<TGrain, StreamId>/Func<StreamId>.
• Rename the current overload to communicate the convention, e.g. ConfigureGrainKeyedExplicitSubscription or ConfigureExplicitSubscriptionForGrainKey, and layer the general Orleans-like overload beside it.

Also note that CreateStreamId discards the key extension returned by TryGetGuidKey/TryGetIntegerKey (:441-:449), so compound-key grains cannot round-trip their Orleans identity into the stream id convention.

P2 - C# 14 extension blocks leak generated public nested types into the package API

The extension block declarations (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/OutboxProcessorExtensions.cs:11, Streams/StreamManagerExtensions.cs:11, State/StateManagerExtensions.cs:11, State/StateManagerRegistrationExtensions.cs:11, etc.) compile into exported public nested types with generated names. Reflection over the Release assembly shows public types like:

• Orleans.StreamManagerExtensions+<G>$5588D94CEEAB9159D96D3F2B24A5C17A...
• Microsoft.Extensions.DependencyInjection.OutboxPostmanServiceCollectionExtensions+<G>$7B30185E57B8298C6754F16C0BCF4773...

That is a public API surface concern for a NuGet package with package validation enabled: docs/reflection tools can see these names, and future compiler/lowering changes could look like API churn. If that exposure is acceptable, I would document that decision. If not, use classic extension methods while keeping the owner namespaces (Orleans, Orleans.Hosting, Microsoft.Extensions.DependencyInjection) for discoverability.

P2 - Postman dispatch has ambiguous subtype/base-type behavior

FindPostman returns the first registration whose filter matches (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/OutboxProcessor.cs:364-:371). If a processor registers both AddPostman<IOrderEvent>(...) and AddPostman<OrderSubmitted>(...), the selected handler depends entirely on registration order. The API design doc mentions first-registered-wins (Egil.Orleans.Messaging/api-design.md:1438-:1439), but the public API/README do not make that easy to discover, and it differs from the expectation many users will have from type-specific handler APIs.

I would either reject ambiguous registrations, dispatch to the most-specific registered message type, or document the rule in XML + README directly beside AddPostman<TSub>. The current behavior is easy to misuse when adding a catch-all postman later.

P2 - Duplicate keyed postman registrations are not surfaced

AddOutboxPostman<TMessage, TPostman> and the multi-contract registration path always call AddKeyedScoped (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/OutboxPostmanServiceCollectionExtensions.cs:47-:51, :82-:84). If two postmen register the same (postmanName, IPostman<TMessage>), the container will hold multiple keyed registrations and OutboxProcessor.AddPostman<TSub>(string) resolves a single required service at activation (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/OutboxProcessor.cs:104-:112). That makes a name collision order-dependent instead of a clear configuration error.

Given that postmanName is part of the public routing key, duplicate key+message registrations should probably fail during service registration or at least be detected during validation.

P2 - Public docs promise an outbox maximum that does not exist

The Outbox<T> XML docs say unbounded growth is mitigated by a configurable max via OutboxProcessorOptions<TOutbox>, with oldest messages auto-dropped FIFO (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/Outbox.cs:48-:51). The API design doc says the same (Egil.Orleans.Messaging/api-design.md:450-:458). OutboxProcessorOptions<TOutbox> has no max/dead-letter/drop property (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/OutboxProcessorOptions.cs:30-:83).

This is a public contract mismatch. Either implement the cap explicitly, or remove the promise from XML/design docs and state that cap/dead-letter policy remains the grain's responsibility.

P3 - ThreadPool execution mode is under-documented in XML docs

The API design correctly warns that ThreadPool postmen must not read or mutate activation-local grain state (Egil.Orleans.Messaging/api-design.md:1249-:1254), but the public XML docs only say it executes callbacks on the .NET thread pool (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Outboxes/OutboxProcessorOptions.cs:14-:18). Since this is a runtime safety boundary in Orleans, that warning should be in XML docs on OutboxPostmanExecutionMode.ThreadPool and probably in README near the outbox processor examples.

P3 - The Event Hubs capability is a hard dependency for every consumer

The package references Microsoft.Orleans.Streaming.EventHubs directly (Egil.Orleans.Messaging/src/Egil.Orleans.Messaging/Egil.Orleans.Messaging.csproj:41). That means a consumer using only State, Outboxes, or Tracking still takes the Event Hubs dependency graph. The API design says one package is intentional for now, so this may be acceptable, but it is the main package/runtime footprint tradeoff I see. If dependency weight matters, Streams.EventHubs is the clean future split.

P3 - The PR includes unrelated agent skill files

The diff adds .agents/skills/aspire, .agents/skills/aspireify, .agents/skills/dotnet-inspect, and .agents/skills/playwright-cli alongside the messaging package. These do not appear tied to Egil.Orleans.Messaging behavior or CI. If they are intentional repo tooling, they should probably be split out; otherwise they add noise and maintenance surface to this package PR.

Test gaps I would add before merging

• Implicit subscription namespace mismatch throws instead of log-only no-op.
• Stream handler exception propagation/default policy, including tracker not being advanced.
• Explicit subscription to a non-grain-key StreamId.
• Compound grain key stream-id derivation, if grain-keyed explicit subscriptions remain.
• Base/subtype postman ambiguity.
• Duplicate keyed postman name+message registration.
• Public API snapshot or package inspection to catch generated extension-block surface if C# 14 extension blocks stay.

[egil]

I think we can do better here. The question: When talking to azure storage (table/blob) what exceptions indicates that the server received the put request but without a doubt did not complete the transaction, vs what exceptions means we do not know, e.g., because the server did not respond with a status code?

Short answer: in the Azure SDK for .NET, "the server told us no" exceptions are always RequestFailedException carrying an HTTP Status and a storage ErrorCode. "We don't know" exceptions are the ones where no status code ever came back — i.e. transport/timeout exceptions, plus a small set of ambiguous 5xx codes where the server may have started processing before failing.

Definitively did NOT complete

These are RequestFailedException (Azure.Core) with a non‑zero Status and an ErrorCode. The server received the PUT, parsed it, and rejected it before committing. You can safely treat the operation as not applied.

• 400 Bad Request — InvalidInput, InvalidXmlDocument, InvalidHeaderValue, OutOfRangeInput, InvalidUri, PropertyValueTooLarge, EntityTooLarge, InvalidPk/InvalidRk etc. Request was rejected before being applied.
• 401 Unauthorized — AuthenticationFailed, InvalidAuthenticationInfo. Not applied.
• 403 Forbidden — AuthorizationFailure, InsufficientAccountPermissions, AccountIsDisabled, KeyVaultEncryptionKeyNotFound. Not applied.
• 404 Not Found — ContainerNotFound, BlobNotFound, TableNotFound, ResourceNotFound. Not applied.
• 405 / 406 / 411 / 413 / 414 / 415 / 416 — payload / header / content-length / range validation failures. Not applied.
• 409 Conflict — BlobAlreadyExists, ContainerAlreadyExists, TableAlreadyExists, EntityAlreadyExists, LeaseIdMismatchWithBlobOperation, LeaseAlreadyPresent. Your write was not applied (though something else is there — important nuance for idempotent retries: if you just retried, the prior attempt may actually have succeeded; see below).
• 412 Precondition Failed — ConditionNotMet, LeaseLost, ETag/If‑Match/If‑None‑Match mismatch. Not applied. This is the canonical "use optimistic concurrency to detect the retry-after-success case" code.
• 428 Precondition Required, 431 — not applied.
• 503 Service Unavailable with ServerBusy — throttled; the server rejected the request without processing it. Treat as not applied. (ErrorCode == "ServerBusy".)
• 507 Insufficient Storage — not applied.

"We don't know" (no status, or ambiguous server-side)

You cannot conclude either way. The PUT may have been received, durably committed, and the ack lost on the way back. Retries must be idempotent (use If-None-Match: * for create, ETag/If-Match for update, or read-back to verify).

• No HTTP response received — pre‑response network/transport failures. In the modern SDK these surface as:
  • RequestFailedException with Status == 0 (no response) wrapping an inner exception, or
  • the raw transport exception: HttpRequestException, IOException, SocketException (ConnectionReset, ConnectionAborted, TimedOut), System.Net.Http.HttpIOException.
  • TaskCanceledException / OperationCanceledException where the CancellationToken you passed was not cancelled (i.e. it's actually an HttpClient/per‑try timeout). If your own token was cancelled, you also don't know whether the bytes made it.
  • AuthenticationException / SSL/TLS errors mid‑handshake — unknown if anything was sent.
• 500 Internal Server Error (InternalError, OperationTimedOut) — the server hit an internal failure; the write may or may not have been applied. Treat as unknown.
• 502 Bad Gateway — front‑end/proxy error; backend state is unknown.
• 503 Service Unavailable without ServerBusy (e.g. plain unavailable, no error code) — ambiguous; safest to treat as unknown.
• 504 Gateway Timeout — request reached the service but the response was lost; unknown.
• 404 on a follow‑up read after a retry of a create that previously got an unknown error — still unknown for the original attempt, but if the resource is absent now you can conclude nothing was committed.

Practical guidance for Orleans‑style providers

When implementing Azure Table/Blob persistence, membership, reminders, or queue providers:

1. Catch RequestFailedException and branch on ex.Status / ex.ErrorCode:
   • 4xx and 503/ServerBusy → not applied, surface as a definite error to the caller (no idempotency concerns).
   • 500 / 502 / 503‑unknown / 504 → unknown, retry with ETag / If-Match / If-None-Match: * so a duplicate commit is detected as 409 or 412.
2. Anything not a RequestFailedException (raw HttpRequestException, IOException, SocketException, TimeoutException, TaskCanceledException from a per‑try timeout) → unknown, same idempotent‑retry rule.
3. On a retry that returns 409 *AlreadyExists or 412 ConditionNotMet after a prior unknown attempt, treat that as evidence the prior attempt actually succeeded — read back and reconcile (and capture the new ETag) rather than failing the operation.
4. Don't conflate OperationCanceledException from a user‑supplied token (definitely cancelled, but the write state on the server is still unknown) with one from an internal HttpClient timeout. Either way, the server-side outcome is unknown.

Rule of thumb: RequestFailedException with a 4xx (or 503/ServerBusy) = definitive "no". Everything else that throws on a write = "unknown, must be retried idempotently".

[egil]

The assumption is that tokens are in the same order in items and there are no skipped items.

It does not well handled if one token in the middle ran into a transient error and needs to be retried and is not included.

Something like this should cover holes in tokens:

Suggested change

	{
	var tokenSet = tokens.ToHashSet();
	if (tokenSet.Count == 0)
	{
	return this;
	}
	var removeCount = 0;
	while (removeCount < items.Length && tokenSet.Contains(items[removeCount].Token))
	{
	removeCount++;
	}
	var remaining = items.RemoveRange(0, removeCount);
	if (remaining.Length == items.Length)
	{
	return this;
	}
	{
	var itemBuilder = items.ToBuilder();
	int startIndex = 0;
	int removeLength = 0;
	foreach(var removeToken in tokens)
	{
	for(int i = startIndex; i < itemBuilder.Count; i++)
	{
	if(removeToken == itemBuilder[i].Token)
	{
	removeLength++;
	}
	else
	{
	if(removeLength > 0)
	{
	itemBuilder.RemoveRange(startIndex, removeLength);
	}
	removeLength = 0;
	startIndex = 1;
	}
	}
	}

[egil]

lets use a switch instead searching through an array

[egil]

Why do we need an jsonmodel object when it has the exact same signature as OutboxMessageEnvelope itself. Redundant?

[egil]

This does not follow the recommended pattern from Microsoft: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to#sample-factory-pattern-converter

[egil]

1. for orleans serializable types, dont use primary ctor properties, use regular required init only properties. add empty ctor and ctor that takes all properties and sets properties for convenience.

2. For custom json converters, lets drop using "json payload types" and instead follow conventions established in https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to#sample-factory-pattern-converter.

3. follow code comment skill and ensure all files have appropriate code comments.

[egil]

For orleans serializable types it is better not to use primary ctor properties but have init only normal properties. An empty ctor and a ctor that takes both required properties is nice to have too.

[egil]

lets follow recommend pattern from here for this scenario: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to#sample-factory-pattern-converter

[egil]

Suggested change

	public Func<ImmutableArray<(TOutbox Item, Exception Error, int Attempt)>,
	CancellationToken, ValueTask>? ReconcileFailedAsync
	{ get; init; }
	public Func<ImmutableArray<(TOutbox Item, Exception Error, int Attempt)>, CancellationToken, ValueTask>? ReconcileFailedAsync { get; init; }

one-liner

[egil]

Suggested change

	public required Func<ImmutableArray<TOutbox>, CancellationToken, ValueTask>
	AcknowledgePostedAsync
	{ get; init; }
	public required Func<ImmutableArray<TOutbox>, CancellationToken, ValueTask> AcknowledgePostedAsync { get; init; }

one liner

[egil]

lets use to point to the callbacks in question above to be explicit

[egil]

Suggested change

	/// <summary>
	/// Registers a custom keyed open-generic state manager factory on the silo builder.
	/// </summary>
	public ISiloBuilder AddStateManagerFactory(
	string storageName,
	Type openGenericFactoryType)
	{
	ArgumentNullException.ThrowIfNull(builder);
	StateManagerRegistrationExtensions.ValidateStorageName(storageName);
	StateManagerRegistrationExtensions.ValidateOpenGenericFactoryType(openGenericFactoryType);
	builder.ConfigureServices(services =>
	services.AddStateManagerFactory(storageName, openGenericFactoryType));
	return builder;
	}
	/// <summary>
	/// Registers a custom keyed state manager factory for a specific state type on the silo builder.
	/// </summary>
	public ISiloBuilder AddStateManagerFactory<TState, TFactory>(string storageName)
	where TState : class, IEquatable<TState>
	where TFactory : class, IStateManagerFactory<TState>
	{
	ArgumentNullException.ThrowIfNull(builder);
	StateManagerRegistrationExtensions.ValidateStorageName(storageName);
	builder.ConfigureServices(services =>
	services.AddStateManagerFactory<TState, TFactory>(storageName));
	return builder;
	}
	}
	/// <summary>
	/// Registers a custom keyed open-generic state manager factory on the silo builder.
	/// </summary>
	public ISiloBuilder AddStateManagerFactory(
	string storageName,
	Type openGenericFactoryType)
	{
	ArgumentNullException.ThrowIfNull(builder);
	StateManagerRegistrationExtensions.ValidateStorageName(storageName);
	StateManagerRegistrationExtensions.ValidateOpenGenericFactoryType(openGenericFactoryType);
	builder.ConfigureServices(services =>
	services.AddStateManagerFactory(storageName, openGenericFactoryType));
	return builder;
	}
	/// <summary>
	/// Registers a custom keyed state manager factory for a specific state type on the silo builder.
	/// </summary>
	public ISiloBuilder AddStateManagerFactory<TFactory>(string storageName)
	where TFactory : class, IStateManagerFactory
	{
	ArgumentNullException.ThrowIfNull(builder);
	StateManagerRegistrationExtensions.ValidateStorageName(storageName);
	builder.ConfigureServices(services =>
	services.AddStateManagerFactory<TFactory>(storageName));
	return builder;
	}
	}

[egil]

Potentially more flexible if we make this a string, and trim - from guid. still fine to use a guid as the base though.

[egil]

is enqueued too eventhub specific or general enough?

[egil]

should these extension methods be on a JsonSerializationOption type instead. That way, they are added to a specific one, not to a static type somewhere. Seems more correct.

[egil]

it does not make sense to track if StreamSequenceToken is null. So, method signature should require that. Same elsewhere in this class, or is it better to allow it to be null and just return true in case of null?

[egil]

Addressed the actionable review items in the latest push:

• Changed IStateManagerFactory<T> to a non-generic IStateManagerFactory with generic Create<T> and moved DefaultStateManagerFactory to its own file.
• Simplified Azure Storage error-code matching with explicit switches.
• Added comments around ambiguous ClearAsync recovery.
• Fixed Outbox.RemoveRange so successful items after a failed gap can still be acknowledged.
• Converted OutboxSequenceToken and OutboxMessageEnvelope<T> away from primary-constructor records for Orleans serializer friendliness.
• Reworked outbox STJ converters to avoid private JSON DTO models, reuse typed converters, and share a direct GrainId converter while preserving the JSON shape.
• Clarified OutboxProcessorOptions docs around reconciliation callbacks.
• Changed tokenless stream messages to be accepted without advancing MessageTracker, since a null StreamSequenceToken cannot provide a dedup high-water mark.

Validation after the fixes:

• dotnet format Egil.Orleans.Messaging\Egil.Orleans.Messaging.slnx --no-restore
• dotnet build Egil.Orleans.Messaging\Egil.Orleans.Messaging.slnx -c Release --no-restore
• dotnet test Egil.Orleans.Messaging\Egil.Orleans.Messaging.slnx -c Release --no-build

Open design questions I did not change yet:

• VersionedState.Version: I held off changing the generated version string format. Removing dashes is persisted-state visible, so I want an explicit yes before changing it.
• IStreamSequenceTokenMetadata.TryGetEnqueuedTime: I kept the name. It is Event Hubs-shaped, but also matches common broker terminology. We can rename if you prefer a more generic concept such as accepted/published time.
• StreamSequenceTokenJsonConverters: I kept the explicit static registry model. Moving registration to JsonSerializerOptions would likely mean dropping the current attribute-driven convenience on StreamCursor/MessageTracker or requiring users to configure every storage serializer option path explicitly.