Repository navigation
fix(sync): JsonElement value-equality in EntityUpdate dedup + MeshNode reduce - #112
Merged
Merged
Conversation
…e reduce
Two JsonElement-as-object defects in the sync core:
FIX 1 — JsonSynchronizationStream.ToEntityUpdates (both overloads):
DistinctBy(x => new { x.Id, x.Collection }) never deduped. x.Id is the result
of DecodePointerSegment → JsonSerializer.Deserialize<object> → a boxed
JsonElement, whose struct (_parent/_idx cursor) has no value equality, so the
anonymous key compared every id as distinct and duplicate per-entity
EntityUpdates leaked into the stream (contradicting the documented "one distinct
update per affected entity"). Key on je.GetRawText() (stable; ToString() stays
stable for string/guid ids).
FIX 2 — MeshDataSource.ReduceToMeshNode patch path:
`change.Value as MeshNode` returned null when change.Value is a JsonElement
(the cross-hub / mirror patch path), pushing a null-content ChangeItem into the
mirror. Deserialize the JsonElement to MeshNode the same way the sibling
PatchMeshNode does (threaded the hub's JsonSerializerOptions into the reducer).
Repros: EntityUpdateDedupTest (count 2 → 1), ReduceToMeshNodeJsonElementTest
(null → node). Full Data.Test (254) + Graph.Test (436) green under
DOTNET_PROCESSOR_COUNT=2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Test Results 50 files ± 0 50 suites ±0 24m 47s ⏱️ -40s For more details on these failures, see this check. Results for commit 4ff3ad6. ± Comparison against base commit ea3b837. This pull request removes 83 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Test Results (shard 2)926 tests ±0 924 ✅ - 1 7m 55s ⏱️ +41s For more details on these failures, see this check. Results for commit 4ff3ad6. ± Comparison against base commit ea3b837. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 1) 12 files ± 0 12 suites ±0 6m 33s ⏱️ -6s For more details on these failures, see this check. Results for commit 4ff3ad6. ± Comparison against base commit ea3b837. This pull request removes 83 tests.♻️ This comment has been updated with latest results. |
#119 added src/MeshWeaver.AI/Data/Skill/provider-keys.md (an 8th built-in skill) but left the two exact-set assertions in SkillNodeTypeTest + SkillHarnessImportSourceTest at the prior 7, turning main red. Add provider-keys (sorts after model) so the assertions match the shipped Data/Skill/*.md set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Two
JsonElement-as-objectserialization defects in the sync core. Both are now fixed and pinned by a deterministic repro that fails pre-fix and passes post-fix.FIX 1 —
JsonSynchronizationStream.ToEntityUpdatesdedup never fired (both overloads, lines 748 & 994)DistinctBy(x => new { x.Id, x.Collection })silently never deduplicated.x.Idis the return ofDecodePointerSegment→JsonSerializer.Deserialize<object>(...), which yields a boxedJsonElement.JsonElementis a struct cursor (_parent/_idx) with no value equality, so the anonymous-key comparison treated every id as distinct — duplicate per-entityEntityUpdates flowed into the sync stream, contradicting the method's documented contract ("one distinct update per affected entity, deduplicated by id and collection").Fix: key on the JsonElement's raw text —
new { Id = x.Id is JsonElement je ? je.GetRawText() : x.Id?.ToString(), x.Collection }.ToString()stays stable for string/guid ids.FIX 2 —
MeshDataSource.ReduceToMeshNodepatch path returned a null node (line 1155)new(change.Value as MeshNode, …)returned null wheneverchange.Valueis aJsonElement(the cross-hub / mirror patch path, where the update value is the JSON-serialized node, not aMeshNode), pushing a null-contentChangeIteminto the mirror. The siblingPatchMeshNodealready deserializesJsonElement → MeshNodecorrectly; this branch now does the same.Fix: thread the hub's
JsonSerializerOptionsinto the reducer andje.Deserialize<MeshNode>(options)when the value is aJsonElement. Narrow, mirrorsPatchMeshNode.Repros (both fail pre-fix, pass post-fix)
test/MeshWeaver.Data.Test/EntityUpdateDedupTest.cs— two patch ops on the same (id, collection) → pre-fix count 2, post-fix 1.test/MeshWeaver.Graph.Test/ReduceToMeshNodeJsonElementTest.cs— aJsonElement-valuedEntityUpdatethrough the reduce → pre-fixValueis null, post-fix equals the expectedMeshNode.Verification
MeshWeaver.Data.Test(254/254) andMeshWeaver.Graph.Test(436/436) green underDOTNET_PROCESSOR_COUNT=2.🤖 Generated with Claude Code