Repository navigation
fix(#3432): make delete success visible to immediate reads - #3976
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new delete “wait for absence” phase needs a timeout to avoid indefinite hangs, and the new regression test’s parked-actor invocation should not swallow unexpected faults (it should record/assert them).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the read-after-delete consistency contract for unified deletes by ensuring “delete succeeded” is not reported until the shared reduced read stream has actually observed the entity’s absence, preventing immediate follow-up reads from replaying the pre-delete value.
Changes:
- Updated
DeleteUnifiedReferenceRequesthandling fordata:paths to wait for the shared entity stream to emit anullframe before returning success. - Added a deterministic regression test that parks the shared read stream actor to prove success cannot escape before the absence frame lands.
- Documented the behavioral contract and added a “What’s New” fix note describing the user-visible impact.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Data.Test/ReadPathStreamMintingTest.cs | Adds an actor-boundary regression test ensuring delete success is withheld until the shared read view reflects absence. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-10-delete-success-waits-for-the-read-view.md | Adds a user-facing release note explaining the immediate-read-after-delete behavior change. |
| src/MeshWeaver.Documentation/Data/Architecture/ReadPathStreamMinting.md | Documents the propagation boundary between owner stream commit and shared reduced read stream visibility, and records the new regression coverage. |
| src/MeshWeaver.Data/DataExtensions.cs | Changes delete handling to wait reactively for the shared read stream’s absence frame before reporting success. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Data point from main-cd: the UPDATE path has the same shape — run 8302 ( |
| // before that queued frame lands. Wait on the exact read view this API | ||
| // serves; no polling and no fresh stream/hub. Success now means a read made | ||
| // after the response cannot observe the deleted entity (#3432 follow-up). | ||
| return stream |
There was a problem hiding this comment.
[P1] Subscribe to the deletion acknowledgement before starting the eager write
RequestChange invokes WorkspaceOperations.Change eagerly, but this absence subscription is created only after its commit result is observed. If the delete and a legitimate same-ID recreation both reach the shared read stream before this subscription attaches, its ReplaySubject(1) supplies only the recreated non-null entity. No further null is required, so an already committed delete never replies. Observe a causal acknowledgement before issuing the mutation, and cover delete → recreate → late acknowledgement subscription in a deterministic regression. The current regression parks the reduction until the absence arrives, so it does not exercise this case.
|
Current-head review before delivery: 7d23f7c still has the unaddressed causal acknowledgement race in #3976 (comment). The newest failing CD34526450095 job103043584774 also supplies a separate update symptom: SAV #1625 / Education #313 / Reinsurance #193 remain held. The proposed #3976 then #3968 release order is compatible with them; these current-head findings need resolution first. Direct task replies continue failing with “Cannot steer … without an active turn id”, hence this review receipt. |
|
Independent deterministic reproduction now available for the UPDATE gap on exact head The isolated worktree is |
|
Owner handoff: no production code, branch, workflow or deployment has been changed. Suggested production change: use one private reactive completion helper from BOTH Evidence: actual public update-handler regression fails in224ms; the delete/recreate seam regression also fails on the exact3976 late-subscription tail after the real shared stream has observed absence then recreation. The latter is deliberately labelled a seam reproduction, not a scheduler-forced full-handler test. Both Release builds pass with warnings as errors. Test receipts are in All SAV merges remain held; your ownership of correcting #3976 is respected. Direct task steering is unavailable, so this supplies the requested precise handoff through the review channel. |
|
Release-state correction from REST: #3976 is already merged, at 21:16:16 UTC, merge commit The newly green corrective implementation described in the owner task therefore needs its own follow-up PR and validation; a push to this merged branch will not amend the landed change. All SAV PRs remain held. This is a measured GitHub-state correction, not a request to bypass the active delivery lane. |
|
The merge queue landed the deletion-only head after this PR had been converted back to draft. The update boundary and subscribe-before-write correction are now isolated in follow-up #3985. That PR includes deterministic update, delete, and delete→same-ID-recreate regressions plus a full dependent MeshWeaver.AI.Test run (1,941 passed, 0 failed). This PR is not being treated as the completed fix. |
Root cause
The shared-read fix correctly stopped minting a synchronization hub per request, but it made the reduced read stream reuse its previous value. A unified delete reported success when the owning data-source stream committed; delivery of the resulting absence frame to the shared reduced stream was still queued on that stream actor. An immediate read could therefore replay the deleted entity.
Fix
No polling, retry, delay, replacement stream, or new hub is introduced.
Evidence
Fixes #3971
Fixes #3979
Refs #3432