Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -404,10 +404,11 @@ private IObservable<IMessageDelivery> DispatchObservable(IMessageDelivery delive
if (result.State == MessageDeliveryState.Failed)
{
// Preserve the RoutingGrain's message so the GUI's
// IsExpectedUserActionFailure classifier can match it.
var failureMessage = result.Properties.TryGetValue("Error", out var errObj) && errObj is string errStr
? errStr
: $"Delivery failed to {address}";
// IsExpectedUserActionFailure classifier can match it. GetFailureMessage, not a
// raw `is string` test: a delivery that crossed a hub boundary can carry the text
// as an untyped JsonElement, and dropping it would also drop the phrase the
// classification fallback below matches on.
var failureMessage = result.GetFailureMessage() ?? $"Delivery failed to {address}";
// 🚨 NOT every Failed result is terminal, and assuming so cost this project the
// shard-0 Orleans flake cluster. This comment used to read "Grain returned a
// non-transient failure (e.g. node doesn't exist)" and SendDeliveryFailure
Expand All @@ -430,10 +431,26 @@ private IObservable<IMessageDelivery> DispatchObservable(IMessageDelivery delive
// all already treat "is shutting down" as retry-worthy, and NackThroughParent's
// own comment calls the wording CONTRACT. This makes the fourth layer agree with
// the other three rather than contradict them.
var failureErrorType = ClassifyRoutedFailure(failureMessage);
//
// 🚨 The CARRIED verdict comes first, the text rule is only the FALLBACK — the
// two must not diverge from the silo-side twin in RoutingGrain, because "a fix
// landed on one site and missed the other" is precisely how #2346 outlived both
// of its earlier fixes. A site that recorded a verdict (MessageService's intake
// gate, MessageHubGrain's activation/disposal arms) knows more than any matcher
// can recover from prose: "Hub disposed before delivery for …" is ShuttingDown
// and contains no phrase a text rule could catch.
var failureErrorType = result.GetFailureErrorType(ClassifyRoutedFailure(failureMessage));
logger.LogWarning("Orleans: delivery FAILED for {MessageType} to {Address}: {FailureMessage} (as {ErrorType})",
msgType, address, failureMessage, failureErrorType);
SendDeliveryFailure(delivery, failureMessage, failureErrorType);

// 🚨 ANSWER ONCE — the same contract MessageService.ReportRoutingFailure and
// the silo-side RoutingGrain apply. FailedAndNacked DECLARES that the failing
// site posted its own DeliveryFailure; answering again gives ONE request TWO
// answers, and Observe resolves on whichever lands first.
if (result.SenderWasNacked)
OrleansRouteTrace.Write($"OrleansRoutingService.Deliver DISPATCH_FAILED_ALREADY_NACKED addr={address} id={delivery.Id}");
else
SendDeliveryFailure(delivery, failureMessage, failureErrorType);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,32 @@ re-derived downstream from message text) and whether it already answered the sen
routing loop reports as `ErrorType.RoutingLoop`. A site that posts its own NACK (the routing
services' `NotFound`, on a hot path) marks the delivery answered so the sender never gets two.

#### Recording a verdict is not the same as delivering one (#2346)

Both halves above are carried in the delivery's `Properties`, and both leaked between the site that
wrote them and the site that acts on them. Worth knowing, because the failure is **silent in both
directions** and the symptom appears somewhere else entirely — as a rotating cast of Orleans
teardown flakes, which is how it was eventually found:

- **The verdict did not survive a hub boundary** — the only trip it exists for. `Properties` is
`IReadOnlyDictionary<string, object>`, serialized by writing each value by its RUNTIME type, so an
`ErrorType` goes out as the enum NAME and comes back as a `string`. `GetFailureErrorType`'s
`value is ErrorType` test then failed and every consumer read its FALLBACK — the carefully-recorded
`ShuttingDown` reached the router as "no verdict was reached". It now recovers the enum, its name,
an integral value or a `JsonElement`; an unrecognised value still falls back, which is honest
rather than invented.
- **The Orleans routing layer read neither.** `RoutingGrain.DeliverToGrainWithRetry` — the ONLY site
that reports a grain-routed delivery's failure, because `RouteMessage` returns `Forwarded`
unconditionally and delivers on a background route — hard-coded the terminal `ErrorType.Failed`
and ignored `SenderWasNacked`. So a disposal race was reported as permanent, *and* the sender got
a second, contradictory answer racing the hub's own.

The rule the two together produce, and the one to apply at any new reporting site: **honour the
answered flag first, then read the CARRIED verdict, and fall back to the shared text rule
(`ClassifyRoutedFailure` — the same phrase list `AreaErrorClassifier.IsTransientHubFailure` and
`MeshNodeStreamCache.IsTransientOwnerFailure` use) rather than to a terminal default.** A verdict
that cannot be read is not evidence of a permanent failure.

### Long-running operations (activity → activity log)

An import / compile / mirror runs as an activity. A fault must not strand the activity "Running" forever — it writes `Status = Error` with the message onto the activity node, which the activity log and any progress reader render. Persistence at the bottom of the stack never re-gates and never fail-closes a write that was already approved; it forwards. See [Activity Control Plane](/Doc/Architecture/ActivityControlPlane) and [Activity Operations](/Doc/Architecture/ActivityOperations).
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
Name: A page caught mid-recycle recovers instead of erroring
Category: Fix
Description: A request that arrived while its node was restarting was reported as a permanent failure; it is now recognised as the temporary state it is, and the view reloads by itself.
Icon: ArrowSync
Order: -20260826
---

# A page caught mid-recycle recovers by itself

Nodes restart routinely — you save an edit that recompiles a view, an administrator recycles a
space, a background release rolls a node forward. Restarting takes a moment, and any request that
arrives inside that moment cannot be served yet.

The platform has always known the difference between *"this address is coming back in a second"* and
*"this address is gone"*, and everything that reads live data is built to sit out the first and give
up only on the second. What went wrong is that the verdict was being lost in transit: the node
correctly said "I am restarting", and the layer that carried the answer back to the page rewrote it
as a permanent failure. The page then stopped waiting and showed an error for something that had
already fixed itself.

Now the original verdict travels intact. In practice: a view you open (or that is already open)
while its node is restarting waits the extra moment and renders, instead of showing an error you had
to reload past. Genuinely missing content is unaffected — that still reports as missing straight
away, with no retrying.
55 changes: 50 additions & 5 deletions src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs
Original file line number Diff line number Diff line change
Expand Up @@ -995,14 +995,59 @@ internal static IDisposable DeliverToGrainWithRetry(
RoutingGrainTrace.Write($"RoutingGrain.RouteMessage GRAIN_CALL_OK id={deliveryId} grainKey={grainKey} state={result.State}");
if (result.State == MessageDeliveryState.Failed)
{
// 🚨 ANSWER ONCE. FailedAndNacked DECLARES that the failing site already posted
// its own DeliveryFailure — which MessageService.NackThroughParent does on every
// targeted hub disposal (recycle, node delete). A second NACK from here gives ONE
// request TWO answers, and Observe resolves on whichever lands first, so the
// caller's verdict becomes a coin toss even once both sites classify correctly.
// Same rule, same signal, as MessageService.ReportRoutingFailure.
if (result.SenderWasNacked)
{
RoutingGrainTrace.Write($"RoutingGrain.RouteMessage GRAIN_CALL_FAILED_ALREADY_NACKED id={deliveryId} grainKey={grainKey}");
logger.LogDebug(
"[ROUTE] Grain {GrainKey} returned Failed and had ALREADY answered the sender — not NACKing twice",
grainKey);
return;
}

// The owning grain resolved but could NOT service the delivery (unmaterializable /
// unregistered node type, failed activation, access denial). Surface as a
// DeliveryFailure so the caller gets a fast, deterministic OnError instead of parking.
var failMsg = result.Properties.TryGetValue("Error", out var errObj) && errObj is string errStr
? errStr
: $"Delivery to '{addressPath}' failed at its owning hub.";
logger.LogWarning("[ROUTE] Grain {GrainKey} returned Failed: {Error}", grainKey, failMsg);
postFailureToSender(failMsg, ErrorType.Failed);
// GetFailureMessage, not a raw `is string` test: Properties values come back
// re-materialised from JSON, so the text can arrive as an untyped JsonElement.
// Losing it here costs twice — the sender loses its diagnostic AND the
// classification below loses the phrase its fallback rule matches on.
var failMsg = result.GetFailureMessage()
?? $"Delivery to '{addressPath}' failed at its owning hub.";

// 🚨 CARRY the verdict the owning hub recorded; NEVER re-decide it here. This line
// read `ErrorType.Failed` — terminal, unconditionally — and that is issue #2346.
//
// Three layers below this one classify carefully and every one of those verdicts
// died here: MessageService's intake gate answers a disposal race with
// ShuttingDown (#2350), and MessageHubGrain.DeliverMessage classifies its own two
// arms (Unavailable for an activation fault #1693, ShuttingDown for "hub disposed
// before delivery"). This is the ONLY site that reports them, because
// RouteMessage returns Forwarded unconditionally and delivers on a background
// route — so the client-side classifier in OrleansRoutingService.DispatchObservable
// never sees a Failed result for a grain-routed address and could not run.
//
// That is why OrleansMeshTests.HubWorksAfterDisposal kept failing in ~2.4 s with
// the exact text "Hub is shutting down" raised INSIDE the retry that matches
// ShuttingDown — on branches carrying both earlier fixes.
//
// The FALLBACK is the shared text rule, not the terminal default: a delivery that
// crossed a hub boundary arrives with Properties values re-materialised from JSON,
// so a recorded verdict can legitimately be unreadable here. ClassifyRoutedFailure
// is the same rule AreaErrorClassifier.IsTransientHubFailure and
// MeshNodeStreamCache.IsTransientOwnerFailure already apply, so all four layers
// agree; everything it does not recognise stays terminal, which keeps "No node
// found" authoritative.
var failureErrorType = result.GetFailureErrorType(
OrleansRoutingService.ClassifyRoutedFailure(failMsg));
logger.LogWarning("[ROUTE] Grain {GrainKey} returned Failed: {Error} (as {ErrorType})",
grainKey, failMsg, failureErrorType);
postFailureToSender(failMsg, failureErrorType);
}
},
ex =>
Expand Down
109 changes: 108 additions & 1 deletion src/MeshWeaver.Messaging.Contract/IMessageDelivery.cs
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,121 @@ IMessageDelivery FailedAndNacked(string message) =>
/// <summary>
/// The classification recorded by <see cref="Failed(string, ErrorType)"/>, or
/// <paramref name="fallback"/> when the failing site recorded none.
///
/// <para>🚨 <b>The verdict must survive a HUB BOUNDARY, which is the only situation it exists
/// for.</b> This read used to be a bare <c>value is ErrorType</c> — the trap-door AGENTS.md
/// names: an <see cref="object"/> that crossed a boundary is no longer the CLR type it was
/// written as. <c>Properties</c> is serialized by
/// <c>ImmutableDictionaryOfStringObjectConverter</c>, which writes each value by its RUNTIME
/// type, so an <see cref="ErrorType"/> goes out as the enum NAME and comes back — through
/// <c>ObjectPolymorphicConverter</c>, which materialises a JSON string as
/// <see cref="string"/> — as a string. The type test then failed and EVERY consumer silently
/// read <paramref name="fallback"/> instead of the recorded verdict.</para>
///
/// <para>Silently, and with real consequences: a disposal race classified
/// <see cref="ErrorType.ShuttingDown"/> at the hub (#2350) reached the router as the fallback,
/// so a delivery that raced a hub's disposal kept being reported to the sender as TERMINAL even
/// after every producing site had been fixed to classify it (#2346). Reading the serialized
/// forms back is what makes "carried, never re-derived from the message text" true rather than
/// aspirational.</para>
/// </summary>
/// <param name="fallback">The verdict to use when the failing site classified nothing.</param>
/// <returns>The recorded classification, or <paramref name="fallback"/>.</returns>
ErrorType GetFailureErrorType(ErrorType fallback) =>
Properties.TryGetValue(FailureErrorTypeProperty, out var value) && value is ErrorType errorType
Properties.TryGetValue(FailureErrorTypeProperty, out var value) && TryReadErrorType(value, out var errorType)
? errorType
: fallback;

/// <summary>
/// Recovers an <see cref="ErrorType"/> from every shape a JSON round-trip can leave it in: the
/// CLR enum (never left this process), the enum NAME (the hub's own options — the enum
/// converter writes a string and <c>ObjectPolymorphicConverter</c> reads one back), an integral
/// value (options serializing enums numerically), or a raw <c>JsonElement</c> (options
/// without <c>ObjectPolymorphicConverter</c>, which leaves every <see cref="object"/> untyped).
///
/// <para>An unrecognised or undefined value is NOT a verdict — it yields <c>false</c> so the
/// caller's fallback applies, which is the honest answer and never a silently invented one.</para>
/// </summary>
private static bool TryReadErrorType(object? value, out ErrorType errorType)
{
switch (value)
{
case ErrorType typed:
errorType = typed;
return true;
case string name:
// Names only: Enum.TryParse also accepts a NUMERIC string and will happily mint an
// undefined member from it, so IsDefined is the gate, not decoration.
return Enum.TryParse(name, ignoreCase: true, out errorType) && Enum.IsDefined(errorType);
case int number:
return TryReadErrorTypeNumber(number, out errorType);
case long number:
return TryReadErrorTypeNumber(number, out errorType);
case short number:
return TryReadErrorTypeNumber(number, out errorType);
case byte number:
return TryReadErrorTypeNumber(number, out errorType);
case System.Text.Json.JsonElement { ValueKind: System.Text.Json.JsonValueKind.String } json:
return TryReadErrorType(json.GetString(), out errorType);
case System.Text.Json.JsonElement { ValueKind: System.Text.Json.JsonValueKind.Number } json:
return json.TryGetInt64(out var raw)
? TryReadErrorTypeNumber(raw, out errorType)
: Fail(out errorType);
default:
return Fail(out errorType);
}

static bool Fail(out ErrorType errorType)
{
errorType = default;
return false;
}
}

/// <summary>
/// An integral value read as an <see cref="ErrorType"/>, range-checked FIRST.
///
/// <para>🚨 Deliberately not <c>Convert.ToInt32</c>: that THROWS <see cref="OverflowException"/>
/// on an out-of-range value, and this runs on the error-REPORTING path — the one place a throw
/// is worst, because it replaces a failure that was about to be described with a different,
/// undescribed one. An unreadable value is "no verdict was reached", which is exactly what the
/// caller's fallback is for.</para>
/// </summary>
private static bool TryReadErrorTypeNumber(long value, out ErrorType errorType)
{
errorType = default;
if (value is < int.MinValue or > int.MaxValue)
return false;
errorType = (ErrorType)(int)value;
return Enum.IsDefined(errorType);
}

/// <summary>
/// The failure TEXT recorded by <see cref="Failed(string)"/>, or null when this delivery carries
/// none — so a caller can tell "no message was recorded" from an empty one and supply its own
/// description.
///
/// <para>Tolerant of the round-trip for the same reason <see cref="GetFailureErrorType"/> is:
/// <c>Properties</c> values come back re-materialised from JSON, and options WITHOUT
/// <c>ObjectPolymorphicConverter</c> leave a string as an untyped <c>JsonElement</c>. Reading
/// only the CLR <see cref="string"/> silently loses the message — which costs twice over at a
/// routing site, because the text is BOTH the sender's diagnostic AND the fallback the
/// classification rule matches on when the failing site recorded no verdict.</para>
/// </summary>
/// <returns>The recorded failure text, or null.</returns>
string? GetFailureMessage() =>
Properties.TryGetValue(nameof(Error), out var value)
? value switch
{
string text => text,
System.Text.Json.JsonElement { ValueKind: System.Text.Json.JsonValueKind.String } json
=> json.GetString(),
// Anything else recorded under this key is not a message. Null, so the caller
// describes the failure itself rather than printing a type name at a user.
_ => null,
}
: null;

/// <summary>
/// Returns a copy of this delivery with a single property set.
/// </summary>
Expand Down
Loading
Loading