fix(#2229): create-then-point-read of an activity can NotFound-race the router by rbuergi · Pull Request #2359 · Systemorph/MeshWeaver

rbuergi · 2026-08-26T10:32:21Z

Summary

Issue #2229 item A stayed open after #2244/#2267 because the fix landed for exactly one caller
(PackageInstaller.WaitForGating) and the issue explicitly asked for a systematic audit of
every other exact-path GetMeshNodeStream(path) read of a node that may not exist yet — never
a mass-conversion, only genuine, evidenced candidates.

This PR is that audit's first pass: it fixes three call sites that share the exact defect
shape described in Doc/Architecture/CqrsAndContentAccess.md → "An OPTIONAL node", and reports
(without changing) a few lower-confidence findings for anyone continuing the audit.

The defect, in these three call sites

ActivityRunner.RunActivity, ContentIndexingActivity.Run, and MarkdownViewLogic.CreateActivityAndSubmit
all follow the same shape: meshService.CreateNode(activityNode) succeeds, and — in the very same
reactive chain — a point subscribe (workspace.GetMeshNodeStream(activityPath)) opens immediately to
watch for cancel (the first two) or to confirm the kernel activity became routable before posting
code submissions (the third).

CreateNode completing proves the node is persisted, not that it is routable yet. Per
#2229 item A's read-side findings, the read route can answer an authoritative NotFound for a path
whose write already landed — sometimes for minutes under load. An exact-path GetMeshNodeStream
hitting that NotFound terminates the stream with an error rather than waiting, and (per
MeshNodeStreamCache's storm-breaker) that error can suppress subsequent writes to the same path
for a backoff window — i.e. the activity's own later progress/completion writes.

Concretely:

ActivityRunner's and ContentIndexingActivity's cancel-watch subscribed with no onError
handler — an unhandled OnError rethrows on whatever thread is delivering it.
MarkdownViewLogic's Timeout(15s) read does handle the error, but the interactive kernel then
simply never activates for that render (already-logged, but silent to the user beyond "Starting
interactive kernel…" never resolving).

The fix

Updated after review (see the resolved threads below): the first version of this PR gated the
point subscribe on a scope:children listing of the whole _Activity container. Copilot correctly
flagged that as expensive; on reflection it's worse than expensive — _Activity is explicitly
un-pruned (StaticRepoImport.md), so a listing anchored at the container can page past the very
activity it's looking for once the partition has enough of them. That is the identical false
"not there yet" this PR exists to remove, reintroduced by a rarer, harder-to-diagnose route.

All three now use an exact-path query instead — meshService.Query<MeshNode>("path:{activityPath} nodeType:Activity"), no scope: qualifier and no select: projection. path:{x} with no scope:
is QueryScope.Exact, documented in QueryParser.cs as "answered ZERO ROWS with no error" for an
absent path — empty-on-absent, no routing NotFound, no storm-breaker window, exactly like a
listing, but it can return at most one row so it can never page past the target. Gate on
change.Items.Count > 0, then .SelectMany into the point subscribe once corroborated. This is the
identical shape PackageInstaller.WaitForGating already uses (and its own comment names the same
paging hazard) — see src/MeshWeaver.PluginCatalog/PackageInstaller.cs around lines 490-520.

Audit scope and confidence

Enumerated exact-path GetMeshNodeStream(/GetMeshNode( call sites across src/, memex/
(excluding Memex.Portal.Shared), and tools/ (excluding MeshWeaver.Blazor* — under concurrent
restructuring per this session's constraints): 299 call sites in .cs (my own count landed at
~284–298 by a slightly different filter — close enough to confirm the two counts agree). The
issue's originally-cited "560 across 216 files" included 261 markdown-prose matches under
src/MeshWeaver.Documentation/Data/** (API docs describing the pattern, not call sites) — 299 is
the correct code-only figure.
Narrowed to candidates by co-occurrence of an exact-path call with an optional-satellite segment
(_Access, _Notification, _Comment, _Approval, _Tracking, _Usage, _Activity,
_Thread) or absence-aware language (may not exist, not yet routable, does not exist,
absent, …) in the same file — ~37 files — and read each one's surrounding code.
Fixed (this PR): ActivityRunner.cs, ContentIndexingActivity.cs, MarkdownViewLogic.cs —
see above.
Reviewed and already correct (no change): PackageInstaller.cs (fixed in Read an optional node with a listing AND a stream, not one or the other #2244/571707c3c),
StaticRepoImporter.cs (EnsureRoot already gates its point read on a query snapshot; the
manifest-existence checks already use the non-erroring hub.GetMeshNode primitive),
MeshExtensions.cs's partition-bootstrap reads (raw IStorageAdapter.Read + .Catch, not
GetMeshNodeStream — never errors), TokenUsage.cs (already migrated off a point read per its own
doc comment), AiSettingsNodeType.cs, GitHubSyncService.cs, MeshNodeExtensions.cs,
AgentChatClient.cs's provider-selection read (already query-based per its own comment),
NodeTypeCompilationActivity.cs (already sequences create-before-emit so nothing point-reads the
new activity), EventSubscriptionOps.cs, BuildProtocolDriver.cs's OpenChunk/FinishActivity
(same activity-create shape, but a real bake runs between create and the later .Update, so the
race window is materially smaller than the three fixed sites' same-tick subscribe — flagging
rather than changing).
Reported, not fixed (lower confidence / no live caller in this repo):
- StaticRepoImporter.ReadClaimedRoots point-reads each partition root directly (no listing gate),
  but wraps the whole thing in .Catch that degrades to null — so a race here costs at most one
  extra re-sync pass, not a crash or suppressed write. Lower severity; left as-is.
- IssueService.WatchIssue / (likely) the analogous PullRequestService method: their doc comments
  promise "or null when absent", but an exact-path GetMeshNodeStream actually errors on
  absence rather than emitting null — a doc/behavior mismatch. Grep found no non-test caller in
  this repo (test-only), so I did not change it; worth a look if a production caller appears.

Honest exhaustiveness statement: I audited by keyword/pattern co-occurrence, not line-by-line
across all 299 call sites — consistent with the issue's own "this is judgment, not grep" framing. 37
of 299 files were triaged by the satellite-segment/absence-language predicate. I'm confident in the
three fixes and in the "already correct" list I actually read. I cannot claim the audit is
exhaustive; issue #2229 should stay open for whoever picks up the remaining ~262 sites outside this
pattern-driven sample (or narrows the search further).

Verification

dotnet build -c Release -warnaserror clean on MeshWeaver.GitSync,
MeshWeaver.ContentCollections.Indexing.Graph, MeshWeaver.Markdown.
Existing tests exercising the changed paths, run against a fresh build (all green, fresh .trx):
- GitHubActivityTest (4/4, including RunActivity_Cancel_TripsCommandTokenAndEndsCancelled)
- ContentIndexingPipelineTest (3/3)
- KernelOwnerAnchorIntegrationTest / KernelSubscribeGateTest / KernelSubscribeGateIntegrationTest (6/6)
- WhatsNewEntryIntegrityTest, DocumentationLinkIntegrityTest (1/1 each)

Does not close #2229 — the audit is not exhaustive (see above).

🤖 Generated with Claude Code

…he router Item A's audit: three call sites subscribed workspace.GetMeshNodeStream(activityPath) immediately after meshService.CreateNode(activityNode) succeeded, to wait for the just-created progress activity to become live (a cancel watcher in ActivityRunner and ContentIndexingActivity; a routability gate in MarkdownViewLogic's interactive-kernel activation). CreateNode completing proves the node is PERSISTED, not that it is ROUTABLE yet — the read route can answer an authoritative NotFound for a path whose write already landed (#2229 item A: "the create SUCCEEDS and the REPLY is lost"), which TERMINATES the exact-path stream with an error instead of waiting. ActivityRunner's and ContentIndexingActivity's cancel-watch subscribed with no onError handler, so a NotFound there rethrows on whatever thread delivers it. MarkdownViewLogic's Timeout(15s) read handles the error but the interactive kernel then never activates. Fixed all three with the sanctioned composition from CqrsAndContentAccess.md → "An OPTIONAL node": a live children LISTING for existence (empty-on-absent, never errors) gates the point subscribe, which only opens once the index has corroborated the node. Audited ~40 files where an exact-path GetMeshNodeStream/GetMeshNode call co-occurs with an optional-satellite path segment (_Access/_Notification/_Comment/_Approval/_Tracking/_Usage/ _Activity/_Thread) or absence-aware language, out of ~284-298 with-argument call sites across the non-Blazor/non-Portal.Shared surface. Most are already correct (own-node reads, reads of nodes guaranteed to exist by the same reactive chain, or already-fixed listing- gated patterns from earlier #2229 work). See the PR description for the full candidate list, including lower-confidence findings left unfixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Systematically addresses the #2229 “create-then-point-read” NotFound race for newly-created Activity nodes by gating exact-path stream subscriptions on an existence-confirming query, preventing stream termination/storm-breaker side effects and improving reliability for activity cancellation watchers and interactive markdown kernel startup.

Changes:

Gate activity cancel watchers in GitSync and content indexing on a query before attaching GetMeshNodeStream(activityPath), and add an onError handler to avoid unhandled OnError rethrows.
Gate markdown-kernel activity routability on a query before opening the activity stream, avoiding the “persisted but not routable yet” window.
Add a What’s New entry documenting the user-visible reliability fix.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/MeshWeaver.Markdown/MarkdownViewLogic.cs	Gates newly-created markdown activity stream subscription on a query-backed existence check before posting kernel submissions.
src/MeshWeaver.GitSync/ActivityRunner.cs	Gates the cancel watcher’s activity stream subscription on a query before attaching and logs failures via `onError`.
src/MeshWeaver.ContentCollections.Indexing.Graph/ContentIndexingActivity.cs	Same cancel-watch gating pattern as GitSync for indexing activities, plus `onError` handling.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-26-activity-notfound-race-fix.md	Documents the fix as a user-visible reliability improvement.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

rbuergi · 2026-08-26T10:38:58Z

Holding this on green intentionally — do not rerun or merge past the current CI red; it is not caused by this PR.

main itself is red on the same job, three consecutive runs:

32955801178 (5b1901f)
32956130892 (b487efc)
32956976911 (f98a96b, current tip)

All three fail on Landed modules compile against this PR:

Multiple components use the tag 'MemexMobileMenu'.
The type 'MainLayout' / 'Routes' / 'Onboarding' in .../plugins-repo/src/Memex.Portal.Gui/...

Root cause: the GUI extraction landed its additive half (plugins #674) while the deletion half
(#2293, "Delete Blazor and the portal GUI from core", +36/-135237) is still open — so core and the
plugins repo both currently declare the same Blazor components, and this gate correctly reports
the collision. It's a real defect in the current main, not a flake, and not something this PR's
diff touches (this PR does not go near memex/Memex.Portal.Shared/ or src/MeshWeaver.Blazor*,
per this session's constraints).

This PR's own CI will hit the identical red once it reaches that job — expected, not a regression
introduced here. Leaving it as-is rather than rerunning or attempting to route around it. Will
merge once #2293 lands (or otherwise resolves this on main) and CI is genuinely green.

github-actions · 2026-08-26T10:47:29Z