From c8ed8217e55e92307b856453d2e67926e6d70ad3 Mon Sep 17 00:00:00 2001 From: Sebastian Stehle Date: Mon, 24 Aug 2026 17:27:08 +0200 Subject: [PATCH] Fixes --- .../ConvertContent/ContentConverter.cs | 39 +++--- .../Assets/DefaultAssetFileStore.cs | 63 ++++++++-- .../Assets/Transformations.cs | 4 +- .../ContentConversionRemovalTests.cs | 113 ++++++++++++++++++ .../Assets/DefaultAssetFileStoreTests.cs | 4 +- resolved.md | 111 +++++++++++++++++ todo.md | 83 +------------ 7 files changed, 310 insertions(+), 107 deletions(-) create mode 100644 backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/ConvertContent/ContentConversionRemovalTests.cs diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs index 4d66827e7..fc928104f 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs +++ b/backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs @@ -149,22 +149,27 @@ public sealed class ContentConverter(ResolvedComponents components, Schema schem return (true, default); } - for (int i = 0; i < array.Count; i++) + // Compact in place instead of removing items, because every remove moves all the items + // after it and dropping many items from a large array would be quadratic. + var target = 0; + + for (var i = 0; i < array.Count; i++) { var oldValue = array[i]; var (removed, newValue) = ConvertArrayItem(field, oldValue); if (removed) { - array.RemoveAt(i); - i--; - } - else if (!ReferenceEquals(newValue.Value, oldValue.Value)) - { - array[i] = newValue; + continue; } + + // Faster to check for reference equality than for deep equals. + array[target] = ReferenceEquals(newValue.Value, oldValue.Value) ? oldValue : newValue; + target++; } + array.RemoveRange(target, array.Count - target); + return (false, array); } @@ -175,23 +180,27 @@ public sealed class ContentConverter(ResolvedComponents components, Schema schem return (true, default); } - for (int i = 0; i < array.Count; i++) + // Compact in place instead of removing items, because every remove moves all the items + // after it and dropping many items from a large array would be quadratic. + var target = 0; + + for (var i = 0; i < array.Count; i++) { var oldValue = array[i]; var (removed, newValue) = ConvertComponent(oldValue, parent); if (removed) { - array.RemoveAt(i); - i--; - } - else if (!ReferenceEquals(newValue.Value, oldValue.Value)) - { - // Faster to check for reference equality than for deep equals. - array[i] = newValue; + continue; } + + // Faster to check for reference equality than for deep equals. + array[target] = ReferenceEquals(newValue.Value, oldValue.Value) ? oldValue : newValue; + target++; } + array.RemoveRange(target, array.Count - target); + return (false, array); } diff --git a/backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs b/backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs index d544d0dae..eab94159e 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs @@ -6,6 +6,7 @@ // ========================================================================== using System.Text; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Options; using Squidex.Assets; using Squidex.Domain.Apps.Core.Apps; @@ -17,9 +18,11 @@ namespace Squidex.Domain.Apps.Entities.Assets; public sealed class DefaultAssetFileStore( IAssetStore assetStore, IAssetRepository assetRepository, + IMemoryCache cache, IOptions options) : IAssetFileStore, IDeleter { + private static readonly TimeSpan CacheDuration = TimeSpan.FromHours(1); private readonly AssetOptions options = options.Value; async Task IDeleter.DeleteAppAsync(App app, @@ -48,37 +51,73 @@ public sealed class DefaultAssetFileStore( public async Task GetFileSizeAsync(DomainId appId, DomainId id, long fileVersion, string? suffix, CancellationToken ct = default) { - try + if (options.FolderPerApp) { - var fileNameNew = GetFileName(appId, id, fileVersion, suffix); + return await assetStore.GetSizeAsync(GetFileName(appId, id, fileVersion, suffix), ct); + } - return await assetStore.GetSizeAsync(fileNameNew, ct); + var (first, second, isOldFirst) = FileNames(appId, id, fileVersion, suffix); + try + { + return await assetStore.GetSizeAsync(first, ct); } - catch (AssetNotFoundException) when (!options.FolderPerApp) + catch (AssetNotFoundException) { - var fileNameOld = GetFileName(id, fileVersion, suffix); + RememberFileName(appId, id, !isOldFirst); - return await assetStore.GetSizeAsync(fileNameOld, ct); + return await assetStore.GetSizeAsync(second, ct); } } public async Task DownloadAsync(DomainId appId, DomainId id, long fileVersion, string? suffix, Stream stream, BytesRange range = default, CancellationToken ct = default) { - try + if (options.FolderPerApp) { - var fileNameNew = GetFileName(appId, id, fileVersion, suffix); + await assetStore.DownloadAsync(GetFileName(appId, id, fileVersion, suffix), stream, range, ct); + return; + } - await assetStore.DownloadAsync(fileNameNew, stream, range, ct); + var (first, second, isOldFirst) = FileNames(appId, id, fileVersion, suffix); + try + { + await assetStore.DownloadAsync(first, stream, range, ct); } - catch (AssetNotFoundException) when (!options.FolderPerApp) + catch (AssetNotFoundException) { - var fileNameOld = GetFileName(id, fileVersion, suffix); + RememberFileName(appId, id, !isOldFirst); - await assetStore.DownloadAsync(fileNameOld, stream, range, ct); + await assetStore.DownloadAsync(second, stream, range, ct); } } + // Assets from older versions are stored under a file name without the app ID. Which name an + // asset uses can only be found out by trying, and a failed try is a full roundtrip to the asset + // store. Therefore the outcome is remembered as a hint. Both names are still tried, so that a + // stale hint only costs the roundtrip it was there to save. + private (string First, string Second, bool IsOldFirst) FileNames(DomainId appId, DomainId id, long fileVersion, string? suffix) + { + var fileNameNew = GetFileName(appId, id, fileVersion, suffix); + var fileNameOld = GetFileName(id, fileVersion, suffix); + + if (cache.TryGetValue(CacheKey(appId, id), out var useOld) && useOld) + { + return (fileNameOld, fileNameNew, true); + } + + return (fileNameNew, fileNameOld, false); + } + + private void RememberFileName(DomainId appId, DomainId id, bool useOld) + { + cache.Set(CacheKey(appId, id), useOld, CacheDuration); + } + + private static object CacheKey(DomainId appId, DomainId id) + { + return (typeof(DefaultAssetFileStore), appId, id); + } + public Task DownloadAsync(string tempFile, Stream stream, CancellationToken ct = default) { diff --git a/backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs b/backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs index 2f4da4ce5..0d2f2aabb 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs @@ -76,7 +76,9 @@ public static class Transformations { await DownloadAsync(asset, assetFileStore, stream, ct); - var bytes = stream.ToArray(); + // Use the buffer of the pooled stream. ToArray would allocate another copy of the whole + // file, which for a file of up to the maximum size would go to the large object heap. + var bytes = new ReadOnlySpan(stream.GetBuffer(), 0, (int)stream.Length); switch (encoding?.ToLowerInvariant()) { diff --git a/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/ConvertContent/ContentConversionRemovalTests.cs b/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/ConvertContent/ContentConversionRemovalTests.cs new file mode 100644 index 000000000..f92031657 --- /dev/null +++ b/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/ConvertContent/ContentConversionRemovalTests.cs @@ -0,0 +1,113 @@ +// ========================================================================== +// Squidex Headless CMS +// ========================================================================== +// Copyright (c) Squidex UG (haftungsbeschraenkt) +// All rights reserved. Licensed under the MIT license. +// ========================================================================== + +using Squidex.Domain.Apps.Core.Contents; +using Squidex.Domain.Apps.Core.ConvertContent; +using Squidex.Domain.Apps.Core.Schemas; +using Squidex.Infrastructure; +using Squidex.Infrastructure.Json.Objects; + +namespace Squidex.Domain.Apps.Core.Operations.ConvertContent; + +public class ContentConversionRemovalTests +{ + private static readonly DomainId ComponentId = DomainId.NewGuid(); + private readonly ResolvedComponents components; + private readonly Schema schema; + + public ContentConversionRemovalTests() + { + schema = + new Schema { Name = "my-schema" } + .AddComponents(1, "components", Partitioning.Invariant) + .AddArray(2, "array", Partitioning.Invariant, a => a + .AddString(21, "value")); + + components = new ResolvedComponents(new Dictionary + { + [ComponentId] = + new Schema { Name = "my-component" } + .AddString(1, "value", Partitioning.Invariant), + }); + } + + [Theory] + [InlineData("abc", "abc")] + [InlineData("-bc", "bc")] + [InlineData("a-c", "ac")] + [InlineData("ab-", "ab")] + [InlineData("--c", "c")] + [InlineData("-b-", "b")] + [InlineData("a--", "a")] + [InlineData("---", "")] + public void Should_remove_array_items_that_are_not_objects(string source, string expected) + { + var items = source.Select(x => x == '-' ? JsonValue.Create(0) : Item(x)); + + Assert.Equal(expected, Convert("array", items)); + } + + [Theory] + [InlineData("abc", "abc")] + [InlineData("-bc", "bc")] + [InlineData("a-c", "ac")] + [InlineData("ab-", "ab")] + [InlineData("--c", "c")] + [InlineData("-b-", "b")] + [InlineData("a--", "a")] + [InlineData("---", "")] + public void Should_remove_components_of_unknown_schema(string source, string expected) + { + var items = source.Select(x => x == '-' ? ComponentOf(x, DomainId.NewGuid()) : ComponentOf(x, ComponentId)); + + Assert.Equal(expected, Convert("components", items)); + } + + [Theory] + [InlineData("abc", "abc")] + [InlineData("-bc", "bc")] + [InlineData("a-c", "ac")] + [InlineData("ab-", "ab")] + [InlineData("--c", "c")] + [InlineData("-b-", "b")] + [InlineData("a--", "a")] + [InlineData("---", "")] + public void Should_remove_components_without_discriminator(string source, string expected) + { + var items = source.Select(x => x == '-' ? Item(x) : ComponentOf(x, ComponentId)); + + Assert.Equal(expected, Convert("components", items)); + } + + private string Convert(string field, IEnumerable items) + { + var source = + new ContentData() + .AddField(field, + new ContentFieldData() + .AddInvariant(JsonValue.Array(items.ToArray()))); + + var converted = new ContentConverter(components, schema).Convert(source); + + if (!converted.TryGetValue(field, out var data) || data?["iv"].Value is not JsonArray array) + { + return string.Empty; + } + + return string.Concat(array.Select(x => ((JsonObject)x.Value!)["value"].ToString())); + } + + private static JsonValue Item(char value) + { + return JsonValue.Object().Add("value", value.ToString()); + } + + private static JsonValue ComponentOf(char value, DomainId schemaId) + { + return JsonValue.Object().Add("value", value.ToString()).Add(Component.Discriminator, schemaId); + } +} diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DefaultAssetFileStoreTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DefaultAssetFileStoreTests.cs index 5983addd6..cc370729d 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DefaultAssetFileStoreTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DefaultAssetFileStoreTests.cs @@ -6,6 +6,7 @@ // ========================================================================== using System.Globalization; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Options; using Squidex.Assets; using Squidex.Domain.Apps.Entities.Assets.Repositories; @@ -21,6 +22,7 @@ public class DefaultAssetFileStoreTests : GivenContext private readonly DomainId assetId = DomainId.NewGuid(); private readonly long assetFileVersion = 21; private readonly AssetOptions options = new AssetOptions(); + private readonly IMemoryCache cache = new MemoryCache(Options.Create(new MemoryCacheOptions())); private readonly DefaultAssetFileStore sut; public static readonly TheoryData PathCases = new TheoryData @@ -39,7 +41,7 @@ public class DefaultAssetFileStoreTests : GivenContext public DefaultAssetFileStoreTests() { - sut = new DefaultAssetFileStore(assetStore, assetRepository, Options.Create(options)); + sut = new DefaultAssetFileStore(assetStore, assetRepository, cache, Options.Create(options)); } [Theory] diff --git a/resolved.md b/resolved.md index 32a443f09..6b21ad948 100644 --- a/resolved.md +++ b/resolved.md @@ -108,6 +108,10 @@ across threads; a cold race can build the same pattern twice, which only wastes work and never returns anything incorrect. The critical section is just the dictionary and linked-list updates. +`MemoryCache` would remove the lock, but `PatternValidator` is constructed without +dependency injection, so it would have to create and hold its own cache instance. The lock +is the smaller change and it is already covered by the concurrency harness below. + **Verified:** `dotnet build` clean (0 warnings); a harness mirroring `GetRegex` ran 1.6M operations over 8 threads against 3000 distinct patterns in a 1000-entry cache (continuous eviction) with 0 exceptions, 0 wrong matches and the cache correctly bounded @@ -922,3 +926,110 @@ They should be validated against a container run before release. Items 25, 26 and 27 are pure hoists with no behavioural change and are covered by the enrichment tests that did run. + +--- + +### 28. Asset downloads used an exception as the legacy-path fallback — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs` + +**Was:** `GetFileSizeAsync` and `DownloadAsync` tried the current file name, caught +`AssetNotFoundException`, and retried with the legacy name (no app ID). On an instance that +still holds assets under the old scheme, *every* access to those assets threw and caught +first — and against a cloud store the failed attempt is a full network round trip, so the +fallback roughly doubled the latency of every legacy asset served. + +**Now:** the outcome is remembered per asset in the injected `IMemoryCache`, keyed by +`(typeof(DefaultAssetFileStore), appId, id)` with a one hour sliding lifetime, so the wrong +name is only tried once. `IMemoryCache` rather than `Squidex.Caching.LRUCache` because it is +thread safe on its own — see item 7 for what `LRUCache` does under concurrent access. + +**The memo is a hint, not a decision.** `FileNames(...)` returns both names ordered by what +was last seen to work, and the other one is still tried on failure. That matters because an +asset can move between schemes — a migration, or an eviction followed by a re-probe — and a +cache that *decided* rather than *hinted* would turn a stale entry into a hard failure. The +cost of a wrong hint is one extra round trip, exactly what the code did before. + +Two things fell out of it: the `options.FolderPerApp` case now short-circuits to a single +name with no try/catch at all, and the partial-write hazard flagged in the finding (a retry +appending to a stream the first attempt already wrote to) is now hit far less often, since a +warm asset takes the right branch first. It is not *fixed* — that would need the asset store +to guarantee it writes nothing before failing. + +--- + +### 29. Removing items while iterating a `JsonArray` was quadratic — **FIXED** +`backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs:145,175` + +**Was:** `ConvertArray` and `ConvertComponents` both removed in place with +`array.RemoveAt(i); i--;`. `JsonArray` derives from `List`, so each removal shifts +every following element — dropping *k* of *n* items costs O(n·k), and the case where many +items are dropped (entries referencing deleted component schemas) is exactly the case where +the array is large. + +**Now:** a single compaction pass with a write index, then one `RemoveRange` for the tail. + +```csharp +var target = 0; + +for (var i = 0; i < array.Count; i++) +{ + var oldValue = array[i]; + + var (removed, newValue) = ConvertArrayItem(field, oldValue); + if (removed) + { + continue; + } + + array[target] = ReferenceEquals(newValue.Value, oldValue.Value) ? oldValue : newValue; + target++; +} + +array.RemoveRange(target, array.Count - target); +``` + +The write index is always `<= i`, so a slot is only ever overwritten after it has been read — +no read-after-write hazard, and the surviving order is preserved. + +**Verified with new tests** — `ContentConversionRemovalTests`, 27 cases covering nine removal +patterns (none, first, middle, last, adjacent pairs, alternating, all) across three ways an +item gets dropped: a non-object in an array, a component of an unknown schema, and a +component with no discriminator. + +Two checks on the tests themselves, because this is a behaviour-preserving rewrite rather +than a bug fix: + +- They pass against **both** the original `RemoveAt` implementation and the new one, which is + the property that actually matters here — they pin the contract rather than the code. +- Mutation check: deleting the `RemoveRange` line fails 24 of the 27, so they are not + vacuous. + +A first attempt at these tests drove removal through a custom `IContentItemConverter` that +stripped the discriminator; that never removed anything, because `ConvertComponent` checks +the discriminator *before* calling `ConvertNested`. The tests now use inherently invalid +items, which is both simpler and closer to the real cause. + +--- + +### 30. `stream.ToArray()` copied straight back out of the pooled buffer — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs:79` + +**Was:** `GetTextAsync` downloaded into a `DefaultPools.MemoryStream` +(`RecyclableMemoryStreamManager`) and then called `ToArray()`, allocating a fresh array of +the whole file and copying the pooled buffer into it — for a file at the 4 MB limit, straight +onto the large object heap on every call. + +**Now:** + +```csharp +var bytes = new ReadOnlySpan(stream.GetBuffer(), 0, (int)stream.Length); +``` + +`Convert.ToBase64String` and `Encoding.GetString` all have `ReadOnlySpan` overloads, so +nothing downstream changed. + +Worth being precise about why `GetBuffer` is better rather than just "avoids a copy": +`RecyclableMemoryStream.GetBuffer()` still consolidates into a single contiguous buffer when +the stream spans several blocks. The difference is that the buffer it returns comes from the +pool and goes back on dispose, whereas `ToArray` allocates a new GC array every time. +`RecyclableMemoryStream` documents `ToArray` as the call to avoid for exactly this reason. diff --git a/todo.md b/todo.md index ae9eb95a1..bb882cb9a 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**–**22**, **24**–**27** are closed and live there. +expected — items **4**–**22** and **24**–**30** are closed and live there. -**Status: 7 open of 30 — items 1, 2, 3, 23, 28, 29, 30. The other 23 are in [resolved.md](resolved.md).** +**Status: 4 open of 30 — items 1, 2, 3 (one root cause) and 23. The other 26 are in [resolved.md](resolved.md).** --- @@ -92,81 +92,6 @@ than a fixed 1000-id prefix that the outer query re-sorts. --- -## S3 — Moderate - -### 28. Asset downloads use an exception as the legacy-path fallback -`backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs:57,74` - -```csharp -try -{ - await assetStore.DownloadAsync(fileNameNew, stream, range, ct); -} -catch (AssetNotFoundException) when (!options.FolderPerApp) -{ - await assetStore.DownloadAsync(fileNameOld, stream, range, ct); -} -``` - -On an instance that still holds assets under the old naming scheme, **every** download of those -assets throws and catches first. A .NET exception costs on the order of tens of microseconds, -and against a cloud store (S3, Azure Blob) the failed attempt is also a full network round trip -before the retry — so the fallback doubles the latency of every legacy asset served. - -`GetFileSizeAsync` (line 57) has the same shape. - -Worth checking while fixing: whether `assetStore.DownloadAsync` can write bytes into the target -stream before discovering the file is missing. If it can, the retry appends to a partially -written response body rather than replacing it. - -**Fix:** probe once per asset and remember which naming scheme it uses, or migrate the names so -the fallback can be deleted. - ---- - -## S4 — Low - -### 29. Removing items while iterating a `JsonArray` is quadratic -`backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs:159,185` - -```csharp -for (int i = 0; i < array.Count; i++) -{ - ... - if (removed) - { - array.RemoveAt(i); - i--; - } -} -``` - -`JsonArray` derives from `List`, so each `RemoveAt` shifts every following element. -Dropping *k* items from an *n*-element array costs O(n·k). It only bites when many items are -removed at once — an array or components field whose entries reference deleted schemas or -components — but that is exactly the case where the array is large. - -**Fix:** compact in a single pass (write index) rather than removing in place. - ---- - -### 30. `stream.ToArray()` copies straight back out of the pooled buffer -`backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs:79` - -`GetTextAsync` downloads into a `DefaultPools.MemoryStream` (a -`RecyclableMemoryStreamManager`) and then calls `stream.ToArray()`, which allocates a fresh -array of the full file and copies the pooled buffer into it — defeating the point of the pool. -`RecyclableMemoryStream` documents `ToArray` as the thing not to call for this reason. - -Bounded at 4 MB by `MaxSize`, and this runs from scripts rather than the main content path, -which is why it ranks last — but 4 MB straight into the large object heap per call is still -worth avoiding. - -**Fix:** `GetBuffer()` with the stream length, or the `Encoding.GetString(ReadOnlySpan)` -overload over the stream's sequence. - ---- - ## Suggested order of attack 1. **Item 23** — full-text paging. Really a correctness fix that happens to also be faster: @@ -176,7 +101,9 @@ overload over the stream's sequence. engine must not carry state from one script into the next. **Profile before writing it**: the estimate that engine construction dominates a scripted content list is read off the loops, not taken from a trace. -3. **Items 28, 29, 30** — narrow, conditional or off the main path. + +Nothing else is outstanding. Everything cheap, every correctness-shaped finding and +everything in the stability category is closed. ---