fix(storage): refuse to overwrite a collection record this version cannot parse (follow-up to #194)

src/BLite.Core/Storage/CollectionFeatures.cs

@@ -43,10 +43,30 @@ internal sealed record RequiredFeature(string Id, FeatureRequirement Requirement
 /// know refuses to open or modify the collection instead of silently damaging it.
 /// </summary>
 /// <remarks>
-/// Only versions that contain this check can refuse; releases before it ignore the record and may
-/// also drop it when they rewrite the collection's metadata. An index of a type newer than any this
-/// version knows is refused the same way, so a new index type stays protected even after such a
-/// release dropped the record.
+/// <para>
+/// Only versions that contain this check can refuse. Releases before it ignore the section and drop
+/// it whenever they rewrite the collection's metadata, so a database that uses a feature must not be
+/// opened for writing by them.
+/// </para>
+/// <para>
+/// An index of a type newer than any this version knows is refused the same way. That protects a new
+/// index type after such a release rewrote the record only if that release kept the index entry, as
+/// its typed collections and its saves that start from the stored record (sequence values, root page
+/// ids) do. A release that rebuilds the index list from the indexes it loaded drops the entry along
+/// with the section, and then nothing is left to refuse: in 5.2.3 that is
+/// <c>DynamicCollection.PersistIndexMetadata</c>, which runs on every dynamic index create or drop and
+/// on a vacuum that rebuilds indexes.
+/// </para>
+/// <para>
+/// Format rule: data a future version needs per index goes only in a trailing section of the record
+/// (as the key formats do), never inline in the index list. <c>ReadCollectionMetadata</c> reads the
+/// list by position; bytes it does not expect there shift everything after them, so the record no
+/// longer parses. <c>GetCollectionMetadata</c> then returns <c>null</c> and a dynamic open treats the
+/// collection as new and saves a fresh record over it, so this refusal never runs. This version's
+/// <c>SaveCollectionMetadata</c> refuses to overwrite a stored record it cannot parse, but that still
+/// turns the newer version's clear refusal into a generic one, and every release before it overwrites
+/// the record.
+/// </para>
 /// </remarks>
 internal static class CollectionFeatures
 {
@@ -109,6 +129,17 @@ internal static void EnsureCanWrite(CollectionMetadata metadata)
             "supports these features.");
     }
 
+    /// <summary>
+    /// The exception for a stored record of collection <paramref name="name"/> that this version
+    /// cannot parse, e.g. one a newer version wrote in a layout this one does not know. Such a record
+    /// is never overwritten or dropped, since its features cannot be checked.
+    /// </summary>
+    internal static NotSupportedException UnreadableRecord(string name, Exception cause)
+        => new(
+            $"The stored metadata of collection '{name}' cannot be read by BLite {s_version}; it may have " +
+            "been written by a newer version, or be damaged. It was left unchanged; use the BLite version " +
+            "that wrote it.", cause);
+
     private static List<RequiredFeature> Unsupported(CollectionMetadata metadata, bool readOnly)
     {
         var result = new List<RequiredFeature>();

src/BLite.Core/Storage/StorageEngine.Collections.cs

@@ -180,6 +180,15 @@ public sealed partial class StorageEngine
     /// section after the index list is optional and read only while bytes remain, so records written
     /// by older releases load with defaults for the sections they lack.
     /// </summary>
+    /// <remarks>
+    /// Format rule: new data goes only in new trailing sections, after the last one read here; new
+    /// per-index data too (as the key formats do), never inline in the index list. The list is read by
+    /// position, with the vector fields as the only per-type extra, so a version that finds bytes it
+    /// does not expect there misreads the rest of the record and fails to parse it.
+    /// <see cref="GetCollectionMetadata"/> then returns <c>null</c>, a dynamic open treats the collection
+    /// as new and saves a fresh record over it, and the required-features refusal
+    /// (<see cref="CollectionFeatures"/>) never runs.
+    /// </remarks>
     private static CollectionMetadata ReadCollectionMetadata(string name, BinaryReader reader)
     {
         var metadata = new CollectionMetadata { Name = name };
@@ -406,14 +415,11 @@ public void SaveCollectionMetadata(CollectionMetadata metadata)
             throw new InvalidOperationException("Page 1 is not a collection metadata page.");
         var (foundPage, foundSlot) = FindMetadataRecord(chain, metadata.Name);
 
-        // Never rewrite the record of a collection that requires features this version lacks
-        // (see CollectionFeatures). Checked before every write path, the in-place one included.
-        // Nothing has been written yet.
-        if (foundPage >= 0
-            && TryReadRecord(chain[foundPage].Buffer, ReadSlot(chain[foundPage].Buffer, foundSlot)) is { } stored)
-        {
-            CollectionFeatures.EnsureCanWrite(stored);
-        }
+        // Never rewrite the record of a collection that requires features this version lacks, or
+        // one it cannot parse (see CollectionFeatures). Checked before every write path, the
+        // in-place one included. Nothing has been written yet.
+        if (foundPage >= 0)
+            EnsureCanReplaceRecord(metadata.Name, chain[foundPage].Buffer, foundSlot);
 
         // ── 1. No larger than the stored record: overwrite it where it is ─────
         // A single page write. Covers the frequent saves (sequence values, root page ids) whose
@@ -654,19 +660,40 @@ private static void WriteSlot(byte[] page, int slotIndex, SlotEntry slot)
         }
     }
 
-    /// <summary>Parses the collection record in <paramref name="slot"/>, or returns <c>null</c> if it is malformed.</summary>
-    private static CollectionMetadata? TryReadRecord(byte[] page, SlotEntry slot)
+    /// <summary>
+    /// Throws unless this version may overwrite or drop the stored record of collection
+    /// <paramref name="name"/> in slot <paramref name="slotIndex"/>: the record must parse, and every
+    /// feature it requires must be known (see <see cref="CollectionFeatures"/>). A record that does not
+    /// parse is refused too, since its features cannot be checked.
+    /// </summary>
+    /// <exception cref="NotSupportedException">The record cannot be parsed, or requires features this version lacks.</exception>
+    private static void EnsureCanReplaceRecord(string name, byte[] page, int slotIndex)
     {
+        var slot = ReadSlot(page, slotIndex);
+        CollectionMetadata stored;
         try
         {
             using var ms     = new MemoryStream(page, slot.Offset, slot.Length, false);
             using var reader = new BinaryReader(ms);
-            return ReadCollectionMetadata(reader.ReadString(), reader);
+            stored = ReadCollectionMetadata(reader.ReadString(), reader);
         }
-        catch
+        catch (Exception ex)
         {
-            return null;
+            throw CollectionFeatures.UnreadableRecord(name, ex);
         }
+        CollectionFeatures.EnsureCanWrite(stored);
+    }
+
+    /// <summary>
+    /// Like <see cref="EnsureCanReplaceRecord"/>, for the first live record of collection
+    /// <paramref name="name"/> (the one <see cref="SaveCollectionMetadata"/> replaces), if there is one.
+    /// </summary>
+    private void EnsureCanReplaceStoredRecord(string name)
+    {
+        var chain = LoadMetadataChain();
+        var (page, slot) = FindMetadataRecord(chain, name);
+        if (page >= 0)
+            EnsureCanReplaceRecord(name, chain[page].Buffer, slot);
     }
 
     /// <summary>
@@ -680,10 +707,9 @@ public void DeleteCollectionMetadata(string name)
             throw new TimeoutException("Timed out acquiring metadata lock (DeleteCollectionMetadata).");
         try
         {
-        // Never drop the record of a collection that requires features this version lacks
-        // (see CollectionFeatures).
-        if (GetCollectionMetadata(name) is { } stored)
-            CollectionFeatures.EnsureCanWrite(stored);
+        // Never drop the record of a collection that requires features this version lacks, or one
+        // it cannot parse (see CollectionFeatures).
+        EnsureCanReplaceStoredRecord(name);
 
         var buffer = new byte[PageSize];
         uint pageId = 1;

tests/BLite.Tests/RequiredFeaturesTests.cs

@@ -120,12 +120,38 @@ public async Task RecordWithoutTheSection_AsOlderReleasesWriteIt_LoadsWithNoFeat
 
     /// <summary>Removes the last <paramref name="bytes"/> bytes of the collection's stored record.</summary>
     private void ShortenRecord(string collection, int bytes)
+        => SetRecordLength(collection, (length, _) => length - bytes);
+
+    /// <summary>Cuts the collection's stored record to its name and the next <paramref name="bytes"/> bytes.</summary>
+    private void CutRecordAfterName(string collection, int bytes)
+        => SetRecordLength(collection, (_, nameLength) => nameLength + bytes);
+
+    private void SetRecordLength(string collection, Func<int, int, int> newLength)
     {
         using var storage = new StorageEngine(_dbPath, PageFileConfig.Default);
-        var page = new byte[storage.PageSize];
+        var (pageId, page, slotOffset, slot, nameLength) = FindRecord(storage, collection)
+            ?? throw new InvalidOperationException($"No record for '{collection}'.");
+        slot.Length = (ushort)newLength(slot.Length, nameLength);
+        slot.WriteTo(page.AsSpan(slotOffset));
+        storage.WritePageImmediate(pageId, page);
+    }
+
+    /// <summary>The bytes of the collection's live stored record, or <c>null</c> if there is none.</summary>
+    private byte[]? RecordBytes(string collection)
+    {
+        using var storage = new StorageEngine(_dbPath, PageFileConfig.Default);
+        return FindRecord(storage, collection) is { } record
+            ? record.Page.AsSpan(record.Slot.Offset, record.Slot.Length).ToArray()
+            : null;
+    }
+
+    private static (uint PageId, byte[] Page, int SlotOffset, SlotEntry Slot, int NameLength)? FindRecord(
+        StorageEngine storage, string collection)
+    {
         uint pageId = 1;
         while (pageId != 0)
         {
+            var page = new byte[storage.PageSize];
             storage.ReadPage(pageId, null, page);
             var header = SlottedPageHeader.ReadFrom(page);
             for (ushort i = 0; i < header.SlotCount; i++)
@@ -135,15 +161,11 @@ private void ShortenRecord(string collection, int bytes)
                 if ((slot.Flags & SlotFlags.Deleted) != 0) continue;
                 using var reader = new BinaryReader(new MemoryStream(page, slot.Offset, slot.Length, false));
                 if (reader.ReadString() != collection) continue;
-
-                slot.Length -= (ushort)bytes;
-                slot.WriteTo(page.AsSpan(offset));
-                storage.WritePageImmediate(pageId, page);
-                return;
+                return (pageId, page, offset, slot, (int)reader.BaseStream.Position);
             }
             pageId = header.NextOverflowPage;
         }
-        throw new InvalidOperationException($"No record for '{collection}'.");
+        return null;
     }
 
     // ── Typed collections ────────────────────────────────────────────────────
@@ -267,6 +289,37 @@ public async Task Storage_RefusesToRewriteDeleteOrFree_ACollectionWithAnUnknownF
         Assert.Equal(new[] { FutureIndex }, stored.RequiredFeatures);
     }
 
+    [Fact]
+    public async Task RecordThisVersionCannotParse_IsNeitherOverwrittenNorDropped()
+    {
+        // E.g. a record a newer version extended inline: this version cannot read it, so it cannot
+        // check its features either. GetCollectionMetadata returns null for it, so without the
+        // refusal a dynamic open would save a fresh, empty record over it.
+        await SeedDynamicAsync("future", "items");
+        CutRecordAfterName("future", 6); // the primary root page id and half the schema root page id
+        var stored = RecordBytes("future");
+
+        using (var storage = new StorageEngine(_dbPath, PageFileConfig.Default))
+        {
+            Assert.Null(storage.GetCollectionMetadata("future"));
+            var ex = Assert.Throws<NotSupportedException>(() =>
+                storage.SaveCollectionMetadata(new CollectionMetadata { Name = "future" }));
+            Assert.Contains("'future'", ex.Message);
+            Assert.Throws<NotSupportedException>(() => storage.DeleteCollectionMetadata("future"));
+        }
+
+        using (var engine = new BLiteEngine(_dbPath))
+        {
+            Assert.Throws<NotSupportedException>(() => engine.GetOrCreateCollection("future"));
+
+            var items = engine.GetOrCreateCollection("items");
+            await items.InsertAsync(engine.CreateDocument(["name"], b => b.AddString("name", "items-2")));
+            Assert.Equal(2, await items.FindAllAsync().CountAsync());
+        }
+
+        Assert.Equal(stored, RecordBytes("future"));
+    }
+
     [Fact]
     public async Task IndexOfATypeThisVersionDoesNotKnow_IsRefused_EvenWithoutARecordedFeature()
     {