Repository navigation
fix: an upsert always answers (#2454), and CD gains a force-rebuild button (#2622) - #2825
Merged
Merged
Conversation
… completes empty HandleCreateOrUpdateNodeRequest subscribed to its existing-node read with TWO arms — a value and a fault — and returned request.Processed() immediately. A source that COMPLETES WITHOUT EMITTING matches neither arm, so no response was ever posted while the delivery had already been marked handled. That is #2454's signature verbatim, and it is not theoretical. Measured on shard 3 of run 33325655611: a per-node hub that could not quiesce — Hub Cornerstone/Insured RunLevel=Quiescing Disposal=Pending PendingCallbacks=1[CreateOrUpdateNodeRequest] CreateOrUpdateNodeRequest: AWAITING … → HANDLER_ENTER@portal/nodeops-… → HANDLER_EXIT state=Processed ⇒ a handler was entered and no reply, completion or fault has been recorded since — holding an entire mesh disposal open for 19 s, and taking the test's 20 s method timeout with it. #2454 recorded the identical trail on RolePlay/Story. ## Why DefaultIfEmpty would be the WRONG repair DefaultIfEmpty() emits null, and this handler's value arm reads null as "the node does not exist" and turns it into a CREATE. An empty read means the read produced no answer — not that the node is absent. Inventing a create from it is a silent write on missing information, which is worse than the hang it replaces. So empty gets its own outcome and that outcome is a refusal: the caller is told the read produced no result, so create-vs-update could not be decided, and it can retry. Same idiom as SelfUpdateHostedService's .DefaultIfEmpty(SelfUpdateVerdict.NoOutcome()) and for the reason stated there: "the only thing that stops a third one being added is making 'produced nothing' itself an outcome that gets reported." ## The test Decorates the registered IStorageAdapter so exactly one Read — on a marked path — completes empty; everything else forwards to the real adapter, so the guard and version-writing chain stay intact and the mesh does all the real work. Mutation-checked, and the first two attempts were FALSE PASSES worth recording: a mutation that does not compile leaves --no-build running the previous binary, which reports green. The third attempt used a runtime-true condition the compiler cannot fold, verified "Build succeeded" BEFORE trusting the result, and then failed correctly: Expected the observable to emit a value within 20s … but it did not. The observable emitted nothing at all. Failed: 1, Duration: 20 s dotnet build src/MeshWeaver.Mesh.Contract -c Release -warnaserror 0 Error(s) dotnet test --filter UpsertEmptyReadIsAnsweredTest|CreateOrUpdateNodeRequestTest 12 passed, 0 failed Fixes #2454 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FhP7Jg65q8cAATnFfeaTK
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a request/response correctness hole in HandleCreateOrUpdateNodeRequest where an existing-node read that completed without emitting could leave the caller without any response, despite the handler already returning Processed()—matching the teardown/quiesce hang reported in #2454.
Changes:
- Update the upsert handler to explicitly handle the “completed empty” terminal state by posting a refusal response instead of remaining silent.
- Add an integration-style regression test that decorates
IStorageAdapter.Readto complete empty for one marked path and asserts the caller receives a refusal. - Add a What’s New entry documenting the failure mode and the behavioral change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/UpsertEmptyReadIsAnsweredTest.cs | Adds regression coverage by forcing a single storage read to complete empty and asserting the upsert answers with a refusal. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Adds an onCompleted arm to ensure the upsert always answers, including when the existing-node read completes empty. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-30-an-upsert-always-answers-its-caller.md | Documents the bug, impact, and why “completed empty” must be answered as a refusal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
3918
to
+3922
| PostFail(grantRejection, NodeUpsertRejectionReason.ValidationFailed); | ||
| return Observable.Empty<MeshNode?>(); | ||
| }); | ||
|
|
||
| // 🚨 THREE terminal states, not two (MeshWeaver#2454). A two-arm Subscribe answers the |
Contributor
…nge can ship CD's image-set check is keyed on CORE's HEAD sha. The portal hosts live in MeshWeaver.Plugins, so a merge there that edits a file shipping IN the image (appsettings, csproj) changes what the image should contain and changes nothing this check can see. Measured when it bit: Plugins#814 fixed fresh-install engine activation by editing src/Memex.Portal.Distributed/appsettings.json at 14:54Z; the newest image, ci.6327, was built at ~14:4x from core bf1ae3a and therefore predated it — with no producer that would ever rebuild. Delivery had nothing to notice. 🚨 And a manual dispatch was a STRUCTURAL NO-OP for exactly that case: it takes reason=reconcile → check-image-set.sh <core-sha> → COMPLETE=true → bake_only. So an operator who KNEW the hosts had changed had no way to say so. `workflow_dispatch` now takes `rebuild` (boolean, default false). When set, the decide step's complete-image-set branch is skipped and the full set is built, with its own decision line naming why — so a forced build is never mistaken for an ordinary publish in the log. ## This is the escape hatch, and the commit says so Option 1 of the three the issue lists. The durable shape is option 2 — record the plugins sha the image was baked from and have check-image-set.sh compare it against plugins@main — so the RECONCILER notices rather than a human who happens to know. That stays open; this only means someone who does know can now act. ## Verified through the extracted step, not a copy test-cd-steps.py executes the real `decide` body (extracted from the workflow by `id:`, never copied) and gains three cases: * rebuild=true overrides a COMPLETE set and publishes; * ...and says it was an explicit rebuild, not an ordinary publish; * rebuild=false leaves the bake-only branch exactly as it was — without this, "the override works" would also pass if the override were always on. Mutation-checked: removing the `FORCE_REBUILD` guard fails "rebuild=true overrides a COMPLETE image set and publishes"; restored, all pass. python3 .github/scripts/test-cd-steps.py all cases passed, 2 steps python3 .github/scripts/check-workflow-shell.py clean Refs #2622 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FhP7Jg65q8cAATnFfeaTK
rbuergi
changed the title
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HandleCreateOrUpdateNodeRequestsubscribed to its existing-node read with two arms — a value and a fault — and returnedrequest.Processed()immediately. A source that completes without emitting matches neither, so no response was ever posted while the delivery had already been marked handled.That is #2454's signature verbatim, and it is not theoretical. Measured on shard 3 of run 33325655611:
— a per-node hub holding an entire mesh disposal open for 19 s, taking the test's 20 s method timeout with it. #2454 recorded the identical trail on
RolePlay/Story.Why
DefaultIfEmptywould be the wrong repairDefaultIfEmpty()emitsnull, and this handler's value arm readsnullas "the node does not exist" and turns it into a create. An empty read means the read produced no answer — not that the node is absent. Inventing a create from it is a silent write on missing information, which is worse than the hang it replaces.So empty gets its own outcome, and that outcome is a refusal: the caller is told the read produced no result, so create-vs-update could not be decided, and it can retry.
Same idiom as
SelfUpdateHostedService's.DefaultIfEmpty(SelfUpdateVerdict.NoOutcome()), and for the reason stated there — "the only thing that stops a third one being added is making 'produced nothing' itself an outcome that gets reported."The test
Decorates the registered
IStorageAdapterso exactly oneRead— on a marked path — completes empty; everything else forwards to the real adapter, so the guard and version-writing chain stay intact and the mesh does all the real work. Not a mock of the mesh.Mutation-checked — and the first two attempts were false passes worth recording. A mutation that does not compile leaves
--no-buildrunning the previous binary, which reports green. The third attempt used a runtime-true condition the compiler cannot fold, verifiedBuild succeededbefore trusting the result, and then failed correctly:Fixes #2454
🤖 Generated with Claude Code
https://claude.ai/code/session_014FhP7Jg65q8cAATnFfeaTK
CD gains a force-rebuild button — #2622 (option 1 of three)
CD's image-set check is keyed on core's HEAD sha. The portal hosts live in
MeshWeaver.Plugins, so a merge there editing a file that ships inside the image (appsettings, csproj) changes what the image should contain and changes nothing this check can see.Measured when it bit: Plugins#814 fixed fresh-install engine activation by editing
src/Memex.Portal.Distributed/appsettings.jsonat 14:54Z; the newest imageci.6327was built at ~14:4x from corebf1ae3aand therefore predated it — with no producer that would ever rebuild.🚨 And a manual dispatch was a structural no-op for exactly that case: it takes
reason=reconcile→check-image-set.sh <core-sha>→COMPLETE=true→bake_only. An operator who knew the hosts had changed had no way to say so.workflow_dispatchnow takesrebuild(boolean, defaultfalse). When set, the complete-image-set branch is skipped and the full set is built, with its own decision line — so a forced build is never mistaken for an ordinary publish in the log.This is the escape hatch, not the fix. The durable shape is option 2 — record the plugins sha the image was baked from and have
check-image-set.shcompare it againstplugins@main— so the reconciler notices rather than a human who happens to know. That stays open; the issue is referenced, not closed.Verified through the extracted step, not a copy.
test-cd-steps.pyexecutes the realdecidebody (found byid:, never copied) and gains three cases — the override publishes; it says it was an explicit rebuild; andrebuild=falseleaves the bake-only branch exactly as it was. That last one matters: without it, "the override works" would also pass if the override were always on.Mutation-checked: removing the
FORCE_REBUILDguard fails the first case; restored, all pass.Refs #2622