feat: SolidSyslogBlockDevice abstraction with file-backed driver (S18.02)#243
Merged
Conversation
Adds the SolidSyslogBlockDevice abstraction (consumer header, vtable header, thin dispatch). Block-indexed I/O with Acquire/Dispose lifecycle, Exists for startup scan, Read/Append/WriteAt for record I/O, Size for resume. No driver yet; that lands in the next slice. Part of S18.02 (#235).
…ence-numbered names Adds SolidSyslogFileBlockDevice — the file-backed implementation of the BlockDevice abstraction. Acquire creates an empty file at <prefix><NN>.log; Dispose deletes the file; Read/Append/WriteAt/Size operate on whichever block the caller addresses. Internally the driver caches one read handle and one write handle, lazily reopening when the requested block changes. Storage is integrator-allocated (SolidSyslogFileBlockDeviceStorage opaque blob, mirrors SolidSyslogFormatterStorage). No callers yet — BlockSequence still talks to SolidSyslogFile directly. That repointing lands in the next slice. Part of S18.02 (#235).
…e; FileStoreConfig takes blockDevice
Repoints the internal BlockSequence and RecordStore layers onto SolidSyslogBlockDevice
and flips the public SolidSyslogFileStoreConfig surface to match.
Public API changes:
- SolidSyslogFileStoreConfig swaps readFile / writeFile / pathPrefix for one
blockDevice field. Integrators construct a SolidSyslogFileBlockDevice
separately and pass it in. The file handle ownership boundary moves outside
FileStore — Destroy no longer closes files; the integrator destroys the
block device when finished.
- FileFake_FailNext{Open,Write,Read} now take the target file as an argument.
The dual-handle test setup needs to specify which fake to fail.
Internal-only changes:
- BlockSequenceConfig and BlockSequence drop readFile/writeFile/pathPrefix and
the FormatFilename helper (now in FileBlockDevice). They hold one
blockDevice pointer instead.
- RecordStore_{Append,Read,MarkLastReadAsSent,FindFirstUnsent} signatures
swap (file) for (blockDevice, blockIndex). RecordStore remembers the block
index of the last-read record for MarkLastReadAsSent.
- BlockSequence_Open: scan via Exists; Acquire(0) on cold start; Size on
resume. The implicit O_CREAT-via-Open behaviour is now an explicit Acquire.
Behaviour notes:
- Lazy open/close of the underlying file is now FileBlockDevice's
responsibility. EnsureHandleOpenOnBlock consults SolidSyslogFile_IsOpen as
the source of truth so it tolerates external close (test corruption helpers
exploit this).
- The fast-fail-on-Open semantics that the old FileStoreErrors group asserted
are gone — a transient open failure during Create heals on the next op.
This is strictly better behaviour. Tests updated to assert the recovery.
S3 and S4 from the original plan are combined into one slice. The S3-only
shape would have required changing every FileStore TEST_GROUP setup anyway
(dual handles), so doing the public-surface flip at the same time avoided a
transitional FileBlockDevice-built-inside-FileStore hack and the storage
bloat that would have come with it.
Part of S18.02 (#235).
Adds a baseline regression scenario for S18.02: under a workload that does not hit capacity, no block file is disposed. Pins today's dispose-on- capacity-pressure-only semantics before S18.03 introduces dispose-on-empty. Two new steps: - 'Given the set of existing block files is recorded' — snapshots the current <prefix>NN.log set. - 'Then no recorded block file has been disposed' — asserts every recorded file still exists. Part of S18.02 (#235).
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR implements S18.02 by introducing a ChangesBlockDevice Abstraction & File-Backed Store Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 999 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Core/Interface/SolidSyslogFileStore.h (1)
33-45: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument
blockDeviceownership/lifetime in the public API contract.Line 35 introduces an externally supplied dependency, but this header doesn’t state whether
SolidSyslogFileStore_Destroyowns it. Please add explicit contract text thatblockDevicemust outlive the store and is not destroyed bySolidSyslogFileStore_Destroy.Suggested doc update
struct SolidSyslogFileStoreConfig { + /* Required. Caller-owned dependency; must remain valid for the entire + * lifetime of the FileStore. Not destroyed by SolidSyslogFileStore_Destroy. */ struct SolidSyslogBlockDevice* blockDevice; @@ -struct SolidSyslogStore* SolidSyslogFileStore_Create(SolidSyslogFileStoreStorage * storage, const struct SolidSyslogFileStoreConfig* config); -void SolidSyslogFileStore_Destroy(struct SolidSyslogStore * store); +/* Create does not take ownership of config->blockDevice. */ +struct SolidSyslogStore* SolidSyslogFileStore_Create(SolidSyslogFileStoreStorage * storage, const struct SolidSyslogFileStoreConfig* config); +/* Destroy releases store state only; caller destroys the block device separately. */ +void SolidSyslogFileStore_Destroy(struct SolidSyslogStore * store);Based on learnings: "Tier 1 (Full support, highest review bar):
Core/Interface/,Core/Source/— stable API required".Also applies to: 57-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Interface/SolidSyslogFileStore.h` around lines 33 - 45, The public header lacks ownership/lifetime contract for the externally supplied blockDevice in SolidSyslogFileStoreConfig; update the API docs in Core/Interface/SolidSyslogFileStore.h around the SolidSyslogFileStoreConfig struct (and any other mentions at the later occurrences) to state explicitly that the blockDevice pointer is owned by the caller, must outlive the SolidSyslogFileStore instance, and will NOT be freed or destroyed by SolidSyslogFileStore_Destroy; keep the wording concise and place it next to the blockDevice field and in the top-level description of SolidSyslogFileStoreConfig so callers clearly understand the lifetime guarantee.Core/Source/BlockSequence.c (2)
166-195:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOnly advance sequence state after block-device mutation succeeds.
Line 171 ignores the result of
SolidSyslogBlockDevice_Acquire(), and Line 187 ignores the result ofSolidSyslogBlockDevice_Dispose(). A transient create/delete failure will still advancewriteSequenceoroldestSequence, leaving the in-memory ring out of sync with the actual blocks on disk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Source/BlockSequence.c` around lines 166 - 195, RotateToNextFile and DiscardOldestFile currently advance in-memory sequence state unconditionally; change them to check the return value of SolidSyslogBlockDevice_Acquire and SolidSyslogBlockDevice_Dispose and only update writeSequence/writePosition/writeFileCorrupt or oldestSequence (and call ResetReadToOldest) when the corresponding device call succeeds; if the device call fails, leave sequences unchanged and propagate or handle the error (e.g., return false or an error status) so the in-memory ring stays consistent with on-disk blocks. Ensure you reference RotateToNextFile, DiscardOldestFile, SolidSyslogBlockDevice_Acquire, SolidSyslogBlockDevice_Dispose, writeSequence, oldestSequence, NextSequence, and ResetReadToOldest when making the changes.
88-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle wrapped block ranges when resuming.
ScanForExistingBlocks()currently treats the lowest existing sequence as the oldest block and the highest as the write block. That breaks as soon as retained files straddle99 -> 00: a persisted set like98, 99, 00, 01resumes as oldest00/ write99, soFileCount(), read order, and future discards all become wrong after restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Source/BlockSequence.c` around lines 88 - 113, ScanForExistingBlocks currently assumes non-wrapping sequences; fix it by building a presence map of size MAX_SEQUENCE using SolidSyslogBlockDevice_Exists for each seq, return false if none set, then find the longest contiguous run of true entries allowing wrap-around (treat array as circular) — pick its first index as blockSequence->oldestSequence, set blockSequence->writeSequence = (oldest + run_length - 1) % MAX_SEQUENCE, and blockSequence->readSequence = oldest; ensure you handle uint8_t casts consistently and keep MAX_SEQUENCE used for loop bounds when computing run lengths.Core/Source/RecordStore.c (1)
271-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat sent-flag read failures as corruption instead of “sent”.
IsRecordSent()seedsflagwithSENT_FLAG_SENTand ignores theSolidSyslogBlockDevice_Read()result. If that read fails,AdvancePastSentRecord()skips the record as already sent, so resume can walk past an unsent/truncated record and lose data.💡 Suggested fix
-static inline bool IsRecordSent(const struct RecordStore* recordStore, struct SolidSyslogBlockDevice* blockDevice, size_t blockIndex, size_t recordStart, - uint16_t length) +static inline bool TryReadSentFlag(const struct RecordStore* recordStore, struct SolidSyslogBlockDevice* blockDevice, size_t blockIndex, + size_t recordStart, uint16_t length, bool* isSent) { - uint8_t flag = SENT_FLAG_SENT; - SolidSyslogBlockDevice_Read(blockDevice, blockIndex, SentFlagFileOffset(recordStore, recordStart, length), &flag, SENT_FLAG_SIZE); - return flag == SENT_FLAG_SENT; + uint8_t flag = SENT_FLAG_UNSENT; + if (!SolidSyslogBlockDevice_Read(blockDevice, blockIndex, SentFlagFileOffset(recordStore, recordStart, length), &flag, SENT_FLAG_SIZE)) + { + return false; + } + + *isSent = flag == SENT_FLAG_SENT; + return true; }bool isSent = false; if (!TryReadSentFlag(recordStore, blockDevice, blockIndex, *cursor, length, &isSent)) { *cursor = fileSize; *corrupt = true; } else if (isSent) { SkipRecord(recordStore, cursor, length); advanced = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Source/RecordStore.c` around lines 271 - 299, AdvancePastSentRecord currently treats read errors as "sent" because IsRecordSent seeds flag with SENT_FLAG_SENT and ignores SolidSyslogBlockDevice_Read's return; update the logic so a failed sent-flag read is treated as corruption: change IsRecordSent into a TryReadSentFlag (or make it return a success boolean and an out-parameter isSent) that calls SolidSyslogBlockDevice_Read and returns false on failure, then in AdvancePastSentRecord (after ReadAndValidateRecord succeeds) check the read result—on read failure set *cursor = fileSize and *corrupt = true, otherwise if isSent call SkipRecord and set advanced = true; reference symbols: AdvancePastSentRecord, IsRecordSent (or new TryReadSentFlag), SolidSyslogBlockDevice_Read, SentFlagFileOffset, SENT_FLAG_SENT, SENT_FLAG_SIZE, SkipRecord, ReadAndValidateRecord.
🧹 Nitpick comments (1)
Tests/SolidSyslogFileBlockDeviceTest.cpp (1)
144-154: ⚡ Quick winAdd a boundary test for unsupported block indices.
You already validate two-digit naming for
7and42; please add one test for an out-of-range index (e.g.,100or256) to lock expected behavior and prevent filename-collision regressions.🧪 Example test shape
+TEST(SolidSyslogFileBlockDevice, AcquireRejectsOutOfRangeBlockIndex) +{ + CHECK_FALSE(SolidSyslogBlockDevice_Acquire(device, 256)); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SolidSyslogFileBlockDeviceTest.cpp` around lines 144 - 154, Add a boundary test that verifies out-of-range block indices are rejected: call SolidSyslogBlockDevice_Acquire(device, 100) (or 256) and assert it returns a failure status and that no file was created at "/tmp/blockdev_100.log" by using SolidSyslogFile_Exists(writeFile, "/tmp/blockdev_100.log") == false; place this alongside the existing tests (referencing SolidSyslogBlockDevice_Acquire, SolidSyslogFile_Exists, and writeFile) to lock the expected unsupported-index behavior and prevent filename-collision regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Source/SolidSyslogFileBlockDevice.c`:
- Around line 157-163: Validate blockIndex before narrowing to uint8_t in
FormatBlockFilename and in Dispose, Exists, Read, Append, WriteAt, and Size:
check that blockIndex is within the device's valid range (e.g., compare against
device->blockCount or the appropriate max-blocks field) and return an
error/false for operations (and return 0 for Size) if out of range, only casting
to (uint8_t) after this guard so large indices cannot wrap and collide on-disk
filenames; update all references to FormatBlockFilename usage accordingly so
callers never pass an unchecked index.
In `@Tests/FileFake.c`:
- Around line 138-151: Add null-handle guards to the fail-injection helpers so
they don't dereference a null pointer; in FileFake_FailNextOpen,
FileFake_FailNextWrite, and FileFake_FailNextRead check that the incoming struct
SolidSyslogFile* file is non-NULL before calling AsFake(file) and either
assert/return with a clear error (e.g., use a test assertion or early return)
when file is NULL to avoid segfaults and provide a useful failure message.
---
Outside diff comments:
In `@Core/Interface/SolidSyslogFileStore.h`:
- Around line 33-45: The public header lacks ownership/lifetime contract for the
externally supplied blockDevice in SolidSyslogFileStoreConfig; update the API
docs in Core/Interface/SolidSyslogFileStore.h around the
SolidSyslogFileStoreConfig struct (and any other mentions at the later
occurrences) to state explicitly that the blockDevice pointer is owned by the
caller, must outlive the SolidSyslogFileStore instance, and will NOT be freed or
destroyed by SolidSyslogFileStore_Destroy; keep the wording concise and place it
next to the blockDevice field and in the top-level description of
SolidSyslogFileStoreConfig so callers clearly understand the lifetime guarantee.
In `@Core/Source/BlockSequence.c`:
- Around line 166-195: RotateToNextFile and DiscardOldestFile currently advance
in-memory sequence state unconditionally; change them to check the return value
of SolidSyslogBlockDevice_Acquire and SolidSyslogBlockDevice_Dispose and only
update writeSequence/writePosition/writeFileCorrupt or oldestSequence (and call
ResetReadToOldest) when the corresponding device call succeeds; if the device
call fails, leave sequences unchanged and propagate or handle the error (e.g.,
return false or an error status) so the in-memory ring stays consistent with
on-disk blocks. Ensure you reference RotateToNextFile, DiscardOldestFile,
SolidSyslogBlockDevice_Acquire, SolidSyslogBlockDevice_Dispose, writeSequence,
oldestSequence, NextSequence, and ResetReadToOldest when making the changes.
- Around line 88-113: ScanForExistingBlocks currently assumes non-wrapping
sequences; fix it by building a presence map of size MAX_SEQUENCE using
SolidSyslogBlockDevice_Exists for each seq, return false if none set, then find
the longest contiguous run of true entries allowing wrap-around (treat array as
circular) — pick its first index as blockSequence->oldestSequence, set
blockSequence->writeSequence = (oldest + run_length - 1) % MAX_SEQUENCE, and
blockSequence->readSequence = oldest; ensure you handle uint8_t casts
consistently and keep MAX_SEQUENCE used for loop bounds when computing run
lengths.
In `@Core/Source/RecordStore.c`:
- Around line 271-299: AdvancePastSentRecord currently treats read errors as
"sent" because IsRecordSent seeds flag with SENT_FLAG_SENT and ignores
SolidSyslogBlockDevice_Read's return; update the logic so a failed sent-flag
read is treated as corruption: change IsRecordSent into a TryReadSentFlag (or
make it return a success boolean and an out-parameter isSent) that calls
SolidSyslogBlockDevice_Read and returns false on failure, then in
AdvancePastSentRecord (after ReadAndValidateRecord succeeds) check the read
result—on read failure set *cursor = fileSize and *corrupt = true, otherwise if
isSent call SkipRecord and set advanced = true; reference symbols:
AdvancePastSentRecord, IsRecordSent (or new TryReadSentFlag),
SolidSyslogBlockDevice_Read, SentFlagFileOffset, SENT_FLAG_SENT, SENT_FLAG_SIZE,
SkipRecord, ReadAndValidateRecord.
---
Nitpick comments:
In `@Tests/SolidSyslogFileBlockDeviceTest.cpp`:
- Around line 144-154: Add a boundary test that verifies out-of-range block
indices are rejected: call SolidSyslogBlockDevice_Acquire(device, 100) (or 256)
and assert it returns a failure status and that no file was created at
"/tmp/blockdev_100.log" by using SolidSyslogFile_Exists(writeFile,
"/tmp/blockdev_100.log") == false; place this alongside the existing tests
(referencing SolidSyslogBlockDevice_Acquire, SolidSyslogFile_Exists, and
writeFile) to lock the expected unsupported-index behavior and prevent
filename-collision regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9665a7cb-357a-43e5-b284-c1df12126963
📒 Files selected for processing (25)
Bdd/features/block_lifecycle.featureBdd/features/steps/syslog_steps.pyCLAUDE.mdCore/Interface/SolidSyslogBlockDevice.hCore/Interface/SolidSyslogBlockDeviceDefinition.hCore/Interface/SolidSyslogFileBlockDevice.hCore/Interface/SolidSyslogFileStore.hCore/Source/BlockSequence.cCore/Source/BlockSequence.hCore/Source/CMakeLists.txtCore/Source/RecordStore.cCore/Source/RecordStore.hCore/Source/SolidSyslogBlockDevice.cCore/Source/SolidSyslogFileBlockDevice.cCore/Source/SolidSyslogFileStore.cDEVLOG.mdExample/Threaded/main.cTests/CMakeLists.txtTests/FileFake.cTests/FileFake.hTests/FileFakeTest.cppTests/SolidSyslogBlockDeviceTest.cppTests/SolidSyslogFileBlockDeviceTest.cppTests/SolidSyslogFileStorePosixTest.cppTests/SolidSyslogFileStoreTest.cpp
…d sequence on resume
Two correctness fixes raised in PR review:
- SolidSyslogFileBlockDevice: blockIndex was cast to uint8_t before
filename formatting. Indices > 99 silently aliased to existing blocks
(256 -> 00) and could overwrite or delete unrelated content. Now
validated against MAX_BLOCK_INDEX (= 99, the two-digit on-disk
capacity) at every entry point and rejected if out of range.
- BlockSequence: ScanForExistingBlocks treated the lowest-numbered file
as oldest and the highest as the write block. After enough rotations
the on-disk set straddles the 99 -> 00 boundary (e.g. {98, 99, 0, 1})
and the naive scan misclassifies it as oldest=00 / write=99 — the
next rotation would then truncate block 00 and we'd read in the
wrong order. Now finds the gap and reads the run as its complement,
treating the sequence space as circular.
Added BlockSequenceTest.cpp with focused tests for the scan algorithm
(cold start, linear range, wrapped range, single-block-at-boundary).
Pre-existing in the old code; surfaced by the BlockDevice abstraction.
Part of S18.02 PR review (#243).
…ent comment
Three review-feedback follow-ups, none behaviour-changing:
- SolidSyslogFileStoreConfig.blockDevice now carries an explicit
ownership/lifetime contract: caller-owned, must outlive the FileStore,
not destroyed by SolidSyslogFileStore_Destroy.
- FileFake_FailNext{Open,Write,Read} reject a null file with TestAssert_Fail
instead of segfaulting on dereference. Defensive guard in the test helper.
- IsRecordSent: the deliberate intent of seeding flag = SENT_FLAG_SENT and
ignoring the BlockDevice_Read return value is now documented in code.
Treating an unreadable sent-flag as already-sent keeps the scan moving;
the alternative (stop on the first bad byte) would jam the logger and
skip every record after it. The single skipped record surfaces as a
sequenceId gap downstream, which the receiver detects per RFC 5424
§6.3.1, and a future integrator-supplied error reporter will cover the
persistent-media-error case directly.
Also adds pr*-comments.json to .gitignore so the gh-api scratch file
doesn't get accidentally staged.
Part of S18.02 PR review (#243).
Owner
Author
|
Responses to the four outside-diff findings in the CodeRabbit review (couldn't be replied to inline):
The "boundary test for unsupported block indices" nitpick is also addressed in 4e8842c. |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1012 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Source/BlockSequence.c (1)
218-245:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon’t commit rotation state before the block-device mutation succeeds.
RotateToNextFile()advanceswriteSequence/writePositionbefore it knowsSolidSyslogBlockDevice_Acquire()actually prepared an empty block, andDiscardOldestFile()advancesoldestSequenceeven ifDispose()fails. That breaks the new block-device contract inCore/Interface/SolidSyslogBlockDevice.h:9-14: on anAcquirefailure the nextAppendcan land on stale data, and on aDisposefailure a later restart can rediscover records that were supposed to be evicted. This path needs a real success/failure result from rotation, not justreadFileChanged.Possible shape
-static bool RotateToNextFile(struct BlockSequence* blockSequence); +static bool RotateToNextFile(struct BlockSequence* blockSequence, bool* readFileChanged); ... - *readFileChanged = RotateToNextFile(blockSequence); + spaceAvailable = RotateToNextFile(blockSequence, readFileChanged);Then:
- acquire the next block into a local
nextSequencefirst,- only publish
writeSequence/writePositionafterAcquiresucceeds,- only advance
oldestSequenceafterDisposesucceeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Source/BlockSequence.c` around lines 218 - 245, RotateToNextFile currently advances writeSequence/writePosition before calling SolidSyslogBlockDevice_Acquire and DiscardOldestFile advances oldestSequence before calling SolidSyslogBlockDevice_Dispose; change the flow so you first call Acquire into a local nextSequence (do not modify blockSequence->writeSequence/writePosition until Acquire returns success), only publish writeSequence/writePosition after Acquire succeeds, and change DiscardOldestFile to call Dispose and only update blockSequence->oldestSequence (and call ResetReadToOldest if needed) when Dispose returns success; preserve the existing return bool (readFileChanged) but make it reflect successful rotation/discard operations and use NextSequence, SolidSyslogBlockDevice_Acquire, SolidSyslogBlockDevice_Dispose, ResetReadToOldest, writeSequence, writePosition and oldestSequence to locate the relevant code paths.
🧹 Nitpick comments (1)
Tests/SolidSyslogFileBlockDeviceTest.cpp (1)
48-58: ⚡ Quick winAdd a re-acquire truncation test.
These cases prove a brand-new block starts empty, but not that reacquiring an existing block clears prior contents.
Core/Interface/SolidSyslogBlockDevice.h:9-14makes that guarantee, and the new BlockSequence rotation path now depends on it.Suggested test shape
TEST(SolidSyslogFileBlockDevice, AcquireFreshBlockHasZeroSize) { SolidSyslogBlockDevice_Acquire(device, 0); LONGS_EQUAL(0, SolidSyslogBlockDevice_Size(device, 0)); } + +TEST(SolidSyslogFileBlockDevice, ReacquireTruncatesExistingBlock) +{ + CHECK_TRUE(SolidSyslogBlockDevice_Acquire(device, 0)); + CHECK_TRUE(SolidSyslogBlockDevice_Append(device, 0, "abc", 3)); + + CHECK_TRUE(SolidSyslogBlockDevice_Acquire(device, 0)); + LONGS_EQUAL(0, SolidSyslogBlockDevice_Size(device, 0)); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SolidSyslogFileBlockDeviceTest.cpp` around lines 48 - 58, Add a test that verifies re-acquiring an already-existing block truncates its contents: create a new TEST (e.g., AcquireReacquireTruncates) that calls SolidSyslogBlockDevice_Acquire(device, 0), writes some data with SolidSyslogBlockDevice_Write(device, 0, buf, len), checks the size is >0 via SolidSyslogBlockDevice_Size(device, 0), then calls SolidSyslogBlockDevice_Acquire(device, 0) again and asserts SolidSyslogBlockDevice_Size(device, 0) == 0 (and optionally SolidSyslogBlockDevice_Exists(device, 0) is true) to confirm the block was cleared on re-acquire.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/BlockSequenceTest.cpp`:
- Around line 114-156: Each test must assert that BlockSequence_Open(&sequence)
actually succeeds before checking read/write values: insert
CHECK_TRUE(BlockSequence_Open(&sequence)); at the start of each test
(ColdStartAcquiresBlockZero, ResumesContiguousLinearRange,
ResumesAtZeroWhenOnlyBlockZeroExists, ResumesWrappedSequenceRangeCorrectly,
ResumesWrappedSingleBlockAtBoundary) immediately before calling
BlockSequence_ReadSequence and BlockSequence_WriteSequence to prevent the
zero-initialized fixture from masking open/acquire failures.
---
Outside diff comments:
In `@Core/Source/BlockSequence.c`:
- Around line 218-245: RotateToNextFile currently advances
writeSequence/writePosition before calling SolidSyslogBlockDevice_Acquire and
DiscardOldestFile advances oldestSequence before calling
SolidSyslogBlockDevice_Dispose; change the flow so you first call Acquire into a
local nextSequence (do not modify blockSequence->writeSequence/writePosition
until Acquire returns success), only publish writeSequence/writePosition after
Acquire succeeds, and change DiscardOldestFile to call Dispose and only update
blockSequence->oldestSequence (and call ResetReadToOldest if needed) when
Dispose returns success; preserve the existing return bool (readFileChanged) but
make it reflect successful rotation/discard operations and use NextSequence,
SolidSyslogBlockDevice_Acquire, SolidSyslogBlockDevice_Dispose,
ResetReadToOldest, writeSequence, writePosition and oldestSequence to locate the
relevant code paths.
---
Nitpick comments:
In `@Tests/SolidSyslogFileBlockDeviceTest.cpp`:
- Around line 48-58: Add a test that verifies re-acquiring an already-existing
block truncates its contents: create a new TEST (e.g.,
AcquireReacquireTruncates) that calls SolidSyslogBlockDevice_Acquire(device, 0),
writes some data with SolidSyslogBlockDevice_Write(device, 0, buf, len), checks
the size is >0 via SolidSyslogBlockDevice_Size(device, 0), then calls
SolidSyslogBlockDevice_Acquire(device, 0) again and asserts
SolidSyslogBlockDevice_Size(device, 0) == 0 (and optionally
SolidSyslogBlockDevice_Exists(device, 0) is true) to confirm the block was
cleared on re-acquire.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01e03c27-cefa-415d-bbae-2261711eb578
📒 Files selected for processing (9)
.gitignoreCore/Interface/SolidSyslogFileStore.hCore/Source/BlockSequence.cCore/Source/RecordStore.cCore/Source/SolidSyslogFileBlockDevice.cTests/BlockSequenceTest.cppTests/CMakeLists.txtTests/FileFake.cTests/SolidSyslogFileBlockDeviceTest.cpp
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- Core/Interface/SolidSyslogFileStore.h
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/FileFake.c
- Core/Source/SolidSyslogFileBlockDevice.c
After S18.02 BlockSequence and RecordStore work in block-space (over a SolidSyslogBlockDevice). Their lingering 'File' terminology now misreads — blocks may not be files at all once flash drivers land. Renamed internal-only symbols across BlockSequence, RecordStore, and FileStore: BlockSequence_AdvanceToNextReadFile -> _AdvanceToNextReadBlock BlockSequence_IsReadingOlderFile -> _IsReadingOlderBlock BlockSequence_MarkWriteFileCorrupt -> _MarkWriteBlockCorrupt field writeFileCorrupt -> writeBlockCorrupt static FileIsFull / FileCount -> BlockIsFull / BlockCount static RotateToNextFile -> RotateToNextBlock static DiscardOldestFile -> DiscardOldestBlock parameter readFileChanged -> readBlockChanged local readingOldestFile -> readingOldestBlock RecordStore_FindFirstUnsent fileSize -> blockSize static IntegrityChecksumFileOffset -> IntegrityChecksumOffset static SentFlagFileOffset -> SentFlagOffset field lastSentFlagFileOffset -> lastSentFlagOffset comments referring to 'file' updated to 'block' static ResumeFromExistingFile -> ResumeFromExistingBlock Public surface unchanged. SolidSyslogFileStoreConfig.maxFileSize / .maxFiles deliberately retained — they describe the file-backed store's configuration and match the user-facing CLI vocabulary (--max-file-size, --max-files).
…NS macro Three small clarity wins surfaced by the hygiene pass: - BlockSequence_PrepareForWrite evaluated BlockIsFull twice. Cache the result in a local. Same logic, one call site. - NotifyThresholdCrossed mixed the 'is the threshold even configured' check with the 'is usage above the threshold' check inline. Extract IsAboveThreshold so the body reads as the state machine it is. Single return preserved per the project's MISRA leanings. - FileBlockDevice's EnsureHandleOpenOnBlock had a three-condition composite (cached open + underlying actually open + matching block) on the early-out line. Extract IsHandleAlreadyOpenOnBlock — the early-out now reads as one named predicate. On the test side, repeated 'Acquire + Append + read-and-MEMCMP' shapes become CHECK_BLOCK_CONTAINS(blockIndex, offset, expected, length). Macro (not function) so failures report the caller's __FILE__/__LINE__, following the CHECK_PRIVAL pattern in SolidSyslogTest.cpp. Coverage holds at 100% lines + functions; 1398 checks (was 1393).
…single-return Three small MISRA-leaning fixes from the hygiene pass: - BlockIsFull mixed || with the > comparison without explicit bracketing. Operator precedence makes the result well-defined but the read is ambiguous; bracket each operand of || explicitly. - ScanForExistingBlocks had two return statements (an early 'no files' exit, and a final 'found and configured' return). Restructure to single return via the foundAny flag, wrapping the configure step in if(foundAny) and returning that flag. - BlockSequence_UsedBytes had two return statements (atCapacity early exit and the normal compute path). Restructure to compute into a single 'used' local and return it once. No behaviour change. All other compound conditions in the E18 code are plain '&& between bool-typed operands' with no precedence ambiguity, and match the project's existing convention (see SolidSyslog.c, StreamSender.c). Mixed-type arithmetic was audited and found clean — every integer narrowing is via an explicit cast, every uint8_t-to-int promotion is on expressions that subsequently get an explicit (uint8_t) or (size_t) cast.
Eleven FileStore TEST_GROUPs each declared the same six fixture members (readStorage, storage, readFile, file, deviceStorage, device) and ran the same three-line setup + four-line teardown. Lift the boilerplate into a shared TEST_BASE; each TEST_GROUP becomes TEST_GROUP_BASE + a setupBlockDeviceFakes() / teardownBlockDeviceFakes() call. Test bodies still reference 'file', 'readFile', 'device' directly — they're inherited members. Two pre-existing groups that called the write-side fake 'writeFile' (Rotation, CorruptionRecovery) renamed to 'file' for consistency with the base. SolidSyslogFile_* call sites updated accordingly. 168 lines deleted, 94 added. 1007 tests, 100% coverage preserved.
Public API rename for vocabulary consistency. After S18.02 the FileStore operates on a SolidSyslogBlockDevice — what the integrator configures is the BlockDevice's behavior (block count + per-block size), and the 'file' framing was a leftover from the pre-BlockDevice API. Renamed for clarity before the library hits beta. Touched: - SolidSyslogFileStoreConfig.maxFileSize/.maxFiles → .maxBlockSize/.maxBlocks - BlockSequence internal struct + config + clamp constants (MIN_MAX_FILES → MIN_MAX_BLOCKS, MAX_MAX_FILES → MAX_MAX_BLOCKS) - BuildBlockSequenceConfig locals (minFileSize → minBlockSize, etc.) - ExampleOptions.maxFiles/.maxFileSize → .maxBlocks/.maxBlockSize, plus the example's CLI args (--max-files → --max-blocks, --max-file-size → --max-block-size) and their default constants - ExampleCommandLineTest: variable references, CLI arg strings, test names - BDD step definitions: regex pattern + context attribute names + the comment that referenced MIN_MAX_FILE_SIZE - BDD scenarios: 4 .feature files updated to the new CLI vocabulary - FileStoreTest: TEST_RECORDS_PER_FILE → TEST_RECORDS_PER_BLOCK, TEST_MAX_FILE_SIZE → TEST_MAX_BLOCK_SIZE, TEST_MAX_FILES → TEST_MAX_BLOCKS, CreateWithMaxFiles → CreateWithMaxBlocks, CreateWithMaxFileSize → CreateWithMaxBlockSize, all helper parameters No behaviour change. 1007 tests, 100% Core coverage.
The post-S18.02 name 'IsReadingOlderBlock' read awkwardly because 'older' is implicit context (older than what?). The semantics is 'the read sequence has not caught up to the write sequence' — i.e. the reader is on a block that the writer has already moved past. Rename makes the intent direct: ReadIsBehindWrite. Internal-only, BlockSequence isn't a public API.
Adds a Design Patterns section after Code Style. Covers production code (single return, intent-naming static-inline predicates, function ordering, selective bracketing, null-object preference) and test code (TEST_BASE for shared fixture, CHECK_* macros for assertion shapes, DRY setup + asserts). Each pattern points at a concrete example in the codebase so future drift is easy to flag at review time.
…eOnBlock Two cyclomatic-complexity reductions surfaced in the Phase 2 scan. BlockSequence::ScanForExistingBlocks (was CC=8): - Extract ScanForBlockPresence: walks 0..MAX_SEQUENCE-1, fills the presence map and records foundAny / foundAbsent / firstAbsent into a new file-private BlockPresence struct. - Extract LocateRunBoundaries: given a BlockPresence with foundAbsent, walk outward from the gap to find the run endpoints. - ScanForExistingBlocks now reads as: scan presence, if anything found locate the run, store the result. Top-level CC drops to 2. FileBlockDevice::EnsureHandleOpenOnBlock (was CC=6): - Extract OpenHandleOnBlock: closes if open, formats the block filename, opens, records the new state. Returns whether the open succeeded. - EnsureHandleOpenOnBlock becomes: 'if already open on this block, done; else reopen'. CC drops to 2. Coverage holds at 100% (391 functions, was 388 — three new helpers all exercised by the existing tests). Single return preserved throughout.
After S18.02 the store is BlockDevice-backed and storage-medium-agnostic; the File in the name was a pre-S18.02 leftover. Renames the public type, header, and storage size macro, the internal struct/helper, the test files and TEST_GROUP_BASE names, the BDD step phrase, and the going-forward references in CLAUDE.md, README.md, docs/iec62443.md, and inline comments. DEVLOG entries left as historical context.
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1012 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/Example/ExampleCommandLineTest.cpp (1)
24-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale test name:
DefaultMaxFilesshould beDefaultMaxBlocks.The body was updated to assert
options.maxBlocksbut the test name was left asDefaultMaxFiles, which will mislead future readers and tooling (test reports, search).🔧 Proposed fix
-TEST(ExampleCommandLine, DefaultMaxFiles) +TEST(ExampleCommandLine, DefaultMaxBlocks) { char arg0[] = "test"; char* argv[] = {arg0, nullptr}; Parse(1, argv); LONGS_EQUAL(10, options.maxBlocks); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Example/ExampleCommandLineTest.cpp` around lines 24 - 30, Rename the test from DefaultMaxFiles to DefaultMaxBlocks to match the assertion and avoid confusion: update the TEST macro invocation (TEST(ExampleCommandLine, DefaultMaxFiles)) to TEST(ExampleCommandLine, DefaultMaxBlocks) so the test name aligns with the body that calls Parse(1, argv) and asserts LONGS_EQUAL(10, options.maxBlocks).
🧹 Nitpick comments (1)
Tests/SolidSyslogBlockStoreTest.cpp (1)
505-533: 💤 Low valueTest names still use "MaxFiles" terminology instead of "MaxBlocks".
The test names reference the old file-store vocabulary (
MaxFilesZeroClampedToMinimum,MaxFileSizeZeroClampedToMinimum) while the implementation uses block-store terminology (maxBlocks,maxBlockSize). This creates a terminology mismatch.♻️ Suggested rename for consistency
-TEST(SolidSyslogBlockStoreConfig, MaxFilesZeroClampedToMinimum) +TEST(SolidSyslogBlockStoreConfig, MaxBlocksZeroClampedToMinimum) ... -TEST(SolidSyslogBlockStoreConfig, MaxFilesOneClampedToMinimum) +TEST(SolidSyslogBlockStoreConfig, MaxBlocksOneClampedToMinimum) ... -TEST(SolidSyslogBlockStoreConfig, MaxFilesHundredClampedToMaximum) +TEST(SolidSyslogBlockStoreConfig, MaxBlocksHundredClampedToMaximum) ... -TEST(SolidSyslogBlockStoreConfig, MaxFileSizeZeroClampedToMinimum) +TEST(SolidSyslogBlockStoreConfig, MaxBlockSizeZeroClampedToMinimum) ... -TEST(SolidSyslogBlockStoreConfig, MaxFileSizeOneClampedToMinimum) +TEST(SolidSyslogBlockStoreConfig, MaxBlockSizeOneClampedToMinimum)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SolidSyslogBlockStoreTest.cpp` around lines 505 - 533, Rename the test identifiers to use "MaxBlocks" and "MaxBlockSize" to match the implementation: update TEST names MaxFilesZeroClampedToMinimum -> MaxBlocksZeroClampedToMinimum, MaxFilesOneClampedToMinimum -> MaxBlocksOneClampedToMinimum, MaxFilesHundredClampedToMaximum -> MaxBlocksHundredClampedToMaximum, MaxFileSizeZeroClampedToMinimum -> MaxBlockSizeZeroClampedToMinimum, and MaxFileSizeOneClampedToMinimum -> MaxBlockSizeOneClampedToMinimum; leave the bodies calling CreateWithMaxBlocks/CreateWithMaxBlockSize and VerifyWriteAndReadBack unchanged so the test semantics remain the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Source/SolidSyslogBlockStore.c`:
- Around line 114-127: ResumeFromExistingBlock passes the write-block size to
RecordStore_FindFirstUnsent but must use the actual read-block size when
scanning the ReadSequence; change the third argument so it calls
SolidSyslogBlockDevice_Size for the read block (i.e. use
SolidSyslogBlockDevice_Size(BlockSequence_BlockDevice(&blockStore->blockSequence),
BlockSequence_ReadSequence(&blockStore->blockSequence)) instead of
BlockSequence_WritePosition(&blockStore->blockSequence)) so the cursor is
computed using the read block's real size before calling
BlockSequence_SetReadCursor and handling corrupt.
---
Outside diff comments:
In `@Tests/Example/ExampleCommandLineTest.cpp`:
- Around line 24-30: Rename the test from DefaultMaxFiles to DefaultMaxBlocks to
match the assertion and avoid confusion: update the TEST macro invocation
(TEST(ExampleCommandLine, DefaultMaxFiles)) to TEST(ExampleCommandLine,
DefaultMaxBlocks) so the test name aligns with the body that calls Parse(1,
argv) and asserts LONGS_EQUAL(10, options.maxBlocks).
---
Nitpick comments:
In `@Tests/SolidSyslogBlockStoreTest.cpp`:
- Around line 505-533: Rename the test identifiers to use "MaxBlocks" and
"MaxBlockSize" to match the implementation: update TEST names
MaxFilesZeroClampedToMinimum -> MaxBlocksZeroClampedToMinimum,
MaxFilesOneClampedToMinimum -> MaxBlocksOneClampedToMinimum,
MaxFilesHundredClampedToMaximum -> MaxBlocksHundredClampedToMaximum,
MaxFileSizeZeroClampedToMinimum -> MaxBlockSizeZeroClampedToMinimum, and
MaxFileSizeOneClampedToMinimum -> MaxBlockSizeOneClampedToMinimum; leave the
bodies calling CreateWithMaxBlocks/CreateWithMaxBlockSize and
VerifyWriteAndReadBack unchanged so the test semantics remain the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83b3e53f-93db-4938-a8a8-72eb88c5ddbd
📒 Files selected for processing (28)
Bdd/features/block_lifecycle.featureBdd/features/capacity_threshold.featureBdd/features/power_cycle_replay.featureBdd/features/steps/syslog_steps.pyBdd/features/store_and_forward.featureBdd/features/store_capacity.featureCLAUDE.mdCore/Interface/SolidSyslogBlockStore.hCore/Source/BlockSequence.cCore/Source/BlockSequence.hCore/Source/CMakeLists.txtCore/Source/RecordStore.cCore/Source/RecordStore.hCore/Source/SolidSyslogBlockStore.cCore/Source/SolidSyslogFileBlockDevice.cCore/Source/SolidSyslogFileStore.cExample/Common/ExampleCommandLine.cExample/Common/ExampleCommandLine.hExample/Threaded/main.cPlatform/Windows/Source/SolidSyslogWindowsFile.cREADME.mdTests/BlockSequenceTest.cppTests/CMakeLists.txtTests/Example/ExampleCommandLineTest.cppTests/SolidSyslogBlockStorePosixTest.cppTests/SolidSyslogBlockStoreTest.cppTests/SolidSyslogFileBlockDeviceTest.cppdocs/iec62443.md
💤 Files with no reviewable changes (1)
- Core/Source/SolidSyslogFileStore.c
✅ Files skipped from review due to trivial changes (6)
- Bdd/features/store_and_forward.feature
- Core/Source/CMakeLists.txt
- Platform/Windows/Source/SolidSyslogWindowsFile.c
- Bdd/features/block_lifecycle.feature
- README.md
- Tests/BlockSequenceTest.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/Source/RecordStore.h
- Tests/SolidSyslogFileBlockDeviceTest.cpp
…ce tests - Rename DefaultMaxFiles -> DefaultMaxBlocks (ExampleCommandLineTest) to match the maxBlocks field the body asserts. - Rename MaxFiles*/MaxFileSize* -> MaxBlocks*/MaxBlockSize* in the BlockStoreConfig group (still referenced the old field names after the 31aea6c rename). - Add ReacquireTruncatesExistingBlock to the FileBlockDevice tests; covers the BlockDevice contract that BlockSequence_RotateToNextBlock relies on. - CHECK_TRUE(BlockSequence_Open(&sequence)) in the resume tests so a future Open/Acquire regression cannot pass under a zero-init fixture.
ResumeFromExistingBlock passed WritePosition to FindFirstUnsent as the scan cap, but WritePosition is the size of the WRITE block, not the read block. On a multi-block resume the read block is a closed earlier block whose size is independent of the write block's fill level. With the buggy cap the scan stopped early, causing already-sent records past the cap to be re-emitted on resume. Query SolidSyslogBlockDevice_Size for the read block instead. The BlockSequence_WritePosition accessor was the only caller — removed, along with its declaration. Adds a regression test (ResumeFindsUnsentInClosedReadBlockWhenWriteBlockPartial) that fails without the fix and passes with it. Three records per block, two drained, one record in the partial write block — the configuration needed to expose the cap-stopping-too-early scan.
8 tasks
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1014 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Owner
Author
|
Sweep through the outstanding CodeRabbit comments — pushed two follow-up commits and deferred one item to S18.03. Addressed in 556b896 (test hygiene):
Fixed in 8b13d2f (production bug + regression test):
Deferred to S18.03 #236:
Already addressed in earlier commits — leaving as historical context:
Declining — see in-tree design rationale:
|
5 tasks
This was referenced May 13, 2026
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
1 participant
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
SolidSyslogBlockDevice— block-indexed I/O withAcquire/Disposelifecycle (Acquire/Dispose/Exists/Read/Append/WriteAt/Size). New public headers:SolidSyslogBlockDevice.h,SolidSyslogBlockDeviceDefinition.h.SolidSyslogFileBlockDevice— file-backed driver wrapping twoSolidSyslogFile*instances with sequence-numbered filenames<prefix><NN>.log. Storage-injected per CLAUDE.md callback-conventions.BlockSequenceandRecordStoreonto BlockDevice.SolidSyslogFileStoreConfigswapsreadFile/writeFile/pathPrefixfor oneblockDevicefield — integrators now construct the device separately and pass it in.FileStore:SolidSyslogFileStore_Destroyno longer closes files; the integrator destroys the block device explicitly. The behavioural cost is small and the contract is cleaner.EnsureHandleOpenOnBlockheals transient open failures on next op (the old fast-fail-on-Open semantics are gone — this is strictly better behaviour).Bdd/features/block_lifecycle.feature). S18.03 will overturn this; pinned now per the issue spec.Slicing notes
Per the original plan, S3 (internal repoint) and S4 (public flip) were intended as separate slices. Combined into one commit because the dual-handle test setup change forced by S3 is the same change S4 needs — separating them would have meant a transitional FileBlockDevice-built-inside-FileStore hack and a temporary storage bloat. See DEVLOG.
Test plan
block_lifecycle.feature— passes (the existing capacity / tcp_reconnect features error pre-existing onmaindue to a syslog-ng config-file permission issue in this dev environment, unrelated to this PR)Closes #235.
Summary by CodeRabbit
New Features
Documentation