Repository navigation
fix(routing): the pod-hub leg classifies a deactivated activation instead of defaulting to terminal (#2299) - #5174
Conversation
…stead of defaulting to terminal `IsDeactivatedActivation` (the arm for Orleans' `… after "DeactivateOnIdle was called." to invalid activation. Rejecting now.`) is gated on a discriminator whose default is `true` — assume the worse case — so any caller that cannot consult `GrainActivationFailureRegistry` keeps the terminal verdict. `BuildPodHubRoute`'s terminal arm passed that default, so on the pod-hub leg the arm could never fire — and that is the leg the rejection names (`IPodHubGrain.Deliver`) and where #2299's evidence lives: 947 occurrences, every one of the newest samples, each logged as "surfacing Failed DeliveryFailure to sender …". The predicate shipped, its facts were green, and production kept printing the pre-fix verdict. On this leg `false` is a fact, not an assumption. The registry holds the last activation failure "for each per-node-hub grain" and only `MessageHubGrain` writes it; a `PodHubGrain` has no NodeType, no hub configuration to materialise and a contract that it must not throw from `OnActivateAsync` (its refusal is the call's answer, `PodHubNotHereException`), so it can never enter the persistent activation-fault loop the discriminator separates. The only way its activation becomes invalid is the idle deactivation it requested of itself. Consumers that carry their own recovery machinery ride out `ShuttingDown` and tear down on `Failed`, so the wrong verdict ended deliveries the address's next claim would have served — the newest sample's sender is an agent thread. Nothing else is widened: every other arm is evaluated unchanged and an unrecognised rejection stays terminal. This does not address the re-activation bounce that PRODUCES the rejection; that is a lifecycle question, recorded on #2299 and on the doc page with its three candidates and their costs. - `RoutingGrain.ClassifyPodHubDeliveryException` — the seam carrying the argument - `PodHubDeactivatedActivationClassificationTest` — 8 facts, both production texts verbatim, the verdict asserted on BOTH sides of the discriminator, the container probe asserted forwarded - docs: the mechanism on `ADepartedSiloIsNotADeliveryDefect` (which already carried the row); the cluster grouping and its two corrected readings on a new `TheSubsystemInTheTitleIsNotTheCause` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add route-level coverage or a tested seam to prevent the production wiring regression from recurring unnoticed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates pod-hub routing so deactivated activations are classified as transient rather than terminal.
Changes:
- Adds pod-hub-specific classification and wiring.
- Adds classification regression tests.
- Documents the routing diagnosis and related incidents.
| File | Summary |
|---|---|
test/MeshWeaver.Hosting.Orleans.Test/PodHubDeactivatedActivationClassificationTest.cs |
Adds classifier behavior tests. |
src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs |
Applies pod-hub transient classification. |
src/MeshWeaver.Documentation/Data/Architecture/TheSubsystemInTheTitleIsNotTheCause.md |
Adds incident-analysis guidance. |
src/MeshWeaver.Documentation/Data/Architecture/ADepartedSiloIsNotADeliveryDefect.md |
Documents the classification fix and remaining bounce issue. |
src/MeshWeaver.Documentation/Data/Architecture.md |
Indexes the new documentation page. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // deactivated-activation arm defaults to the terminal answer for a caller that cannot | ||
| // consult the activation-failure registry, which made it INERT on this leg — see | ||
| // ClassifyPodHubDeliveryException. | ||
| var errorType = ClassifyPodHubDeliveryException(ex, IsServiceScopeDisposed); |
There was a problem hiding this comment.
Accepted, and it is a regression my diff introduces rather than a pre-existing gap — I have reproduced it as a red control and fixed it by discovery.
Verified your reading first: EnsurePartitionProvisioned (PostgreSqlPartitionStorageProvider.cs:413-451) collects every PartitionDefinition.TableMappings value and Table outside DefaultSegmentTableMappings() into extraTables and creates each through the same GetSatelliteTableScript. So a bespoke table created before this change has no authorship columns, CREATE TABLE IF NOT EXISTS never adds them, a seven-name list skips it, and the adapter half of this PR then selects and binds the trio against it.
Measured, with the frozen list still in place and a bespoke ledger table created from the pre-change DDL:
APreExistingBespokeSatelliteTable_IsDiscoveredAndUpgraded [FAIL]
Expected 0 to be greater than 0 because the bespoke table is satellite-SHAPED, so discovery
must count it among the tables still missing authorship.
AfterTheUpgrade_ABespokeRoutedNode_RoundTripsItsAuthorship [FAIL]
42703: column "created_by" of relation "ledger" does not exist
That second line is exactly the failure you predicted. Note the first one too: the REHEARSAL answered "nothing to do" for the table the statement then left broken — a gate passing on no evidence, which is the worse half.
The fix is discovery by SHAPE, not a wider list. V57SatelliteAuthorshipSql.DiscoverSatelliteTables selects every BASE TABLE in current_schema() carrying all ten columns GetSatelliteTableScript produces (namespace, id, path, node_type, content, main_node, desired_id, version, state, last_modified), excluding mesh_nodes and mesh_node_history BY NAME. Those two exclusions are load-bearing, not defensive: both match the same ten-column shape — mesh_nodes got its columns from V45, and history is not a node table (it carries changed_by and a version in its PK). Nothing else in a partition schema has all ten; content_chunks, partition_objects, user_effective_permissions and partition_access each lack most of them. The rehearsal counts over the SAME discovery, so it can no longer report zero for a table the statement alters.
On the tension with freezing the list — you are right that I froze it deliberately, and right that discovery is compatible with that instinct. What a migration must not do is change its BEHAVIOUR after it has run by reading something that moved underneath it. A name list read from SatelliteTableMapping.Defaults at runtime would be exactly that. The SQL is still frozen; what it now asks about is the SHAPE of the database it is being run against, which is a fact about that database at that moment. V45 discovered its target SCHEMAS from information_schema for the same reason; this discovers TABLES the same way. So: discovery, and the reason is that the frozen thing is the SQL, not the answer.
Tests, control on each side — BespokeSatelliteAuthorshipUpgradeTests (new): a bespoke ledger table built from the PRE-upgrade DDL is discovered and gains all three columns (0 before, 3 after; rehearsal > 0 before, 0 after), and a node routed to it round-trips its authorship through the adapter. Both fail as quoted above against the frozen list and pass after. MigrationPlanTest now asserts the plan DISCOVERS (information_schema.tables present, 'threads' absent, both exclusions present, and the rehearsal counting over the same discovery) instead of asserting seven literals.
One thing worth flagging because it shaped the fix: the test executes the migration's OWN SQL, not a copy. This project's established shape is a const copy under a "must match it" comment (MigrationUserBackfillFromIndexTests, V05), and a copy is a guard whose subject can move while its roots do not — edit the migration and the test still passes having checked nothing. The PG suite may not ProjectReference Memex.Database.Migration (it is an Exe whose top-level Program collides with a test host's, which is why Memex.Database.Migration.Test takes it under an alias), so the SQL moved into its own file and the test project <Compile Include>s it. One compiled source in two assemblies cannot drift.
There was a problem hiding this comment.
Accepted and fixed — and you are right that it stings, because the looseness is the same defect class as the Contains("mesh_nodes") I was replacing.
How it got past me: I reasoned carefully about the DIRECTION of the predicate (which tables should read real columns and which must not) and then copied the matching STYLE from the line I was editing without re-examining it. The inversion was the change I was making; the substring test was scenery. content_chunks also happens to be the one table name in the file that could never be a schema in the author's mental model — which is precisely the assumption a partition name is free to violate. I confirmed it is free to: PartitionDefinition.IsValidPartitionSegment admits letters, digits, ., - and _, pinned by PartitionSegmentValidationTest.
The fix compares the RELATION exactly. PostgreSqlSqlGenerator.RelationName takes the part after the LAST dot and trims quotes (the last dot, because a dot inside a quoted schema name precedes the separator), and AuthorColumns now tests RelationName(tableName) == "content_chunks".
I swept the siblings rather than leaving one exact predicate beside three loose ones, since that asymmetry is how this class of bug survives review. IsMeshNodesTable, SyncBehaviorColumn and ExcludeFromContextColumn in the generator, and SyncBehaviorCol, ExcludeCol and writeSync in PostgreSqlStorageAdapter, all now go through the same helper. That one is pre-existing rather than introduced by this PR, and it is worse than the authorship case when it fires: a partition named mesh_nodes made every SATELLITE read name n.sync_behavior, which is 42703 on the whole statement rather than a silently wrong value.
Tests, one case on each side of the rule (a test naming only one member cannot see a rule that chooses among members):
AuthorshipProjection_AsksTheRelation_NotTheQualifiedString—content_chunksand"acme"."content_chunks"project typed NULLs;"my_content_chunks"."mesh_nodes","my_content_chunks"."threads","content_chunks"."mesh_nodes"and"content_chunks"."threads"project the real columns. The last two are the sharp case: a partition named EXACTLYcontent_chunks.SyncBehaviourProjection_AlsoAsksTheRelation—"mesh_nodes"."threads"and"my_mesh_nodes"."threads"must NOT namen.sync_behavior;mesh_nodesand"acme"."mesh_nodes"must.RelationName_TakesTheLastPart_Unquoted— including"my.org"."threads", since a dot is valid in a partition name.
Review on this PR caught that the eight new facts call the classifiers and nothing executes `BuildPodHubRoute`, so reverting the one line that chooses between them left every fact green — the same shape as the defect being fixed (an argument not written at a call site), one level out. `TerminalCallFailure` is now a one-line delegation to a new `AnswerPodHubCallFailure`, which holds both decisions of that arm: which classifier the leg uses, and the log level the verdict deserves. Two facts drive that function — the same code production runs — capturing the (message, ErrorType) handed to the sender and the level handed to the logger. Control, with the classifier choice reverted inside the function: 2 of 10 facts red, 8 green — exactly the blindness the review named. Restored: 10 of 10, and 53 of 53 over the six classification classes, 0 failed, 0 skipped. The level was equally unpinned and mattered for the same reason: a lifecycle transition reported at Error is what files an incident for a pod that was merely finishing, and it is what kept this fingerprint collecting recurrences. It is asserted on both sides, so it cannot be pinned to one value. Doc updated — the "what is pinned" section said the wiring was not pinned, which is no longer true, and it now carries the transferable half: when the defect is "a decision was not expressed at a call site", a fix pinned only by facts about that decision's INPUTS reproduces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #2299's classification half. The bounce underneath is NOT closed by this — see What this deliberately does not fix, so the issue stays open for it.
The defect: a safe default made a shipped predicate inert on the one leg that produces the fault
RoutingGrain.IsDeactivatedActivationrecognises Orleans'and answers
ErrorType.ShuttingDown. It is deliberately gated: the same text also means a per-node hub in a persistent activation-fault loop (a NodeType whose compile cannot materialise a hub configuration), which never recovers, so the registry is the discriminator andactivationErrorRecordeddefaults totrue— assume the worse case — for any caller that cannot consult it.BuildPodHubRoute's terminal arm passed that default, two arguments:So on the pod-hub leg the arm could never fire — and that is the leg the rejection names (
IPodHubGrain.Deliver) and where the evidence lives: 947 occurrences on #2299, every one of the newest samples, each logged verbatim as "surfacing Failed DeliveryFailure to sender …". The per-node-hub leg passesactivationErrorRecorded: !string.IsNullOrEmpty(activationError)and works. The predicate shipped, its facts were green, and production kept printing the pre-fix verdict in one word.Why
falseis a FACT on this leg, not a guessGrainActivationFailureRegistryis documented as holding the last activation failure "for each per-node-hub grain", and onlyMessageHubGrainwrites it.PodHubGrainhas no NodeType and no hub configuration to materialise, and its class contract is that it must not throw fromOnActivateAsync— the refusal is the CALL's answer (PodHubNotHereException). It cannot enter the loop the discriminator separates, so it never records an error and never could.ClassifyDeliveryExceptionsets.ClassifyPodHubDeliveryExceptionstates that once, with the argument attached, and the leg calls it.Why the verdict matters
SynchronizationStream's resubscribe latch andMeshNodeStreamCache's transient-owner rule ride outShuttingDownand tear down onFailed. The newest #2299 sample's sender is an agent thread (UWDeepfield/IndustryNews/_Thread/…), so the terminal verdict ended one agent round that the address's next claim would have served.What this deliberately does not fix
The re-activation bounce that produces the rejection: with
[PreferLocalPlacement], a delivery to a pod-hub address with no live owner activation is placed on the caller's silo, which holds no local route;PodHubGrain.Deliverthen requests its own deactivation and throws, so messages arriving in that window are answered transiently by Orleans and each of the six retries can place another throw-away activation. That is a lifecycle question with a real trade-off and no local repro — three candidates and their costs are written up on the doc page and left on #2299. It is not addressed here by narrowing the retry, which would reduce a symptom's volume rather than remove its cause.Verification
dotnet build src/MeshWeaver.Hosting.Orleans -c Release -warnaserror0 Error(s),0 Warning(s), 23 s (every dll emitted — not a no-op)dotnet build test/MeshWeaver.Hosting.Orleans.Test -c Release -warnaserror0 Error(s),0 Warning(s)AnswerPodHubCallFailureDocumentationLinkIntegrityTestThe facts assert the verdict on both sides of the discriminator for the identical exception, so the rule that CHOOSES between the two readings is visible rather than one member of it.
🚨 The first push pinned only the classifiers, and review was right that that reproduces the defect one level out — reverting the single line in
BuildPodHubRoutewould have left all eight facts green.TerminalCallFailureis now a one-line delegation toAnswerPodHubCallFailure, which holds both decisions of that arm (which classifier, and the level the verdict deserves), and two facts drive that function capturing what it hands the sender and the logger. The control row above is the measurement. The log LEVEL was unpinned for the same reason and mattered for the same one — a lifecycle transition reported atErroris what files an incident for a pod that was merely finishing (#2638) — and it is asserted on both sides, so it cannot be pinned to one value.Docs
ADepartedSiloIsNotADeliveryDefectalready carried this rejection as a table row ("its own predicate — a separate change"); the mechanism, the three bounce candidates and the pinned/not-pinned note go there.TheSubsystemInTheTitleIsNotTheCauseis new: the grouping of the six-ticket Orleans delivery cluster by causal arrow rather than by title, plus two corrected readings — a pod-hub address suffix is a mesh id, not an operation id (which moves RoutingGrain surfaces DeliveryFailure when the import-manifest activity's cancellation token cancels directed deliveries to import/cache pod hubs #5034 to the other side of the call), and the twomemory-streamstickets have different live owners.No addresses need recycling: this is
src/framework code carried by the image, with no NodeType, node content or layout area in the diff.🤖 Generated with Claude Code