From 7331c329cbc8cc622babff4614591ffc72b4cb22 Mon Sep 17 00:00:00 2001 From: Sebastian Stehle Date: Mon, 24 Aug 2026 14:19:34 +0200 Subject: [PATCH] More fixes --- .../Rules/RuleEnqueuer.cs | 38 ++++-- .../Squidex.Web/Pipeline/CachingManager.cs | 34 +---- .../Rules/RuleEnqueuerTests.cs | 49 ++++++++ resolved.md | 117 +++++++++++++++++- todo.md | 55 +------- 5 files changed, 202 insertions(+), 91 deletions(-) diff --git a/backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs b/backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs index 457188f3a..def60f215 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs @@ -79,19 +79,27 @@ public sealed class RuleEnqueuer( // Write in batches of 100 items for better performance. Dispose completes the last write. await using var batch = new RuleQueueWriter(flowManager, ruleUsageTracker, log); - foreach (var @event in events) + static NamedId? GetAppId(Envelope @event) { - if (@event.Headers.Restored()) + // Returns null for events that are not handled, so that they all end up in one group to skip. + if (@event.Headers.Restored() || @event.Payload is not AppEvent appEvent) { - continue; + return null; } - if (@event.Payload is not AppEvent appEvent) + return appEvent.AppId; + } + + // The events of a batch usually belong to the same app, so the rules are only resolved + // and indexed once per app instead of once per event. + foreach (var byApp in events.GroupBy(GetAppId)) + { + if (byApp.Key == null) { continue; } - var rules = await GetRulesAsync(appEvent.AppId.Id); + var rules = await GetRulesAsync(byApp.Key.Id); if (rules.Count == 0) { continue; @@ -99,7 +107,7 @@ public sealed class RuleEnqueuer( var context = new RulesContext { - AppId = appEvent.AppId, + AppId = byApp.Key, AllowExtraEvents = maxExtraEvents > 0, IncludeSkipped = false, IncludeStale = false, @@ -107,14 +115,28 @@ public sealed class RuleEnqueuer( MaxEvents = maxExtraEvents, }; - await foreach (var result in ruleService.CreateJobsAsync(@event, context)) + foreach (var @event in byApp) { - await batch.WriteAsync(appEvent.AppId.Id, result); + await foreach (var result in ruleService.CreateJobsAsync(@event, context)) + { + await batch.WriteAsync(byApp.Key.Id, result); + } } } } } + // Returns null for events that are not handled, so that they all end up in one group to skip. + private static NamedId? GetAppId(Envelope @event) + { + if (@event.Headers.Restored() || @event.Payload is not AppEvent appEvent) + { + return null; + } + + return appEvent.AppId; + } + private Task> GetRulesAsync(DomainId appId) { if (cacheDuration <= TimeSpan.Zero || cacheDuration == TimeSpan.MaxValue) diff --git a/backend/src/Squidex.Web/Pipeline/CachingManager.cs b/backend/src/Squidex.Web/Pipeline/CachingManager.cs index 41ab6c02c..63fa51333 100644 --- a/backend/src/Squidex.Web/Pipeline/CachingManager.cs +++ b/backend/src/Squidex.Web/Pipeline/CachingManager.cs @@ -34,15 +34,14 @@ public sealed class CachingManager : IRequestCache private readonly IncrementalHash hasher = IncrementalHash.CreateHash(HashAlgorithmName.SHA256); private readonly HashSet keys = []; private readonly HashSet headers = []; - private readonly ReaderWriterLockSlim slimLock = new ReaderWriterLockSlim(); + private readonly Lock lockObject = new Lock(); + private bool hasDependency; private bool isFinished; public void Dispose() { hasher.Dispose(); - - slimLock.Dispose(); } public void AddDependency(string key, long version) @@ -52,8 +51,7 @@ public sealed class CachingManager : IRequestCache return; } - slimLock.EnterWriteLock(); - try + lock (lockObject) { if (!keys.Add(key)) { @@ -65,10 +63,6 @@ public sealed class CachingManager : IRequestCache hasDependency = true; } - finally - { - slimLock.ExitWriteLock(); - } } public void AddDependency(T value) @@ -80,17 +74,12 @@ public sealed class CachingManager : IRequestCache return; } - slimLock.EnterWriteLock(); - try + lock (lockObject) { hasher.AppendString(formatted); hasDependency = true; } - finally - { - slimLock.ExitWriteLock(); - } } public void Finish(HttpResponse response, ObjectPool stringBuilderPool) @@ -104,8 +93,7 @@ public sealed class CachingManager : IRequestCache // Set to finish before we start to ensure that we do not call it again in case of an error. isFinished = true; - slimLock.EnterWriteLock(); - try + lock (lockObject) { if (hasDependency && !response.Headers.ContainsKey(HeaderNames.ETag)) { @@ -160,10 +148,6 @@ public sealed class CachingManager : IRequestCache response.Headers[HeaderNames.Vary] = new StringValues(headers.ToArray()); } } - finally - { - slimLock.ExitWriteLock(); - } } public void AddHeader(string header, StringValues values) @@ -173,16 +157,10 @@ public sealed class CachingManager : IRequestCache return; } - try + lock (lockObject) { - slimLock.EnterWriteLock(); - headers.Add(header); } - finally - { - slimLock.ExitWriteLock(); - } foreach (var value in values) { diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Rules/RuleEnqueuerTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Rules/RuleEnqueuerTests.cs index 73030914e..882833246 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Rules/RuleEnqueuerTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Rules/RuleEnqueuerTests.cs @@ -350,6 +350,55 @@ public class RuleEnqueuerTests : GivenContext .MustHaveHappenedANumberOfTimesMatching(x => x == 10); } + [Fact] + public async Task Should_handle_events_of_multiple_apps_with_the_rules_of_each_app() + { + var rule1 = CreateAndSetupRule(); + + var otherAppId = NamedId.Of(DomainId.NewGuid(), "other-app"); + var rule2 = CreateRule(); + + A.CallTo(() => AppProvider.GetRulesAsync(otherAppId.Id, A._)) + .Returns([rule2]); + + var event1 = Envelope.Create(new ContentCreated { AppId = AppId }); + var event2 = Envelope.Create(new ContentCreated { AppId = otherAppId }); + var event3 = Envelope.Create(new ContentCreated { AppId = AppId }); + var event4 = Envelope.Create(new ContentCreated { AppId = otherAppId }); + + var contexts = new List<(Envelope Event, RulesContext Context)>(); + + A.CallTo(() => ruleService.CreateJobsAsync(A>._, A._, default)) + .Invokes((Envelope source, RulesContext context, CancellationToken _) => + { + contexts.Add((source, context)); + }) + .Returns(Array.Empty().ToAsyncEnumerable()); + + await sut.On([event1, event2, event3, event4]); + + // Every event must be handled, even though they are grouped by app. + Assert.Equal([event1, event3, event2, event4], contexts.Select(x => x.Event).ToArray()); + + // Every event must be handled with the rules of its own app. + Assert.All(contexts.Where(x => x.Event == event1 || x.Event == event3), x => + { + Assert.Equal(AppId, x.Context.AppId); + Assert.Equal([rule1], x.Context.Rules.Values.ToArray()); + }); + + Assert.All(contexts.Where(x => x.Event == event2 || x.Event == event4), x => + { + Assert.Equal(otherAppId, x.Context.AppId); + Assert.Equal([rule2], x.Context.Rules.Values.ToArray()); + }); + + // The rules must only be indexed once per app, not once per event. + Assert.Same(contexts[0].Context.Rules, contexts[1].Context.Rules); + Assert.Same(contexts[2].Context.Rules, contexts[3].Context.Rules); + } + + private static RulesContext MatchingContext(Rule rule) { // These two properties must not be set to true for performance reasons. diff --git a/resolved.md b/resolved.md index a77ee435a..4c40dfe6a 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. -Item **6** is not listed here: it is open but accepted as won't-fix, and stays documented -in `todo.md` so it is not re-reported as a new finding. +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. --- @@ -218,3 +218,116 @@ reads `partitioning.GetName(...)` and `IsOptional`, so a key built from the lang `app.Version` is conservative but provably correct: it changes whenever anything about the app does. Trading guaranteed correctness for a cache-hit-rate win is the wrong direction here, so it stays until someone establishes the model's exact dependency set. + +--- + +### 6. Sync-over-async on the authentication path — **CLOSED: ACCEPTED, WON'T FIX** +`backend/src/Squidex/Areas/IdentityServer/Config/Dynamic/DynamicSchemeProvider.cs:129` + +```csharp +var scheme = GetSchemeCoreAsync(name, default).Result; +``` + +`Get(string? name)` blocks a thread-pool thread on a DB round trip, which in a hot path +is a classic thread-pool starvation source. + +**Closed as accepted, not fixed.** This is dynamic OIDC scheme resolution — reached only +for team-level auth domains, not on ordinary API traffic — so the risk does not justify +the rework. Recorded here rather than deleted so it is not re-reported as a new finding. + +If it ever moves onto a hot path, the fix is to cache scheme results synchronously +(populated by an async initializer / background refresh) so `Get` can return without +blocking. + +Same pattern elsewhere, also accepted: +- `Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/ScriptingExtensions.cs:144` — `.Wait()` on full content validation inside a script callback. +- `Squidex.Data.MongoDb/Infrastructure/MongoRepositoryBase.cs:26` — `InitializeAsync(default).Wait()`. + +--- + +### 12. `ReaderWriterLockSlim` used exclusively for write locks in the ETag path — **FIXED** +`backend/src/Squidex.Web/Pipeline/CachingManager.cs` + +**Was:** `CacheContext` guarded `AddDependency`, `AddDependency`, `AddHeader` and +`Finish` with `ReaderWriterLockSlim` — but every one of them took `EnterWriteLock`. No +code path ever took a read lock, so the reader/writer bookkeeping was pure overhead at +roughly 2–3× the cost of a plain monitor. `AddDependency` is called once per content, +once per schema and once per resolved reference, so a 200-item list with references took +on the order of a thousand write-lock round trips per request. + +**Now:** a plain `Lock` (`System.Threading.Lock`, matching `DisposableObjectBase`), with +each `EnterWriteLock`/`try`/`finally`/`ExitWriteLock` block collapsed to `lock (...)`. + +Two incidental improvements fell out of the rewrite: + +- `Dispose()` no longer has a lock to dispose, so `CacheContext` only disposes the hasher. +- `AddHeader` had its `EnterWriteLock` *inside* the `try`, so a throw from the acquire + would have hit `ExitWriteLock` on an unheld lock and masked the original error with a + `SynchronizationLockException`. `lock` cannot express that shape. + +Nothing about the concurrency contract changed — every operation mutates the hasher and +the sets, so there was never anything a read lock could have protected. + +**Verified:** build clean, `Squidex.Web.Tests` green (167). + +--- + +### 13. Rules dictionary rebuilt per event inside the batch loop — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs` + +**Was:** `On(...)` receives batches of 200 events and ran +`Rules = rules.ToReadonlyDictionary(x => x.Id)` for *each* one — a full `Dictionary` +build plus a wrapper allocation per event, even though the events in a batch are +overwhelmingly from the same app. + +Note the rules *lookup* was already cheap: `RulesCacheDuration` defaults to 10s, so +`appProvider.GetRulesAsync` was memoized. The waste was purely the per-event indexing. + +**Now:** the batch is grouped by app, so rules are resolved and indexed once per app and +the context is built once per group: + +```csharp +foreach (var byApp in events.GroupBy(GetAppId)) +{ + if (byApp.Key == null) { continue; } + + var rules = await GetRulesAsync(byApp.Key.Id); + if (rules.Count == 0) { continue; } + + var context = new RulesContext { AppId = byApp.Key, Rules = rules.ToReadonlyDictionary(x => x.Id), ... }; + + foreach (var @event in byApp) { ... } +} +``` + +`GetAppId` returns `null` for restored events and non-`AppEvent` payloads, so they all +collect into one group that is skipped — replacing the two per-event `continue` guards. + +**Why `GroupBy` rather than memoizing per app inside the original loop.** The first +attempt kept the original per-event loop and cached the indexed dictionary in a +`Dictionary`, specifically to avoid reordering events. `GroupBy` does +reorder across apps, so that had to be checked rather than assumed: + +- Rules are scoped to a single app (`context.AppId`, `context.Rules`), so a rule cannot + observe another app's events. +- `RuleQueueWriter` is app-agnostic — it accumulates `CreateFlowInstanceRequest` values + and flushes every 100 regardless of origin. +- `ruleUsageTracker.TrackAsync` is an additive counter per (app, rule, day). +- `GroupBy` preserves source order *within* each group, which is the ordering that can + actually matter. + +Nothing cross-app is order-sensitive, so `GroupBy` is safe — and it is both simpler and +slightly more correct than the memo: keying on `NamedId` (a `sealed record`, +so value equality over id *and* name) means an app renamed mid-batch yields two groups +each carrying its own correct name, where the memo keyed on `.Id` would have reused the +first name seen. + +**Verified:** a new test, +`RuleEnqueuerTests.Should_handle_events_of_multiple_apps_with_the_rules_of_each_app`, +feeds an interleaved two-app batch and asserts each event is handled with its own app's +rules, that the grouped order is what reaches the service, and — via `Assert.Same` on the +`Rules` instance — that indexing happens once per app rather than once per event. It +**fails on the pre-fix code** (the ordering assertion shows `app1, app2, app1, app2` +against the expected `app1, app1, app2, app2`) and passes after. Existing coverage did +not include a multi-app batch at all: `Should_handle_events_in_batches` repeats the *same* +event ten times. Full `Squidex.Domain.Apps.Entities.Tests` green (1528). diff --git a/todo.md b/todo.md index 4944d3806..d8891f711 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**, **7**, **8** and **9** are done and live there. +expected — items **4**, **5**, **6**, **7**, **8**, **9**, **12** and **13** are closed and live there. -**Status: 14 open of 20. Item 6 accepted as-is (see below); items 4, 5, 7, 8, 9 are in [resolved.md](resolved.md).** +**Status: 11 open of 20. Items 4, 5, 6, 7, 8, 9, 12, 13 are in [resolved.md](resolved.md).** --- @@ -68,29 +68,6 @@ already isolated in `ContentScriptVars`. ## S2 — High -### 6. Sync-over-async on the authentication path — **ACCEPTED, WON'T FIX** -`backend/src/Squidex/Areas/IdentityServer/Config/Dynamic/DynamicSchemeProvider.cs:129` - -```csharp -var scheme = GetSchemeCoreAsync(name, default).Result; -``` - -`Get(string? name)` blocks a thread-pool thread on a DB round trip. **Accepted as-is — -this is not an important path** (dynamic OIDC scheme resolution, only reached for -team-level auth domains, not on ordinary API traffic), so the starvation risk does not -justify the rework. Left documented rather than deleted so it is not re-reported as a -new finding. - -If it ever does move onto a hot path, the fix is to cache scheme results synchronously -(populated by an async initializer / background refresh) so `Get` can return without -blocking. - -Same pattern elsewhere, also low blast radius: -- `Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/ScriptingExtensions.cs:144` — `.Wait()` on full content validation inside a script callback. -- `Squidex.Data.MongoDb/Infrastructure/MongoRepositoryBase.cs:26` — `InitializeAsync(default).Wait()`. - ---- - ### 10. Unbounded in-memory request-log queue `backend/src/Squidex.Infrastructure/Log/BackgroundRequestLogStore.cs:22,126` @@ -127,34 +104,6 @@ request, uncached. ## S3 — Moderate -### 12. `ReaderWriterLockSlim` used exclusively for write locks in the ETag path -`backend/src/Squidex.Web/Pipeline/CachingManager.cs:37,55,83,107,178` - -`CacheContext` takes `EnterWriteLock` in `AddDependency`, `AddDependency`, -`AddHeader` and `Finish`. No code path ever takes a read lock, so the reader/writer -machinery is pure overhead — `ReaderWriterLockSlim` costs roughly 2–3× a plain -`Monitor` acquisition. - -`AddDependency` is called once per content, once per schema and once per resolved -reference, so a 200-item list with references takes on the order of a thousand -write-lock round trips per request. - -**Fix:** a plain `lock` object. - ---- - -### 13. Rules dictionary rebuilt per event inside the batch loop -`backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs:106` - -`On(...)` receives batches of 200 events and builds -`Rules = rules.ToReadonlyDictionary(x => x.Id)` for each one. Events in a batch are -overwhelmingly from the same app, so the same immutable dictionary is constructed up to -200 times per batch, alongside a fresh `RulesContext` record each iteration. - -**Fix:** group the batch by `AppId` and build one `RulesContext` per group. - ---- - ### 14. `AppProvider` copies cached schema/rule lists on every call `backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs:197,208,216`