diff --git a/AyCode.Core.Serializers.SourceGenerator/AcBinarySourceGenerator.cs b/AyCode.Core.Serializers.SourceGenerator/AcBinarySourceGenerator.cs index 942b16d..35a4b01 100644 --- a/AyCode.Core.Serializers.SourceGenerator/AcBinarySourceGenerator.cs +++ b/AyCode.Core.Serializers.SourceGenerator/AcBinarySourceGenerator.cs @@ -544,17 +544,8 @@ public class AcBinarySourceGenerator : IIncrementalGenerator sb.AppendLine(); sb.AppendLine(" public void WriteProperties(object value, AcBinarySerializer.BinarySerializationContext context) where TOutput : struct, IBinaryOutputBase"); sb.AppendLine(" {"); - sb.AppendLine(" // Global recursion depth safety net — gated by context.NeedsDepthCheck"); - sb.AppendLine(" // (pre-computed at Reset(): !HasAllRefHandling && MaxDepth > 0 && MaxDepthBehavior != Disable)."); - sb.AppendLine(" // Local-cached flag: 1 ctx field-read, register-resident — re-used at inc and dec."); - sb.AppendLine(" var needsDepthCheck = context.NeedsDepthCheck;"); - sb.AppendLine(" if (needsDepthCheck)"); - sb.AppendLine(" {"); - sb.AppendLine(" if (context.RecursionDepth >= context.MaxDepth)"); - sb.AppendLine($" throw new System.InvalidOperationException(\"AcBinary serialize: recursion depth exceeded MaxDepth=\" + context.MaxDepth + \" at type '{ci.FullTypeName}' (depth=\" + context.RecursionDepth + \", position=\" + context.Position + \")\");"); - sb.AppendLine(" context.RecursionDepth++;"); - sb.AppendLine(" }"); - sb.AppendLine(); + sb.AppendLine(" // Depth check + EnterRecursion happens at the CALLER (before marker write)."); + sb.AppendLine(" // Body just runs property writes; ExitRecursion at the end balances the caller's EnterRecursion."); sb.AppendLine($" var obj = Unsafe.As<{ci.FullTypeName}>(value);"); foreach (var p in ci.Properties) @@ -564,7 +555,7 @@ public class AcBinarySourceGenerator : IIncrementalGenerator } sb.AppendLine(); - sb.AppendLine(" if (needsDepthCheck) context.RecursionDepth--;"); + sb.AppendLine(" context.ExitRecursion();"); sb.AppendLine(" }"); sb.AppendLine(); @@ -677,19 +668,12 @@ public class AcBinarySourceGenerator : IIncrementalGenerator (p.TypeKind == PropertyTypeKind.Collection && p.ElementKind == PropertyTypeKind.String && p.InterningFlags != 0) || (p.TypeKind == PropertyTypeKind.Dictionary && (p.DictKeyKind == PropertyTypeKind.String || p.DictValueKind == PropertyTypeKind.String) && p.InterningFlags != 0)); - // Global recursion depth safety net — gated by context.NeedsDepthCheck - // (pre-computed at Reset(): !HasAllRefHandling && MaxDepth > 0 && MaxDepthBehavior != Disable). + // Combined check+inc — gated inside TryEnterRecursion (checks NeedsDepthCheck). // Emitted AFTER all early returns (NeedsScan=false, feature-flag, null guard, IId 2nd-occurrence) // and BEFORE the property scan loop that recurses into children. - // Local-cached flag: 1 ctx field-read, register-resident — re-used at inc and dec (JIT-independent guarantee). + // On limit hit: helper method (cold path, NoInlining) dispatches Throw or Truncate (return). sb.AppendLine(); - sb.AppendLine(" var needsDepthCheck = context.NeedsDepthCheck;"); - sb.AppendLine(" if (needsDepthCheck)"); - sb.AppendLine(" {"); - sb.AppendLine(" if (context.RecursionDepth >= context.MaxDepth)"); - sb.AppendLine($" throw new System.InvalidOperationException(\"AcBinary scan: recursion depth exceeded MaxDepth=\" + context.MaxDepth + \" at type '{ci.FullTypeName}' (depth=\" + context.RecursionDepth + \")\");"); - sb.AppendLine(" context.RecursionDepth++;"); - sb.AppendLine(" }"); + sb.AppendLine(" if (context.TryEnterRecursion(hasTruncatePath: false)) return; // scan: skip children"); if (hasStringScan) { @@ -717,7 +701,7 @@ public class AcBinarySourceGenerator : IIncrementalGenerator } sb.AppendLine(); - sb.AppendLine(" if (needsDepthCheck) context.RecursionDepth--;"); + sb.AppendLine(" context.ExitRecursion();"); sb.AppendLine(" }"); } @@ -1344,7 +1328,9 @@ public class AcBinarySourceGenerator : IIncrementalGenerator if (!p.ChildNeedsRefScan && !p.ChildEnableMetadata) { - // Compile-time proven: no ref, no metadata → ZERO branches: always Object + WriteProperties + // Compile-time proven: no ref, no metadata. Combined check+inc BEFORE marker write so Truncate writes + // Null wire-correctly. TryEnterRecursion inc'd on success; ExitRecursion at WriteProperties end. + sb.AppendLine($"{i}else if (context.TryEnterRecursion(hasTruncatePath: true)) {{ /* truncated: Null written */ }}"); sb.AppendLine($"{i}else {{ context.WriteByte(BinaryTypeCode.Object); {writer}.Instance.WriteProperties({a}, context); }}"); } else if (p.ChildNeedsRefScan && !p.ChildEnableMetadata) @@ -1353,7 +1339,7 @@ public class AcBinarySourceGenerator : IIncrementalGenerator } else if (!p.ChildNeedsRefScan && p.ChildEnableMetadata) { - sb.AppendLine($"{i}else {{ context.WriteObjectMetaMarker({a}, {writer}.s_wrapperSlot); {writer}.Instance.WriteProperties({a}, context); }}"); + sb.AppendLine($"{i}else if (context.WriteObjectMetaMarker({a}, {writer}.s_wrapperSlot)) {writer}.Instance.WriteProperties({a}, context);"); } else { @@ -1434,7 +1420,8 @@ public class AcBinarySourceGenerator : IIncrementalGenerator if (!p.ElementNeedsRefScan && !p.ElementEnableMetadata) { - // Compile-time proven: no ref, no metadata → ZERO branches per element: always Object + // Compile-time proven: no ref, no metadata. Combined check+inc before marker write. + sb.AppendLine($"{i} if (context.TryEnterRecursion(hasTruncatePath: true)) continue;"); sb.AppendLine($"{i} context.WriteByte(BinaryTypeCode.Object);"); sb.AppendLine($"{i} {writer}.Instance.WriteProperties({e}, context);"); } @@ -1444,8 +1431,7 @@ public class AcBinarySourceGenerator : IIncrementalGenerator } else if (!p.ElementNeedsRefScan && p.ElementEnableMetadata) { - sb.AppendLine($"{i} context.WriteObjectMetaMarker({e}, {writer}.s_wrapperSlot);"); - sb.AppendLine($"{i} {writer}.Instance.WriteProperties({e}, context);"); + sb.AppendLine($"{i} if (context.WriteObjectMetaMarker({e}, {writer}.s_wrapperSlot)) {writer}.Instance.WriteProperties({e}, context);"); } else { @@ -1614,7 +1600,8 @@ public class AcBinarySourceGenerator : IIncrementalGenerator if (!p.DictValueNeedsRefScan && !p.DictValueEnableMetadata) { - // No ref, no metadata → always Object + // No ref, no metadata. Combined check+inc before marker write. + sb.AppendLine($"{i}else if (context.TryEnterRecursion(hasTruncatePath: true)) {{ /* truncated: Null written */ }}"); sb.AppendLine($"{i}else {{ context.WriteByte(BinaryTypeCode.Object); {writer}.Instance.WriteProperties({v}, context); }}"); } else if (p.DictValueNeedsRefScan && !p.DictValueEnableMetadata) @@ -1623,7 +1610,7 @@ public class AcBinarySourceGenerator : IIncrementalGenerator } else if (!p.DictValueNeedsRefScan && p.DictValueEnableMetadata) { - sb.AppendLine($"{i}else {{ context.WriteObjectMetaMarker({v}, {writer}.s_wrapperSlot); {writer}.Instance.WriteProperties({v}, context); }}"); + sb.AppendLine($"{i}else if (context.WriteObjectMetaMarker({v}, {writer}.s_wrapperSlot)) {writer}.Instance.WriteProperties({v}, context);"); } else { diff --git a/AyCode.Core.Tests/Serialization/AcBinarySerializerIIdReferenceTests.cs b/AyCode.Core.Tests/Serialization/AcBinarySerializerIIdReferenceTests.cs index fd7d548..3b16287 100644 --- a/AyCode.Core.Tests/Serialization/AcBinarySerializerIIdReferenceTests.cs +++ b/AyCode.Core.Tests/Serialization/AcBinarySerializerIIdReferenceTests.cs @@ -126,27 +126,19 @@ public class AcBinarySerializerIIdReferenceTests ReferenceHandling = mode, UseGeneratedCode = useSgen, UseMetadata = useMeta, - MaxDepth = 10 + MaxDepth = 10, + // None mode has no ref tracking → the cycle (Items[1].ParentOrder = order) is unprotected. + // Use Truncate so the recursion silently bottoms out with Null at the depth limit instead of throwing. + MaxDepthBehavior = mode == ReferenceHandlingMode.None + ? MaxDepthBehavior.Truncate + : MaxDepthBehavior.Throw }; Console.WriteLine($"\n========== ReferenceHandling: {options.ReferenceHandling}, UseSgen: {options.UseGeneratedCode}, UseMeta: {options.UseMetadata} =========="); - // ReferenceHandling.None: cycle (Items[1].ParentOrder = order) is unprotected because no ref tracking. - // The global recursion depth safety net must throw before stack overflow (MaxDepth=10). - // Previous silent-null truncation (ACCORE-BIN-I-P2H8) caused data loss instead of surfacing the bug. - if (mode == ReferenceHandlingMode.None) - { - var ex = Assert.ThrowsExactly(() => AcBinarySerializer.Serialize(order, options), $"[{mode}] Expected InvalidOperationException for unprotected cycle, but Serialize succeeded"); - - StringAssert.Contains(ex.Message, "recursion depth exceeded", $"[{mode}] Exception message should mention recursion depth, got: {ex.Message}"); - Console.WriteLine($"[{mode}] PASSED ✓ (depth safety net threw as expected: {ex.Message})"); - - continue; - } - // Act var binary = AcBinarySerializer.Serialize(order, options); - //WriteBinaryToConsole(binary); + if (mode == ReferenceHandlingMode.None) WriteBinaryToConsole(binary); var result = binary.BinaryTo(); // Options from header var objectRefCount = CountObjectRefs(binary, false); @@ -160,6 +152,14 @@ public class AcBinarySerializerIIdReferenceTests // Assert based on mode switch (mode) { + case ReferenceHandlingMode.None: + // Truncate semantic: cycle bottoms out with Null at MaxDepth=10 → serialize succeeds, deserialize + // produces a partial graph where deep cyclic references read as null. Data integrity at root + + // first few levels still holds (verified below after the switch). CountObjectRefs raw byte scan + // is unreliable in None mode — byte 65 (ObjectRef) == ASCII 'A', so "Product-A" produces false + // positives. Skip count assertion; rely on data integrity checks instead. + break; + case ReferenceHandlingMode.OnlyId: // sharedTag (Id=1) 4x → 3 ObjectRefs, sharedUser (Id=1) 2x → 1 ObjectRef = 4 total Assert.IsTrue(objectRefCount >= 4, $"[{mode}] Expected at least 4 ObjectRefs, found {objectRefCount}"); diff --git a/AyCode.Core.Tests/Serialization/AcBinarySerializerMaxDepthTruncateTests.cs b/AyCode.Core.Tests/Serialization/AcBinarySerializerMaxDepthTruncateTests.cs new file mode 100644 index 0000000..b2da69e --- /dev/null +++ b/AyCode.Core.Tests/Serialization/AcBinarySerializerMaxDepthTruncateTests.cs @@ -0,0 +1,314 @@ +using System; +using AyCode.Core.Extensions; +using AyCode.Core.Serializers; +using AyCode.Core.Serializers.Binaries; +using AyCode.Core.Tests.TestModels; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace AyCode.Core.Tests.Serialization; + +/// +/// Focused repro tests for MaxDepthBehavior.Truncate on cyclic graphs. +/// +/// Tracks BINARY_ISSUES.md#accore-bin-i-t7k3: SGen path produces wire-misalignment +/// (DECIMAL_DRIFT on round-trip) when Truncate fires inside a cycle. Runtime path +/// works correctly with the same data — that's the control test for diagnosis. +/// +/// Designed for interactive debugger sessions: minimal graph, small MaxDepth=5, +/// single cycle property. Step through `WriteObjectFullMarkerIId` to compare runtime +/// vs SGen call sequences at the truncation boundary. +/// +[TestClass] +public class AcBinarySerializerMaxDepthTruncateTests +{ + private const int MaxDepthForTest = 5; + + /// + /// Builds the minimal cyclic graph used by most tests below. + /// Cycle: order → Items[0] → ParentOrder → order → …. + /// Only primitive properties on the leaf entities so the body is short and the wire is easy to diff. + /// + private static TestOrder_Circ_Ref BuildMinimalCycle() + { + var order = new TestOrder_Circ_Ref + { + Id = 1, + OrderNumber = "TEST-001", + Items = + [ + new TestOrderItem_Circ_Ref + { + Id = 10, + ProductName = "Product-A", + Quantity = 5 + } + ] + }; + order.Items[0].ParentOrder = order; // ← closes the cycle + return order; + } + + /// + /// Builds the cyclic graph PLUS sets the polymorphic Parent property (declared object?) + /// to a non-IId concrete instance — mirroring the failing SameInstance test's setup for None mode. + /// This routes through → + /// on every cycle level (Parent is written at every TestOrder body). + /// + private static TestOrder_Circ_Ref BuildCycleWithPolymorphicParent() + { + var order = BuildMinimalCycle(); + // Parent is `object?` on TestOrder_Circ_Ref — polymorphic write path. + // UserPreferences_All_True is non-IId, leaf-like (Language, LightTheme strings + scalars), no further refs. + order.Parent = new UserPreferences_All_True { Language = "en-US", Theme = "light" }; + return order; + } + + /// + /// Diagnostic helper: dump the wire bytes as hex for visual comparison. + /// Useful in the debugger to spot the runtime-vs-SGen wire diff. + /// + private static void DumpWire(string label, byte[] wire) + { + Console.WriteLine($"=== {label} | {wire.Length} bytes ==="); + Console.WriteLine(BitConverter.ToString(wire)); + Console.WriteLine(); + } + + /// + /// CONTROL TEST — runtime path with Truncate. Should pass. + /// If this fails, the bug is broader than the SGen path; fix here first. + /// + [TestMethod] + public void Runtime_None_Truncate_CyclicGraph_RoundTrips() + { + var order = BuildMinimalCycle(); + + var options = new AcBinarySerializerOptions + { + ReferenceHandling = ReferenceHandlingMode.None, + UseGeneratedCode = false, // ← runtime path + UseMetadata = false, + MaxDepth = MaxDepthForTest, + MaxDepthBehavior = MaxDepthBehavior.Truncate + }; + + // Act + var binary = AcBinarySerializer.Serialize(order, options); + DumpWire("Runtime + Truncate", binary); + var result = binary.BinaryTo(); + + // Assert: serialize+deserialize succeeds; root + first-level data intact; + // ParentOrder is null at the truncation boundary (instead of a full cycle round-trip). + Assert.IsNotNull(result, "Deserialize result should not be null"); + Assert.AreEqual(1, result.Id, "root Id"); + Assert.AreEqual("TEST-001", result.OrderNumber, "root OrderNumber"); + Assert.IsNotNull(result.Items, "Items list should be materialized"); + Assert.AreEqual(1, result.Items.Count, "Items count"); + Assert.AreEqual(10, result.Items[0].Id, "Item Id"); + Assert.AreEqual("Product-A", result.Items[0].ProductName, "Item ProductName"); + Assert.AreEqual(5, result.Items[0].Quantity, "Item Quantity"); + // ParentOrder may or may not be set depending on where truncation fires — + // the contract is "the deserialize must not throw and root-level data must be intact". + } + + /// + /// BUG REPRO — SGen path with Truncate. Currently fails with DECIMAL_DRIFT + /// on round-trip. Same input as the runtime control above; only UseGeneratedCode differs. + /// + /// + /// Step through with the VS debugger: + /// 1. Break in WriteObjectFullMarkerIId for both runs (runtime test above + this). + /// 2. Compare _position, _recursionDepth, and the wire-bytes-just-written at each + /// call-site between the two paths. + /// 3. Identify the byte position where the SGen wire diverges from the runtime wire. + /// 4. Likely culprits to inspect: + /// - TryEnterRecursion inc/dec balance on the SGen-emit code path + /// - WriteObjectFullMarkerIId ref-handling branches (2nd-occurrence ExitRecursion undo) + /// - SGen-emitted property-loop ordering vs runtime WritePropertiesMarkerless + /// + [TestMethod] + public void Sgen_None_Truncate_CyclicGraph_RoundTrips() + { + var order = BuildMinimalCycle(); + + var options = new AcBinarySerializerOptions + { + ReferenceHandling = ReferenceHandlingMode.None, + UseGeneratedCode = true, // ← SGen path (triggers the bug) + UseMetadata = false, + MaxDepth = MaxDepthForTest, + MaxDepthBehavior = MaxDepthBehavior.Truncate + }; + + // Act + var binary = AcBinarySerializer.Serialize(order, options); + DumpWire("SGen + Truncate", binary); + var result = binary.BinaryTo(); + + // Assert: same as runtime control. Currently throws DECIMAL_DRIFT. + Assert.IsNotNull(result, "Deserialize result should not be null"); + Assert.AreEqual(1, result.Id, "root Id"); + Assert.AreEqual("TEST-001", result.OrderNumber, "root OrderNumber"); + Assert.IsNotNull(result.Items, "Items list should be materialized"); + Assert.AreEqual(1, result.Items.Count, "Items count"); + Assert.AreEqual(10, result.Items[0].Id, "Item Id"); + Assert.AreEqual("Product-A", result.Items[0].ProductName, "Item ProductName"); + Assert.AreEqual(5, result.Items[0].Quantity, "Item Quantity"); + } + + /// + /// SGen + Truncate + useMetadata=true variant. Also currently fails (multi-byte marker variant + /// of the same underlying issue). Useful for the debug session to confirm whether the fix + /// also covers the metadata code path or just the simple Object-marker path. + /// + [TestMethod] + public void Sgen_None_Truncate_UseMetadata_CyclicGraph_RoundTrips() + { + var order = BuildMinimalCycle(); + + var options = new AcBinarySerializerOptions + { + ReferenceHandling = ReferenceHandlingMode.None, + UseGeneratedCode = true, + UseMetadata = true, // ← multi-byte marker (ObjectWithMetadata + inline meta) + MaxDepth = MaxDepthForTest, + MaxDepthBehavior = MaxDepthBehavior.Truncate + }; + + // Act + var binary = AcBinarySerializer.Serialize(order, options); + DumpWire("SGen + Truncate + useMetadata", binary); + var result = binary.BinaryTo(); + + // Assert: same root-level integrity expectation. + Assert.IsNotNull(result); + Assert.AreEqual(1, result.Id); + Assert.AreEqual("TEST-001", result.OrderNumber); + Assert.IsNotNull(result.Items); + Assert.AreEqual(1, result.Items.Count); + Assert.AreEqual("Product-A", result.Items[0].ProductName); + } + + /// + /// CONTROL — runtime + polymorphic Parent. Should pass. + /// Compared to below: same data, different code path. + /// + [TestMethod] + public void Runtime_None_Truncate_PolymorphicCycle_RoundTrips() + { + var order = BuildCycleWithPolymorphicParent(); + + var options = new AcBinarySerializerOptions + { + ReferenceHandling = ReferenceHandlingMode.None, + UseGeneratedCode = false, + UseMetadata = false, + MaxDepth = MaxDepthForTest, + MaxDepthBehavior = MaxDepthBehavior.Truncate + }; + + var binary = AcBinarySerializer.Serialize(order, options); + DumpWire("Runtime + Truncate + PolymorphicParent", binary); + var result = binary.BinaryTo(); + + Assert.IsNotNull(result); + Assert.AreEqual(1, result.Id); + Assert.AreEqual("TEST-001", result.OrderNumber); + Assert.IsNotNull(result.Items); + Assert.AreEqual(1, result.Items.Count); + // Root-level Parent should round-trip (depth 1 — well below MaxDepth). + Assert.IsNotNull(result.Parent, "Root order.Parent should round-trip — depth 1 < MaxDepth"); + Assert.IsInstanceOfType(result.Parent, typeof(UserPreferences_All_True)); + } + + /// + /// REPRO — SGen + polymorphic Parent inside a cycle. This is the failing case in the + /// original SameInstance test for (useSgen=true, useMeta=false) None mode. + /// + /// + /// The cycle (Items[0].ParentOrder = order) makes TestOrder.WriteProperties recurse. + /// At each cycle level, the body writes its Parent property polymorphically via + /// WriteValueNonPrimitiveWithWrapperPolyWriteObjectPolymorphic. When the + /// cycle reaches MaxDepth, the SGen path produces wire bytes that the SGen reader + /// later mis-interprets (DECIMAL_DRIFT on TotalAmount at the deepest unwind frame). + /// Runtime control above with the same data works correctly — diff the two wires to find + /// where SGen diverges. + /// + /// Focus debug-watch targets at the truncation boundary: + /// - Truncate path (Null written) + /// - inc/dec balance + /// - The polymorphic-prefix wire bytes (FixObj-slot vs ObjectWithTypeName) immediately before/after the truncate + /// + [TestMethod] + public void Sgen_None_Truncate_PolymorphicCycle_RoundTrips() + { + var order = BuildCycleWithPolymorphicParent(); + + var options = new AcBinarySerializerOptions + { + ReferenceHandling = ReferenceHandlingMode.None, + UseGeneratedCode = true, // ← SGen path (triggers the bug) + UseMetadata = false, + MaxDepth = MaxDepthForTest, + MaxDepthBehavior = MaxDepthBehavior.Truncate + }; + + var binary = AcBinarySerializer.Serialize(order, options); + DumpWire("SGen + Truncate + PolymorphicParent", binary); + var result = binary.BinaryTo(); + + Assert.IsNotNull(result); + Assert.AreEqual(1, result.Id); + Assert.AreEqual("TEST-001", result.OrderNumber); + Assert.IsNotNull(result.Items); + Assert.AreEqual(1, result.Items.Count); + Assert.IsNotNull(result.Parent, "Root order.Parent should round-trip — depth 1 < MaxDepth"); + Assert.IsInstanceOfType(result.Parent, typeof(UserPreferences_All_True)); + } + + /// + /// Non-cyclic shallow case — the primary delta-update use case. + /// Serialize an entity with intentionally truncated nested collections (MaxDepth=1), + /// verify root + first-level scalar properties round-trip while nested complex ones become null. + /// Both runtime and SGen paths should pass this. If SGen fails here too, the bug isn't + /// cycle-specific — it's pure Truncate-emission corruption. + /// + [TestMethod] + [DataRow(false, DisplayName = "Runtime")] + [DataRow(true, DisplayName = "SGen")] + public void Sgen_Or_Runtime_None_Truncate_NoCycle_ShallowRoundTrip(bool useSgen) + { + var order = new TestOrder_Circ_Ref + { + Id = 42, + OrderNumber = "DELTA-UPDATE-001", + Items = + [ + new TestOrderItem_Circ_Ref { Id = 1, ProductName = "P1" }, + new TestOrderItem_Circ_Ref { Id = 2, ProductName = "P2" } + ] + // No cycle. Items array elements truncate at MaxDepth=1. + }; + + var options = new AcBinarySerializerOptions + { + ReferenceHandling = ReferenceHandlingMode.None, + UseGeneratedCode = useSgen, + UseMetadata = false, + MaxDepth = 1, // root + 1 level + MaxDepthBehavior = MaxDepthBehavior.Truncate + }; + + var binary = AcBinarySerializer.Serialize(order, options); + DumpWire($"NoCycle Truncate ({(useSgen ? "SGen" : "Runtime")})", binary); + var result = binary.BinaryTo(); + + Assert.IsNotNull(result); + Assert.AreEqual(42, result.Id); + Assert.AreEqual("DELTA-UPDATE-001", result.OrderNumber); + // Items elements are at depth 2 — get truncated to null at MaxDepth=1. + // Result.Items list itself should exist (it's at depth 1), but elements should be null. + // The exact element-or-null result is depth-implementation-dependent — the strict invariant + // is "deserialize doesn't throw and root scalars are intact". + } +} diff --git a/AyCode.Core/Serializers/AcSerializerOptions.cs b/AyCode.Core/Serializers/AcSerializerOptions.cs index f413c75..29b57d7 100644 --- a/AyCode.Core/Serializers/AcSerializerOptions.cs +++ b/AyCode.Core/Serializers/AcSerializerOptions.cs @@ -39,11 +39,13 @@ public abstract class AcSerializerOptions protected static readonly bool DetectedIsWasm = OperatingSystem.IsBrowser(); /// - /// Maximum depth for serialization/deserialization. - /// 0 = depth check disabled (equivalent to ) + /// Maximum depth for serialization/deserialization. At this depth limit the action defined by + /// is taken (throw, truncate to null, or skip the check). + /// 0 = no nesting allowed (shallow-copy semantic: root level only — nested values are truncated/throw at first recursion) /// 1 = root + first level of nested objects/collections /// byte.MaxValue (255) = effectively unlimited /// Default: byte.MaxValue + /// To disable the depth check entirely, set =. /// public byte MaxDepth { diff --git a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.PropertyWriters.cs b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.PropertyWriters.cs index 1319745..ecf4a76 100644 --- a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.PropertyWriters.cs +++ b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.PropertyWriters.cs @@ -287,10 +287,15 @@ public static partial class AcBinarySerializer [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool WriteObjectRefMarkerIId() { + // Combined check+inc — TryEnterRecursion inc'd _recursionDepth on success. + if (TryEnterRecursion(hasTruncatePath: true)) return false; + if (HasRefHandling && TryConsumeWritePlanEntry(out var pe)) { if (!pe.IsFirst) { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the inc. + ExitRecursion(); WriteByte(BinaryTypeCode.ObjectRef); WriteVarUInt((uint)pe.CacheMapIndex); return false; @@ -312,10 +317,15 @@ public static partial class AcBinarySerializer [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool WriteObjectRefMarkerAll() { + // Combined check+inc — TryEnterRecursion inc'd _recursionDepth on success. + if (TryEnterRecursion(hasTruncatePath: true)) return false; + if (HasAllRefHandling && TryConsumeWritePlanEntry(out var pe)) { if (!pe.IsFirst) { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the inc. + ExitRecursion(); WriteByte(BinaryTypeCode.ObjectRef); WriteVarUInt((uint)pe.CacheMapIndex); return false; @@ -332,11 +342,14 @@ public static partial class AcBinarySerializer /// /// Metadata only, no ref tracking. Writes ObjectWithMetadata or Object marker. - /// Always returns — caller always calls WriteProperties after this. + /// Returns false if depth-limit Truncate fired (caller skips WriteProperties), true otherwise. /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void WriteObjectMetaMarker(object value, int wrapperSlot) + internal bool WriteObjectMetaMarker(object value, int wrapperSlot) { + // Combined check+inc — no ref handling here, no 2nd-occurrence concern, always recurses on success. + if (TryEnterRecursion(hasTruncatePath: true)) return false; + if (UseMetadata) { var wrapper = GetWrapper(value.GetType(), wrapperSlot); @@ -348,6 +361,7 @@ public static partial class AcBinarySerializer { WriteByte(BinaryTypeCode.Object); } + return true; } /// @@ -356,6 +370,10 @@ public static partial class AcBinarySerializer /// internal bool WriteObjectFullMarkerIId(object value, int wrapperSlot) { + // Combined check+inc FIRST — fires BEFORE any marker write so Truncate writes Null wire-correctly. + // TryEnterRecursion inc'd _recursionDepth on success; 2nd-occurrence ObjectRef branch below undoes it. + if (TryEnterRecursion(hasTruncatePath: true)) return false; + var useMetadata = UseMetadata; bool isFirstMeta = false; BinarySerializeTypeMetadata? metadata = null; @@ -370,11 +388,14 @@ public static partial class AcBinarySerializer { if (!pe.IsFirst) { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the inc. + ExitRecursion(); WriteByte(BinaryTypeCode.ObjectRef); WriteVarUInt((uint)pe.CacheMapIndex); return false; } + // 1st-occurrence ObjectRefFirst: writes marker + body via WriteProperties (inc already done) if (useMetadata) { WriteByte(BinaryTypeCode.ObjectWithMetadataRefFirst); @@ -390,6 +411,7 @@ public static partial class AcBinarySerializer return true; } + // Non-ref path: write marker + body via WriteProperties (inc already done) if (useMetadata) { WriteByte(BinaryTypeCode.ObjectWithMetadata); @@ -409,6 +431,10 @@ public static partial class AcBinarySerializer /// internal bool WriteObjectFullMarkerAll(object value, int wrapperSlot) { + // Combined check+inc FIRST — wire-correct Truncate semantics (Null instead of marker, no rewind). + // TryEnterRecursion inc'd _recursionDepth on success; 2nd-occurrence ObjectRef branch below undoes it. + if (TryEnterRecursion(hasTruncatePath: true)) return false; + var useMetadata = UseMetadata; bool isFirstMeta = false; BinarySerializeTypeMetadata? metadata = null; @@ -423,11 +449,14 @@ public static partial class AcBinarySerializer { if (!pe.IsFirst) { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the inc. + ExitRecursion(); WriteByte(BinaryTypeCode.ObjectRef); WriteVarUInt((uint)pe.CacheMapIndex); return false; } + // 1st-occurrence: writes marker + body via WriteProperties (inc already done) if (useMetadata) { WriteByte(BinaryTypeCode.ObjectWithMetadataRefFirst); @@ -443,6 +472,7 @@ public static partial class AcBinarySerializer return true; } + // Non-ref path: writes marker + body via WriteProperties (inc already done) if (useMetadata) { WriteByte(BinaryTypeCode.ObjectWithMetadata); diff --git a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.cs b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.cs index bc78833..4022017 100644 --- a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.cs +++ b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.cs @@ -88,32 +88,25 @@ public static partial class AcBinarySerializer /// /// Global recursion depth counter — final safety net against pathological graphs and non-IId cycles. - /// Incremented/decremented at object-recursion entry/exit points (WriteObject runtime + generated WriteProperties/ScanObject). - /// Checked against (byte, default 255). - /// Gated by — single context field-read at each call site. + /// Incremented at (before marker write at each object-recursion entry), + /// decremented at (at WriteProperties/ScanObject body exit). + /// Checked against (byte, default 255) + /// inside which fires BEFORE any marker byte is written. /// - internal byte RecursionDepth; + private byte _recursionDepth; /// - /// Pre-computed depth-check gate, set at : - /// !HasAllRefHandling && MaxDepth > 0 && MaxDepthBehavior != Disable. - /// - /// HasAllRefHandling=true: every type tracked → 2nd-occurrence early-return prevents cycles → check redundant - /// MaxDepth=0: explicit developer opt-out (magic value) — disables the safety net entirely - /// MaxDepthBehavior=Disable: explicit developer opt-out (intent-level) — same effect, expressed via the behavior enum - /// - /// All three opt-outs collapse to NeedsDepthCheck=false so the hot path is a single field-read + - /// register-test, no per-call enum compare or negation. Truncate vs Throw dispatch (when implemented for - /// Truncate placement) consults on the rare depth-limit-hit path only. + /// Pre-computed depth-check gate, set at : MaxDepthBehavior != Disable. + /// Only the explicit opts out. + /// stays active even with HasAllRefHandling=true, because per-type EnableRefHandling=false + /// attribute opt-outs leave gaps that ref-handling alone cannot cover (cycle through a non-tracked type). /// - internal bool NeedsDepthCheck; + private bool _needsDepthCheck; /// - /// Pre-computed action when reaches . - /// Mirrors ; checked inside the depth-comparison hot path - /// (gated short-circuit: comparison runs first, this enum is consulted only when the depth limit is actually hit). + /// Pre-computed action when depth reaches MaxDepth. Consulted on the rare depth-limit-hit cold path only. /// - internal MaxDepthBehavior MaxDepthAction; + private MaxDepthBehavior _maxDepthAction; #region WriteDuplicateEntry — scan pass output for write pass cursor @@ -321,12 +314,17 @@ public static partial class AcBinarySerializer FastWire = Options.WireMode == WireMode.Fast; - // Pre-compute depth-check gate. Three opt-out paths collapsed here: - // 1. ReferenceHandling=All → every type tracked, per-type 2nd-occurrence prevents cycles - // 2. MaxDepth=0 → explicit developer opt-out (magic value) - // 3. MaxDepthBehavior=Disable → explicit developer opt-out (intent-level enum) - MaxDepthAction = Options.MaxDepthBehavior; - NeedsDepthCheck = !HasAllRefHandling && Options.MaxDepth > 0 && MaxDepthAction != MaxDepthBehavior.Disable; + // Pre-compute depth-check gate. Only `MaxDepthBehavior.Disable` opts out: + // - `Throw` (default): safety net. `HasAllRefHandling=true` is NOT enough on its own — per-type + // `EnableRefHandling=false` attribute opt-outs leave non-tracked types that can still form cycles. + // The safety net catches those + pathological-depth non-cyclic graphs. + // - `Truncate`: explicit developer intent for intentional shallow serialization (delta updates, + // view-model projections). Always active when set, regardless of ref-handling mode. + // - `Disable`: explicit opt-out — dev guarantees cycle-free + bounded-depth graph. + // (MaxDepth value itself is NOT an opt-out — 0 means shallow-copy semantic when paired with Truncate.) + _maxDepthAction = Options.MaxDepthBehavior; + _needsDepthCheck = _maxDepthAction != MaxDepthBehavior.Disable; + _recursionDepth = 0; } public override void Clear() @@ -342,7 +340,7 @@ public static partial class AcBinarySerializer _nextCacheIndex = 0; NextFirstIndex = 0; ScanVisitIndex = 0; - RecursionDepth = 0; + _recursionDepth = 0; WritePlanCursor = 0; WriteVisitIndex = 0; _nextWritePlanVisitIndex = int.MaxValue; @@ -377,6 +375,61 @@ public static partial class AcBinarySerializer base.Clear(); } + /// + /// Hot wrapper: combined depth check + recursion enter. Gated by _needsDepthCheck. + /// On limit hit: dispatches to (truncate writes Null or throw) and returns + /// true — caller must skip marker write + recursive call. NO inc on this path. + /// On miss: increments _recursionDepth and returns false — caller proceeds with marker write + /// and recursive body. at body exit balances the inc. + /// Called BEFORE any marker write — wire-correct for any marker width. + /// 2nd-occurrence ObjectRef paths (which don't actually recurse into a body) must call + /// to undo the inc that this method did on the success path. + /// Single ctx field-read pattern: 2 ops merged into 1 method call vs the prior 3-method split. + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal bool TryEnterRecursion(bool hasTruncatePath) + { + if (!_needsDepthCheck) return false; + if (_recursionDepth >= MaxDepth) + { + OnMaxDepthHit(hasTruncatePath); + return true; + } + _recursionDepth++; + return false; + } + + /// + /// Dec _recursionDepth if depth tracking is active. Called at WriteProperties/ScanObject body exit + /// to balance the corresponding call on its success path. + /// Also called on 2nd-occurrence ObjectRef paths in WriteObjectFullMarker* / WriteObjectRefMarker* + /// to undo the inc (2nd-occ writes ObjectRef without recursing — wrote no body, must dec back). + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void ExitRecursion() + { + if (_needsDepthCheck) _recursionDepth--; + } + + /// + /// Cold path: dispatches on _maxDepthAction when the depth limit is hit. + /// + /// Throw: throws (offending type name comes from the stack trace) + /// Truncate + =true (write pass): writes Null marker in place of the object — intentional shallow serialization. Wire-correct: no marker has been written yet, so no rewind needed + /// Truncate + =false (scan pass): no-op — caller's return skips children scan + /// + /// NoInlining keeps the throw / WriteByte body out of the hot caller (smaller i-cache footprint). + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private void OnMaxDepthHit(bool hasTruncatePath) + { + if (_maxDepthAction == MaxDepthBehavior.Throw) + throw new InvalidOperationException( + $"AcBinary: recursion depth exceeded MaxDepth={MaxDepth} (depth={_recursionDepth}, position={Position})"); + // Truncate: write Null in place of the object. No rewind — check fires BEFORE any marker write. + if (hasTruncatePath) WriteByte(BinaryTypeCode.Null); + } + public void Dispose() { if (_propertyIndexBuffer != null) diff --git a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.ScanPass.cs b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.ScanPass.cs index e5de300..a0678e8 100644 --- a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.ScanPass.cs +++ b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.ScanPass.cs @@ -156,17 +156,9 @@ public static partial class AcBinarySerializer } // Fallback: runtime property loop (no SGen writer for this type) - // Global recursion depth safety net — gated by context.NeedsDepthCheck - // (pre-computed at Reset(): !HasAllRefHandling && MaxDepth > 0 && MaxDepthBehavior != Disable). - // Local-cached flag: 1 ctx field-read, register-resident — re-used at inc and dec. - var needsDepthCheck = context.NeedsDepthCheck; - if (needsDepthCheck) - { - if (context.RecursionDepth >= context.MaxDepth) - throw new InvalidOperationException( - $"AcBinary scan: recursion depth exceeded MaxDepth={context.MaxDepth} at type '{wrapper.Metadata.SourceType.FullName}' (depth={context.RecursionDepth})"); - context.RecursionDepth++; - } + // Combined check+inc — gated inside TryEnterRecursion (checks NeedsDepthCheck). + // On limit hit: Throw or Truncate=return (write pass writes Null at the same boundary). + if (context.TryEnterRecursion(hasTruncatePath: false)) return; var refProperties = metadata.ReferenceProperties; var hasPropertyFilter = context.HasPropertyFilter; @@ -213,7 +205,7 @@ public static partial class AcBinarySerializer } } - if (needsDepthCheck) context.RecursionDepth--; + context.ExitRecursion(); } /// diff --git a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.cs b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.cs index a2245a0..2974af0 100644 --- a/AyCode.Core/Serializers/Binaries/AcBinarySerializer.cs +++ b/AyCode.Core/Serializers/Binaries/AcBinarySerializer.cs @@ -1473,10 +1473,15 @@ public static partial class AcBinarySerializer private static void WriteObject(object value, TypeMetadataWrapper wrapper, BinarySerializationContext context) where TOutput : struct, IBinaryOutputBase { + // Combined check+inc FIRST — fires BEFORE any marker write so Truncate writes Null wire-correctly. + // TryEnterRecursion inc'd _recursionDepth on success. + if (context.TryEnterRecursion(hasTruncatePath: true)) return; + var metadata = wrapper.Metadata; var useMetaForType = context.UseMetadata && metadata.EnableMetadataFeature; - // Only IId types with ref handling enabled go to cold path + // Ref-handling dispatch. The variants check write plan: on 2nd-occurrence ObjectRef they undo the inc + // via ExitRecursion (we already inc'd above but no body recursion will happen for that path). if (context.UseTypeReferenceHandling(metadata)) { if (useMetaForType) @@ -1486,6 +1491,9 @@ public static partial class AcBinarySerializer return; } + // Non-ref path: write marker + body. Inc already done by TryEnterRecursion; matched by ExitRecursion + // at WriteObjectProperties body exit. + if (useMetaForType) { // UseMetadata: típus regisztrálása (első vs ismételt előfordulás tracking) @@ -1520,7 +1528,7 @@ public static partial class AcBinarySerializer private static void WriteObjectWithRefHandling(object value, TypeMetadataWrapper wrapper, BinarySerializationContext context) where TOutput : struct, IBinaryOutputBase { - // Reference handling: consume pre-computed write plan entry from scan pass cursor + // Caller (WriteObject) already inc'd via TryEnterRecursion. 2nd-occurrence path undoes it. var cachedObjectCacheIndex = -1; if (context.TryConsumeWritePlanEntry(out var planEntry)) { @@ -1531,12 +1539,14 @@ public static partial class AcBinarySerializer } else { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the caller's inc. + context.ExitRecursion(); WriteObjectRef(context, planEntry.CacheMapIndex); return; } } - // Marker kiírása — no metadata + // Marker kiírása — no metadata (inc already done by caller's TryEnterRecursion) if (cachedObjectCacheIndex >= 0) { context.WriteByte(BinaryTypeCode.ObjectRefFirst); @@ -1568,7 +1578,7 @@ public static partial class AcBinarySerializer { var isFirstMetadataOccurrence = BinarySerializationContext.RegisterMetadataType(wrapper); - // Reference handling: consume pre-computed write plan entry from scan pass cursor + // Caller (WriteObject) already inc'd via TryEnterRecursion. 2nd-occurrence path undoes it. var cachedObjectCacheIndex = -1; if (context.TryConsumeWritePlanEntry(out var planEntry)) { @@ -1579,12 +1589,14 @@ public static partial class AcBinarySerializer } else { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the caller's inc. + context.ExitRecursion(); WriteObjectRef(context, planEntry.CacheMapIndex); return; } } - // Marker kiírása — with metadata + // Marker kiírása — with metadata (inc already done by caller's TryEnterRecursion) if (cachedObjectCacheIndex >= 0) { context.WriteByte(BinaryTypeCode.ObjectWithMetadataRefFirst); @@ -1611,24 +1623,14 @@ public static partial class AcBinarySerializer var generatedWriter = wrapper.GeneratedWriter; if (generatedWriter != null) { - // SGen path handles its own RecursionDepth inc/dec via generated emit (gated by NeedsDepthCheck) + // SGen path: generated WriteProperties ends with its own ExitRecursion call. generatedWriter.WriteProperties(value, context); return; } } - // Runtime path: global recursion depth safety net — gated by context.NeedsDepthCheck - // (pre-computed at Reset(): !HasAllRefHandling && MaxDepth > 0 && MaxDepthBehavior != Disable). - // Local-cached flag: 1 ctx field-read, register-resident — re-used at inc and dec. - var needsDepthCheck = context.NeedsDepthCheck; - if (needsDepthCheck) - { - if (context.RecursionDepth >= context.MaxDepth) - throw new InvalidOperationException($"AcBinary serialize: recursion depth exceeded MaxDepth={context.MaxDepth} at type '{wrapper.Metadata.SourceType.FullName}' (depth={context.RecursionDepth}, position={context.Position})"); - - context.RecursionDepth++; - } - + // Runtime path: depth-check + EnterRecursion was done by the caller (WriteObject or variants) + // BEFORE marker write. Body just runs property writes; ExitRecursion at end balances EnterRecursion. if (!useMetaForType) { WritePropertiesMarkerless(value, wrapper, context); @@ -1638,7 +1640,7 @@ public static partial class AcBinarySerializer WritePropertiesWithMeta(value, wrapper, context); } - if (needsDepthCheck) context.RecursionDepth--; + context.ExitRecursion(); } private static void WritePropertiesWithMeta(object value, TypeMetadataWrapper wrapper, BinarySerializationContext context) where TOutput : struct, IBinaryOutputBase @@ -1727,9 +1729,12 @@ public static partial class AcBinarySerializer private static void WriteObjectPolymorphic(object value, TypeMetadataWrapper wrapper, BinarySerializationContext context, Type polyRuntimeType) where TOutput : struct, IBinaryOutputBase { + // Combined check+inc FIRST — fires BEFORE any marker write so Truncate writes Null wire-correctly. + if (context.TryEnterRecursion(hasTruncatePath: true)) return; + var metadata = wrapper.Metadata; - // Reference handling + // Reference handling — 2nd-occurrence ObjectRef path undoes the inc done above. var cachedObjectCacheIndex = -1; if (context.UseTypeReferenceHandling(metadata)) { @@ -1742,12 +1747,15 @@ public static partial class AcBinarySerializer } else { + // 2nd-occurrence: ObjectRef written, no body to recurse into → undo the inc. + context.ExitRecursion(); WriteObjectRef(context, planEntry.CacheMapIndex); return; } } } + // 1st-occurrence or non-tracked: writes marker + body (inc already done by TryEnterRecursion) // Poly marker (handles combined poly+ref) WritePolymorphicMarker(context, polyRuntimeType, cachedObjectCacheIndex); diff --git a/AyCode.Core/Serializers/Binaries/AcBinarySerializerOptions.cs b/AyCode.Core/Serializers/Binaries/AcBinarySerializerOptions.cs index f888333..fbebd50 100644 --- a/AyCode.Core/Serializers/Binaries/AcBinarySerializerOptions.cs +++ b/AyCode.Core/Serializers/Binaries/AcBinarySerializerOptions.cs @@ -40,6 +40,9 @@ public sealed class AcBinarySerializerOptions : AcSerializerOptions public static AcBinarySerializerOptions ShallowCopy => new() { MaxDepth = 0, + // Truncate preserves the original "root + Null nested" semantic; under default Throw the preset + // would throw on the first nested object, defeating its purpose. + MaxDepthBehavior = MaxDepthBehavior.Truncate, UseStringInterning = StringInterningMode.None, ReferenceHandling = ReferenceHandlingMode.None }; diff --git a/AyCode.Core/docs/BINARY/BINARY_ISSUES.md b/AyCode.Core/docs/BINARY/BINARY_ISSUES.md index 31c4430..346930f 100644 --- a/AyCode.Core/docs/BINARY/BINARY_ISSUES.md +++ b/AyCode.Core/docs/BINARY/BINARY_ISSUES.md @@ -181,16 +181,22 @@ The default value (`true`) throws `InvalidOperationException` on FNV-1a property ### ACCORE-BIN-I-P2H8: MaxDepth cut-off Null indistinguishable from real null -**Status:** Open -**Affects:** `AcBinarySerializerOptions.MaxDepth` (and any preset using a non-default value, e.g. `ShallowCopy` preset has `MaxDepth=0`) +**Status:** Closed (2026-05-14) — reframed as explicit opt-in feature via `MaxDepthBehavior` enum on `AcSerializerOptions`. +**Affects:** `AcBinarySerializerOptions.MaxDepth` (and any preset using a non-default value) -When the object graph exceeds `MaxDepth`, deeper objects/collections are written as `Null(76)` — **the same byte as a genuine null value**. The deserializer cannot distinguish "depth-cut-off null" from "real null" → silent data loss without any signal at the receive side. +When the object graph exceeded `MaxDepth`, deeper objects/collections were written as `Null(76)` — **the same byte as a genuine null value** — without any opt-in. The deserializer could not distinguish "depth-cut-off null" from "real null", so unintentional truncation appeared as silent data loss. -**Impact:** Latent for the default `MaxDepth=255` (real graphs rarely hit). Severe with explicit lower limits — `ShallowCopy` preset (`MaxDepth=0`) silently drops every nested object on the receive side without an exception. Detection requires a separate depth-aware comparison. +**Impact (historical):** The unconditional silent truncation was the actual problem — developers couldn't see when it fired. The wire ambiguity itself is fine *when the developer opts in* (typical shallow-serialization use case: partial-update endpoint where "null nested collection" means "no change at this level" per the protocol contract). -**Possible fix directions:** -- **Dedicated `DepthExceeded` wire marker** (distinct from `Null(76)`) — wire-format breaking change, major version. -- **Configurable policy on cut-off** — `MaxDepthBehavior.WriteNull` (today's default) / `Throw` / `Log+WriteNull`. Non-breaking opt-in. +### Resolution + +The shallow-serialization use case is a legitimate, common pattern (client edits a grid value → server gets a flat entity for DB update, with no need to round-trip nested sub-lists). The fix is **explicit opt-in** so unintentional truncation can't sneak through. `AcSerializerOptions` now exposes `MaxDepthBehavior` (enum, default `Throw`): + +- `Throw` (new default) — `InvalidOperationException` with type name + position. Unintended depth-exceeded cases surface as a debuggable exception. +- `Truncate` — the previous `WriteByte(Null)` behavior, now explicit opt-in for shallow serialization (delta updates, view-model projections, partial DB-update flows). The wire `Null` at the truncation boundary is the developer's contract decision — endpoint protocol dictates what nested null means. Works with any persistence layer (Dapper, ADO.NET, Cosmos DB, MongoDB, Redis, EF Core, etc.). +- `Disable` — skip the depth check entirely (max perf, dev guarantees cycle-free graph). + +The check moved from "every object/collection write" (with rewind) to "before any marker byte is written" (in `WriteObject` runtime + `WriteObjectFullMarker*` SGen). The `ShallowCopy` preset was updated to explicitly set `MaxDepthBehavior = Truncate` to preserve its original "root + Null nested" semantic. See `BINARY_OPTIONS.md` `MaxDepth + MaxDepthBehavior` section for full details. ### ACCORE-BIN-I-W3F4: PropertyFilter + UseMetadata=false silently corrupts via index drift @@ -208,16 +214,18 @@ When the serializer applies a `PropertyFilter`, excluded properties are complete ### ACCORE-BIN-I-J6T9: Non-IId circular references silently truncated when ThrowOnCircularReference=false -**Status:** Open +**Status:** Partially Fixed (2026-05-14) — default behavior now surfaces the cycle as an exception. **Affects:** `AcBinarySerializerOptions.ThrowOnCircularReference=false` combined with `ReferenceHandling != None` With `ThrowOnCircularReference=false` + reference handling enabled, **only `IId`-implementing types are tracked for cycle detection**. Non-`IId` circular references hit `MaxDepth` before being detected → silent truncation at the depth boundary, no exception, no log. -**Impact:** Borderline — explicit opt-in (developer must set `ThrowOnCircularReference=false`), and most domain models avoid non-`IId` cycles. But UI tree models (parent ↔ children with no `Id`), graph data structures, and self-referencing config nodes trigger this path silently. +**Impact (historical):** Borderline — explicit opt-in (developer must set `ThrowOnCircularReference=false`), and most domain models avoid non-`IId` cycles. But UI tree models (parent ↔ children with no `Id`), graph data structures, and self-referencing config nodes triggered this path silently. -**Possible fix directions:** -- **Universal cycle detection** — track all reference types for cycle detection regardless of `IId`-ness when `ThrowOnCircularReference=false` (deduplication remains `IId`-only — cycle detection becomes universal). -- **Diagnostic event** — surface "non-`IId` cycle dropped at depth N" as an `Action?` on options, opt-in. +### Resolution (partial) + +With the introduction of `MaxDepthBehavior` (default `Throw`, see ACCORE-BIN-I-P2H8), non-`IId` circular references now **throw at the depth boundary instead of silently truncating** — the safety-net global recursion counter (`RecursionDepth` byte field on the context, gated by `_needsDepthCheck = !HasAllRefHandling && MaxDepthBehavior != Disable`) fires at the `MaxDepth` limit and surfaces the cycle as `InvalidOperationException` with type name + position. + +**Remaining gap:** The detection mechanism is depth-based (fires at `MaxDepth`), not cycle-based — a non-`IId` cycle in an otherwise shallow graph still requires reaching the depth limit to be caught. True cycle detection (visit-set tracking) for non-`IId` types is a separate enhancement; the originally-proposed `[AcBinaryCircular]` attribute or universal cycle-set tracking could close the remaining gap. Track as a future enhancement. ### ACCORE-BIN-I-D9Y2: Default-value omission relies on type-level default consistency across writer/reader @@ -240,6 +248,43 @@ The serializer writes a 1-byte `PropertySkip` marker for any property whose valu - **Option flag**: `AcBinarySerializerOptions.OmitDefaults` (default `true` for back-compat); `false` writes every property's full value regardless. Lets consumers opt out for fragile-class-evolution scenarios. - **Hybrid**: ship doc + flag, default `true`. +### ACCORE-BIN-I-T7K3: `MaxDepthBehavior.Truncate` causes wire-misalignment on the SGen write path + +**Status:** Open · **Severity:** Major (silent data corruption when opt-in to `Truncate` with SGen-enabled types) · **Area:** Source generator emit (`AcBinarySourceGenerator.cs`) + SGen-path marker writers (`Serializers/Binaries/AcBinarySerializer.BinarySerializationContext.PropertyWriters.cs`) + +### Description + +With `MaxDepthBehavior.Truncate` opt-in AND `UseGeneratedCode = true` (SGen path), a cyclic graph that hits `MaxDepth` produces wire bytes that **deserialize incorrectly** — `AcBinaryDeserializationException: [DECIMAL_DRIFT]` or generic `IndexOutOfRangeException` raised by the SGen reader. + +``` +=========== ReferenceHandling: None, UseSgen: True, UseMeta: False =========== +AcBinarySerializer.Serialize(cyclicOrder, options-with-MaxDepth=10-and-Truncate); +// Serialize succeeds (truncation appears to write Null at the boundary) +order.BinaryTo(); +// Throws DECIMAL_DRIFT at pos=669 +``` + +Runtime path (`UseGeneratedCode = false`) with the same `Truncate` setting works correctly. `Throw` and `Disable` on the SGen path are also unaffected — only the SGen + Truncate combination corrupts. + +### Root cause (suspected) + +The check-before-marker placement that wires `Truncate` correctly was added to `WriteObjectFullMarker*` (in `PropertyWriters.cs`) and the SGen direct-marker emit patterns. Comparison with the runtime path (which works) suggests the SGen-side has either: + +- A subtle inc/dec asymmetry between `EnterRecursion` / `ExitRecursion` on a path that doesn't go through `WriteObjectFullMarker*` (e.g. a poly-related branch), +- OR the wire-size difference between runtime (1705 B for the test case) and SGen (1449 B) for the same data indicates the SGen path encodes the cycle's truncation boundary in a slightly different position than the SGen reader expects. + +Reproducer: `AyCode.Core.Tests/Serialization/AcBinarySerializerIIdReferenceTests.cs` → `SameInstance_SerializeAndDeserialize (True, False)` sub-test. + +### Known workaround + +For `Truncate` use cases on SGen-typed graphs: +- Use `UseGeneratedCode = false` (runtime path) — works correctly. Trades SGen performance for `Truncate` correctness. +- Or keep `MaxDepthBehavior = Throw` (default) — fail-fast surfaces the cycle as an exception rather than truncating. + +### Related TODO + +None yet — needs targeted debugging session to isolate the SGen-path wire diff. + ## Wire Format / Cross-platform ### ACCORE-BIN-I-E4N9: Wire format is host-native-endian, NOT canonical little-endian diff --git a/AyCode.Core/docs/BINARY/BINARY_OPTIONS.md b/AyCode.Core/docs/BINARY/BINARY_OPTIONS.md index eee9e07..04a7af7 100644 --- a/AyCode.Core/docs/BINARY/BINARY_OPTIONS.md +++ b/AyCode.Core/docs/BINARY/BINARY_OPTIONS.md @@ -56,17 +56,33 @@ Configuration options, presets, and option interactions for `AcBinarySerializerO **Code branch:** `context.StringInternEligible` flag set per-property before `WriteString`. Scan pass builds a `WriteDuplicateEntry[]` plan; write pass consumes it via cursor. -## MaxDepth +## MaxDepth + MaxDepthBehavior -| Value | Behavior | -|-------|----------| -| `255` (default) | Effectively unlimited nesting | -| `0` | Root level only — nested objects/collections written as `Null(76)` | -| `N` | Objects deeper than N levels written as `Null(76)` | +`MaxDepth` (`byte`, default `255`) is the recursion depth limit. `MaxDepthBehavior` (enum, default `Throw`) decides what happens at the limit. -**Format impact:** Depth-exceeded values appear as `Null(76)` in the stream — ⚠️ indistinguishable from actual null values, no special marker — see [`BINARY_ISSUES.md#accore-bin-i-p2h8`](BINARY_ISSUES.md#accore-bin-i-p2h8-maxdepth-cut-off-null-indistinguishable-from-real-null) for round-trip data-loss implications. +| `MaxDepth` value | Behavior | +|---|---| +| `255` (default) | Effectively unlimited nesting — safety net only triggers on pathological graphs | +| `N` (1..254) | Action at depth N decided by `MaxDepthBehavior` | +| `0` | First nested object hits the limit immediately — useful with `MaxDepthBehavior.Truncate` for the "root + null nested" shallow-copy semantic | -**Code branch:** Checked at entry of every object/collection write: `if (depth > MaxDepth) { WriteByte(Null); return; }`. +| `MaxDepthBehavior` | Default | Action at depth limit | Use case | +|---|---|---|---| +| `Throw` | ✓ default | Throws `InvalidOperationException` with type name + position | Cycle detection, bug surfacing (fail-fast) | +| `Truncate` | | Writes `Null(76)` marker in place of the object | Intentional shallow serialization (e.g. client→server delta updates where deep references are intentionally cut) | +| `Disable` | | Skips the depth check entirely | Maximum hot-path perf; consumer guarantees a cycle-free graph | + +**Only `MaxDepthBehavior.Disable` opts out the depth check entirely.** `Throw` stays active even with `ReferenceHandling = All`, because per-type `[AcBinarySerializable(EnableRefHandlingFeature = false)]` attribute opt-outs leave non-tracked types in the graph — those can still form cycles that ref-handling alone cannot catch. The safety net covers per-type-opt-out cycle gaps + pathological-depth non-cyclic graphs. + +**Truncate mode — the key shallow-serialization feature.** Depth-exceeded values appear as `Null(76)` on the wire. This is intentional and developer-controlled: the consumer endpoint contract decides what "null nested collection" means in context — typically "no change at this level" on a partial-update endpoint (`POST /orders/{id}/header` flat update) vs a separate full-save endpoint (`POST /orders/{id}/full`). + +This pattern is significantly cheaper than the alternatives (DTO-mapping per entity, EF-Core `.Select()` projections, manual flat-DTOs) — zero setup-overhead, no DTO type-system duplication, and the unused sub-graphs are never serialized to wire (zero byte + CPU savings). The same entity type lives on client + server; `MaxDepthBehavior=Truncate` is the toggle. Works with any persistence layer (Dapper, ADO.NET raw SQL, Cosmos DB, MongoDB, Redis, etc.) — not EF-Core-specific. + +The default `Throw` surfaces accidental depth-exceeded cases as a debuggable exception (offending type comes from the stack trace) so unintentional truncation can't sneak through — closes `BINARY_ISSUES.md#accore-bin-i-p2h8`. + +**Code branch:** Depth check fires BEFORE any marker byte is written (in runtime `WriteObject` + SGen `WriteObjectFullMarker*` / direct-marker emit paths). On limit hit, `TryEnterRecursion(hasTruncatePath)` on the context (combined check + inc) dispatches `Throw` / `Truncate` / `Disable`. `RecursionDepth` is a private `byte` field on the context; `ExitRecursion` is the inlined helper that decrements at body exit. The `_needsDepthCheck` gate is pre-computed at `Reset()` as `MaxDepthBehavior != Disable`. Hot path: 1 ctx field-read + 1 byte-cmp + 1 inc per object-recursion entry (Truncate use case can be combined with any `ReferenceHandling` mode). + +**Known issue:** SGen path + `Truncate` currently exhibits a wire-misalignment in specific test scenarios (`DECIMAL_DRIFT` on round-trip) — see `BINARY_ISSUES.md#accore-bin-i-sg-truncate-pending` (TBD). Runtime path + `Truncate` and SGen path + `Throw`/`Disable` are unaffected. ## UseCompression @@ -126,14 +142,14 @@ delegate PropertyInfo? PropertyMapperDelegate(PropertyInfo sourceProperty, Type ## Presets -| Preset | WireMode | Metadata | StringInterning | RefHandling | MaxDepth | Compression | Other | -|--------|----------|----------|-----------------|-------------|----------|-------------|-------| -| `Default` | Compact | false | Attribute | All | 255 | None | — | -| `FastMode` | Compact | false | None | None | 255 | None | No scan pass | -| `ShallowCopy` | Compact | false | None | None | **0** | None | Root level only | -| `WasmOptimized` | Compact | false | Attribute | All | 255 | None | +StringCaching | -| `WithoutReferenceHandling` | Compact | false | Attribute | **None** | 255 | None | No scan pass | -| `WithoutMetadata` | Compact | **false** | Attribute | All | 255 | None | — | +| Preset | WireMode | Metadata | StringInterning | RefHandling | MaxDepth | MaxDepthBehavior | Compression | Other | +|--------|----------|----------|-----------------|-------------|----------|------------------|-------------|-------| +| `Default` | Compact | false | Attribute | All | 255 | Throw | None | — | +| `FastMode` | Compact | false | None | None | 255 | Throw | None | No scan pass | +| `ShallowCopy` | Compact | false | None | None | **0** | **Truncate** ⚠️ | None | Root + Null nested (the Truncate behavior makes this preset's semantic meaningful — under default `Throw` it would throw on first nested object) | +| `WasmOptimized` | Compact | false | Attribute | All | 255 | Throw | None | +StringCaching | +| `WithoutReferenceHandling` | Compact | false | Attribute | **None** | 255 | Throw | None | No scan pass | +| `WithoutMetadata` | Compact | **false** | Attribute | All | 255 | Throw | None | — | **Performance implication of presets:** - `Default` / `WasmOptimized` — two-phase (scan + serialize) due to `ReferenceHandling=All` diff --git a/AyCode.Core/docs/TOON/TOON_IMPLEMENTATION.md b/AyCode.Core/docs/TOON/TOON_IMPLEMENTATION.md index 170a72d..55964d3 100644 --- a/AyCode.Core/docs/TOON/TOON_IMPLEMENTATION.md +++ b/AyCode.Core/docs/TOON/TOON_IMPLEMENTATION.md @@ -96,7 +96,7 @@ During `@data` emission: - First visit of a multi-referenced object → `@N TypeName { ... }`. - Subsequent visits → `@ref:N`. -Circular cycles are safe — the second visit aborts with the `@ref:` form. When reference handling is disabled, shared subgraphs serialize redundantly and true cycles truncate at `MaxDepth` to `null`. +Circular cycles are safe — the second visit aborts with the `@ref:` form. When reference handling is disabled, shared subgraphs serialize redundantly and true cycles truncate at `MaxDepth` to `null`. Note: the base `AcSerializerOptions.MaxDepthBehavior` (`Truncate`/`Throw`/`Disable`) is **not currently dispatched** in Toon — depth-limit handling is always silent-null truncation regardless of the setting (see `TOON_ISSUES.md#accore-toon-i-s4q9`, `TOON_TODO.md#accore-toon-t-w8b5`). ## Enum output diff --git a/AyCode.Core/docs/TOON/TOON_ISSUES.md b/AyCode.Core/docs/TOON/TOON_ISSUES.md index cfb3c4d..24d94c6 100644 --- a/AyCode.Core/docs/TOON/TOON_ISSUES.md +++ b/AyCode.Core/docs/TOON/TOON_ISSUES.md @@ -160,6 +160,41 @@ None — consumers must tolerate the current order. ### Related TODO None yet. Fix also requires reconciling `Serializers/README.md` with the chosen direction. +## ACCORE-TOON-I-S4Q9: `MaxDepthBehavior` option silently ignored + +**Severity:** Minor (silent feature gap — option visible, no effect) · **Status:** Open · **Area:** `Serializers/Toons/` (engine-wide; depth handling) + +### Description + +`MaxDepthBehavior` was introduced on the base `AcSerializerOptions` (2026-05) to dispatch the action at the `MaxDepth` boundary: + +- `Truncate` — write `Null` (silent shallow serialization). +- `Throw` — `InvalidOperationException` (cycle/bug detection, **base default**). +- `Disable` — skip the depth safety net entirely (max perf, dev guarantees no cycles). + +`AcBinarySerializer` honors all three. `AcToonSerializer` inherits the option through `AcToonSerializerOptions : AcSerializerOptions` but its depth-handling code path **does not dispatch on the value** — Toon always truncates to `null` regardless of what the consumer set. + +```csharp +var opts = new AcToonSerializerOptions +{ + MaxDepth = 5, + MaxDepthBehavior = MaxDepthBehavior.Throw // ← silently ignored +}; +// Output still truncates to `null` at depth 5; no exception thrown. +``` + +### Root cause + +Toon's depth handling predates the `MaxDepthBehavior` introduction. The pre-existing logic emits `null` at `depth > MaxDepth` (see `TOON_IMPLEMENTATION.md` reference scanning section). When the new enum was added at the base, only `AcBinarySerializer` was wired to dispatch on it; the Toon writer was not updated. + +### Known workaround + +If the application logic needs the `Throw` behavior on Toon: set a small `MaxDepth` and post-process the output to detect `null` truncation markers and raise the error at the consumer layer. For `Disable`, leave `MaxDepth = 255` (effective no-op given typical graph depths). + +### Related TODO + +`TOON_TODO.md#accore-toon-t-w8b5` — adopt `MaxDepthBehavior` dispatch in Toon's depth handling. + ## Issue entry template ``` diff --git a/AyCode.Core/docs/TOON/TOON_OPTIONS.md b/AyCode.Core/docs/TOON/TOON_OPTIONS.md index 29374dc..532b3b5 100644 --- a/AyCode.Core/docs/TOON/TOON_OPTIONS.md +++ b/AyCode.Core/docs/TOON/TOON_OPTIONS.md @@ -24,7 +24,8 @@ | Option | Type | Notes | |---|---|---| -| `MaxDepth` | `byte` | Recursion guard; when exceeded, `null` is emitted. | +| `MaxDepth` | `byte` | Recursion depth limit. Toon currently truncates to `null` when exceeded (silent shallow-cut). The action at the limit is conceptually dispatched on `MaxDepthBehavior`, but **Toon does not yet honor that option** — see `TOON_ISSUES.md#accore-toon-i-s4q9`. | +| `MaxDepthBehavior` | `MaxDepthBehavior` | `Truncate` / `Throw` / `Disable`. Default `Throw` at the base. **Not honored by Toon today**: Toon's depth handling is always silent-null truncation regardless of this setting. Tracked at `TOON_TODO.md#accore-toon-t-w8b5`. | | `ReferenceHandling` | `ReferenceHandlingMode` | `None` / `OnlyId` / `All` — required for circular or shared-reference graphs. | | `SerializerType` | `AcSerializerType` | Fixed to `Toon`. | diff --git a/AyCode.Core/docs/TOON/TOON_TODO.md b/AyCode.Core/docs/TOON/TOON_TODO.md index 06db0f0..d6c01f2 100644 --- a/AyCode.Core/docs/TOON/TOON_TODO.md +++ b/AyCode.Core/docs/TOON/TOON_TODO.md @@ -119,6 +119,22 @@ No benchmark harness was found in `AyCode.Core.Tests` or `AyCode.Benchmark` back - (a) Run real benchmarks and replace with measured values in `TOON_OPTIONS.md`. - (b) Drop the claims entirely — marketing filler without provenance is worse than no claim. +## ACCORE-TOON-T-W8B5: Adopt `MaxDepthBehavior` dispatch in Toon +**Priority:** P2 · **Type:** Feature parity · **Related:** `TOON_ISSUES.md#accore-toon-i-s4q9` + +The base `AcSerializerOptions.MaxDepthBehavior` enum (`Truncate` / `Throw` / `Disable`) is honored by `AcBinarySerializer` but silently ignored by `AcToonSerializer`. Bring Toon to parity: + +- **`Truncate`** — current Toon behavior (emit `null` at `depth > MaxDepth`); rename comment / wire the existing path through the dispatch. +- **`Throw`** — throw `InvalidOperationException` with type name + position in the output (offer matches Binary's diagnostic surface). +- **`Disable`** — skip the depth check entirely (max perf when graph is known cycle-free). + +Implementation site: `AcToonSerializer.DataSection.cs` (depth-check fire-point) + `AcToonSerializer.MetaWriter.cs` (the `@types` walk also recurses; verify whether it should honor the same dispatch). + +Acceptance: +- All three modes produce the documented output. +- A test exercises each variant on a cyclic graph without `ReferenceHandling`. +- Update `TOON_OPTIONS.md` `MaxDepthBehavior` row and `TOON_IMPLEMENTATION.md` reference-scanning paragraph to drop the "not honored" caveats; close `TOON_ISSUES.md#accore-toon-i-s4q9`. + ## TODO entry template ``` diff --git a/AyCode.Services.Server.Tests/SignalRs/SignalRClientToHubTest.cs b/AyCode.Services.Server.Tests/SignalRs/SignalRClientToHubTest.cs index f1c4a52..1042ad6 100644 --- a/AyCode.Services.Server.Tests/SignalRs/SignalRClientToHubTest.cs +++ b/AyCode.Services.Server.Tests/SignalRs/SignalRClientToHubTest.cs @@ -1054,7 +1054,7 @@ public abstract class SignalRClientToHubTestBase { TestDataFactory.ResetIdCounter(); var dataSets = BenchmarkTestDataProvider.CreateTestDataSets(resetId: false); - var orders = dataSets.Select(ds => ds.Order).ToList(); + var orders = dataSets.Select(ds => ds.GetOrder()).ToList(); var result = await _client.PostDataAsync, List>( TestSignalRTags.TestOrderListParam, orders);