Repository navigation
test(ComboGate): wait until a recorded verdict is READABLE — the gate read before the write was visible - #3324
Conversation
…just written
`ComboGateRollTest` has now failed twice today on two different methods:
* `ANotVerifiableVerdict_NeitherClearsNorRefuses_AndTheUnverifiedRollIsRecorded` (run 33863349195)
— the verdict text lacked "could NOT answer";
* `ARedVerdict_BlocksTheRoll_AndNamesTheModuleThatWouldBreak` (shard 5 on #3319) — "a refusal
that leaves no trace is the silent freeze this gate must never become … found False".
#3319's entire diff is two YAML files and two Markdown docs, so it cannot reach
`Memex.Portal.Shared.Test` at all. That settles the attribution: this is the test's own race, and
both failures are the same one wearing different assertions.
`Record` awaited `RecordVerification(...).FirstAsync()`. A completing `stream.Update` says the
patch was ACCEPTED — not that every reader can see it. The gate under test reads the policy through
its own stream, so when the check ran first it honestly reported the state it could see ("no combo
verification has been recorded for '<tag>' on this instance") and the assertion failed on text that
was never about a defect.
Now the record is followed by a read-back on the SAME predicate a real reader uses: the verification
for this candidate tag is present in the content. No bound is widened and no retry is added — the
test simply waits for the condition it actually depends on, which is the cure the other timing
assertions in this repo got today (CascadeTest, DisposalRaceNackTest).
🚨 And a correction to the record: when the first of these failed on #3291 I attributed it to that
PR's late-verdict change. That attribution was wrong — the class flakes on a diff that cannot reach
it. #3291's revert stands on its own evidence (SubscribeDuringRecycleTest's documented contract,
and the measurement that the gate cleared none of #3261's reds), not on this.
Verified: Memex.Portal.Shared.Test ComboGateRollTest 7/7 in Release.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new helper comment is now misleading about timeout/bound behavior and the read-back predicate should use the existing VerificationFor(...) helper to avoid duplicated logic and drift.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates ComboGateRollTest to eliminate a read-after-write visibility race by waiting until a newly recorded combo-verification verdict is readable through the policy node’s stream (not just accepted by the write observable), improving test determinism.
Changes:
- Convert
Record(ComboVerification)into a two-step operation: record verdict, then wait until the verdict is observable inUpdatePolicyContent. - Add/expand inline documentation explaining the underlying race and why a read-back is required.
File summaries
| File | Description |
|---|---|
| test/Memex.Portal.Shared.Test/ComboGateRollTest.cs | Makes verdict recording wait for read visibility to avoid flakes caused by read-after-write propagation lag. |
Review details
Suppressed comments (1)
test/Memex.Portal.Shared.Test/ComboGateRollTest.cs:620
- The XML comment says this removes the race “without widening any bound”, but
Recordnow performs two sequential waits, each independently bounded byBudget, so the total time this helper can take is up to ~2×Budget. Consider rewording the comment to reflect the actual timeout behavior (or refactor to a single overall timeout if you truly need an unchanged total bound).
/// <para>So the record is followed by a read-back on the SAME predicate a real reader uses: the
/// verification for this candidate tag is present in the content. That is the condition the
/// test actually depends on, and waiting on it removes the race without widening any bound —
/// the same cure as the other timing assertions fixed in this repo today.</para>
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// assertion then failed on text that was never about a defect. Measured twice on 2026-09-04 | ||
| /// (runs 33863349195 and the shard-5 red on #3319, a PR whose whole diff is two YAML files and | ||
| /// two Markdown docs and therefore cannot reach this test at all).</para> |
| await WaitForContent(c => c.ComboVerifications.Any( | ||
| v => string.Equals(v.CandidateTag, verdict.CandidateTag, StringComparison.OrdinalIgnoreCase))); |
|
Confirming from #3319's side, and adding a control arm your attribution does not yet have. The failure is exactly as you describe — run 33925367758, The control arm: #3320 ran the same shard in the same window and passed it. Its build (33925271322) is One more datum from the same shard, since it discriminates between "the shard was unhealthy" and "this one test raced": every other project in shard 5 exited 0 — The host completed normally and recorded exactly one failure, so this is not a crashed or starved shard either. I am not re-running #3319 and have not touched it — a passing re-run would only be the other arm of the race. I will merge Not in |
Why
ComboGateRollTestfailed twice on 2026-09-04, on two different methods:ANotVerifiableVerdict_NeitherClearsNorRefuses_AndTheUnverifiedRollIsRecordedcould NOT answerARedVerdict_BlocksTheRoll_AndNamesTheModuleThatWouldBreak#3319's entire diff is two YAML files and two Markdown docs —
deploy/aks/scripts/values.observability.yaml,deploy/helm/templates/memex-portal/deployment.yaml, and two files undersrc/MeshWeaver.Documentation/Data/Architecture/. It cannot reachMemex.Portal.Shared.Testat all. That settles attribution without argument: this is the test's own race, and both failures are the same one wearing different assertions.The mechanism
A completing
stream.Updatesays the patch was accepted — not that every reader can see it. The gate under test reads the policy through its own stream. When the check ran before the verdict was visible, the gate honestly reported the state it could see —— and the assertion failed on text that was never about a defect. Both observed failures are that same read-after-write gap, surfacing through whichever assertion happened to inspect the verdict text.
What changes
Recordnow waits until the verdict is readable, on the same predicate a real reader uses — the verification for this candidate tag is present in the content.No bound is widened, no retry is added, and the assertions are untouched. The test simply waits for the condition it actually depends on, which is the same cure
CascadeTestandDisposalRaceNackTestgot today.A correction to the record
When the first of these failed on #3291, I attributed it to that PR's late-verdict change. That attribution was wrong — the class flakes on a diff that cannot reach it. #3291's revert stands on its own evidence (
SubscribeDuringRecycleTest's documented contract, plus the measurement that the gate cleared none of #3261's reds), not on this failure.Verification
Memex.Portal.Shared.Test→ComboGateRollTest7/7 in Release.Pairs-with: none — test-only.
🤖 Generated with Claude Code