Add object-log chunking and no-copy flushing - #2081
Draft
Ted Hart (TedHartMS) wants to merge 118 commits into
Draft
Ted Hart (TedHartMS) wants to merge 118 commits into
Ted Hart (TedHartMS) wants to merge 118 commits into
Conversation
…work, coming next)
…very guard Wires the FLUSH object-log format for values whose length exceeds the 24-bit RDH ValueLength field, fixes an intermittent reader-ring race, and guards the recovery verbatim-copy path against headered records. The no-copy flush optimization was attempted and reverted (see below). VALUE chunked-object encoding (RecordDataHeader/LogRecord/ObjectLogReader/Writer): - Object value <4MB: headerless exact length (bit 23 clear). >=4MB: chunked (bit 23 + 12-bit full-buffer count + 10-bit final-4KB-page count); reader sizes read-ahead from the extent, deserializer self-terminates; >16GB throws. - Overflow value <16MB: exact 24-bit. >=16MB: ValueLength sentinel + leading ChunkHeader (symmetric with the >=sentinel overflow key path). - SetObjectLogLengthHints / EncodeFlushObjectValue / DecodeFlushValueExtent. DiskLogRecord.SetChunkedFlushOverflowLengths (network/migration) unchanged. Reader-ring race fixes (CircularDiskReadBuffer/DiskReadBuffer): - ExtendUnreadLengthRemaining primes empty ring buffers using a reader-thread readIssued flag (not HasData/HasInFlightRead, which reads endPosition before the acquiring countdownEvent.IsSet and can re-read a primed buffer at the wrong offset -> short read); absorb sector-alignment over-read via readAheadSlack; DiskReadBuffer.Dispose drains in-flight reads before returning pooled memory (single-record read path disposes via using without OnEndReadRecords). Recovery snapshot-copy guard (ObjectAllocatorImpl): fail fast when copying a record whose overflow key/value is at/above the RDH sentinel (leading ChunkHeader) -- the keyHint+valueHint sizing would under-copy and truncate it. Chunked object values over-copy harmlessly (deserializer self-terminates). No-copy flush: attempted (useLivePage for ReadOnly full-page aligned flushes) and reverted. It passed the previously-failing SnapshotRecoveryDeferredObjectLoad (10/10) and full recovery (201) + recordops (6x256), but review found a real crash-recovery race: read-only-region records are not content-immutable (Upsert/RMW/Delete seal + dispose the superseded source in place down to HeadAddress), and a ReadOnly flush holds no epoch during the async device write, so writing the live page can persist torn/half-cleared bytes and mark the page durable while the superseding tail record is not. The srcBuffer copy isolates the write. A safe no-copy needs a per-page flush-in-progress freeze on seal/dispose; deferred. Doc section 7 records the full analysis. Tests: LargeObjectDiskWriteReadChunkedValue, LargeOverflowValueChunkedTest, RecordDataHeaderFlushEncodingTests. Validated: recordops 6x (256/4-skip), recovery suite (201), SnapshotRecoveryDeferredObjectLoad 10/10, format clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…y flush safe A no-copy flush attempt that made every mutator flip a superseded read-only record to Invalid atomically before clearing it (SealAndInvalidate then OnDispose, matching CreateNewRecordUpsert/RMW, incl. changing CreateNewRecordDelete) was implemented and reverted: it still doesn't close the race. The async device write (LocalStorageDevice.WriteAsync) reads the live page over the whole I/O duration, so it can read the old Valid RecordInfo at one instant and the concurrently-cleared body at a later instant -> a torn Valid record on disk. Invalidate-before-clear only helps an in-memory reader (one atomic word), not a byte-by-byte device read of a mutating buffer. A safe no-copy needs a per-page flush-in-progress freeze; documented in section 7. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…lush contract A plain ReadOnly, full-page, sector-aligned flush now writes the live main-log page directly to the device (useLivePage) instead of copying it into a srcBuffer; Snapshot/Recovery/partial/unaligned flushes still copy. The live records are stamped in place with the length hints + ObjectLogPosition, which is non-destructive to in-memory readers (the ValueLength property masks the raw field to ObjectIdSize; the objectId slot is untouched) and the page stays resident throughout the flush (HeadAddress <= FlushedUntilAddress). Correctness relies on the contract that OnDispose keeps a record byte-consistent (readable) throughout a flush -- it copies off what it needs for cleanup rather than tearing the record's flush-critical bytes -- so the async device write always observes a consistent record even if a concurrent Upsert/RMW/Delete supersedes it (the only other in-place mutation, Seal(), is a single atomic RecordInfo word). Output is byte-identical to the copy path (perf only). Doc section 7 records the contract and a caveat that ClearHeapFields must be made flush-safe to honor it for every store. Validated: recordops 256/4-skip; main Tsavorite.test 301/26-skip; recovery 201/9-skip (incl. SnapshotRecoveryDeferredObjectLoad 10/10); Garnet SeSaveRecover 17. Format clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…f copy The snapshot-region recovery verbatim copy sized each record by its RDH KeyLength/ValueLength hints, which cap at the sentinel and omit the leading 8-byte ChunkHeader + the length beyond the sentinel for an overflow key/value at/above its sentinel (>= 16 MB value, or a sentinel key) -- so such records were guarded with a fail-fast throw to avoid silent truncation. Now size such a headered record by the successor object record's snapshot position minus this record's: exactly this record's raw key+value+header+padding extent, copied verbatim (the reader re-frames from the record's own ChunkHeader and ignores any trailing over-copy, so this can never under-copy/truncate). This removes the guard for a headered record that has a successor object record on its page (the common case). A headered record that is the LAST object record on its page has no successor to bound it (its exact extent would need a ChunkHeader read up to the full overflow length away), so that narrow case stays guarded. Headerless records and chunked object values are unchanged (their hints are exact / safely over-read). Adds RecoverSnapshotHeaderedOverflowValue (verified to exercise the headered snapshot-copy path). Full recovery suite 202/9-skip; recordops 256/4-skip; no regression to the non-headered paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…on-diff Section 9 now documents that a >=-sentinel overflow key/value record is copied by the successor object record's snapshot-position difference (exact raw extent, never under-copies); only a headered record that is the last object record on its page remains guarded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
Introduce the new FLUSH out-of-line VALUE length encoding that will replace the bit-23-chunked/bit-22-overflow-header/24-bit-exact scheme: low 12 bits of the ValueLength field hold bits 0-9 payload (exact byte size if isExactSize bit 11, else 4KB-page count with 1023 sentinel) and bit 10 hasHeader. Values <=1023 bytes are headerless (isExactSize); longer values carry a leading ChunkHeader and a page-count encoding for a precise (no 4MB over-read) initial read. Pure-additive: new encoders/decoders/predicates and unit tests only; the old scheme and all callers are untouched, rewired in the next increment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
Switch the object-log overflow VALUE path from the old 24-bit-exact/16MB-sentinel encoding to the v2.2 12-bit encoding: values >1023 bytes now carry a leading ChunkHeader (was >=16MB) and the RDH holds a 4KB-page-count/sentinel read hint plus the has-header bit, giving precise reads (no 4MB over-read) and a header signal independent of the sentinel (prerequisite for DMA-padded overflow). Object values keep the current hint encoding (deferred unification); the reader value path is type-selected so the two coexist cleanly. Reader uses the flush-specific FlushValueHasHeader (not the shared ValueLengthIsSentinel, which the network/DiskLogRecord path still uses). ReadOverflowHeaderAndExtend now clamps the read-ahead extend to the shortfall (below-sentinel page-count hints cover the header, so no negative extend) and skips DMA alignment padding. Recovery successor-diff predicate keyed off FlushValuePageCountIsSentinel (only the sentinel under-counts; below-sentinel headered values over-count safely via the hint). Updated RecoverSnapshotHeaderedOverflowValue (trailing record now headerless at 1000B). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
Implement the zero-copy direct-DMA object-log write for overflow spans > MaxCopySpanLen (128KB), replacing the retained/disabled buggy WriteDirect. On-disk layout is [ChunkHeader][alignmentPadding][data]: the header + zero alignment padding + a small source-alignment initial fragment are copied through the sector-aligned buffer so the DMA disk offset lands on a sector boundary while the DMA source (the pinned byte[] data) is also sector-aligned; the sector-aligned interior is DMA'd straight from the byte[]; a small end fragment (plus any remainder past a 1GB segment boundary) is copied through the buffer. Fixes the two bugs in the retained code: (1) it aligned the disk offset but DMA'd from an 8-byte-aligned (unaligned) source -> now the initial fragment is sized to sector-align the SOURCE and header padding sector-aligns the disk offset; (2) the buggy recursive multi-segment path -> replaced with a single-segment DMA + buffered fallback for the rare cross-segment remainder. The alignment math is extracted to ObjectLogDmaAlignment.Compute (non-generic, unit-tested for fragment sizes 0/sectorSize-1/sectorSize/2). The value's alignment padding is threaded via ObjectLogWriter.lastValueAlignmentPadding into the RDH page-count read hint (SetObjectLogPositionAndLengthHints/SetObjectLogLengthHints) so it spans header+padding+data; the reader already reads the padding from the ChunkHeader and skips it. Validated: recordops 318, recovery 202, LargeObjectDiskWriteReadBigKeyAndValue 36 (multi-MB values now DMA'd), LargeOverflowValueChunkedTest (16-21MB DMA'd), all green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
Adds RecoverFoldOverHeaderedOverflowValue covering FoldOver recovery of multiple headered overflow values (below- and above-128KB DMA threshold), verifying the v2.2 overflow header framing survives FoldOver recovery's in-place object-byte reuse and per-record position reconstruction without drift or corruption. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…overflow Root cause: during Snapshot recovery of a downlevel (v2.1) checkpoint, the stable-boundary reposition path (SetRecoveredObjectLogRecordStartPosition) unconditionally clears the ReuseObjectIdForSize flag and marks the record in the current (v2.2) hint format, but does not insert the leading ChunkHeader that a large overflow key (>= the 1023 KeyLength sentinel) or overflow value (> kOutOfLineExactSizeCutoff) requires. The v2.1 object-log stream has no such header, so the v2.2 reader would consume 8 bytes of the value as a bogus header -- silent corruption. The other v2.1 recovery paths are already safe: FoldOver recovery does not re-flush the object log (it reads via the _v21 decode and upgrades lazily), and the Snapshot fuzzy-region verbatim-copy path (RepointObjectLogPosition) preserves the flag so the record stays downlevel. Only the reposition path converts, and only large overflow keys/values differ between v2.1 (dense) and v2.2 (headered) bytes -- everything else is byte-identical and repoints safely. Change: detect a downlevel source in the reposition path and, if it would convert to a headered overflow key/value, throw a clear "not yet implemented" exception instead of silently corrupting. SetDeserializedValueObject now preserves the flag bit across the deserialized-length store (masked back off where the length is read) so the source is still detectable for object values. All of this is a strict no-op for v2.2 sources (the flag is never set on a v2.2 record); verified by the still-green recovery (204), recordops (318), and main Tsavorite.test (301) suites. What NOT to do (future agents): do not implement the full v2.1->v2.2 header insertion (re-serialize-on-recovery, growing the object log) until there is a test fixture that can produce a v2.1 checkpoint -- new records are never written in v2.1, so the conversion is currently untestable and must not ship unvalidated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
….2 recovery Update §10 (Versioning & downlevel) to the now-resolved state: v2.2 = checkpoint version 8, v2.1 = version 7 (HybridLogRecoveryInfo), per-record bit-63 ReuseObjectIdForSize discriminator, and the full v2.1->v2.2 recovery path map (FoldOver lazy upgrade; Snapshot fuzzy verbatim-copy preserves the flag; only the Snapshot stable-boundary reposition converts and fails fast on a large overflow key/value that would need a ChunkHeader). Note the 1<<30 per-chunk cap and that full header-insertion conversion is untestable until a v2.1 fixture exists. Update §9 to correct the flag-bit handling (RepointObjectLogPosition preserves bit 63; SetRecoveredObjectLogRecordStartPosition clears it to convert) and add the v2.1 reposition guard. Remove a stray duplicate "## 9. Recovery & positions" header between §7 and §8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…age) The copy-vs-live-page comment in ObjectAllocatorImpl.WriteAsync now explains the core reason a Snapshot flush must copy: it serializes objects to a SEPARATE snapshotFileObjectLogDevice that is disposed right after the checkpoint and stamps records with positions in it, so stamping the live record (useLivePage) would point it into a soon-disposed file. Also notes the disk-image-only SetInvalid of v+1 CPR records, why ReadOnly no-copy is safe (main-tail position), and the open ReadOnly-flush-racing-snapshot question under investigation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…Page/ConvertToInline invariant Move object values onto the same 12-bit out-of-line ValueLength encoding as overflow (EncodeFlushOutOfLineValue: bits 0-9 exact/page-count, bit 10 hasHeader, bit 11 isExactSize, cutoff 1023), with per-chunk ChunkHeader framing for objects whose serialized length exceeds the cutoff. This frees RDH bits 12-23 for the future inline-portion+object record feature (the "53432153" decision) and gives precise read-ahead sizing for medium objects. On-disk headered-object layout: [1023-byte headerless prefix][hdr_1][chunk_1]...[hdr_N][chunk_N]. An object <= 1023 data bytes stays fully headerless (isExactSize). Each 8-byte ChunkHeader is written on an 8-aligned object-log position (so it never straddles a buffer/segment boundary) and its currentLength (| ContinuationFlag) is back-filled when the buffer fills; headers are appended (position advances monotonically), never slide-inserted. The reader strips the prefix/padding/ headers and follows the continuation chain, extending read-ahead per-chunk for sentinel objects. Recovery reconstructs the exact on-disk extent (prefix + padding + headers + data) via the reader. Key changes: - RecordDataHeader.SetObjectLogLengthHints: objects route through EncodeFlushOutOfLineValue with the writer-supplied on-disk extent (retiring EncodeFlushObjectValue for objects). - LogRecord: GetObjectLogRecordStartPositionAndLengths decodes objects via the 12-bit DecodeFlushValueInitialReadExtent; position/extent hints threaded on flush and recovery. - ObjectLogWriter: self-contained WriteObjectData buffer loop (headerless prefix + 8-aligned back-filled per-buffer ChunkHeaders), lastObjectExtent tracking, CopyRecoveredObjectBytes short-read. - ObjectLogReader: header-stripping ReadObjectData/AdvanceToNextObjectChunk; DoDeserialize stores the on-disk extent for recovery. - ObjectAllocatorImpl: pass the object extent on flush; snapshot verbatim-copy predicate uses FlushValueHasHeader; last-object-record fallback uses the RDH hint + allowShortRead. Fix a latent useLivePage bug this encoding exposed (LogField.ClearObjectIdAndConvertToInline): a no-copy (useLivePage) object-log flush stamps the flush ValueLength encoding into the LIVE record's RDH. Readers are unaffected (the ValueLength property masks the raw field to ObjectIdSize for out-of-line values), but record disposal (Delete/elision) flips the field to inline and previously "kept the current length", after which the property returns the raw stamped value. A 4-byte object stamps raw ValueLength = 2048|4 = 2052 (isExactSize bit), so the disposed record then claimed a 2052-byte inline value -> GetRecordLength/filler/scan-walk corruption (iteration over-counted deleted keys; compaction lost live records). Baseline's EncodeFlushObjectValue returned raw = 4 (== ObjectIdSize) for a 4-byte object, coincidentally preserving the invariant and masking the bug. Fix: ClearObjectIdAndConvertToInline now sets the converted field's RDH length to ObjectIdSize (the physical inline-slot size of the freed ObjectId slot) explicitly. No-op for SpanByte/normal in-memory records. Validated: Tsavorite recovery 204/0/9, recordops 318/0/4 (incl. Revivification 87/0/4), main 301/0/26; Garnet SeSaveRecover+RespObjectCommand 30/0/2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
Update sections 2-9 for the unified 12-bit out-of-line ValueLength encoding (objects now share EncodeFlushOutOfLineValue with overflow: isExactSize/hasHeader/page-count, cutoff 1023) and the headered-object on-disk framing ([1023-byte prefix][8-aligned back-filled per-buffer ChunkHeaders]). Document the useLivePage stamp invariant and the ClearObjectIdAndConvertToInline fix, the successor-diff + allowShortRead snapshot recovery sizing (removing the retired last-record fail-fast throw), and extend the v2.1 reposition guard note to object values. Retire references to the old bit-23-chunked scheme. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…ase-1 wire protocol) The aof-chunk branch must not introduce changes to AOF or Migrate/Replication; those belong to tedhar/aof-migrepl-chunk. Phase-2 (OAImpl object-log) had reworked the migration wire protocol into a "prefix-only-when->=sentinel" optimization built on the branch-added RDH overflow-length hints. This restores the Phase-1 "always send a 4-byte length prefix" protocol on aof-chunk so the cluster files are byte-identical to aof-migrepl-chunk. Reverted to aof-migrepl-chunk (Phase-1, always-prefix): - libs/cluster/Server/Migration/MigrateSessionCommonUtils.cs - libs/cluster/Server/Replication/PrimaryOps/DisklessReplication/ReplicationSnapshotIterator.cs - libs/cluster/Session/ChunkedRecordReassembler.cs Necessary leakage (shared file, Phase-1 migration serialization adapted to the Phase-2 LogRecord API): - libs/storage/Tsavorite/cs/src/core/Allocator/DiskLogRecord.cs SerializeChunked / SerializeInlinePortionForMigration now call SetObjectLogPositionAndLengthHints (renamed from SetObjectLogRecordStartPositionAndLength); the dead GetSerializedSize() wrapper is removed. The object value length is still left zero; the receiver derives it from the always-prefix wire format. The RDH-hint migration optimization can be re-added later as a separate change on top of migrepl-chunk/main. Validated (net10.0): recordops 318/0, recovery 204/0, cluster migrate 55/0, replication.disklesssync 30/0, replication 107/0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
…ffer_end-8 boundary
When the first post-prefix object ChunkHeader lands at exactly buffer_end - ChunkHeader.TotalSize
(RemainingCapacity == 8), the 8-byte header fills the write buffer with no data room. The writer
previously fail-fast threw ("not yet implemented"). It now pokes the placeholder header; the next
CopyObjectDataBytes sees a full buffer and AdvanceObjectBuffer back-fills it as a zero-length
continuation chunk (currentLength = 0 | ContinuationFlag), resuming the object data in the next
buffer. The reader (AdvanceToNextObjectChunk) already skips zero-length continuation chunks.
Only the first post-prefix header can hit this edge -- a fresh buffer always leaves at least one
sector for a header plus data. The PokeObjectChunkPlaceholder assert is relaxed to room >= 8
(room is always a multiple of 8, so the minimum non-zero value is 8).
Adds ObjectChunkZeroLengthFirstChunkTest: dense overflow byte-span fillers (each 8 + length
object-log bytes, no inter-record padding) position the object's start at buffer_end - (1023 + 8),
so the first 8-aligned header lands at buffer_end - 8. A small internal ObjectLogWriterDiagnostics
captures the first-header RemainingCapacity and the zero-length-chunk count so the test asserts the
boundary was actually hit; the object then round-trips from disk through the zero-length-chunk path.
Validated (net10.0): recordops 319/0, recovery 204/0, main Tsavorite.test 301/0.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69a17cc0-e986-4422-bd1d-fd0d0b8a2c37
ComputeRecoveryOverflowKeyHash created and disposed a CircularDiskReadBuffer per overflow-key record, renting its pooled object-log read buffer from the depot each time. Pass the ring by ref from RecoveryStatus.IsolatedKeyReadBuffers so it is created once per recovery and disposed with the RecoveryStatus. A ring is bound to one device, so it is replaced when the main and snapshot object logs alternate; recovery processes those in phases, so that is rare. Also document why this path reads through the streaming ring rather than issuing one direct read into a caller-allocated buffer: the record bounds a headered key by a 4 KB page count that rounds up past the last record's written extent, and a FoldOver Pass 1 has no durable object-log end to clamp against, so ReadDirect would fault on the resulting short read at end of file.
ObjectLogWriterDiagnostics wrote a process-shared static on every headered
object and incremented a shared counter non-atomically from flush threads.
- Move recording behind [Conditional("DEBUG")] entry points so a Release build
compiles out the calls and their arguments entirely, leaving no production
cost. The zero-length test moves into OnChunkBackfilled so Release carries
no comparison either, and the counter increment is interlocked.
- Guard the counter assertions in ObjectChunkZeroLengthFirstChunkTest with
#if DEBUG; its on-disk round-trip still runs in every configuration.
- Drop comments describing the superseded boundary-sector zeroing. The comments
now state only what the code does: bytes above the logical endpoint reach disk
verbatim and recovery never parses them.
A page whose objectIdMap is empty has no overflow keys, overflow values, or heap objects, so it has nothing to serialize to the object log. That shortcut applied only to ReadOnly, so an all-inline Snapshot page still built an ObjectLogWriter, rented a 4 MB object-log ring, and walked every record to write nothing. Two things kept Snapshot out of it: the shortcut wrote the full page rather than stopping at the checkpoint boundary, and it used the main-log file offset rather than the snapshot device's. Move the gate below the partial-range and alignedStartOffset computation so it can issue the same span the serializing path computes -- [alignedStartOffset, RoundUp(endOffset, sectorSize)) at alignedMainLogFlushPageAddress -- which resolves both. Snapshot page ordering and fuzzy rules still apply through the coordination callbacks, and the write sets snapshotDeviceWriteIssued so completion owns releasing the page. Recovery continues to fall through to record stamping; a string-only ObjectAllocator with no flush buffers takes the same direct write it did before.
…n, not per-record bit Recovery already knows the checkpoint metadata version (HybridLogRecoveryInfo.hybridLogRecoveryVersion). Thread it to the object-log page-reading and recovery-flush methods so the downlevel (v2.1) split/dense decode is selected from the version, not the per-record ObjectLogPosition ReuseObjectIdForSize flag (bit 63). - Add HybridLogRecoveryInfo.UsesDownlevelObjectLog(int) plus the ChunkFramedObjectLogCheckpointVersion boundary constant. - Carry the version on RecoveryOptions (read path) and PageAsyncFlushResult (flush path, defaulting to the current version), populated from recoveredHLCInfo.info.hybridLogRecoveryVersion; also on RecoveryStatus for the pass-2 object load and snapshot-recovery flush helpers. - GetObjectLogRecordStartPositionAndLengths, ReadRecordObjects, ReadOverflowKeyHashCodeForRecovery, CalculatePageObjectSizes, FindHeadAddressCutoffOnPage, LoadObjectsForRecoveryPass2, ComputeRecoveryOverflowKeyHash, DeserializeObjectsOnPage and AsyncFlushPagesForRecovery take the checkpoint version; runtime reads pass the current version. - Delete the SetDeserializedValueObject flag-preservation hack and drop the wasReuseObjectIdForSize read in SetRecoveredObjectLogRecordStartPosition (now reached only for a downlevel source, so its guard is unconditional). - Keep kReuseObjectIdForSizeBit defined (v7 files on disk still have it set) but document it as no longer read as the decode discriminator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a425332a-56d9-47ab-bc5f-c144d87b2e4f
…ry tests Synthesize real downlevel (checkpoint version 7) object-store checkpoints as a TEST UTILITY and recover them with the current reader, verifying every value/overflow-key byte-for-byte. There is no v7 page-generation code in production; the fixture takes a current-format FoldOver checkpoint of small (headerless) records — whose object-log bytes are already dense and byte-identical to the v7 dense encoding — and rewrites, on disk, only what distinguishes v7: - each out-of-line record's length is re-stamped into the v7 split form (RDH low bits + objectId-slot high bits) and its ObjectLogPosition word gets the ReuseObjectIdForSize flag (bit 63) with the current size-hint flags cleared, and - the metadata is re-serialized through HybridLogRecoveryInfo.ToByteArray(targetVersion: 7). The object-log bytes and record positions are left untouched, so no v7 object-log writer is needed. Tests (Tsavorite.test.recovery/V7DownlevelRecoveryTests.cs): - RecoverV7ObjectValueFoldOver: inline key + object value; exercises the v21 value decode. - RecoverV7OverflowKeyFoldOver: overflow key + overflow value; exercises the v21 key decode (Pass 1 overflow-key hash) and value decode. - V7FixtureHasDownlevelEncodingOnDisk: raw inspector (no production decoder) asserting metadata version 7 and bit 63 set / size-hint flags clear on every out-of-line record. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a425332a-56d9-47ab-bc5f-c144d87b2e4f
…v7 records Threading the checkpoint version through the recovery decode (previous commit) let a live object read decode a downlevel (v7) record as current format. Recovering a v7 checkpoint under memory pressure evicts records to the main log WITHOUT up-converting them, so those records remain in the v7 split-length encoding; a later runtime read (e.g. a pending Read of an evicted record) reached GetObjectLogRecordStartPositionAndLengths with the current version and mis-decoded the length, hanging on the bogus object-log read. Select the downlevel decode per record via a new LogRecord.IsDownlevelObjectLogRecord(checkpointVersion): true when the checkpoint version predates the chunk-framed format OR the per-record ReuseObjectIdForSize flag is set. Recovery still drives the decode from the metadata version; the flag additionally covers live reads of a downlevel record left on the main log by a memory-pressured downlevel recovery (a page can mix such records with up-converted ones, so the decision must be per record). GetObjectLogRecordStartPositionAndLengths and the object-log reader's isLegacy both use it. The bit therefore still carries its downlevel meaning and is not yet free for reuse; fully freeing it would require up-converting every record during a downlevel recovery. Adds V7DownlevelRecoveryTests.RecoverV7ObjectValueFoldOverLowMem, which recovers a v7 fixture under a tight LogSizeTracker budget (forcing recovery eviction) and reads every record back byte-for-byte; it hangs without this fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a425332a-56d9-47ab-bc5f-c144d87b2e4f
…ures The v7 fixtures covered only headerless records at or below the 511-byte exact-size cutoff, leaving the size class that v7 stored dense but the current format frames with a leading ChunkHeader unverified. Add a general dense-object-log v7 fixture writer and fixtures over it: - RecoverV7LargeObjectValueFoldOver at 513, 1024, 4096, and 20000 bytes - RecoverV7LargeOverflowValueFoldOver at 512, 1024, 4096, and 20000 bytes - RecoverV7OverflowKeyInlineValueFoldOver Test-only; no production change. A throw injected into the downlevel decoder fails 23 of the 24 v7 tests, the exception being the raw inspector that does not recover, confirming every recovery fixture reaches that decoder. Not covered: Snapshot v7 checkpoints, components above 4 MB, and multi-page v7 logs. The writer is single-page and its largest component is 20 KB, so the reader's multi-buffer dense read is unverified.
The checkpoint already records pageSize and segmentSize, but not the object-log segment size. That value is the split point for ObjectLogFilePositionInfo's packed segment/offset word, so recovering under a different one resolves every object-log position to a different segment and offset. Append it after segmentSize, under the existing v8 log-geometry guard, and include it in the metadata checksum. A v7 checkpoint writes neither trailing value and passes zero for both, which reduces the checksum to the v7 formula.
PageSize and SegmentSize determine how logical addresses resolve to segment files and offsets, and ObjectLogSegmentSize splits the packed object-log position word. Recovering a checkpoint under different values reads the wrong bytes with nothing to detect it, so compare all three against the recorded geometry and fail recovery on a mismatch. Checkpoints below v8 record no geometry and are skipped; the assumed values are logged so an operator can confirm them against the original configuration.
A downlevel object log stores records headerless above the size that now requires a chunk header, so its object bytes cannot be rewritten in place. UpgradeObjectLogDevice receives those bytes in current format while the original object log is read as the source. Recovering a downlevel checkpoint that has an object-log device now fails when no upgrade device is configured, rather than silently leaving the log in a format a later release cannot decode. A store with no object-log device has nothing to convert and is unaffected.
Populates KVSettings.UpgradeObjectLogDevice with a device alongside the object log it will replace, so recovery has somewhere to write object bytes in current format. Requires --recover, since a checkpoint must be read to have anything to convert, and tiered storage, since without it there is no object log on disk. A store with objects disabled needs no conversion and gets no upgrade device.
Recovery of a v7 checkpoint now rewrites the log into the current format instead of leaving records readable only through the per-record downlevel decode path. A dedicated ascending pass runs inside the hybrid-log read loop: each page is read, indexed, and its objects deserialized, then flushed with its objects re-serialized through the normal writer path -- which inserts the ChunkHeaders the current format requires -- into KVSettings.UpgradeObjectLogDevice. Main-log pages are rewritten in place because record inline sizes are unchanged; a per-record assertion enforces that invariant. Pages convert before any eviction can observe them, so no page is evicted still holding downlevel object-log positions. The v7 tail stays in objectLogTail so reads of the downlevel device remain correctly bounded while conversion appends to a separate upgradeObjectLogTail. On completion the device is swapped and SetObjectLogTail adopts the conversion's append position rather than the checkpoint's downlevel tail.
…-log position A page read from a downlevel checkpoint arrives with its PageHeader already carrying an object-log position on the device the up-conversion is replacing. WriteAsync stamped the header with objectLogTail through SetLowestObjectLogPosition, which was doubly wrong during conversion: - SetLowestObjectLogPosition only writes when the word is NotSet, so the call silently no-opped and the converted page kept its downlevel position. - objectLogTail is deliberately unset during the hybrid-log phase; the conversion appends to upgradeObjectLogTail. GetLowestObjectLogSegmentInUse derives truncation segments from these headers, so a converted log would have driven TruncateUntilSegment and RemoveSegment with downlevel segment numbers against the new device, silently discarding live object data. Add a force parameter to SetLowestObjectLogPosition and use it with upgradeObjectLogTail while upgrading, so each converted page is re-stamped with the position on the new device where that page's objects are written. lowestObjectLogSegmentInUse needs no reset: conversion appends from segment 0 and the field is only raised by truncation, which cannot run during the recovery read phase. Assert that. This also settles the SegmentSizeBits TODO in GetLowestObjectLogSegmentInUse - recovery now verifies the checkpoint's ObjectLogSegmentSize against the store's settings, so the live tail's segment bits are the bits the header was stamped under. The on-disk assertions in V7DownlevelRecoveryTests could not see either defect because the fixture fit on one main-log page, where both tails are position 0. Make the fixture transform and the on-disk walks page-aware via a shared GetOnDiskRecordOffsets helper that skips PageHeaders and end-of-page filler, grow RecoverV7UpConvertsMainLogOnDisk to span pages, and assert each page header agrees with the first object record on that page. Both defects were confirmed to fail this test before the fix.
Snapshot recovery of a downlevel checkpoint has two regions to convert, with unrelated object-log address spaces: the hybrid-log region, whose objects are in the main object log, and the snapshot region, whose objects are in the snapshot object log. Only the first was converted; the snapshot region took the verbatim snapshot-to-main object copy, which writes dense downlevel bytes into a current-format object log and leaves the record flagged downlevel. That reads back correctly today only because the per-record ReuseObjectIdForSize bit still selects the downlevel decode, which is exactly the bit being freed. Convert the snapshot region the same way the hybrid-log region is converted: an ascending per-page pass inside the snapshot read loop deserializes the page's objects from the downlevel snapshot object log, then flushes the page to the main log, re-serializing those objects onto the upgrade device with ChunkHeader framing where the current format requires it. Converting per batch keeps every page converted before the next batch's read can evict it, since an evicted page would keep its snapshot object-log positions. The conversion pass supersedes the deferred object-load pass, which has nothing left to load. In WriteAsync the up-conversion branch now precedes the snapshot verbatim copy, since a converted snapshot page has its objects resident and must be re-serialized rather than copied. The conversion now spans both recovery phases, so it stays open across them. BeginObjectLogUpgrade moves above the empty-range early return - a snapshot recovery whose hybrid-log region is empty still has a snapshot region to convert, and without this the whole snapshot path silently kept the old behavior. CompleteObjectLogUpgrade moves to the end of the snapshot phase and re-adopts the converted tail, because recovery sets the tail between the two phases so the snapshot region appends after the hybrid-log objects. Tests: three v7 Snapshot fixtures - snapshot-only, snapshot-only under a small memory budget that forces eviction, and a mixed fixture with both a non-empty hybrid-log region and a non-empty snapshot region - plus an on-disk assertion that the converted main log carries no downlevel records. Disabling the conversion pass fails all eight snapshot tests; dropping the cross-phase tail re-adoption corrupts the object log.
…level selector IsDownlevelObjectLogRecord ORed the per-record ReuseObjectIdForSize flag (bit 63) into the version-based decode selection, so a live read of a record that a memory-pressured downlevel recovery had evicted without up-converting would still decode. Recovery now up-converts every record of a downlevel checkpoint, across both the hybrid-log and snapshot regions, before any page can be evicted or flushed, so a live read can no longer encounter a downlevel record. Select the decode from the checkpoint metadata version alone, which frees bit 63: nothing in production reads or writes it, and only tests that synthesize v7 record images still touch it. Delete SetRecoveredObjectLogRecordStartPosition and its caller. It up-converted a downlevel record's encoding in place without re-serializing, and threw for any large overflow or object that the current format frames with a leading ChunkHeader. The up-conversion pass re-serializes instead, inserting that framing, so the in-place path is unreachable. Its call site becomes a throw, since a downlevel record reaching the recovery flush unconverted would otherwise persist a page later releases cannot decode. With the per-record fallback gone the existing low-memory v7 tests become discriminating rather than incidental: disabling the conversion makes RecoverV7ObjectValueFoldOverLowMem read a garbage extent off disk and hang, where before it passed either way.
…upgrade Recovery up-converts a downlevel object log into the upgrade device and repoints the live allocator at it, but that only lasts for the process: the files on disk still have the downlevel object log under the live name, and the recovered checkpoint still describes it. Finish the upgrade on disk. GarnetServer.RunUpgrade recovers, takes the checkpoint that records the converted object-log positions, closes the store, and renames the converted object log in as the live one. Program.Main routes an --upgrade run to it instead of Start, so the process converts and exits rather than serving. The swap is 2N file moves, so ObjectLogUpgradeSwap journals it: a marker naming the live, upgrade, and retired base names is written before the first move and deleted after the last. Every move is "move only if the source exists and the destination does not", so finishing an interrupted swap is just re-running it. A marker found at startup is resolved before any device opens the object log: an --upgrade run completes it, and any other run refuses to start rather than open a half-renamed object log. The downlevel segments are retained under a timestamped base name. Guards, each of which otherwise loses data silently: - Swap only when recovery actually converted something (LogAccessor.ObjectLogWasUpgraded). On a store already in the current format the upgrade device holds no data, so swapping it in would replace a good object log with an empty one. - Refuse --upgrade when the upgrade device already has segments from a previous attempt, since it is written from its start and the two runs would interleave. - Refuse --upgrade with the append-only file enabled. The run recovers, checkpoints, and exits; an AOF around that would be re-stamped at a version the next normal recovery discards. Tests cover the guards, the journaled rename including resume, and an end-to-end upgrade run against a current-format store that must leave every durable object-log byte in place. Dropping the ObjectLogWasUpgraded guard retires and replaces that store's object log.
Add the option to the configuration table, note the AOF restriction and the already-current no-op in defaults.conf, and describe the run: what it rewrites, where the original object log is retained, and how to finish an interrupted rename.
An upgrade run recovers through StoreWrapper.RecoverAsync, which already replays the AOF, so the version arithmetic works out: the run recovers a checkpoint at version V and runs at V+1, the post-checkpoint AOF records are also at V+1 and so are applied rather than skipped, and the checkpoint a converting run takes captures them. A later start recovers that checkpoint, runs at V+2, and correctly skips the whole AOF as already checkpointed. A run that converts nothing takes no checkpoint, leaving those records in the AOF to be replayed again, which is equally correct. Drop the refusal. Tests: an upgrade run with AOF enabled must preserve both checkpointed and AOF-only writes, and the recover-replay-checkpoint-reopen sequence that a real conversion depends on is covered separately. Skipping the AOF commit in the fixture loses exactly the sixteen AOF-only keys, so the assertions distinguish the two classes of loss. Unresolved: while developing these tests, an upgrade-with-AOF run intermittently tripped the CircularDiskReadBuffer assertion "Increment 47441 must be less than SectorSize (512)" -- twice in six runs of the AOF test, and in three consecutive runs of the fixture. It has not reproduced since, over nineteen runs spanning the same code and invocations, so no cause has been established and nothing here claims to fix it. The object-log read position it reports is far beyond the buffer's ongoing position, which is worth chasing if it resurfaces.
Recovery scans down only to the checkpoint's headAddress: colder pages stay on disk and are never read. The object-log up-conversion inherited that window, so on any store larger than memory most records were left in the downlevel encoding while the object log they point into was retired and replaced. Nothing in the existing tests could see this, because their fixtures are small enough that the recovery window is the whole log. An end-to-end Garnet store with a 400-record FoldOver checkpoint left 300 of those records unconverted. Those records are then unreadable: the per-record downlevel selector is gone, so a live read decodes downlevel bytes with the current reader and gets a garbage extent. This is the most likely source of the intermittent "Increment must be less than SectorSize" assertion noted in the previous commit. Scan from the checkpoint's beginAddress when up-converting. The conversion pass already evicts each page as it walks up, so the cost is IO rather than memory. Also dispose KVSettings.UpgradeObjectLogDevice in GarnetDatabase.Dispose. It was the one device the database left open, so the converted object log could never be renamed into place -- the rename failed with a sharing violation that no amount of retrying would clear. Bound the rename's retry anyway, for the handle lag that outlives a device Dispose. Tests: UpgradeConvertsDownlevelStoreEndToEnd takes a real Garnet store through the whole feature -- rewrite its checkpoint on disk as downlevel, run --upgrade, then start normally and read every key -- with and without the append-only file. It asserts that no record is left in the downlevel encoding, that the downlevel object log is retired and the converted one promoted, and that no rename marker survives. FoldOverCheckpointObjectsSurviveDiskReads is the control that isolates failures to the up-conversion rather than to FoldOver object recovery.
…Ranges The override that makes an up-conversion scan the whole log rather than the preload window sat in InternalRecoverAsync, after SetRecoveryPageRanges had already computed and clamped the same fields. That silently discarded the FlushedUntilAddress clamp. Compute it with the rest of the page-range logic instead, so every field that decides what recovery reads is derived in one place. The clamp is now skipped only for an up-conversion, and says why: being durable in this process says nothing about the on-disk encoding, so skipping those pages would leave downlevel records behind -- the same defect the override exists to prevent. Also cover Snapshot checkpoints in the end-to-end upgrade test. It previously ran only FoldOver, which is not Garnet's default, so the configuration most stores use was untested. The fixture's on-disk v7 rewrite now handles both: FoldOver keeps every record on the main log, while Snapshot splits them between the main log below mainLogRecoveryEndAddress and the snapshot file above it. Skipping the snapshot-region rewrite breaks the Snapshot cases, so the new coverage is load bearing. Its control test is parameterized to match.
No caller ever passed RecoveryPhase.Pass2: recovery reads pages as Pass1 and the post-recovery head read as None, and Pass 2 objects are loaded separately through LoadObjectsForRecoveryPass2. The enum member and everything reachable only from it are dead. Drop the Pass2 branch in AsyncReadPagesForRecovery, which was the only code that attached read buffers to a recovery page read, along with the buffer cleanup in its catch and the objectLogDevice parameter that only that branch used. The read-buffer field itself stays: the scan iterators still set it through AsyncReadPageFromDeviceToFrame. In the object-allocator read callback, selecting the page's objectIdMap was also reachable only for Pass2 -- with None and Pass1 the only remaining phases, the branch always resolved to the transient map. Use it directly.
…tore Cluster startup recovers through the replication manager, which dispatches on the node's role: a replica does not read its checkpoint at all unless ClusterReplicaResumeWithData is set. An upgrade run routed through that path would convert nothing and then report success, leaving a downlevel store that the next release cannot read. Give the upgrade its own recovery entry point that performs the standalone sequence unconditionally -- recover checkpoint, recover and replay AOF -- so it examines the store whatever the node's role. The run never serves requests or establishes replication, so the role has no bearing on what has to be converted on disk. This path has no automated end-to-end test. RunUpgrade itself was observed to complete in cluster mode, but the test then hangs in teardown: something the cluster server holds makes directory enumeration fail, and TestUtils.DeleteDirectory retries that in an unbounded loop regardless of its wait argument. Both of those are worth fixing, neither is the conversion logic, and a hanging test is worse than none. The documentation says the path is untested and tells the operator what to check. Refuse to up-convert a multi-database store. Every database has its own checkpoint directory but they all share one object log, since the device is built from a fixed "Store"/"hlog_objs" descriptor with no database id. Each database's allocator therefore converts into the shared upgrade device from its own position zero, and the second one overwrites the first. Fail before the rename, while the live object log is still untouched, rather than promote a log missing a database's objects. Sharing one hlog across databases already loses data without any upgrade involved: two databases checkpointed with tiered storage recover with one of them empty. That is pre-existing -- the device descriptor is unchanged from before this branch -- and needs the log devices made per-database, as the checkpoint and AOF directories already are.
…zing metadata GarnetClusterCheckpointManager overrides GetLogCheckpointMetadata to normalize legacy trailing-cookie metadata into the embedded-cookie layout, parsing the on-disk bytes and re-serializing them. It used ToByteArray(), which always stamps the current CheckpointVersion. While only one version was recoverable that round-trip was a no-op, but now that recovery accepts a downlevel checkpoint it silently relabels one as current: UsesDownlevelObjectLog then returns false and a dense object log is decoded as chunk-framed. Add ToByteArrayPreservingVersion and use it from both re-serialization sites, so a checkpoint read on a cluster node keeps the version it was written with. Also report that version in the log-geometry mismatch messages, which is what surfaced this.
Recovery errors are swallowed by default so a server can still come up on whatever it recovered. An upgrade run must not do that: it would report that nothing needed converting and then rename a partially converted object log into place. Set FailOnRecoveryError for the duration of the upgrade recovery.
Move the v7 checkpoint synthesis and verification helpers out of ObjectLogUpgradeTests into a shared V7CheckpointFixture parameterized by the store directory, link it into Garnet.test.cluster, and add an end-to-end cluster test that runs --upgrade over a downlevel store across the AOF and FoldOver/Snapshot axes. It asserts that the records were converted, that the downlevel object log was retired with no swap left pending, and that node identity, slot assignment and all data survive the restart.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--upgrademode that up-converts a downlevel (v7) object log to the current chunk-framed format, for FoldOver and Snapshot checkpoints, with or without the append-only file, standalone or on a cluster node. Conversion writes to a separate upgrade object-log device and swaps it in only after a checkpoint has made every converted position durable.pageSize,segmentSizeandobjectLogSegmentSizeinHybridLogRecoveryInfoand verifies them against the store's configuration on recovery.To be done
OnDisposecopy-off/deferred-layout work required by the no-copy flush contract. The live flush-thread defect is fixed (object-log flush captures a record's out-of-line components before consulting its state, and rechecks the liveRecordInfoafterwards); what remains is deferring physical cleanup for records inside an in-flight flush window.snapshotFinalLogicalAddresswithpageSize, appendsegmentSizetoHybridLogRecoveryInfo, preserve downlevel metadata, and complete guarded v2.1 recovery.Known limitations