From 219d6a6eeaa0fe1361861b9bff50ab5da9509f02 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Tue, 14 Jul 2026 11:08:14 +0300 Subject: [PATCH] Close the SettingsClassGenerator concurrency race (T7) GenerateType did a lock-free TryGetValue and, on a miss, called _moduleBuilder.DefineType(name) then wrote the cache. Two threads racing the same interface both missed the cache and both defined the same type name (the second throws "Duplicate type name" -> the resolve/scan aborts). And because System.Reflection.Emit is not thread-safe, two threads generating different interfaces also race the shared ModuleBuilder's metadata. Q4's ConcurrentDictionary made the cache thread-safe but never serialized the check-then-define. Reachable in production: GenericHost registers ISettingsProvider as a process-wide singleton over one SettingsBuilder, and cache-miss resolves fall through to lazy generation on the shared ModuleBuilder from concurrent DI resolutions. Fix: double-checked locking. The warm cache-hit path stays lock-free (TryGetValue before the lock); on a miss, take a single generation gate, re-check, run the whole emit sequence (extracted to DefineImplementationType) plus the cache write inside the lock, and still wrap failures as TypeGenerationException (uncached). One gate over all generation, matching the shared _moduleBuilder's scope - not a per-type Lazy, which would only serialize same-interface generation and leave the distinct-interface module race open. Tests (+2 in SettingsClassGeneratorTests.cs): a same-interface Parallel.For stress (asserts one shared impl) and a distinct+same Barrier/32-thread stress across 8 interfaces (asserts no failures and exactly one impl per interface; regression-guards the lock-all decision against a future switch to Lazy). Suite 84 net10 (was 82); ran 5x green. Reviewed: plan by dotnet-architect (ENDORSE-WITH-CHANGES - lock-all required, Lazy rejected) + perf/security in-context; code by /code-review plus Roslyn detect_antipatterns (0). Also refreshes FIX-PLAN.md (T7 marked done) and SESSION-HANDOFF.md. --- FIX-PLAN.md | 9 +- SESSION-HANDOFF.md | 48 ++--- .../Core/Reflection/SettingsClassGenerator.cs | 165 ++++++++++-------- .../SettingsClassGeneratorTests.cs | 79 +++++++++ 4 files changed, 205 insertions(+), 96 deletions(-) diff --git a/FIX-PLAN.md b/FIX-PLAN.md index d736bac..947c05b 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -10,9 +10,10 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - **P5 merged (#26)** — resolve config section once per type: `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). Plan reviewed by architect+perf+security (chose internal cache over a contract change — layering); code reviewed via `/code-review`. Gated `ConfigBinderBenchmark`: **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**. Suite **71/TFM**. Also gated P4's `ConvertArrayBenchmark` (was never in the CI filter). **Security review surfaced a pre-existing leak → new item S1.** - **Perf track P0–P5 = COMPLETE + merged.** `master` @ `498fc81`. - **S1 shipped + merged (#27).** Conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner; the value-free "required value missing" case split into its own `SettingsPropertyNullException`. `master` @ `5277c60`. Detail in §S1 below. -- **C2 shipped** (branch `refactor/c2-exception-hierarchy`, PR open) — **public exception hierarchy**: `SimpleSettingsException` base, reparent all 10, promote the 4 escapees to public, flatten 3 to root namespace, leak-safe structured properties, retype the `TypeIsNotInterface` throw. Plan reviewed by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly) + perf in-context; code by `/code-review`. Suite **82 net10** (was 76; +6). Detail in §C2 below. -- **In flight:** C2 PR open (branch `refactor/c2-exception-hierarchy`) — carries this fix-plan + handoff refresh. -- **Next:** engine tests (T4 `ValuesPopulator` / T5 `TypeConverter` / T7 generator concurrency race) **or** continue the pre-stable breaking cleanups (A5 make `SettingsHolder` internal / C1 `List` support / A6 command-line quoting / A3 `Core.AspNet` / A4 dependency floor) · **A1 (HIGH)** AOT/trim story · optional P3b. +- **C2 merged (#28).** **Public exception hierarchy**: `SimpleSettingsException` base, reparent all 10, promote the 4 escapees to public, flatten 3 to root namespace, leak-safe structured properties, retype the `TypeIsNotInterface` throw. Plan reviewed by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly) + perf in-context; code by `/code-review`. Suite **82 net10** (was 76; +6). `master` @ `13b78dd`. Detail in §C2 below. +- **T7 (generator concurrency race) shipped** (branch `test/t7-generator-concurrency`, PR open) — `SettingsClassGenerator.GenerateType` now uses double-checked locking + a single generation gate (Reflection.Emit isn't thread-safe: concurrent `DefineType` races the shared `ModuleBuilder` for both same- and distinct-interface generation). Fast cache-hit path stays lock-free. Plan reviewed by `dotnet-architect` (ENDORSE-WITH-CHANGES — lock-all required, `Lazy`-per-type rejected) + perf/security in-context; code by `/code-review`. +2 concurrency stress tests; suite **84 net10** (was 82; 5× green). +- **In flight:** T7 PR open (branch `test/t7-generator-concurrency`) — carries this fix-plan + handoff refresh. +- **Next:** engine tests T4 `ValuesPopulator` / T5 `TypeConverter` (+ optional T7 leftovers: collection not-found / binder edge cases) **or** continue the pre-stable breaking cleanups (A5 make `SettingsHolder` internal / C1 `List` support / A6 command-line quoting / A3 `Core.AspNet` / A4 dependency floor) · **A1 (HIGH)** AOT/trim story · optional P3b. - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17. - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator. - Running status lives in `SESSION-HANDOFF.md`. @@ -55,7 +56,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - [ ] T4 · `ValuesPopulator` unit tests (precedence + exception wrappers) - [ ] T5 · `TypeConverter` unit tests (null / nullable / empty-enumerable / attribute) - [ ] T6 · Converter unit tests (array / enumerable / Uri / DateTime + `List` doc test) -- [ ] T7 · `SettingsClassGenerator` caching + concurrency stress; collection not-found; binder edge cases · *(caching now covered; concurrency race still open — see P/Q4 note)* +- [x] T7 · `SettingsClassGenerator` **concurrency race closed** — double-checked locking + single generation gate (Reflection.Emit isn't thread-safe); + same-type & distinct-type concurrent stress tests · *(caching was already covered; collection not-found / binder edge-case tests remain as a minor optional follow-up)* **Phase 5 — Performance** - [x] P0 · Upgrade the benchmark harness (MemoryDiagnoser + phase-split + fixtures) — do first, to measure P1–P3 diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 41328d7..e460ec0 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -1,47 +1,49 @@ # SESSION HANDOFF — SimpleSettings -_Last updated: 2026-07-13 · owner: Guy Ludvig (guy@frontegg.com)_ +_Last updated: 2026-07-14 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). Perf track **P0–P5 merged**; **S1 (secret redaction) merged (#27)**; **C2 (public exception hierarchy) is now done and in an open PR** on branch **`refactor/c2-exception-hierarchy`** (run `gh pr list` for the number). `master` @ `5277c60`. Suite **82 tests net10** (CI runs net8 + net10). +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). Perf track **P0–P5 merged**; **S1 (#27) + C2 (#28) merged**; **T7 (generator concurrency race) is now done and in an open PR** on branch **`test/t7-generator-concurrency`** (run `gh pr list` for the number). `master` @ `13b78dd`. Suite **84 tests net10** (CI runs net8 + net10). -**C2 — what it did:** added `public abstract class SimpleSettingsException : Exception` and reparented all 10 library exceptions (so consumers can `catch (SimpleSettingsException)`); promoted the 4 build-path escapees to public; flattened 3 mis-namespaced types to the root namespace; exposed leak-safe structured properties; and replaced the untyped `InvalidOperationException(TypeIsNotInterface)` with a typed `SettingsTypeNotInterfaceException` (the one real behavior break). **S1's redaction is now structural** — `SettingsPropertyValueException` takes the failure's `Type`, not the `Exception`, so a value-bearing object can't cross its boundary. +T7 closed a real concurrency bug: `SettingsClassGenerator.GenerateType` had an unsynchronized check-then-`DefineType`, so concurrent first-resolves of a settings interface raced the shared `ModuleBuilder` (`System.Reflection.Emit` isn't thread-safe — for both same- and distinct-interface generation). Fixed with double-checked locking + a single generation gate; the warm cache-hit path stays lock-free. -Still **pre-stable** (no `v*` tag; only auto-alphas), so breaking changes remain free — keep batching the breaking cleanups (A5 / C1 / A6 / A3 / A4) before the first `v2.0.0-beta`. +Still **pre-stable** (no `v*` tag; only auto-alphas), so breaking changes remain free — a good window to finish the breaking cleanups (A5 / C1 / A6 / A3 / A4) before the first `v2.0.0-beta`. ## Do this first (new session) -1. **Verify git state** (`git log`, `gh pr list`) — expect `master` @ `5277c60` (S1 #27 merged), branch **`refactor/c2-exception-hierarchy`** pushed with the C2 commit, and **its PR open**. If C2 already merged, `master` advanced — reconcile. -2. **If the C2 PR is open:** check CI (build+test net8/net10 + the benchmark allocation gate — C2 is off the hot path, so nothing should move). Merge when green (squash, via `guy-lud`). C2 carries this handoff + `FIX-PLAN.md` refresh in the same commit. -3. **Then pick next work** (ranked below). Per the workflow (project memory `[[dotnet-review-workflow]]`): plan → review the plan with `dotnet-architect`/`performance-analyst`/`security-auditor` → implement → review the diff with the **`/code-review` skill**. See the sub-agent gotcha — kit plan-agents are **intermittently** flaky (misfired on S1, fired cleanly on C2); keep the in-context fallback ready. +1. **Verify git state** (`git log`, `gh pr list`) — expect `master` @ `13b78dd`, branch **`test/t7-generator-concurrency`** pushed with the T7 commit, and **its PR open**. If T7 already merged, `master` advanced — reconcile. +2. **If the T7 PR is open:** check CI (build+test net8/10 + the benchmark allocation gate — T7 is off the hot path, so nothing should move). Merge when green (squash, via `guy-lud`). T7 carries this handoff + `FIX-PLAN.md` refresh in the same commit. +3. **Then pick the next item** (ranked below) and follow the workflow (project memory `[[dotnet-review-workflow]]`): plan → review the plan with `dotnet-architect`/`performance-analyst`/`security-auditor` → implement → review the diff with the **`/code-review` skill**. ⚠️ Kit plan-agents are **intermittently** flaky (see the sub-agent gotcha) — verify each returns real tool calls; fall back in-context if one misfires. ## Current state -- On branch **`refactor/c2-exception-hierarchy`** with the C2 commit (code + tests + this docs refresh). **PR open** (see `gh pr list`). `master` @ `5277c60`. -- **Perf track P0–P5, S1, C2 all complete.** Build clean (0 warnings, both TFMs). Suite **82 net10**. -- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the allocation baseline lives there. The remote also still has merged `perf/*`/`security/s1-*` branches (optional cleanup) plus legacy/held branches. Deleting remote branches needs the `guy-lud` push identity. +- On branch **`test/t7-generator-concurrency`** with the T7 commit (code + tests + this docs refresh). **PR open** (see `gh pr list`). `master` @ `13b78dd`. +- **Perf track P0–P5, S1, C2 complete + merged; T7 in an open PR.** Build clean (0 warnings, both TFMs). Suite **84 net10** (the 2 new concurrency stress tests ran 5× green — deterministic). +- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the allocation baseline lives there. The remote also still has merged `perf/*`, `security/s1-*`, `refactor/c2-*` branches (optional cleanup) plus legacy/held branches. Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) -- **C2 — public exception hierarchy (branch `refactor/c2-exception-hierarchy`, PR open).** New `public abstract class SimpleSettingsException : Exception` (protected `(message)` + `(message, inner)` ctors; no parameterless/`[Serializable]` ctor). Reparented all 10 exceptions; promoted the 4 escapees (`SettingsPropertyValueException`, `SettingsPropertyNullException`, `TypeGenerationException`, `SettingsPropertyExtractionException`) to **public**; **flattened 3 to root ns** (`SettingsExtractionException` was `.Core`; `TypeGenerationException` + `SettingsPropertyExtractionException` were `.Core.Reflection`). Added leak-safe structured props (`SettingsBindingException.{BinderType,Section,Key}`; `SettingsPropertyValueException.{SettingsType,PropertyName,TargetType,ConversionErrorType}`; `SettingsType`/`OptionType`/`ArgumentName` on the rest). New `SettingsTypeNotInterfaceException` replaces the 3 `InvalidOperationException(TypeIsNotInterface)` throws (the one behavior break → release notes). **Left** the 2 unreachable "No converter found" `InvalidOperationException`s as invariant guards. **S1 made structural:** `SettingsPropertyValueException` ctor takes the failure `Type` (not the `Exception`); `SettingsBindingException` stores primitives, does not retain `BindingContext` (which holds the bound value). **Reviewed:** plan by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly this time; perf in-context) → adopted: pass `Type` not `Exception`, flatten namespaces, `Section`/`ConversionErrorType` naming, reflection-based "is public" tests (IVT masks accessibility from a plain reference). Code by `/code-review` (high) + Roslyn `detect_antipatterns` (0). **Tests +6** (`SimpleSettings/ExceptionHierarchyTests.cs`): base public+abstract; reflection invariant that every library exception derives from the base; the 4 promotions are public; not-interface→typed+catchable; conversion→structured metadata + `InnerException == null`; binder-throws→context. -- **S1 — redact secret values from conversion-failure exceptions (merged #27).** Two leak vectors closed: our message no longer interpolates the value, and the value-bearing framework inner is no longer chained. New value-free `SettingsPropertyNullException` for the "AllowEmpty=false, no value" path. `ISectionBinder` doc note (custom binders mustn't throw value-bearing messages). +5 redaction tests. (C2 later made the no-chain guarantee structural.) -- **P5 — resolve config section once per type (merged, #26).** `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** The P5 security review surfaced S1. -- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** Shared `CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk). **1.33 KB→688 B (−49%), 5.7×.** -- **P3 — cached "settings plan" (#24).** Per-type `SettingsPlan`; warm re-populate **−55–61%**; gated `ScanBenchmark` ≈flat. Compiled setter reverted (net10 `SetValue` is alloc-free). Follow-up P3b (only if set *time* matters). +- **T7 — close the SettingsClassGenerator concurrency race (branch `test/t7-generator-concurrency`, PR open).** `GenerateType` did a lock-free `TryGetValue` then, on a miss, `_moduleBuilder.DefineType(name)` + cache write — so two threads racing the same interface both `DefineType` the same name (second throws "Duplicate type name" → resolve/scan aborts), and (Reflection.Emit being not thread-safe) two threads generating *different* interfaces corrupt the shared module. **Fix:** double-checked locking — lock-free `TryGetValue` fast path; on a miss take a single `_generationGate`, re-check, run the whole emit sequence (`DefineImplementationType`) + cache write inside the lock; failures still wrap as `TypeGenerationException` (uncached). One gate over ALL generation (scope matches the shared `_moduleBuilder`); the warm path stays lock-free. **Reachable in production:** GenericHost registers `ISettingsProvider` as a process-wide singleton over one `SettingsBuilder`, and cache-miss resolves fall through to lazy generation on the shared `ModuleBuilder` from concurrent DI resolutions. **Plan reviewed** by `dotnet-architect` (ENDORSE-WITH-CHANGES: lock-all is *required* not just safer; `Lazy`-per-type rejected because it only serializes same-key and leaves the distinct-type module race open) + perf/security in-context; **code by `/code-review`** (high) + Roslyn `detect_antipatterns` (0). **+2 tests** in `SettingsClassGeneratorTests.cs`: same-interface `Parallel.For` (single shared impl) and a distinct+same `Barrier`/32-thread stress across 8 interfaces (regression-guards the lock-all decision). Suite **84 net10** (was 82); ran 5× green. +- **C2 — public exception hierarchy (merged, #28).** `public abstract SimpleSettingsException` base; reparent all 10; promote the 4 build-path escapees to public; flatten 3 to root ns; leak-safe structured properties; new `SettingsTypeNotInterfaceException` replaces the untyped `InvalidOperationException(TypeIsNotInterface)`. **S1 made structural** (value exception takes the failure `Type`, not the `Exception`; `SettingsBindingException` stores primitives, not the `BindingContext`). +6 tests incl. a reflection invariant that every library exception derives from the base. +- **S1 — redact secret values from conversion-failure exceptions (merged, #27).** Our message no longer interpolates the value and the value-bearing framework inner is no longer chained; value-free `SettingsPropertyNullException` for the "AllowEmpty=false, no value" path. +5 redaction tests. +- **P5 — resolve config section once per type (merged, #26).** `ConfigurationBinder` caches the `IConfigurationSection` per section name. **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** Its security review surfaced S1. +- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** Shared `CollectionTypeConverter`. **1.33 KB→688 B (−49%), 5.7×.** +- **P3 — cached "settings plan" (#24).** Warm re-populate **−55–61%**; gated `ScanBenchmark` ≈flat. Compiled setter reverted. Follow-up P3b (only if set *time* matters). - **#23** wrap docs · **#22** benchmark-tracking CI (gates PRs on allocation regressions via `gh-pages`) · **#21** perf quick wins Q1–Q4 + M1 + micro-benchmarks · **#20** docs tutorials · **#18** P2 · **#17** P1+C3 · **#16** P0 harness · earlier #8/#10–#15. ## Key decisions & context (carry forward) - **Benchmark tracking gates on ALLOCATIONS, not time.** `gh-pages` (`dev/bench/`) holds the baseline. -- **Exception-redaction invariant (S1+C2).** `SettingsPropertyValueException` **never carries the bound value and never chains an inner** — its ctor takes the failure `Type`, not the `Exception`, so this is now structural. `SettingsBindingException` stores primitives (`BinderType`/`Section`/`Key`), never the `BindingContext` (holds `CurrentValue`). `SettingsPropertyNullException` is the distinct value-free "required value missing" case. Don't add a `Value` property, re-chain an inner, or retain a value-bearing object — that reopens the leak. -- **Exception hierarchy (C2 done).** All library exceptions derive from `public abstract SimpleSettingsException` and live in the **root** namespace. A reflection invariant test enforces the "all derive from base" rule — if you add a new exception, derive it from `SimpleSettingsException` or that test fails. The two "No converter found" `InvalidOperationException`s are deliberate unreachable guards (not part of the family). -- **M1 / generated names.** The generated impl type name (in `SettingsClassGenerator`) is namespace-qualified and must stay **separate** from `GetNormalizeInterfaceName` (drives the default config section name). Don't merge. +- **Generator concurrency (T7 done).** `SettingsClassGenerator.GenerateType` serializes **all** generation behind one gate (double-checked locking; warm cache-hit path lock-free). Do NOT "optimize" to `Lazy`-per-type: `System.Reflection.Emit` isn't thread-safe, so concurrent `DefineType` of *distinct* interfaces also races the shared `ModuleBuilder`; per-type Lazy would reopen that. A distinct-types stress test guards this. +- **Exception-redaction invariant (S1+C2).** `SettingsPropertyValueException` **never carries the bound value and never chains an inner** — its ctor takes the failure `Type`, not the `Exception`, so this is structural. `SettingsBindingException` stores primitives (`BinderType`/`Section`/`Key`), never the `BindingContext` (holds `CurrentValue`). `SettingsPropertyNullException` is the distinct value-free "required value missing" case. Don't add a `Value` property, re-chain an inner, or retain a value-bearing object. +- **Exception hierarchy (C2 done).** All library exceptions derive from `public abstract SimpleSettingsException` and live in the **root** namespace. A reflection invariant test enforces "all derive from base". The two "No converter found" `InvalidOperationException`s are deliberate unreachable guards (not part of the family). +- **M1 / generated names.** The generated impl type name (`SettingsClassGenerator`) is namespace-qualified and must stay **separate** from `GetNormalizeInterfaceName` (drives the default config section name). Don't merge. - **C3 resolved — option 2 (provider-level cache).** Reload/`IOptionsMonitor` is the future "option 3". - **Validations (D1) — HELD, do NOT delete.** Public `Validations/*` + `SettingsPropertyAttribute.ValidatorType` are dead but intended for a feature; reconcile with the `validate-settings` branch. - **`EqualityCompererCreator` (D2) — HELD** (internal, dead, latent invalid-IL bug at `EqualityCompererCreator.cs:38`). - **Pre-stable window:** no `v*` stable tag. Breaking changes free until the first `v2.0.0-beta`. ## Next priorities (ranked — detail in FIX-PLAN.md) -1. **Merge the C2 PR** once CI is green (see Do this first). -2. **Engine tests:** T4 `ValuesPopulator` (precedence + exception wrappers), T5 `TypeConverter` (null/nullable/empty-enumerable/attribute), T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). T6 converters largely done across P4+P5. -3. **Breaking cleanups (batch while pre-stable):** A5 (make `SettingsHolder`/`ISettingsHolder` internal — never in a public signature), C1 (`List`/`IList`/`ICollection` support or a documented limit), A6 (command-line quoted-arg parsing + skip exe path), A3 (`Core.AspNet` — make `Environments` public or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM so net8 consumers aren't pulled to 10.x). -4. **A1 (HIGH):** AOT/trim annotations for the `Reflection.Emit` engine (or plan a source generator); at minimum document the limitation before stable. C2's get-only `Type` props added no trim surface, so no rework needed there. +1. **Merge the T7 PR** once CI is green (see Do this first). +2. **Remaining engine tests:** T4 `ValuesPopulator` (binder precedence + exception wrappers), T5 `TypeConverter` (null/nullable/empty-enumerable/attribute). T6 converters largely done across P4+P5; T7 concurrency now closed (collection-not-found / binder edge-case tests remain as a minor optional leftover). +3. **Breaking cleanups (batch while pre-stable):** A5 (make `SettingsHolder`/`ISettingsHolder` internal), C1 (`List`/`IList`/`ICollection` support or a documented limit), A6 (command-line quoted-arg parsing + skip exe path), A3 (`Core.AspNet` — make `Environments` public or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM). +4. **A1 (HIGH):** AOT/trim annotations for the `Reflection.Emit` engine (or plan a source generator); at minimum document the limitation before stable. 5. **README** — may still have stale `existall/SimpleConfig` links. Optional **P3b** compiled setter (only if a profile shows set *time* matters). 6. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. @@ -57,5 +59,5 @@ Still **pre-stable** (no `v*` tag; only auto-alphas), so breaking changes remain - **Benchmarks:** run from `src/` — `dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- --filter --job short`. Output dir (`BenchmarkDotNet.Artifacts/`) is gitignored. - **`FIX-PLAN.md`** (repo root) is the full, prioritized plan with per-item file:line detail — open it explicitly; it is not auto-injected. - **Wrap ritual — handoff branch rule:** refresh this file so it rides the session's real PR (the current work branch). If there is **no** open work branch at wrap (everything merged), **leave the refresh uncommitted** so the *next* session's first branch carries it. Do **not** commit docs to `master`, and do **not** create a dedicated docs branch/PR: `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only push burns a throwaway `-alpha`. -- **Sub-agent flakiness (dotnet-claude-kit) — INTERMITTENT.** Kit agents sometimes misfire — returning a leaked skill/role preamble with **0 tool calls** instead of working. It's inconsistent: on S1 the `dotnet-architect` + `performance-analyst` misfired (only `security-auditor` worked); on C2 **both `security-auditor` and `dotnet-architect` fired cleanly**. So: spawn them, but **check each result has real tool calls / substance** — if one misfired, retry once or do that lens **in-context** (the reliable fallback). Always use the **`/code-review` skill** (not the `code-reviewer` agent) for the code-review step. Kit is at latest (0.10.0). For perf, `dotnet-diag:analyzing-dotnet-performance` (Microsoft-maintained) is a fallback. +- **Sub-agent flakiness (dotnet-claude-kit) — INTERMITTENT.** Kit agents sometimes misfire — returning a leaked skill/role preamble with **0 tool calls** instead of working. It's inconsistent: on S1 `dotnet-architect` + `performance-analyst` misfired (only `security-auditor` worked); on C2 both fired cleanly; on T7 `dotnet-architect` fired cleanly. So: spawn them, but **check each result has real tool calls / substance** — if one misfired, retry once or do that lens **in-context** (the reliable fallback). Always use the **`/code-review` skill** (not the `code-reviewer` agent) for the code-review step. Kit is at latest (0.10.0). For perf, `dotnet-diag:analyzing-dotnet-performance` (Microsoft-maintained) is a fallback. - Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs index e96b9e0..f9fd29a 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs @@ -1,69 +1,96 @@ -using System; -using System.Collections.Concurrent; -using System.Linq; -using System.Reflection; -using System.Reflection.Emit; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal class SettingsClassGenerator : ISettingsClassGenerator - { - private readonly ITypePropertiesExtractor _typePropertiesExtractor; - private readonly IPropertyCreator _propertyCreator; - private readonly ModuleBuilder _moduleBuilder = null!; - - // A settings interface generates exactly one impl type for the module's lifetime, so cache by the - // interface Type instead of re-querying the module by mangled type name on every call. - private readonly ConcurrentDictionary _generatedTypes = new(); - - internal SettingsClassGenerator(ITypePropertiesExtractor typePropertiesExtractor, - IPropertyCreator propertyCreator) - { - _typePropertiesExtractor = typePropertiesExtractor; - _propertyCreator = propertyCreator; - } - - public SettingsClassGenerator() - : this(new TypePropertiesExtractor(), new PropertyCreator()) - { - var assemblyName = new AssemblyName(Guid.NewGuid().ToString()); - var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.RunAndCollect); - - _moduleBuilder = assemblyBuilder.DefineDynamicModule("ConfingModule"); - } - - public Type GenerateType(Type interfaceType) - { - if (_generatedTypes.TryGetValue(interfaceType, out var existingType)) - return existingType; - - try - { - // Namespace-qualified so two settings interfaces that share a simple name - // (e.g. Foo.ISettings + Bar.ISettings) don't collide on the generated type name and - // abort the scan. Deliberately NOT GetNormalizeInterfaceName() — that helper also backs - // the default config section name (SettingsOptions.SectionNameFormatter), which must stay - // simple-name-based; the generated impl name is an internal detail and can differ. - var name = $"{(interfaceType.FullName ?? interfaceType.Name).Replace('.', '_').Replace('+', '_')}Impl"; - - var properties = _typePropertiesExtractor.ExtractTypeProperties(interfaceType); - - var typeBuilder = _moduleBuilder.DefineType(name, TypeAttributes.Class | TypeAttributes.Public); - - typeBuilder.AddInterfaceImplementation(interfaceType); - - _propertyCreator.CreateAnonymousProperties(typeBuilder, properties.ToArray(), out _); - - var result = typeBuilder.CreateTypeInfo().AsType(); - - _generatedTypes[interfaceType] = result; - - return result; - } - catch (Exception e) - { - throw new TypeGenerationException(interfaceType, e); - } - } - } -} \ No newline at end of file +using System; +using System.Collections.Concurrent; +using System.Linq; +using System.Reflection; +using System.Reflection.Emit; + +namespace ExistForAll.SimpleSettings.Core.Reflection +{ + internal class SettingsClassGenerator : ISettingsClassGenerator + { + private readonly ITypePropertiesExtractor _typePropertiesExtractor; + private readonly IPropertyCreator _propertyCreator; + private readonly ModuleBuilder _moduleBuilder = null!; + + // A settings interface generates exactly one impl type for the module's lifetime, so cache by the + // interface Type instead of re-querying the module by mangled type name on every call. + private readonly ConcurrentDictionary _generatedTypes = new(); + + // Serializes ALL generation on this instance. System.Reflection.Emit is not thread-safe: DefineType and + // the rest of the emit sequence mutate module-scoped state (the type-name table, the metadata/token + // allocator) on the shared _moduleBuilder with no internal synchronization. So two threads generating + // the SAME interface would both DefineType its name (the second throws "Duplicate type name" -> the + // scan aborts), and two threads generating DIFFERENT interfaces would corrupt the shared module. One + // gate whose scope matches _moduleBuilder's scope covers both. The warm cache-hit path stays lock-free + // (the TryGetValue before the lock). This closes the T7 race that Q4's ConcurrentDictionary left open: + // it made the cache thread-safe, but not the check-then-DefineType. + private readonly object _generationGate = new(); + + internal SettingsClassGenerator(ITypePropertiesExtractor typePropertiesExtractor, + IPropertyCreator propertyCreator) + { + _typePropertiesExtractor = typePropertiesExtractor; + _propertyCreator = propertyCreator; + } + + public SettingsClassGenerator() + : this(new TypePropertiesExtractor(), new PropertyCreator()) + { + var assemblyName = new AssemblyName(Guid.NewGuid().ToString()); + var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.RunAndCollect); + + _moduleBuilder = assemblyBuilder.DefineDynamicModule("ConfingModule"); + } + + public Type GenerateType(Type interfaceType) + { + // Warm path: already generated. Lock-free — the cache is a ConcurrentDictionary, and the value it + // holds is a fully-baked Type published (inside the lock) only after CreateTypeInfo() completed, so + // there is no torn/partial read. + if (_generatedTypes.TryGetValue(interfaceType, out var existingType)) + return existingType; + + lock (_generationGate) + { + // Another thread may have generated it while we waited for the lock. + if (_generatedTypes.TryGetValue(interfaceType, out existingType)) + return existingType; + + try + { + var result = DefineImplementationType(interfaceType); + _generatedTypes[interfaceType] = result; + return result; + } + catch (Exception e) + { + // A failure before DefineType (e.g. property extraction) is retryable. A failure after it + // leaves a half-defined TypeBuilder under this name, so a retry would hit "Duplicate type + // name" — pre-existing behavior, only reachable for a malformed interface that would fail + // deterministically anyway. + throw new TypeGenerationException(interfaceType, e); + } + } + } + + private Type DefineImplementationType(Type interfaceType) + { + // Namespace-qualified so two settings interfaces that share a simple name + // (e.g. Foo.ISettings + Bar.ISettings) don't collide on the generated type name and + // abort the scan. Deliberately NOT GetNormalizeInterfaceName() — that helper also backs + // the default config section name (SettingsOptions.SectionNameFormatter), which must stay + // simple-name-based; the generated impl name is an internal detail and can differ. + var name = $"{(interfaceType.FullName ?? interfaceType.Name).Replace('.', '_').Replace('+', '_')}Impl"; + + var properties = _typePropertiesExtractor.ExtractTypeProperties(interfaceType); + + var typeBuilder = _moduleBuilder.DefineType(name, TypeAttributes.Class | TypeAttributes.Public); + + typeBuilder.AddInterfaceImplementation(interfaceType); + + _propertyCreator.CreateAnonymousProperties(typeBuilder, properties.ToArray(), out _); + + return typeBuilder.CreateTypeInfo().AsType(); + } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs index 76dce07..35524ff 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs @@ -1,5 +1,9 @@ using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; using System.Reflection; +using System.Threading; using ExistForAll.SimpleSettings.Core.Reflection; namespace ExistForAll.SimpleSettings.UnitTests.SimpleSettings @@ -95,6 +99,71 @@ public async Task GenerateType_WhenDerivedHidesABasePropertyName_DeduplicatesByN await Assert.That(result.GetProperty(nameof(IHidingChild.Extra))).IsNotNull(); await Assert.That(typeof(IHidingChild).IsInstanceOfType(Activator.CreateInstance(result)!)).IsTrue(); } + + [Test] + public async Task GenerateType_ConcurrentSameInterface_ReturnsSingleSharedType() + { + // The reported T7 race: concurrent first-generation of the SAME interface used to let two threads + // both DefineType the same name (the second throws -> the resolve/scan aborts). Post-fix, every + // caller gets the one shared impl. + var generator = new SettingsClassGenerator(); + var results = new ConcurrentBag(); + + Parallel.For(0, 128, _ => results.Add(generator.GenerateType(typeof(IStressA)))); + + await Assert.That(results.Count).IsEqualTo(128); + await Assert.That(results.Distinct().Count()).IsEqualTo(1); + } + + [Test] + public async Task GenerateType_ConcurrentAcrossSameAndDistinctInterfaces_IsRaceFree() + { + // Guards the design decision (one lock over ALL generation): Reflection.Emit is not thread-safe, so + // concurrent DefineType of DISTINCT interfaces also races the single shared ModuleBuilder — a + // per-type Lazy would not catch that. Fixed threads + a Barrier align the DefineType calls for + // maximum contention (Parallel.For can't guarantee N concurrent workers, which would deadlock a + // Barrier(N)). Each thread hits one of 8 interfaces, so this exercises same-interface AND + // distinct-interface contention at once. + var generator = new SettingsClassGenerator(); + var interfaces = new[] + { + typeof(IStressA), typeof(IStressB), typeof(IStressC), typeof(IStressD), + typeof(IStressE), typeof(IStressF), typeof(IStressG), typeof(IStressH), + }; + + const int threadCount = 32; + var barrier = new Barrier(threadCount); + var perInterface = new ConcurrentDictionary>(); + var failures = new ConcurrentBag(); + var workers = new List(); + + for (var i = 0; i < threadCount; i++) + { + var iface = interfaces[i % interfaces.Length]; + var worker = new Thread(() => + { + try + { + barrier.SignalAndWait(); + perInterface.GetOrAdd(iface, _ => new ConcurrentBag()).Add(generator.GenerateType(iface)); + } + catch (Exception e) + { + failures.Add(e); + } + }); + workers.Add(worker); + worker.Start(); + } + + foreach (var worker in workers) + worker.Join(); + + await Assert.That(failures.Count).IsEqualTo(0); + await Assert.That(perInterface.Count).IsEqualTo(interfaces.Length); + foreach (var iface in interfaces) + await Assert.That(perInterface[iface].Distinct().Count()).IsEqualTo(1); + } } public interface IHidingChild : IRoot @@ -102,6 +171,16 @@ public interface IHidingChild : IRoot new string Value { get; set; } int Extra { get; set; } } + + // Distinct marker interfaces for the concurrency stress tests (GenerateType_Concurrent* above). + public interface IStressA { int Value { get; set; } } + public interface IStressB { int Value { get; set; } } + public interface IStressC { int Value { get; set; } } + public interface IStressD { int Value { get; set; } } + public interface IStressE { int Value { get; set; } } + public interface IStressF { int Value { get; set; } } + public interface IStressG { int Value { get; set; } } + public interface IStressH { int Value { get; set; } } } namespace ExistForAll.SimpleSettings.UnitTests.SimpleSettings.DupA