Repository navigation
Fix #2260: an automatic re-drive must never un-park a NodeType - #2262
Merged
Merged
Conversation
RequirePrebuiltParkTest failed intermittently on CI because a type that had parked really was ending up un-parked — the assertion was right and the production code was wrong. Mechanism, measured (registry instrumented with the instance id, plus a stack trace on every entry actually removed): PARK-ADDED sources=null <- the adopt-only gate refuses, parks ISPARKED => False <- the TEST reads here, in the window UNPARK-REMOVED <- the failed-verdict re-drive un-parks PARK-ADDED sources=0 <- the gate refuses again, re-parks The failed-verdict re-drive (#1793) called parkRegistry.Unpark(hubPath) before its Update so its CompilationStatus=Pending flip would not be swallowed by the compile watcher's parked short-circuit. That has two defects: 1. The un-park was decided from a SNAPSHOT and never re-validated. The Update lambda re-checks current state and can decline to flip — and by then the park is already gone, with nothing to re-park it. The type is left neither parked nor re-driven: the one state the park exists to make impossible. 2. Even a perfectly validated un-park leaves the type un-parked for the whole Pending -> refusal -> re-park round-trip, and any trigger arriving in that window is admitted. Under Modules:RequirePrebuilt that window opens on EVERY refusal, because the gate's settle records no verdict inputs and so always reads as "never attempted". The re-drive never needed the failure CLEARED — only its one flip let through. It now asks for a one-shot admission (AdmitOneRetry / TryConsumeRetryAdmission) and the park entry is never touched, so IsParked is true at every instant. Lifting a park stays reserved for the two events that genuinely end the failure: a human-requested build and a successful compile. The admission is claimed INSIDE the Update lambda (ApplyFailedVerdictRedrive), so the decision and its effect commit against one observed state — every early return leaves the registry untouched. ParkAndNotify now refreshes the standing record (compare-and-replace, never a blind write) when an admitted retry re-fails, instead of returning blind: ShouldRetryForSourceChange reads that source snapshot, and a stale one would keep re-arming the source-change retry. It still never re-notifies. Verified on this machine, same amplifier (--logger "console;verbosity=detailed", DOTNET_PROCESSOR_COUNT=4), before/after arms identical: before 12/15 pass (2/10 with the diagnostic instrumentation amplifying it) after 15/15 pass Three new pins in NodeTypeCompileParkTest assert the invariant directly rather than by consequence: a park is never removed by a re-drive that did not observe it; a genuinely stale verdict still earns its one attempt without un-parking; an in-flight compile is never clobbered. Fixes #2260 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the NodeType compile “park” invariant regression where the failed-verdict automatic re-drive could temporarily (or permanently) un-park a type, allowing stray triggers to slip through and undermining storm containment.
Changes:
- Introduces a one-shot “retry admission” in
NodeTypeCompileParkRegistryto let exactly one automatic re-drivePendingflip through the parked short-circuit without removing the park. - Refactors the failed-verdict re-drive to commit the flip (and admission) inside the
Updatelambda (ApplyFailedVerdictRedrive) rather than un-parking ahead of time. - Adds targeted tests pinning the invariant directly, and adds a What’s New entry documenting the behavioral change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeCompileParkTest.cs | Adds three focused tests asserting that automatic re-drive never lifts/removes a park and that admissions are one-shot. |
| src/MeshWeaver.Graph/Configuration/NodeTypeCompileParkRegistry.cs | Adds the retry-admission mechanism and refreshes the parked record on re-fail without re-notifying. |
| src/MeshWeaver.Graph/Configuration/NodeTypeCompilationHelpers.cs | Updates the compile watcher to honor one-shot admissions and moves failed-verdict redrive into a single commit-step helper. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-25-parked-type-stays-parked.md | Documents the behavior change in the What’s New feed. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review catch (Copilot, PR #2262). ApplyFailedVerdictRedrive granted the one-shot admission unconditionally whenever it flipped the node back to Pending. An admission for an UN-parked type is never consumed — the parked short-circuit it exists to pass is not taken — so it sat in the registry until some LATER park, where a stray Pending flip could spend it and get through the very containment this PR restores. That case is the common one, not a corner: a failure predating this PROCESS is not in the in-memory registry at all, so the re-drive routinely runs against an un-parked type. Gating the grant on IsParked is deliberately stronger than pruning stale admissions afterwards, because it makes the leak impossible rather than unlikely. It establishes the invariant "an admission implies a standing park" — and _parked entries are removed in exactly two places, Unpark and OnCompileSucceeded, both of which already clear _retryAdmissions. So an admission can never outlive the park it was granted against, and can never be waiting when a later park arrives. Trade-off, stated: if a park lands between this check and the watcher's read, the flip meets the short-circuit and is answered with the cached error instead of being dispatched — one automatic retry lost, in a window where the type has just re-failed and the newest verdict is the one being served. That costs a retry, never containment. Pinned by RedriveCommit_OnATypeThatIsNotParked_FlipsAndLeavesNoAdmissionBehind, which uses the exact shape the review named. Verified on the amended tree: NodeTypeCompileParkTest 10/10, NeverCompiledFailureRedriveTest 11/11, FailedVerdictRedriveTest 20/20, RedriveObservationReplay / CodeEditRecompile / OverlaySelfHealInstanceRecycle green, and RequirePrebuiltParkTest 20/20 under the detailed-logger amplifier. Release -warnaserror clean for MeshWeaver.Graph (--no-incremental) and MeshWeaver.Hosting.Monolith.Test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Test Results (shard 2)3 280 tests - 5 3 280 ✅ - 5 9m 42s ⏱️ +59s Results for commit bce3359. ± Comparison against base commit 8906cad. This pull request removes 107 and adds 102 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Test Results (shard 1)1 881 tests +14 1 881 ✅ +14 7m 47s ⏱️ -38s Results for commit bce3359. ± Comparison against base commit 8906cad. This pull request removes 97 and adds 111 tests. Note that renamed tests count towards both. |
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.
Fixes #2260. Unblocks #2238, whose required check is red on
RequirePrebuiltParkTest— the failure is this defect onmain, not that diff.The test was right: a parked type really was being un-parked
Measured with the registry instrumented (instance id on every entry, plus a stack trace on every entry actually removed, so nothing is inferred):
A passing run has the same first five lines and a sixth,
ISPARKED => True. So the assertion is notracy in the "test is wrong" sense — it reads a moment in which the type genuinely is not parked.
Root cause
The failed-verdict re-drive (#1793) called
parkRegistry.Unpark(hubPath)before itsUpdate, so itsCompilationStatus = Pendingflip would not be swallowed by the compile watcher's parkedshort-circuit. Two defects:
Updatelambda re-checkscurrent state and can decline to flip — and the park is already gone by then, with nothing to
re-park it. The type ends up neither parked nor re-driven: the one state the park exists to make
impossible. The file's own comment ("correctness is the re-check inside the Update lambda, not
this") held for the Pending flip and not for the un-park.
Pending -> refusal -> re-parkround-trip, and any trigger arriving in that window is admitted.Under
Modules:RequirePrebuiltthat window opens on every refusal.Fixing only (1) would not have closed the window — this was verified against the measured trace
before choosing the shape.
The fix: the re-drive asks to be let through, it does not clear the failure
The automatic re-drive never needed the terminal failure cleared — only its one flip let through.
Conflating those two is the defect.
AdmitOneRetry/TryConsumeRetryAdmission— a one-shot admission. The park entry is nevertouched, so
IsParkedis true at every instant, and every other trigger still meets the park.Updatelambda (ApplyFailedVerdictRedrive), so thedecision and its effect commit against one observed state — every early return leaves the registry
untouched.
build (
Unpark, unchanged) and a successful compile (OnCompileSucceeded).ParkAndNotifynow refreshes the standing record when an admitted retry re-fails(compare-and-replace, never a blind write) instead of returning blind:
ShouldRetryForSourceChangereads that source snapshot, and a stale one would keep re-arming the source-change retry. It still
never re-notifies — one notification per broken type.
No retry, no delay, no watchdog, no widened bound, and
RequirePrebuiltParkTestis untouched.Verification
RequirePrebuiltParkTest, same amplifier both arms (--logger "console;verbosity=detailed",DOTNET_PROCESSOR_COUNT=4), same machine:origin/main, plain amplifierorigin/main, + registry instrumentation (more I/O widens the window)Post-fix the log shows exactly one
PARKED after compile failureand one notification (it wastwo of each while the park was being dropped and re-added), and the second refusal still reaches the
gate — i.e. the admission let the flip through without the park moving.
Regression suites, all green:
NodeTypeCompileParkTest(9),NeverCompiledFailureRedriveTest(11),RedriveObservationReplayTest,CodeEditRecompileTest,OverlaySelfHealInstanceRecycleTest,RequirePrebuilt*,MeshWeaver.Graph.Test/FailedVerdictRedriveTest.dotnet build -c Release -warnaserrorclean (0 warnings, 0 errors) forMeshWeaver.Graph(
--no-incremental) andMeshWeaver.Hosting.Monolith.Test.New tests
RequirePrebuiltParkTestcatches this only by consequence. Three new pins inNodeTypeCompileParkTestassert the invariant itself, on the re-drive's commit step, with a realregistry and no timing at all:
one-shot;
Follow-up (not in this PR)
SettleAsErrornever stampsFailedBuildInputs, so the adopt-only gate's refusal reads as "neverattempted" and burns one automatic re-drive per refused type. That is the trigger that made this
race reachable, but it is not the race, and removing it would not have closed the window (the first
refusal settles before
CurrentSourceVersionsis established, so the token moves anyway). Worth itsown issue — see the report.
🤖 Generated with Claude Code