From 46c0cf61f3c7590e00e103f7254d199c9fbd46f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Gro=C3=9F?= Date: Tue, 13 Oct 2026 15:44:14 +0200 Subject: [PATCH] fix(storage): refuse to overwrite a collection record this version cannot parse (#187) Follow-up to the #194 review: - SaveCollectionMetadata and DeleteCollectionMetadata now refuse a stored record that does not parse, with a NotSupportedException naming the collection, instead of skipping the feature check and overwriting it. An unparsable record made GetCollectionMetadata return null, so a dynamic open would save a fresh, empty record over it. EnsureCanReplaceRecord replaces TryReadRecord; the Save guard stays right after FindMetadataRecord, before the in-place step. The Delete guard uses LoadMetadataChain/FindMetadataRecord instead of GetCollectionMetadata, which skips unparsable records. - CollectionFeatures remarks: the index-type rule protects a new index type only while the older release keeps the index entry; 5.2.3's dynamic PersistIndexMetadata (index create/drop, vacuum with RebuildIndexes) drops the entry together with the section. - CollectionFeatures and ReadCollectionMetadata: new per-index data goes only in trailing sections, never inline in the index list, and why. - Test: a record cut after its name is neither overwritten nor dropped, the dynamic open refuses, and other collections stay usable. Co-Authored-By: Claude Opus 5.5 --- src/BLite.Core/Storage/CollectionFeatures.cs | 39 +++++++++-- .../Storage/StorageEngine.Collections.cs | 60 ++++++++++++----- tests/BLite.Tests/RequiredFeaturesTests.cs | 67 +++++++++++++++++-- 3 files changed, 138 insertions(+), 28 deletions(-) diff --git a/src/BLite.Core/Storage/CollectionFeatures.cs b/src/BLite.Core/Storage/CollectionFeatures.cs index be5ee42..38160e3 100644 --- a/src/BLite.Core/Storage/CollectionFeatures.cs +++ b/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. /// /// -/// 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. +/// +/// 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. +/// +/// +/// 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 +/// DynamicCollection.PersistIndexMetadata, which runs on every dynamic index create or drop and +/// on a vacuum that rebuilds indexes. +/// +/// +/// 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. ReadCollectionMetadata reads the +/// list by position; bytes it does not expect there shift everything after them, so the record no +/// longer parses. GetCollectionMetadata then returns null and a dynamic open treats the +/// collection as new and saves a fresh record over it, so this refusal never runs. This version's +/// SaveCollectionMetadata 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. +/// /// internal static class CollectionFeatures { @@ -109,6 +129,17 @@ internal static void EnsureCanWrite(CollectionMetadata metadata) "supports these features."); } + /// + /// The exception for a stored record of collection 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. + /// + 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 Unsupported(CollectionMetadata metadata, bool readOnly) { var result = new List(); diff --git a/src/BLite.Core/Storage/StorageEngine.Collections.cs b/src/BLite.Core/Storage/StorageEngine.Collections.cs index c1c6c10..15354dd 100644 --- a/src/BLite.Core/Storage/StorageEngine.Collections.cs +++ b/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. /// + /// + /// 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. + /// then returns null, a dynamic open treats the collection + /// as new and saves a fresh record over it, and the required-features refusal + /// () never runs. + /// 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) } } - /// Parses the collection record in , or returns null if it is malformed. - private static CollectionMetadata? TryReadRecord(byte[] page, SlotEntry slot) + /// + /// Throws unless this version may overwrite or drop the stored record of collection + /// in slot : the record must parse, and every + /// feature it requires must be known (see ). A record that does not + /// parse is refused too, since its features cannot be checked. + /// + /// The record cannot be parsed, or requires features this version lacks. + 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); + } + + /// + /// Like , for the first live record of collection + /// (the one replaces), if there is one. + /// + private void EnsureCanReplaceStoredRecord(string name) + { + var chain = LoadMetadataChain(); + var (page, slot) = FindMetadataRecord(chain, name); + if (page >= 0) + EnsureCanReplaceRecord(name, chain[page].Buffer, slot); } /// @@ -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; diff --git a/tests/BLite.Tests/RequiredFeaturesTests.cs b/tests/BLite.Tests/RequiredFeaturesTests.cs index 42d90b6..cd58976 100644 --- a/tests/BLite.Tests/RequiredFeaturesTests.cs +++ b/tests/BLite.Tests/RequiredFeaturesTests.cs @@ -120,12 +120,38 @@ public async Task RecordWithoutTheSection_AsOlderReleasesWriteIt_LoadsWithNoFeat /// Removes the last bytes of the collection's stored record. private void ShortenRecord(string collection, int bytes) + => SetRecordLength(collection, (length, _) => length - bytes); + + /// Cuts the collection's stored record to its name and the next bytes. + private void CutRecordAfterName(string collection, int bytes) + => SetRecordLength(collection, (_, nameLength) => nameLength + bytes); + + private void SetRecordLength(string collection, Func 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); + } + + /// The bytes of the collection's live stored record, or null if there is none. + 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(() => + storage.SaveCollectionMetadata(new CollectionMetadata { Name = "future" })); + Assert.Contains("'future'", ex.Message); + Assert.Throws(() => storage.DeleteCollectionMetadata("future")); + } + + using (var engine = new BLiteEngine(_dbPath)) + { + Assert.Throws(() => 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() {