Repository navigation
fix(#2543): a synchronous fault while building the flush is a NACK with a name, never silence - #3480
Conversation
…th a name, never silence CD 7937's bake host (core with #3464): 13 stalls, every trail ending at PATCH_ECHO_SEEN, zero PATCH_FLUSH_SUBSCRIBED, pool pendingWork=0 — the only code between the stages is the ack watcher's onNext, where a throw escapes the echo subscription. The watcher now catches while building the flush, records PATCH_FLUSH_FAULTED_SYNC building <Exception> and NACKs once with the classified error; StoragePostCommitFlush guards its ack-path service resolution so a closed scope faults the observable instead of escaping. Test falsified before the change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, aligns with the documented failure mode, and is covered by a targeted regression test plus updated diagnostic documentation.
Pull request overview
This PR hardens the mesh-node patch ack pipeline so that synchronous exceptions thrown while constructing the durable flush can no longer escape an Rx onNext and leave writers waiting in silence; instead, the owner produces a single retryable NACK and records a diagnostic stage naming the exception.
Changes:
- Wraps
ArmPatchAckWatcher’s flush construction in atry/catchso synchronous faults become a classified NACK and aPATCH_FLUSH_FAULTED_SYNC …stage instead of an unobservable stall. - Guards
StoragePostCommitFlush.FlushagainstObjectDisposedExceptionduring service resolution by surfacing it as an observable fault. - Adds a regression test for the “flush factory throws” case and documents the new trail stage in
DebuggingMessageFlow.md.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Data.Test/PatchAckTotalityTest.cs | Adds a test proving a synchronous flush-factory throw yields exactly one classified NACK and records the fault stage. |
| src/MeshWeaver.Hosting/Persistence/StoragePostCommitFlush.cs | Converts disposed-scope service-resolution throws into an observable fault rather than a synchronous escape. |
| src/MeshWeaver.Documentation/Data/Architecture/DebuggingMessageFlow.md | Documents the new PATCH_FLUSH_FAULTED_SYNC … trail meaning for #2543 debugging. |
| src/MeshWeaver.Data/DataExtensions.cs | Catches flush-construction exceptions inside the commit-echo onNext, records a stage, logs, and NACKs once to avoid silence. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 0)251 tests 251 ✅ 2m 12s ⏱️ Results for commit 7d55681. |
Test Results (shard 3)1 016 tests 1 016 ✅ 1m 4s ⏱️ Results for commit 7d55681. |
Test Results (shard 1)488 tests 488 ✅ 45s ⏱️ Results for commit 7d55681. |
Test Results (shard 5) 5 files 5 suites 1m 52s ⏱️ Results for commit 7d55681. |
Test Results (shard 4)1 597 tests 1 403 ✅ 2m 5s ⏱️ Results for commit 7d55681. |
Test Results (shard 2)1 048 tests 1 048 ✅ 3m 24s ⏱️ Results for commit 7d55681. |
Test Results 16 files 16 suites 11m 23s ⏱️ Results for commit 7d55681. |
#2543: a synchronous fault on the flush path is a NACK with a name, never silence
The observation (CD 7937's bake host, core
85ab1dfbe, which carries #3464): 13OwnerUnreachable … no verdict within 31sstalls; every trail ends atPATCH_ECHO_SEEN; zeroPATCH_FLUSH_SUBSCRIBED; every[STALE-CALLBACK]line readspendingWork=0. The starved-pool arm is refuted, andIoPool.Run/InvokebothSubscribeOn(TaskPoolScheduler.Default), so the storage write cannot park the subscribing thread. The only code between the two stages is the ack watcher's own onNext:flush(committed)—StoragePostCommitFlush.Flush, which resolves three services from the hub's provider — thenSubscribe. An exception thrown there escapes the commit-echo subscription's onNext (Rx routes an onNext throw to the producer, not to onError): no stage, no ack, no bound, silence. A closed lifetime scope makesGetServicethrow exactly there — the incident #3196 documents for the siblinglateVerdictsresolution — and a bake host recycles hubs en masse. Reading on the issue: #2543 (comment)What changes
ArmPatchAckWatcher: building the flush is inside try/catch; a throw recordsPATCH_FLUSH_FAULTED_SYNC building <Exception>, logs[PatchAck] FLUSH_FAULTED_SYNC, and NACKs once with the classified error — the verdict the async fault arm already posts, so the writer retries instead of waiting out its bound. (A rawSubscribethrow is not a case:Take(1)subscribes throughSubscribeSafe, which forwards it into the fault arm — measured, and the doc says so.)StoragePostCommitFlush.Flush: the three service resolutions are guarded;ObjectDisposedExceptionbecomes the observable's fault instead of a synchronous escape.DebuggingMessageFlow.md: the new stage row.Verified
PatchAckTotalityTest.AFlushFactoryThatThrows_NacksOnceWithTheClassifiedError_AndNamesTheException— before the change the throw surfaced out ofArmPatchAckWatcher(measured on the first run of this test); after: one NACK, the named stage, nothing subscribed. Ack suites 22/22; Documentation + guards 304/304;-c Release -warnaserrorclean on Data.Test, Memex.Portal.Shared.Test (Hosting closure), Documentation.Test.This does not yet prove the closed-scope cause — the next occurrence's trail names the exception, which is the proof. It does close the silence: a stall in this arm is now a retryable NACK. Refs #3355.