More fixes

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<IMongoCollection<MongoContentEntity>>> collections =
-        new ConcurrentDictionary<(DomainId, DomainId), Task<IMongoCollection<MongoContentEntity>>>();
+    private readonly ConcurrentDictionary<(DomainId AppId, DomainId SchemaId), Lazy<Task<IMongoCollection<MongoContentEntity>>>> collections =
+        new ConcurrentDictionary<(DomainId AppId, DomainId SchemaId), Lazy<Task<IMongoCollection<MongoContentEntity>>>>();
 
-    public Task<IMongoCollection<MongoContentEntity>> GetCollectionAsync(DomainId appId, DomainId schemaId)
+    public async Task<IMongoCollection<MongoContentEntity>> 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<Task<IMongoCollection<MongoContentEntity>>>>(key, collection));
+            throw;
+        }
+    }
+
+    private Lazy<Task<IMongoCollection<MongoContentEntity>>> CreateLazyCollection((DomainId AppId, DomainId SchemaId) key)
+    {
+        return new Lazy<Task<IMongoCollection<MongoContentEntity>>>(
+            () => CreateCollectionAsync(key),
+            LazyThreadSafetyMode.ExecutionAndPublication);
     }
 
-    private async Task<IMongoCollection<MongoContentEntity>> CreateCollectionAsync((DomainId, DomainId) key)
+    private async Task<IMongoCollection<MongoContentEntity>> CreateCollectionAsync((DomainId AppId, DomainId SchemaId) key)
     {
         var (appId, schemaId) = key;
 

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);

backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs

@@ -136,9 +136,13 @@ public sealed class ResolveReferences(Lazy<IContentQueryService> contentQuery, I
 
     private static void AddReferenceIds(HashSet<DomainId> ids, Schema schema, ResolvedComponents components, IEnumerable<EnrichedContent> 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);
         }
     }
 

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)

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<Task<...>>` 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<IGrouping<...>>` through the method signatures.
+
+**Verified:** build clean, all suites green.

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<T>` 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<Task<T>>` 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`