Skip to content

fix(webhook-inbox): a stalled target read answers 503 + Retry-After, not a bare 500 (#6125) - #6322

Merged
rbuergi merged 2 commits into
mainfrom
fix/6125-6039-webhook-inbox
Oct 8, 2026
Merged

rbuergi merged 2 commits into
mainfrom
fix/6125-6039-webhook-inbox

Conversation

@rbuergi

@rbuergi rbuergi commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

#6125: the root cause

memex/Memex.Portal.Shared/Api/WebhookInboxEndpoints.cs:108 (before this change) awaited WebhookInbox.Deliver(...) with no mapping for availability faults. Deliver's target-existence read (path:{target}, WebhookEventNodeType.cs Store) can end in QueryProviderStalledException when a provider misses the fan-in's Initial budget. That exception escaped the handler, so ASP.NET answered a bare 500.

The fix: the delivery verdict is mapped to an IResult in a Select, then passed through InstanceAuthResponses.UnavailableOnAStalledRead, the same convention the plugin-bundle routes use (#5345). Only what AreaErrorClassifier.IsStorageUnavailable classifies becomes 503 + Retry-After. Every other fault still escapes. ReadUnavailable and UnavailableOnAStalledRead gain an optional error text, so the 503 body names what this route could not read ("nothing was stored; retry shortly"). The bundle routes keep their wording because the default is unchanged.

The underlying pg-read starvation (#5315 / QueryFanInStallTerminal) is untouched, and QueryInitialBudget is not changed.

Test and negative control

test/Memex.Portal.Shared.Test/WebhookInboxStalledReadTest.cs uses a real TestServer host over the real mesh. A test IMeshQueryProvider, the same harness as PluginBundleStalledReadTest, stalls or faults only the target read. The stall fires at the production rung. There are three cases:

  • a stalled read answers 503, with Retry-After set to 5 s and the route's own error text;
  • an answered read still answers 200 accepted;
  • an ordinary fault still escapes and is not answered 503.

Negative control, run locally: with the UnavailableOnAStalledRead line commented out, AStalledTargetRead_Answers503WithRetryAfter FAILS with the production exception verbatim (QueryProviderStalledException … for query 'path:Hosting/StalledInbox' (user 'system-security') at WebhookInboxEndpoints.Deliver). With the line in, all 8 Webhook* tests pass, and PluginBundleStalledReadTest passes 4/4.

#6039: investigated, no code change; findings committed to Doc/Architecture/WebhookInbox

Closes #6125 is deliberately not used, because sev:M issues close after deployed verification. Refs #6125. Refs #6039.

Pairs-with: none — no public type or member removed (one optional parameter added to two internal methods)
Mirror-sync: none — no i18n catalog change

🤖 Generated with Claude Code

…not a bare 500 (#6125)

WebhookInboxEndpoints.Deliver let QueryProviderStalledException from its
path:{target} existence read escape, so ASP.NET answered a bare 500. Map it
through InstanceAuthResponses.UnavailableOnAStalledRead (classifier
AreaErrorClassifier.IsStorageUnavailable), exactly as the plugin-bundle routes
do; every other fault still escapes. ReadUnavailable takes an optional error
text so the body names what this route could not read.

Also records the #6039 investigation on the WebhookInbox doc page: no
post-creation handler matches WebhookEvent, and the reply's dispatch needs the
nodeops block regardless of the issuing hub (the responder enqueues its own
outgoing reply), so no endpoint-local change decouples it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new test uses an unsafe reactive impersonation scope that can leak System identity across threads.

1 open finding
What changed in this PR

Maps stalled webhook target reads to retryable HTTP 503 responses and documents related delivery behavior.

Changes:

  • Adds 503/Retry-After mapping with route-specific error text.
  • Adds integration coverage for stalled, successful, and ordinary-fault cases.
  • Documents #6125 and the #6039 investigation.
File Description
memex/​Memex.Portal.Shared/​Api/​WebhookInboxEndpoints.cs Maps availability failures to 503 responses.
memex/​Memex.Portal.Shared/​Api/​InstanceAuthResponses.cs Supports route-specific 503 error text.
test/​Memex.Portal.Shared.Test/​WebhookInboxStalledReadTest.cs Tests webhook target-read outcomes.
src/​MeshWeaver.Documentation/​Data/​Architecture/​WebhookInbox.md Documents behavior and investigation findings.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment on lines +124 to +133
await Observable.Using(
() => access.ImpersonateAsSystem(),
_ => Mesh.ServiceProvider.GetRequiredService<IMeshService>().CreateOrUpdateNode(
new MeshNode(Target)
{
Name = Target,
NodeType = "Markdown",
Content = new MarkdownContent { Content = "# Inbox target\n" },
}))
.FirstAsync().Timeout(TestTimeouts.Convergence).Await(ct);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct — fixed in 2c1b4ed. The target seed now goes through access.RunAsSystem(() => …CreateOrUpdateNode(...)), the sealed boundary WebhookInbox.Store itself uses (#1790), so the impersonation enters and leaves inside one synchronous Subscribe and cannot latch the test thread as System. Rebuilt Release with -warnaserror (0 errors / 0 warnings); all 8 Webhook* tests in Memex.Portal.Shared.Test pass.

…Using (review)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

    22 files  +    1      22 suites  +1   48m 17s ⏱️ + 2m 47s
11 144 tests +1 330  10 951 ✅ +1 330  193 💤 ±0  0 ❌ ±0 
11 157 runs  +1 333  10 964 ✅ +1 333  193 💤 ±0  0 ❌ ±0 

Results for commit 2c1b4ed. ± Comparison against base commit 3fc485c.

This pull request removes 29 and adds 1341 tests. Note that renamed tests count towards both.

   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---, expected: True)
   --- End of inner exception stack trace ---, isDenial: True)
 ---> (Inner Exception #1) MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432<---
 ---> (Inner Exception #1) System.InvalidOperationException: boom<---
 ---> (Inner Exception #1) System.InvalidOperationException: source B is misconfigured<---
 ---> (Inner Exception #1) System.Net.Sockets.SocketException (0xFFFDFFFF): Name or service not known<---
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
 ---> System.AggregateException: One or more errors occurred. (Failed to connect to 10.42.18.4:5432)
…
Memex.Portal.Shared.Test.InstanceIdRulesMatchTheRegistryTest ‑ TheSetupHostAgreesWithTheRegistry(candidate: "fec53f59-6c80-436a-ae1c-90decad9ab50")
Memex.Portal.Shared.Test.SessionDenialIsAnAnswerTest ‑ OnlyAVerdictReadsAsADenial(shape: "the same verdict nested, as a late denial dispatch"···, failure: System.InvalidOperationException: write failed
 ---> System.UnauthorizedAccessException: Access denied
   --- End of inner exception stack trace ---, isDenial: True)
Memex.Portal.Shared.Test.WebhookInboxStalledReadTest ‑ AStalledTargetRead_Answers503WithRetryAfter
Memex.Portal.Shared.Test.WebhookInboxStalledReadTest ‑ AnAnsweredTargetRead_StillAccepts
Memex.Portal.Shared.Test.WebhookInboxStalledReadTest ‑ AnOrdinaryFault_OnTheTargetRead_IsNotConvertedTo503
MeshWeaver.Compiler.Pipeline.Test.AFailedCompileCarriesTheSetItConsumedTest ‑ TheFailureResult_FoldsTheConsumedSetUnderTheSameRuleAsTheLiveSnapshot
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ APrebuiltAdoption_RetiresThePreviousBuildsPendingRelease
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ AReleaseAtAnotherPath_IsNotAdopted_AndTheStampStands
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ ARelease_LandingAtTheStampedPath_IsAdopted
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ AReplayOfThePendingBuild_PreservesItsMarker
…

♻️ This comment has been updated with latest results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Webhook inbox delivery answers a bare 500 on a fan-in stall instead of 503, and is starved by the known pedestrian-walk pg-read head-of-line blocking

2 participants