Repository navigation
docs: Timeout DOES manufacture a termination — fix the over-correction from #3919 - #3921
Conversation
…review, #3919) The correction over-corrected. "No composition over a source can invent a termination it never produces" is false, and self-contradicting two sentences later: `Timeout` is exactly such a composition, and the same paragraph then points at the two bounds that use it. The limitation is specific to `DetachedReplyOutcome.Of` and to WHY: every operator it applies — Take(1), Select, Catch, DefaultIfEmpty — REACTS to a termination; none MANUFACTURES one. `Timeout` manufactures one, which is what makes it the right tool for the fourth outcome. Compose `Of` AROUND a Timeout and both properties hold: the timeout supplies liveness, `Of` turns whichever termination arrives into an answer. Fixed at all three sites Copilot named — the type's remarks, the `Of` summary, and the doc page's caveat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7
There was a problem hiding this comment.
🟡 Changes recommended
Narrow the silent-source wording and address the uncovered NodeType resolution path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Documentation-only follow-up clarifying termination versus liveness for detached Rx replies.
Changes:
- Refines
DetachedReplyOutcomedocumentation. - Updates write-verdict architecture guidance and examples.
File summaries
| File | Summary |
|---|---|
src/MeshWeaver.Mesh.Contract/DetachedReplyOutcome.cs |
Clarifies Of and Timeout termination behavior. |
src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md |
Aligns architecture guidance with the revised explanation. |
Review details
Suppressed comments (2)
src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md:472
- The list omits the NodeType gate, which is another detached-reply leg:
ApplyUpdateViaStreamwrapsNodeTypeResolution.Resolves(...)inOfwithout aTimeout, andResolvesdelegates toIStorageAdapter.Exists(...).Take(1). The normal persistence implementation usesMerge().Any(...), so a lost/stalled adapter can leave an absent type non-terminating and the caller still receives no reply. Add a bound to that leg or explicitly document it as an uncovered case rather than implying the starvation-prone legs are covered by the two examples here.
manufactures one is `Timeout`, and it is separately present where the source can starve:
`InnerCreateVerdictBound` on the inner create, `NodeOpForwardTimeout` inside the no-op probe.
src/MeshWeaver.Mesh.Contract/DetachedReplyOutcome.cs:74
- The summary repeats an overly broad statement:
Take(1)manufactures an output completion after a value, andCatchreplaces an error with a newObservable.Returnsequence. WhatOfcannot do is manufacture a termination from a source that stays silent; narrowing this sentence avoids documenting Rx behavior incorrectly.
/// answer a source that TERMINATES. <b>It adds no deadline</b> — none of its operators
/// manufactures a termination, they only react to one — so a source that never terminates still
/// reaches nobody. Bounding that is a <c>Timeout</c> the caller composes UNDER this (see the
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "The FOURTH outcome" above — reaches no arm at all, and `Of` cannot rescue it, because every | ||
| operator it applies **reacts** to a termination and none **manufactures** one. The operator that | ||
| manufactures one is `Timeout`, and it is separately present where the source can starve: |
There was a problem hiding this comment.
Same fix, applied here too — see the thread on DetachedReplyOutcome.cs. The doc caveat now spells out which notification each operator waits for (Take(1) after a value, Catch after a fault, DefaultIfEmpty on a completion), says that none can fire from a source that has produced nothing, and names Timeout as the one clock-driven operator and therefore the only one that can terminate a silent source.
| /// — reaches no arm at all, and <c>Of</c> cannot rescue it: every operator it applies | ||
| /// (<c>Take(1)</c>, <c>Select</c>, <c>Catch</c>, <c>DefaultIfEmpty</c>) reacts to a termination and | ||
| /// none MANUFACTURES one. Read the guarantee strictly: <i>if the source terminates, exactly one | ||
| /// outcome is emitted.</i> The operator that DOES manufacture a termination is <c>Timeout</c> — it |
There was a problem hiding this comment.
Right on both counts, and the narrowed version is more useful than the one it replaces — thank you for pushing on it three times.
The distinction that actually holds is notification-driven vs clock-driven:
- Every operator
Ofapplies needs an upstream notification before it can do anything.Take(1)synthesises a completion — only after a value.Catchsubstitutes a sequence — only after a fault.DefaultIfEmptyfires only on a completion. From a source that has produced nothing at all, none of them can fire. Timeoutis the exception because it is driven by a clock, not by the source — which is precisely why it is the one composition that can terminate a silent source, and why a leg whose source can starve needs one of its own.
Applied at all three sites. Pushed to this branch directly, which is the point of opening it as a draft: the previous two rounds were auto-armed and enqueued before the review arrived, and a queued branch cannot be updated.
…ion (Copilot review, #3921) "None of Of's operators manufactures a termination" is not literally true, as the review points out: Take(1) DOES synthesise an OnCompleted, and Catch DOES switch to a Return sequence that completes. The accurate limitation is narrower and more useful. Every operator Of applies is NOTIFICATION-DRIVEN — each needs an upstream notification before it can do anything: Take(1) synthesises a completion only AFTER a value, Catch substitutes only AFTER a fault, DefaultIfEmpty fires only ON a completion. From a source that has produced nothing at all, none of them can fire. Timeout is the exception, and naming WHY is the part worth writing down: it is CLOCK-driven, so it is the one composition that can terminate a silent source. That is the whole reason a leg whose source can starve needs one of its own. Third and final narrowing of this claim; each round moved it from "any source" → "if the source terminates" → this. Applied at all three sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7
Follow-up to #3919 (merged). Documentation only.
🚨 Opened as a DRAFT on purpose.
auto-arm.ymlarms every non-draft PR, and both previous PRs in this chain were enqueued before the Copilot review landed — a queued branch cannot be updated, so each round of review had to become another PR. Draft is the documented opt-out. It will be marked ready once review has landed and been addressed, which is where this loop stops.The over-correction
#3919 replaced one overbroad claim with another. It said:
That is false, and self-contradicting two sentences later — the very same paragraph names
Timeoutas what supplies liveness, andTimeoutis a composition over a source that manufactures anOnErrorwhen the source is silent. Copilot flagged it at all three sites.The fix, stated positively
The limitation is specific to
DetachedReplyOutcome.Of, and now the reason is given rather than the claim merely narrowed:Timeoutmanufactures one, which is exactly what makes it the right tool for the fourth outcome. So the guidance is now actionable instead of merely cautionary:which is, not by accident, what
DispatchInnerCreate(.Take(1).Timeout(InnerCreateVerdictBound)) and the no-op probe (.Timeout(NodeOpForwardTimeout)) already do.Applied at the three sites named: the type's remarks, the
Ofsummary, and the doc page's caveat.Verification
Mesh.Contract,Documentation,Documentation.Testeach0 Error(s)in Release with-warnaserror;DocumentationLinkIntegrityTestgreen.Chain: #3915 (the fix, merged) → #3919 (narrowed two overclaims, merged) → this. Related: #3917 (the pre-existing i18n gap in the upsert failure surface), #3674.
🤖 Generated with Claude Code
https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7