Repository navigation
fix(#4280): an adoption is not judged while the bundle's sources are still landing; a failure formed under a set that moved is re-driven, not parked - #4293
Conversation
…still landing, and a failure formed under a set that moved is re-driven, not left parked Two separable fixes for the install-order race measured on MeshWeaver.Reinsurance run 34804118498: a NodeType adopted 1.7 s into a 6.7 s install had its adoption judged against 8 of the 14 files its bundle names, the fingerprints disagreed (six files short), the build went StaleAdopted, a compile of the 8-file set was dispatched, failed CS0246 on names declared in the six still landing, and PARKED. Sixteen seconds later the same type compiled cleanly in the same process; the gate had taken the first terminal status. 1. The bundle now carries the PATHS it was built from. Both bake paths already wrote sourceVersions per assembly and every reader skipped it; BundleReader now reads the key set (Payload.SourcePaths), the seeder stamps it as NodeTypeDefinition.AdoptedSourcePaths (operational, mirrored on the compile-state satellite), and CanJudgeAdoption treats a live snapshot short of ANY of those paths as ARRIVING, not moved: the judgement is deferred exactly as #4208's empty-set case is — the record returned untouched, the one-shot request left standing, the sources watcher's publication carrying the last arrival judges it. The seeder's pre-write decline reads the same witness. Legacy bundles (no paths) judge on the fingerprint alone as before; an honest fingerprint match still verifies whatever the paths say; a COMPLETE set that disagrees still goes StaleAdopted/Refused. 2. A compile failure whose consumed source set the live set has MOVED PAST is not a verdict about the code. The sources watcher's parked auto-retry only fires on a LATER arrival; when the last one landed between the compile's snapshot and the failure write, nothing re-drove and the type rested at Error. The terminal stamp now makes that retry itself, in the same write: the failure is recorded in full, the park is lifted, and the type is flipped Pending through the one door with the live set stamped. Bounded by construction — each re-drive needs a live set that differs from the one just consumed. Pinned by AdoptionIsNotJudgedFromAPartialSourceSetTest (red on the two-witness rule, green now) and two BundleReaderTest cases; Compiler.Pipeline.Test 949/949, PluginTester.Test 362/362, full solution -c Release -p:CIRun=true -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGsQEwX8ohS51nUpcsr4mi
There was a problem hiding this comment.
🟡 Changes recommended
Critical gaps remain in include-closure completeness and failure-source propagation, so adoption or retry can still occur incorrectly.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request addresses install-order races by deferring adoption until required sources are live and re-driving failures when compile inputs move.
Changes:
- Propagates bundle source-path provenance through adoption and operational state.
- Defers incomplete-source adoption decisions.
- Detects source movement during failure handling and retries compilation.
File summaries
| File | Reviewed changes / findings |
|---|---|
test/MeshWeaver.Compiler.Pipeline.Test/BundleReaderTest.cs |
Tests source-path manifest round-tripping and legacy bundles. |
test/MeshWeaver.Compiler.Pipeline.Test/AdoptionIsNotJudgedFromAPartialSourceSetTest.cs |
Tests partial-source deferral, convergence, and source movement semantics. |
src/MeshWeaver.PluginCatalog/PluginBundleClient.cs |
Passes bundle source paths to adoption. |
src/MeshWeaver.Plugin.Packaging/BundleReader.cs |
Reads manifest source paths. Nit: exposes a mutable list instead of an immutable snapshot. |
src/MeshWeaver.Mesh.Contract/NodeTypeOperationalContent.cs |
Preserves adopted source paths as operational content. |
src/MeshWeaver.Hosting/ShippedPrebuiltBundles.cs |
Passes source paths for shipped bundles. |
src/MeshWeaver.Graph.Contract/NodeTypeDefinition.cs |
Stores adopted source paths. |
src/MeshWeaver.Compiler.Pipeline/PrebuiltAssemblySeeder.cs |
Propagates paths and applies adoption deferral checks. |
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompileState.cs |
Mirrors adopted source-path state. |
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompileParkRegistry.cs |
Compares consumed and live source snapshots. |
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs |
Implements completeness and retry logic. Critical: the witness omits the full include closure, and failed results do not carry the consumed source snapshot. Nits: misleading partial-set logging, mutable collections, and missing integration coverage for the terminal retry path. |
Review details
Suppressed comments (5)
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs:1582
- For the new partial-set case, the log says there is “NOTHING established,” even though
snapshotis explicitly non-empty and the following reason reports the live count and missing paths. This contradicts the evidence operators need to distinguish #4280 (partial) from #4208 (empty) and makes the new diagnosis misleading.
"[AdoptedSourceStamp] {HubPath}: the adoption is NOT judged yet ({Issue}) — the "
+ "bundle records source fingerprint {Adopted}, and there is NOTHING established to "
+ "compare it against: {Reason} (live fingerprint {Live}). The stamp request is left "
+ "STANDING and the adopted build keeps serving; the sources watcher's next "
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs:1240
- The new helper allocates mutable
List<string>andHashSet<string>instances viaToList()andnew HashSet. The repository's collection policy requires immutable collections for these snapshots; use immutable list/set implementations instead of introducing mutable backing objects.
return adoptedSourcePaths.ToList();
var live = new HashSet<string>(liveSources.Keys, StringComparer.OrdinalIgnoreCase);
return adoptedSourcePaths.Where(p => !live.Contains(p)).ToList();
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs:4481
- The new terminal-failure branch is only exercised by the pure
SourcesMovedSinceassertion; the existingFailedVerdictRedriveTestcovers a different kickoff and does not execute this compile watcher write. That leaves the critical behavior untested: a real failure must persist its diagnostics, unpark the type, and commitPendingso the watcher actually performs the replacement compile when the last source arrival raced the failure write. Add an integration/regression test that drives this terminal path and asserts the second compile and final state.
if (!IsAvailabilityNonVerdict(outcome.Error)
&& NodeTypeCompileParkRegistry.SourcesMovedSince(
outcome.Result?.CompiledSources, def.CurrentSourceVersions))
{
var consumedCount = outcome.Result!.CompiledSources!.Count;
var liveCount = def.CurrentSourceVersions!.Count;
logger?.LogWarning(
"Compile failure for {HubPath} was formed under a source set the live one "
+ "has MOVED PAST (#4280): Roslyn was handed {Consumed} source(s), the mesh "
+ "now holds {Live} — the sources were still landing while it compiled. "
+ "The failure is recorded and NOT left standing: the type is re-driven "
+ "against the live set in this same write, as the sources watcher would "
+ "have done had one more source arrived after the verdict.",
hubPath, consumedCount, liveCount);
// Idempotent, so a re-run of this lambda on a version conflict is safe
// (the retried lambda re-derives the same decision from the same facts).
parkRegistry?.Unpark(hubPath);
return curr with
{
Content = DispatchPending(failed, modulesHashNow, def.CurrentSourceVersions),
};
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs:1286
- This completeness check only verifies that the bundle's paths exist, not that their contents have finished landing.
PackageInstallerupdates existingSourcenodes one at a time (PackageInstaller.cs:3134-3137), so an update can have every path present whileCurrentSourceVersionsstill represents a mixed old/new snapshot; the fingerprints then differ,SourcePathsNotYetLiveis empty, and this branch judges/dispatches against that incomplete set. Gate this witness on the install's completion, or carry a per-source content/version identity that can prove the snapshot is complete, rather than using path presence alone.
// #4280 — the fingerprints differ and something matched: was the live set COMPLETE when
// it was measured? Ordered after the equality check for the same reason the empty
// witness is — an honest match is an honest match whatever the paths say.
return SourcePathsNotYetLive(adoptedSourcePaths, liveSources).Count == 0;
src/MeshWeaver.Plugin.Packaging/BundleReader.cs:280
- The new source-path payload is materialized with
ToList(), so the bundle reader exposes a mutable list through its read-only interface. The repository requires immutable collection values for these snapshots; return an immutable list here.
SourcePaths = reference.SourceVersions is { Count: > 0 } versions
? versions.Keys.OrderBy(k => k, StringComparer.Ordinal).ToList()
: null,
- Files reviewed: 11/11 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.
| // #4280 — the fingerprints differ and something matched: was the live set COMPLETE when | ||
| // it was measured? Ordered after the equality check for the same reason the empty | ||
| // witness is — an honest match is an honest match whatever the paths say. | ||
| return SourcePathsNotYetLive(adoptedSourcePaths, liveSources).Count == 0; |
There was a problem hiding this comment.
Also correct, and a narrower hole than the one above: the witness compares the manifest's sourceVersions keys (query-resolved roots, both bake paths) against CurrentSourceVersions (which also excludes includes by design — see NodeTypeSourceFingerprint's "residue" paragraph). An @@-included Code node still landing is therefore invisible to it, and because a genuinely ABSENT include is an answer in SourceFingerprintIncludeReader (contributes nothing, deliberately — a stalled read is what is inconclusive), the live fold is shorter than the producer's, the fingerprints differ, and the judgement reads "moved" and dispatches the premature compile. Taking it as its own PR after #4302: both sides have the include closure in hand (CompileInputs.ResolvedIncludes at the bake, CollectIncludeClosure in the sources watcher), so the bundle will carry it beside sourceVersions and the owner will publish the includes it resolved as present, and CanJudgeAdoption will wait on both sets. Note it must NOT be done by adding include paths to sourceVersions alone — the live snapshot never contains them, so that would defer every include-bearing type forever.
| if (!IsAvailabilityNonVerdict(outcome.Error) | ||
| && NodeTypeCompileParkRegistry.SourcesMovedSince( | ||
| outcome.Result?.CompiledSources, def.CurrentSourceVersions)) |
There was a problem hiding this comment.
Correct, and verified on main after the merge: the failure branch at MeshNodeCompilationService.cs:1241 destructured sources and returned the result without it, so SourcesMovedSince(null, …) could not fire on the CS0246 path — the park-lift half was inert on exactly the shape it targets; only the deferral half was live. This PR merged before the review posted (#4299), so the fix is #4302: the failure result now folds the consumed set under the same SourceVersionOf rule as the success result and the sources watcher, pinned by a real failing compile (AFailedCompileCarriesTheSetItConsumedTest, red on main, green with the fix).
Closes #4280. Closes Systemorph/MeshWeaver.Reinsurance#206 (its satellite ledger). Related: Systemorph/MeshWeaver.Plugins#1827 / #1823 (the Plugins-side half, held by another agent) and #4208 (the empty-set sibling this extends).
The measurement (Reinsurance run 34804118498, shard 1/1)
Different types every run, including ones the repo does not own — an install-order race, not a property of any type.
Fix 1 — the judgement defers while the live set is short of what the bundle was built from
BundleReader: the manifest's per-assemblysourceVersions(written by both bake paths since the fingerprint landed, skipped by every reader) now comes back asPayload.SourcePaths— its sorted key set. Init properties only (no constructor-signature change).PrebuiltAssemblySeeder: a new overload carries the paths; the adoption stampsNodeTypeDefinition.AdoptedSourcePaths(operational — inNodeTypeOperationalContent, mirrored onNodeTypeCompileState). Both consumers (PluginBundleClient,ShippedPrebuiltBundles, whichBakeSeeddelegates to) pass it.CanJudgeAdoptiongets the third witness: fingerprints differ and something matched and every path the bundle names is live ⇒ judgeable. Otherwise deferred —ApplyAdoptedSourceStampreturns the record untouched, the one-shot request stays standing, the publication carrying the last arrival judges it. The log names the missing paths (… 6 of the 14 the bundle was built from have not landed on this mesh yet: Ifrs17/Source/Enums, …). The seeder's pre-write decline (GitSync adopts a stale prebuilt assembly over the source it just synced — silently running last week's code, and it destroyed client data #2813) reads the same witness.IsDirtyrecord from the complete set; a non-compiling one keeps serving the same bytes StaleAdopted would have).Fix 2 — a failure formed under a set the live one has moved past is re-driven in the same write
The park registry's source-change retry only fires on a later arrival; when the last source landed between the compile's snapshot and the failure write, nothing re-drove and the type rested at
Error(which the #1793 kickoff also skips, because the adopted build's coordinates are still on the record). The terminal stamp now comparesResult.CompiledSources(what Roslyn was handed) withCurrentSourceVersionson the node being committed against (NodeTypeCompileParkRegistry.SourcesMovedSince, the registry's own equality): if they differ, the failure is recorded in full, the park is lifted, and the type flips Pending through the one door with the live set stamped. Availability non-verdicts excluded. Bounded: each re-drive needs a live set that differs from the one just consumed.This composes with #4295 (
InMemoryStorageAdapterchildren-index consistency — doc pagesrc/MeshWeaver.Documentation/Data/Architecture/InMemoryChildIndexConsistency.md): there, the pending index is one set of truth (what the rebuild iterates and what writers index into) and snapshot-vs-write is serialised by one lock, so a write either precedes the snapshot and is in it, or follows it and lands in Pending. This PR stops the judgement and the park at the worst moment; #4295 makes a listing taken during a reindex complete. The two incidents separate cleanly on that line: Reinsurance recovered on its third compile (this mechanism, live in its log), while Plugins#1823 never recovered because the partialInitialwas cached before anyAddedcould repair it (#4295's mechanism). Neither alone would have cleared both.Tested
AdoptionIsNotJudgedFromAPartialSourceSetTest(beside the Gate: a NodeType activated before its shared sources landed adopts NOTHING and is PARKED red — 'MISSING SOURCES: 3 of 3 queries matched NO nodes', live fingerprint e3b0c442 (empty) #4208 test, with the run's own 14-path / 8-path sets): deferral + convergence red on the two-witness rule, green now (A/B run locally); complete-set-disagrees still StaleAdopted; grown set judged; honest match verifies;SourcesMovedSincesemantics.BundleReaderTest:sourceVersionsround-trips as a sorted key set; a legacy bundle yields null.NodeTypeCompileStateTest(the operational-mirror guard) and theDispatchedBuildInputsdoor guards pass with the new field / new door use.-c Release -p:CIRun=true -warnaserrorclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01HGsQEwX8ohS51nUpcsr4mi