feat: S11.05 BlockStore composition migration (part B)#403
Conversation
First of three commits closing out S11.05. RecordStore is the TU-internal record-format helper that BlockStore composes inside its slot today — appending magic-prefixed, length-prefixed, integrity- checked records to a block. This commit gives it the canonical E11 3-TU split; BlockStore still embeds the RecordStore by value (the composition rewrite is commit 3/3). - Rename Core/Source/RecordStore.h -> Core/Source/RecordStorePrivate.h per the canonical 3-TU shape. RecordStore is a TU-internal class (no public Interface/ header), so the Private.h stays test-visible — that's the special-case rule clarified in project_e11_three_tu_split. - Rename RecordStore_Init -> RecordStore_Initialise; add a no-op RecordStore_Cleanup for symmetry with the canonical pair. RecordStore owns no resources, so Cleanup has nothing to release. - New Core/Source/RecordStoreStatic.c — pool, public RecordStore_Create(securityPolicy), RecordStore_Destroy(self), IndexFromHandle reverse lookup, CleanupAtIndex bridge — delegating to SolidSyslogPoolAllocator. - New tunable SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE, default 1, floor 1. Sizes three pools 1:1 — the BlockStore slot itself + the RecordStore and BlockSequence inner pools. The 1:1 invariant is what makes commit 3/3's "BlockStore_Create always finds an inner slot" hold. - TU-internal fallback shape: Create returns NULL on pool exhaustion (no shared null-object makes sense — the only consumer is BlockStore_Create, which routes to its own NullStore fallback in commit 3/3). Destroy(NULL) is a silent no-op — the only legitimate way to hold a NULL handle is a failed Create, and the consumer's own error reporting covers that. - BlockStore.c updated to include RecordStorePrivate.h and call RecordStore_Initialise. Still embeds by value at this commit; commit 3/3 switches to a pointer-into-pool. New Tests/RecordStorePoolTest.cpp pins the four contract points: fresh-pool Create returns non-NULL, exhaustion returns NULL, Destroy releases for re-use, Destroy(NULL) is a silent no-op. Suite: 1182 tests total (+4 from main), all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (22)
📝 WalkthroughWalkthroughThis PR implements a static pool-based allocation pattern for RecordStore instances. It introduces a compile-time tunable for pool sizing, refactors RecordStore with renamed initialization and new cleanup APIs, implements a fixed-size pool backed by SolidSyslogPoolAllocator, integrates the new APIs into BlockStore, and validates the pool behavior with comprehensive tests. ChangesRecordStore Static Pool Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1186 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Second of three commits closing out S11.05. BlockSequence is the other
TU-internal sub-component BlockStore composes — owns the rotating
block-window state (read/write/oldest sequence numbers, block-rotation
algorithm, capacity-threshold edge detection). Mirrors commit 1/3's
RecordStore migration in shape.
- Rename Core/Source/BlockSequence.h -> Core/Source/BlockSequencePrivate.h.
TU-internal, no public Interface/ header — Private.h stays test-visible.
- Rename BlockSequence_Init -> BlockSequence_Initialise. New no-op
BlockSequence_Cleanup. BlockSequence holds a caller-owned BlockDevice
pointer (integrator-supplied via BlockSequenceConfig) — no resources
to release on Cleanup, so the body is just (void) blockSequence.
- New Core/Source/BlockSequenceStatic.c — pool + BlockSequence_Create(config)
+ BlockSequence_Destroy(self) + IndexFromHandle + CleanupAtIndex,
delegating to SolidSyslogPoolAllocator. Pool sized by the same
SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE tunable commit 1/3 introduced —
this is the 1:1 sizing invariant the briefing called out.
- TU-internal fallback: _Create returns NULL on exhaustion;
_Destroy(NULL) is a silent no-op. Same shape as RecordStore in
commit 1/3 — the only consumer is BlockStore_Create, which routes
to NullStore in commit 3/3.
- BlockStore.c updated to include BlockSequencePrivate.h and call
BlockSequence_Initialise. Still embeds by value; commit 3/3 switches
to pointer-into-pool.
Tests/BlockSequenceTest.cpp updated to drive lifecycle through Create
+ Destroy instead of stack-allocating the struct:
- struct BlockSequence sequence = {} -> struct BlockSequence* sequence = nullptr.
- setup() calls BlockSequence_Create(&config); teardown() destroys.
- Test bodies' &sequence calls become sequence (no &).
New Tests/BlockSequencePoolTest.cpp pins the same four contract points
(Create-non-NULL / overflow-returns-NULL / Destroy-releases-slot /
Destroy-NULL-is-silent). BlockSequence_Initialise only copies config
fields without dereferencing them, so the pool test can use a
zero-filled config and skip the BlockDevice fake.
Suite: 1186 tests total (+4 from commit 1/3), all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1190 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Substantive commit of S11.05. The BlockStore slot stops embedding
RecordStore and BlockSequence by value and starts holding pointers
into their TU-internal pools (commits 1/3 and 2/3 prepared those).
BlockStore_Create acquires its own slot and an inner slot from each
sibling pool — the 1:1 sizing invariant from the shared
SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE tunable means under normal use the
inner Creates never return their fallback.
- Canonical E11 3-TU split:
* SolidSyslogBlockStore.c — vtable + algorithm + _Initialise/_Cleanup.
Cleanup is a pure NullStore vtable swap on the slot's abstract base;
Static.c destroys the inner pool slots after FreeIfInUse releases
the outer ConfigLock so each pool's lock acquisition is sequential
rather than nested.
* SolidSyslogBlockStorePrivate.h — struct (now two pointers + the
abstract base) + private Initialise/Cleanup signatures.
* SolidSyslogBlockStoreStatic.c — pool of slots, public
SolidSyslogBlockStore_Create(config) + _Destroy(base), the
ResolveSecurityPolicy + BuildBlockSequenceConfig helpers (moved
out of the algorithm TU since Static.c owns the orchestration of
the inner Creates), IndexFromHandle reverse lookup, CleanupAtIndex
bridge.
- Public-header changes: deleted SolidSyslogBlockStoreStorage typedef
and the SOLIDSYSLOG_BLOCK_STORE_STORAGE_SIZE enum that sized it.
SolidSyslogBlockStore_Create's signature drops the leading storage
parameter — same shape Part A applied to StreamSender and
FileBlockDevice.
- Bad-setup contract: pool exhaustion at either layer (BlockStore or
one of its inner pools) routes to SolidSyslogNullStore_Get() and
emits SOLIDSYSLOG_ERROR_MSG_BLOCKSTORE_POOL_EXHAUSTED at ERROR
severity. Under the 1:1 invariant the inner branches are unreachable
during normal operation, but they're covered for the misconfiguration
case. UNKNOWN_DESTROY warning fires when _Destroy is called with a
handle that doesn't trace back to the pool — same crash-safe,
noisy-on-misuse shape as the rest of E11.
- Use-after-destroy is crash-safe via the NullStore vtable swap.
Existing DoubleDestroyDoesNotCrash now exercises the
UNKNOWN_DESTROY warning path on the second Destroy (slot already
released — pool returns 'not in use', _Destroy emits WARNING). New
SolidSyslogBlockStorePool group adds two contract tests:
FillingPoolThenOverflowResolvesToNullStore and
UseAfterDestroyIsCrashSafeViaNullStoreVtable.
- Caller updates: drop SolidSyslogBlockStoreStorage locals from
Tests/SolidSyslog{BlockStore,BlockStoreDrainOrdering,BlockStorePosix}Test.cpp
and the BDD targets (Linux/main.c, Windows/BddTargetWindows.c,
FreeRtos/main.c). The _Create call sites drop the storage argument.
- CLAUDE.md audience-table row for SolidSyslogBlockStore.h rewritten
to match the new pool-based shape (no Storage typedef, 1:1-sized
inner pools, NullStore fallback). NAMING.md updated to drop
BlockStore from the storage-cast example list since it no longer
uses that pattern; the Posix/Windows mutex example takes its place.
Suite: 1188 tests (+2 from commit 2/3), all green locally on
cpputest-freertos with FREERTOS_KERNEL_PATH set. Pool-size override
validation at SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE=3 will follow in a
DEVLOG-only commit before opening for merge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the three commits (RecordStore + BlockSequence + BlockStore composition rewrite), the design decisions taken on this branch — the TU-internal Private.h test-visibility clarification, the silent-NULL Destroy contract, the lock-nesting workaround that moved inner-pool destruction to Static.c — and the AC #9 pool-size override validation at SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE=3. Closes the E11 sweep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1192 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1192 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Three gates went red on the open PR after pushing parts 1-3:
- analyze-iwyu: three include mismatches surfaced by the Private.h
renames. The public BlockStore header carried the full
SolidSyslogSecurityPolicyDefinition.h when only a pointer-to-struct
type appears in the config, swap for a forward declaration. The
BlockStore.c translation unit grew a (uint16_t) cast in
BlockStore_StoreRecord and needs an explicit <stdint.h>; it also
no longer references SOLIDSYSLOG_MAX_INTEGRITY_SIZE / _MESSAGE_SIZE
directly so the SolidSyslogTunables.h include is gone. RecordStore.c
was reaching its types through the old RecordStore.h chain —
RecordStorePrivate.h still satisfies the compile path but iwyu wants
the directly-used names declared via their canonical headers
(<stdbool.h>, <stdint.h>, SolidSyslogSecurityPolicyDefinition.h,
struct SolidSyslogBlockDevice; forward decl).
- analyze-cppcheck: the BlockSequenceTest.cpp setup() functions write
members (sequence, fakeDevice.existing, fakeDevice.calls) that
cppcheck cannot trace through the TEST_GROUP / TEST() macro
indirection. Add the standard cppcheck-suppress unreadVariable
comment with a per-line rationale, matching the pattern already in
place for fakeDevice.sizes one line down.
- analyze-tidy: BlockSequenceRotation::ForceRotation no longer
mutates *this (the change from struct BlockSequence sequence = {} to
struct BlockSequence* sequence = nullptr means the pointer field
isn't reassigned; the data behind the pointer mutates instead).
readability-make-member-function-const flags it. Adding the const
qualifier closes the gate.
All three analyse presets now run clean locally in the CI images
(cpputest:sha-18f19e1 for tidy/cppcheck, cpputest-clang:sha-7eac3ab
for iwyu); the test suite is unchanged at 1188.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1192 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Round-trip with CI flushed out one more transitively-imported set left behind by the BlockSequence.h -> BlockSequencePrivate.h rename. BlockSequence.c now states its <stdbool.h> / <stddef.h> / <stdint.h> needs directly, includes SolidSyslogBlockStore.h for SolidSyslogDiscardPolicy, and forward-declares struct SolidSyslogBlockDevice — matching the shape RecordStore.c already took in the previous fix-up. Confirmed locally via `cmake --build --preset iwyu --target iwyu` (no findings). Suite unchanged at 1188 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1192 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Part B of S11.05 — the BlockStore composition migration. Part A (#402) migrated the public storage-cast classes (StreamSender, FileBlockDevice) and shipped two shared GoF null objects. Part B finishes the story by migrating the two TU-internal sub-components BlockStore composes — RecordStore and BlockSequence — onto the canonical E11 3-TU + PoolAllocator shape, then rewrites BlockStore's slot from "embedded by value" to "pointer-into-pool".
Three commits planned on this branch:
0602362).SOLIDSYSLOG_BLOCK_STORE_STORAGE_SIZEenum; slot holds pointers into the inner pools (next).Pushing the first commit now so CI gates the foundation before commits 2 and 3 land on top.
Closes #401
Change Description
0602362— RecordStore onto PoolAllocator:Core/Source/RecordStore.h→Core/Source/RecordStorePrivate.hper the canonical 3-TU shape. RecordStore is TU-internal (no public Interface/ header), so its Private.h stays test-visible — a special-case rule clarified inproject_e11_three_tu_splitmemory on this branch.RecordStore_Init→RecordStore_Initialise. New no-opRecordStore_Cleanupfor symmetry (RecordStore owns no resources).Core/Source/RecordStoreStatic.c— pool +RecordStore_Create(securityPolicy)+RecordStore_Destroy(self)+IndexFromHandlereverse lookup +CleanupAtIndexbridge, all delegating toSolidSyslogPoolAllocator.SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE(default 1, floor 1) sizes three pools 1:1 — the BlockStore slot itself + the RecordStore and BlockSequence inner pools. The 1:1 invariant is what makes commit 3's "BlockStore_Create always finds an inner slot" hold under normal use._CreatereturnsNULLon exhaustion (the only consumer isBlockStore_Create, which routes to its ownNullStorefallback in commit 3);_Destroy(NULL)is a silent no-op since the only legitimate way to hold a NULL handle is a failed Create.BlockStore.cupdated to includeRecordStorePrivate.hand callRecordStore_Initialise. Still embeds by value at this commit — commit 3 switches to a pointer-into-pool.Test Evidence
New
Tests/RecordStorePoolTest.cpppins the four contract points (4 tests):CreateReturnsNonNullForFreshPool— fresh-pool Create returns non-NULL.FillingPoolThenOverflowReturnsNull— exhaustion returns NULL (no fallback object — TU-internal).DestroyReleasesSlotForReuse— Destroy releases a slot for re-acquisition.DestroyOfNullIsSilentNoop— NULL handle is silently ignored.Suite: 1182 tests total (+4 from main), all green locally on
cpputest-freertos:sha-bbc958bwithFREERTOS_KERNEL_PATH=/opt/freertos/kernel.Pool-size override validation (per S11.05 AC #9) deferred to the end of the branch — once commits 2 and 3 land, the full suite gets re-run at
SOLIDSYSLOG_BLOCK_STORE_POOL_SIZE=3via theSOLIDSYSLOG_USER_TUNABLES_FILEoverride mechanism. That run is the proof that the 1:1 invariant holds: bumping the symbol grows all three pools and the suite stays green.Areas Affected
Core/Interface/SolidSyslogTunablesDefaults.h— newSOLIDSYSLOG_BLOCK_STORE_POOL_SIZEtunable.Core/Source/— RecordStore split into 3 TUs; BlockStore include + function-name updates.Tests/— newRecordStorePoolTest.cpp.No public-API changes in this commit. The public BlockStore API (
SolidSyslogBlockStore_Create(storage, config),SolidSyslogBlockStoreStoragetypedef,SOLIDSYSLOG_BLOCK_STORE_STORAGE_SIZEenum) is unchanged here and will change in commit 3.Summary by CodeRabbit
New Features
Tests