Skip to content

fix: resolve code-review findings across engine, caching, and execution - #142

Merged
asulwer merged 1 commit into
masterfrom
fix/review-findings
Jul 21, 2026
Merged

fix: resolve code-review findings across engine, caching, and execution#142
asulwer merged 1 commit into
masterfrom
fix/review-findings

Conversation

@asulwer

@asulwer asulwer commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Addresses a full-codebase review. Fixes correctness bugs in the execution engine and result cache, two latent memory leaks in the compiler, and hardens the XML/JSON extensions. All 7 projects build with 0 warnings; 690/690 tests pass on net8.0 and net9.0.

Core engine

  • CacheKeyBuilder: hash reference-type parameters by VALUE (public readable properties) instead of identity. Mutating an object in place now yields a different key and re-evaluates, rather than returning a stale cached result. Enums are keyed by underlying value; recursion is bounded by the existing depth guard with an identity fallback for deep/cyclic graphs. Added a doc note that lazy-loading proxies (e.g. EF entities) should not be used as cached parameters.
  • Workflow.ExecuteBufferedAsync: respect DependsOnRuleId. Previously it chunked rules by buffer size with no dependency check, so a dependent rule could run in the same parallel batch as its dependency and read a missing result. Now uses dependency-satisfied, level-based batching capped at bufferSize; also validates bufferSize >= 1.
  • Workflow.ExecuteParallel / ExecuteParallelAsync: tolerate dangling dependencies (a DependsOnRuleId pointing at an inactive/missing rule) the same way sequential Execute does, instead of throwing. Reworked both to level-based batching via a shared DependencySatisfied helper, which also removes the previous under-parallelization (independent rules following a dependent one in sort order were skipped in the first pass).
  • RuleResult.FirstFailure: return null when no child failed. It used FirstOrDefault over a non-nullable struct, which returned a default RuleResult (Success=false) and made "FirstFailure != null" always true.
  • GraphAlgorithms.TopologicalSort: replace the SortedSet ready-queue with a list. SortedSet collapsed comparer-equal nodes, silently dropping distinct equal-priority rules and misfiring cycle detection. Added an id->node lookup to remove the O(n^2) neighbor scans.
  • RuleCache: evict entries. Expired entries are now removed on read, and a configurable size cap (default 10,000) sweeps expired entries then trims the oldest, bounding memory for high-cardinality keys.
  • ExpressionCompiler: on recycle, clear the delegate cache first. Cached delegates pinned the outgoing AssemblyLoadContext, so recycling never actually reclaimed memory. Split the recycle trigger into its own counter so lifetime CompileCount is preserved while the context still recycles every maxCompilesBeforeRecycle compiles (previously it recycled on every compile once the threshold was crossed). Cache Lazy values so compilation runs once per key under contention, fold the reference provider into the cache key, and drop failed entries so compilation can be retried.
  • Rule execution: only materialize the delegate argument when a compiled delegate exists, avoiding an IndexOutOfRange for parent rules that have only child rules and are executed with no parameters.
  • Docs: clarify that AssemblyReferenceProvider is reference scoping, not a security sandbox — expression strings are compiled as trusted, full-trust code and must not come from untrusted input.

Extensions

  • RoslynRules.Xml: explicitly set DtdProcessing.Prohibit and XmlResolver=null on the schema-validation reader (XXE defense-in-depth; matches the safe framework defaults but states the intent).
  • RoslynRules.Json: suppress IL2026 with justification for the intentional reflection-based convenience API, consistent with how core suppresses IL2026 and avoiding an [RequiresUnreferencedCode] cascade through the ISnapshotSerializer interface. The *Aot methods and source-gen context remain the trim/AOT-safe path.

Benchmarks

  • CacheMemoryBenchmark: measure below the new 10,000-entry cache cap so the bytes/entry figure is accurate.

Demo

  • DemoRunner.LoadCustomers: resolve Data/Customers.json against AppContext.BaseDirectory instead of the current working directory, so the demo runs from any location (e.g. dotnet run from the repo root).

Tests

  • Update two CacheKeyBuilder tests that asserted the old identity-based behavior to assert value-based keys.
  • Add coverage: mutated reference parameter invalidates the cache, FirstFailure is null when all children pass, ExecuteParallel tolerates dangling dependencies, ExecuteBufferedAsync never runs a dependent in the same chunk as its dependency (and rejects bufferSize < 1), and a new RuleCacheEvictionTests suite for expiry and size-cap eviction.

Addresses a full-codebase review. Fixes correctness bugs in the execution
engine and result cache, two latent memory leaks in the compiler, and
hardens the XML/JSON extensions. All 7 projects build with 0 warnings;
690/690 tests pass on net8.0 and net9.0.

Core engine
-----------
* CacheKeyBuilder: hash reference-type parameters by VALUE (public readable
  properties) instead of identity. Mutating an object in place now yields a
  different key and re-evaluates, rather than returning a stale cached
  result. Enums are keyed by underlying value; recursion is bounded by the
  existing depth guard with an identity fallback for deep/cyclic graphs.
  Added a doc note that lazy-loading proxies (e.g. EF entities) should not
  be used as cached parameters.
* Workflow.ExecuteBufferedAsync: respect DependsOnRuleId. Previously it
  chunked rules by buffer size with no dependency check, so a dependent rule
  could run in the same parallel batch as its dependency and read a missing
  result. Now uses dependency-satisfied, level-based batching capped at
  bufferSize; also validates bufferSize >= 1.
* Workflow.ExecuteParallel / ExecuteParallelAsync: tolerate dangling
  dependencies (a DependsOnRuleId pointing at an inactive/missing rule) the
  same way sequential Execute does, instead of throwing. Reworked both to
  level-based batching via a shared DependencySatisfied helper, which also
  removes the previous under-parallelization (independent rules following a
  dependent one in sort order were skipped in the first pass).
* RuleResult.FirstFailure: return null when no child failed. It used
  FirstOrDefault over a non-nullable struct, which returned a default
  RuleResult (Success=false) and made "FirstFailure != null" always true.
* GraphAlgorithms.TopologicalSort: replace the SortedSet ready-queue with a
  list. SortedSet collapsed comparer-equal nodes, silently dropping distinct
  equal-priority rules and misfiring cycle detection. Added an id->node
  lookup to remove the O(n^2) neighbor scans.
* RuleCache: evict entries. Expired entries are now removed on read, and a
  configurable size cap (default 10,000) sweeps expired entries then trims
  the oldest, bounding memory for high-cardinality keys.
* ExpressionCompiler: on recycle, clear the delegate cache first. Cached
  delegates pinned the outgoing AssemblyLoadContext, so recycling never
  actually reclaimed memory. Split the recycle trigger into its own counter
  so lifetime CompileCount is preserved while the context still recycles
  every maxCompilesBeforeRecycle compiles (previously it recycled on every
  compile once the threshold was crossed). Cache Lazy<Delegate> values so
  compilation runs once per key under contention, fold the reference
  provider into the cache key, and drop failed entries so compilation can
  be retried.
* Rule execution: only materialize the delegate argument when a compiled
  delegate exists, avoiding an IndexOutOfRange for parent rules that have
  only child rules and are executed with no parameters.
* Docs: clarify that AssemblyReferenceProvider is reference scoping, not a
  security sandbox — expression strings are compiled as trusted, full-trust
  code and must not come from untrusted input.

Extensions
----------
* RoslynRules.Xml: explicitly set DtdProcessing.Prohibit and XmlResolver=null
  on the schema-validation reader (XXE defense-in-depth; matches the safe
  framework defaults but states the intent).
* RoslynRules.Json: suppress IL2026 with justification for the intentional
  reflection-based convenience API, consistent with how core suppresses
  IL2026 and avoiding an [RequiresUnreferencedCode] cascade through the
  ISnapshotSerializer interface. The *Aot methods and source-gen context
  remain the trim/AOT-safe path.

Benchmarks
----------
* CacheMemoryBenchmark: measure below the new 10,000-entry cache cap so the
  bytes/entry figure is accurate.

Demo
----
* DemoRunner.LoadCustomers: resolve Data/Customers.json against
  AppContext.BaseDirectory instead of the current working directory, so the
  demo runs from any location (e.g. `dotnet run` from the repo root).

Tests
-----
* Update two CacheKeyBuilder tests that asserted the old identity-based
  behavior to assert value-based keys.
* Add coverage: mutated reference parameter invalidates the cache,
  FirstFailure is null when all children pass, ExecuteParallel tolerates
  dangling dependencies, ExecuteBufferedAsync never runs a dependent in the
  same chunk as its dependency (and rejects bufferSize < 1), and a new
  RuleCacheEvictionTests suite for expiry and size-cap eviction.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@asulwer
asulwer merged commit 4e11f3a into master Jul 21, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant