From 840bec7414258cbdf51a5a0140266cb8f4d08633 Mon Sep 17 00:00:00 2001 From: Sebastian Stehle Date: Mon, 24 Aug 2026 14:46:30 +0200 Subject: [PATCH] More fixes --- .../Entities/Contents/CollectionProvider.cs | 34 ++++- .../AppProvider.cs | 2 +- .../Queries/Steps/ResolveReferences.cs | 6 +- .../Squidex.Domain.Apps.Entities/Context.cs | 2 +- resolved.md | 125 +++++++++++++++++- todo.md | 74 +---------- 6 files changed, 161 insertions(+), 82 deletions(-) diff --git a/backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs b/backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs index 673a23750..02925d9b3 100644 --- a/backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs +++ b/backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs @@ -13,15 +13,39 @@ namespace Squidex.Domain.Apps.Entities.Contents; internal class CollectionProvider(IMongoClient mongoClient, string prefixDatabase, string prefixCollection) { - private readonly ConcurrentDictionary<(DomainId, DomainId), Task>> collections = - new ConcurrentDictionary<(DomainId, DomainId), Task>>(); + private readonly ConcurrentDictionary<(DomainId AppId, DomainId SchemaId), Lazy>>> collections = + new ConcurrentDictionary<(DomainId AppId, DomainId SchemaId), Lazy>>>(); - public Task> GetCollectionAsync(DomainId appId, DomainId schemaId) + public async Task> GetCollectionAsync(DomainId appId, DomainId schemaId) { - return collections.GetOrAdd((appId, schemaId), CreateCollectionAsync); + var key = (appId, schemaId); + + // The lazy ensures that the indexes are only created once, even when the same collection is + // requested concurrently. GetOrAdd alone can run the factory several times for one key. + var collection = collections.GetOrAdd(key, CreateLazyCollection); + + try + { + return await collection.Value; + } + catch + { + // A failed attempt must not stay in the cache. Creating the indexes can fail for a + // transient reason and the collection would be unusable until the process is restarted. + // Only remove our own entry, so that a newer successful one is not thrown away. + collections.TryRemove(new KeyValuePair<(DomainId AppId, DomainId SchemaId), Lazy>>>(key, collection)); + throw; + } + } + + private Lazy>> CreateLazyCollection((DomainId AppId, DomainId SchemaId) key) + { + return new Lazy>>( + () => CreateCollectionAsync(key), + LazyThreadSafetyMode.ExecutionAndPublication); } - private async Task> CreateCollectionAsync((DomainId, DomainId) key) + private async Task> CreateCollectionAsync((DomainId AppId, DomainId SchemaId) key) { var (appId, schemaId) = key; diff --git a/backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs b/backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs index 0d21e61ff..dec84f969 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs @@ -242,7 +242,7 @@ public sealed class AppProvider( return await result; } - + private static object AppCacheKey(DomainId appId) { return (nameof(AppProvider), "APPS_ID", appId); diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs index 00e3d46f9..b48f44336 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs @@ -136,9 +136,13 @@ public sealed class ResolveReferences(Lazy contentQuery, I private static void AddReferenceIds(HashSet ids, Schema schema, ResolvedComponents components, IEnumerable contents) { + // ResolvingReferences is a lazy query over all fields of the schema, therefore it is only + // evaluated once here and not again for every content. + var fields = schema.ResolvingReferences().ToList(); + foreach (var content in contents) { - content.Data.AddReferencedIds(schema.ResolvingReferences(), ids, components); + content.Data.AddReferencedIds(fields, ids, components); } } diff --git a/backend/src/Squidex.Domain.Apps.Entities/Context.cs b/backend/src/Squidex.Domain.Apps.Entities/Context.cs index be1dfe389..11260798b 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Context.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Context.cs @@ -29,7 +29,7 @@ public sealed class Context public App App { get; set; } - public bool IsFrontendClient { get; }; + public bool IsFrontendClient { get; } public Context(ClaimsPrincipal user, App app) : this(app, user, user.Claims.Permissions(), EmptyHeaders) diff --git a/resolved.md b/resolved.md index eb21ca39d..73ed64c26 100644 --- a/resolved.md +++ b/resolved.md @@ -3,8 +3,8 @@ Items from the backend performance review that are done. Numbering matches [todo.md](todo.md) — resolved items keep their original number so references stay valid. -Most entries are fixes. Item **6** is closed as *accepted, won't fix* — kept here so it is -not re-reported as a new finding. +Most entries are fixes. Items **6** and **14** are closed as *accepted* rather than fixed — +kept here so they are not re-reported as new findings. --- @@ -403,3 +403,124 @@ Notes: **Verified:** build clean (0 warnings). `Squidex.Domain.Apps.Core.Tests` (1243), `Squidex.Domain.Apps.Entities.Tests` (1528), `Squidex.Infrastructure.Tests` (1031) and `Squidex.Web.Tests` (167) all green. + +--- + +### 14. `AppProvider` copies cached schema/rule lists on every call — **CLOSED: ACCEPTED** +`backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs` + +`GetSchemasAsync` and `GetRulesAsync` end with `?.ToList() ?? []`, a defensive copy of the +cached list on every call including cache hits, and `GetRuleAsync` copies the whole rule +list just to `Find` one element. + +**Closed as accepted, not fixed.** The copy is a single shallow `List` allocation of +already-immutable elements; returning the cached instance directly would expose it to +mutation by callers, which is a worse trade than the allocation. Recorded here so it is +not re-reported as a new finding. + +--- + +### 15. Faulted tasks were cached permanently in `CollectionProvider` — **FIXED** +`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs` + +**Was:** + +```csharp +return collections.GetOrAdd((appId, schemaId), CreateCollectionAsync); +``` + +Two defects. `CreateCollectionAsync` creates indexes, so it can fail transiently — and +`GetOrAdd` stored the returned `Task` including a *faulted* one for the process lifetime, +so a single Mongo hiccup on first access permanently broke queries for that app/schema +until restart. Separately, `GetOrAdd` may invoke its factory concurrently for the same +key, issuing duplicate `CreateManyAsync` calls. + +**Now:** the dictionary holds `Lazy>` with `LazyThreadSafetyMode.ExecutionAndPublication`, +so the factory runs exactly once per key even under concurrent access, and the entry is +evicted when it fails: + +```csharp +var collection = collections.GetOrAdd(key, CreateLazyCollection); + +return AwaitCollectionAsync(key, collection); +... +try +{ + return await collection.Value; +} +catch +{ + collections.TryRemove(new KeyValuePair<...>(key, collection)); + throw; +} +``` + +The removal uses the `TryRemove(KeyValuePair)` overload, which only removes when the value +is still the *same* `Lazy` instance. The plain `TryRemove(key)` would race: a second thread +that had already retried and succeeded would have its good entry discarded by the first +thread's cleanup. + +A `using` alias for the key tuple was tried first, but StyleCop's SA1008 rejects the space +before the parenthesis in `using X = (A, B);`, so the tuple type is written out instead. + +**Verified:** build clean, `Squidex.Data.Tests` (180) and all other suites green. + +--- + +### 16. `IsFrontendClient` re-scanned claims on every access — **FIXED (verified)** +`backend/src/Squidex.Domain.Apps.Entities/Context.cs:32,51` +`backend/src/Squidex.Infrastructure/Security/Extensions.cs:70` + +**Was:** `public bool IsFrontendClient => UserPrincipal.IsInClient(DefaultClients.Frontend);` +— a computed property whose implementation was `principal.Claims.Any(x => ...)`, walking +every identity and every claim and allocating an enumerator plus a delegate per call. It is +read from several enrichment steps and from `ConvertData.GenerateConverter` per schema +group, so it ran many times per request against a value that cannot change. + +**Now:** a get-only auto-property assigned once in the private constructor, and +`IsInClient` rewritten from LINQ `Any` to a plain `foreach`, dropping the closure. + +**Verification found the commit did not compile.** Line 32 read +`public bool IsFrontendClient { get; };` — a stray semicolon, `error CS1597: Semicolon +after method or accessor block is not valid`. Removed the semicolon. + +Beyond compiling, the assignment is correct for every construction path: the public +`Context(ClaimsPrincipal, App)` chains to the private constructor via `: this(...)`, +`Anonymous` and `Admin` both go through that public one, and `HeaderBuilder.Build` calls +the private 4-argument constructor directly. All four paths therefore set the field. + +--- + +### 17. `ResolvingReferences()` re-evaluated per content — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs` + +**Was:** `SchemaExtensions.ResolvingReferences` is a lazy +`Fields.OfType<...>().Where(...)` that is never materialized, and `AddReferenceIds` called +it *inside* the per-content loop — so the full field scan plus two LINQ iterator +allocations happened once per content instead of once per schema. + +**Now:** hoisted out of the loop. + +```csharp +var fields = schema.ResolvingReferences().ToList(); + +foreach (var content in contents) +{ + content.Data.AddReferencedIds(fields, ids, components); +} +``` + +(The other call site, the outer `foreach` in `ResolveReferencesAsync`, enumerates the +sequence exactly once and was left alone.) + +**The double `GroupBy` was deliberately left alone.** `ResolveReferences.EnrichAsync` and +`ConvertData` each build `contents.GroupBy(x => x.SchemaId.Id)` twice. This does *not* +cause duplicate schema fetches: `ContentEnricher` passes a `ProvideSchema` delegate backed +by a per-call `schemaCache` dictionary, so the second grouping resolves every schema from +memory. The only real cost is re-materializing the LINQ `Lookup` — one extra pass over the +contents and one set of bucket allocations per step. + +Deduplicating it was tried and reverted: the gain is small enough that it does not justify +threading a materialized `List>` through the method signatures. + +**Verified:** build clean, all suites green. diff --git a/todo.md b/todo.md index 2474d32fe..f8e786f0e 100644 --- a/todo.md +++ b/todo.md @@ -9,9 +9,9 @@ overhead / allocation churn), **S4** low (worth fixing while nearby). Item numbers are stable and never reused. Completed items move to [resolved.md](resolved.md) keeping their number, so gaps in the sequence here are -expected — items **4**, **5**, **6**, **7**, **8**, **9**, **12**, **13** and **20** are closed and live there. +expected — items **4**, **5**, **6**, **7**, **8**, **9**, **12**, **13**, **14**, **15**, **16**, **17** and **20** are closed and live there. -**Status: 10 open of 20. Items 4, 5, 6, 7, 8, 9, 12, 13, 20 are in [resolved.md](resolved.md).** +**Status: 7 open of 20 — items 1, 2, 3, 10, 11, 18, 19. The other 13 are in [resolved.md](resolved.md).** --- @@ -104,76 +104,6 @@ request, uncached. ## S3 — Moderate -### 14. `AppProvider` copies cached schema/rule lists on every call -`backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs:197,208,216` - -`GetSchemasAsync` and `GetRulesAsync` end with `?.ToList() ?? []` — a defensive copy of -the cached list allocated per call, even on a cache hit. `GetRuleAsync` (line 216) -copies the entire rule list just to `Find` one element. - -These are called per request in the query pipeline and per event in `RuleEnqueuer`. - -**Fix:** return the cached `IReadOnlyList` directly (the cached instances are already -immutable) and have `GetRuleAsync` search without materialising. - ---- - -### 15. Faulted tasks are cached permanently in `CollectionProvider` -`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs:21` - -```csharp -return collections.GetOrAdd((appId, schemaId), CreateCollectionAsync); -``` - -`CreateCollectionAsync` creates indexes, so it can fail transiently. `GetOrAdd` stores -the returned `Task` — including a *faulted* one — for the process lifetime. One -transient Mongo hiccup during first access permanently breaks queries for that -app/schema until restart. - -`GetOrAdd` can also invoke the factory concurrently for the same key, issuing duplicate -`CreateManyAsync` calls. - -The same faulted-task-caching pattern exists in `AppProvider.GetOrCreate` -(`AppProvider.cs:213`), though the local cache is request-scoped so the window is small. - -**Fix:** evict the entry when the task faults; wrap in `Lazy>` with -`ExecutionAndPublication` to deduplicate. - ---- - -### 16. `IsFrontendClient` re-scans claims on every access -`backend/src/Squidex.Domain.Apps.Entities/Context.cs:32` - -```csharp -public bool IsFrontendClient => UserPrincipal.IsInClient(DefaultClients.Frontend); -``` - -`IsInClient` is `principal.Claims.Any(x => ...)` — `ClaimsPrincipal.Claims` walks every -identity and every claim, and the LINQ `Any` allocates an enumerator per call. It is -read in the enrichment steps, in `ConvertData.GenerateConverter` (per schema group) and -in `ShouldEnrich` guards, so it runs many times per request against an unchanging value. - -**Fix:** compute once in the constructor into a `readonly bool`. - ---- - -### 17. `ResolvingReferences()` re-evaluated per content -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs:63,141` - -`SchemaExtensions.ResolvingReferences` is a lazy `Fields.OfType<...>().Where(...)` — it -is not materialised. Line 141 calls it inside `foreach (var content in contents)`, so -the full field scan plus two LINQ iterator allocations happen once per content rather -than once per schema. - -`ResolveReferences.EnrichAsync` also enumerates `contents.GroupBy(...)` twice -(lines 37 and 47), as does `ConvertData` (lines 39 and 67) — safe for a `List`, wasteful -for anything lazy. - -**Fix:** hoist to `var refFields = schema.ResolvingReferences().ToList();` outside the -loop; materialise `contents` once at the top of each step. - ---- - ### 18. Sequential N+1 schema and component lookups `backend/src/Squidex.Domain.Apps.Entities/AppProviderExtensions.cs:30` `backend/src/Squidex/Areas/Api/Controllers/Contents/Generator/SchemasOpenApiGenerator.cs:39,48`