refactor: SolidSyslogFileBlockDevice keeps at most one open handle per path#354
Conversation
…r path Drops the readHandle / writeHandle pair from SolidSyslogFileBlockDevice and keeps a single OpenHandle that gets re-pointed only on block-index change. Same-block runs (multi-Append, drain Reads, Append-then-WriteAt during MarkSent) reuse the handle, so the perf shape of the dual-handle design is preserved. SolidSyslogFileBlockDevice_Create takes one SolidSyslogFile* now instead of two. Cascades through three test files and the two BDD targets. FileFake gains per-FileEntry openOwner tracking — a second Open by a different instance on an already-owned path trips TestAssert_Fail. This pins the S27.01 single-handle-per-path invariant as a test-time regression guard. FileFakeTest.TwoInstancesShareFilesystem updated to Close between the two instances' Opens. SolidSyslogStore's public contract is unchanged. Closes #353. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors the FileBlockDevice to cache a single SolidSyslogFile handle per path, enforces single-open-per-path in FileFake, updates the public API, and updates Linux/Windows targets and unit tests to the new single-file usage. ChangesSingle-Handle FileBlockDevice Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 Generate unit tests (beta)
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 (✔️ 1114 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
IWYU expects every CppUTest TEST() macro instantiation to have a matching forward declaration of the generated test class at the top of the file — the existing six forward decls in this file are the precedent. The new `ReadFollowedByWriteOnSameBlockDoesNotOpenTwoHandles` test added in the prior commit was missing one, tripping analyze-iwyu. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1114 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
…yslogFile* per S27.01 The S27.01 refactor (PR #354, on main) dropped FileBlockDevice's readHandle/writeHandle pair in favour of a single handle. The Linux and Windows BDD targets were updated at that time; the FreeRTOS target lived on this branch and missed the cascade. This commit cascades: - Replaces g_storeReadFile / g_storeReadFileStorage / g_storeWriteFile / g_storeWriteFileStorage with a single g_storeFile / g_storeFileStorage. - Updates SolidSyslogFileBlockDevice_Create and matching Destroys. - Drops RunFatFsDualFilProbe and its caller — the probe walked FileBlockDevice's dual-FIL pattern to surface the FatFs FF_FS_LOCK rejection. With S27.01 the pattern is gone, the rejection can't happen, and the probe has nothing left to prove. Cross-built clean against the freertos-cross preset. FF_FS_LOCK=4 walk-back, [fatfs]/[diskio]/[fatfs-test] diagnostic-print strip, and the FatFsFake f_sync stub are separate housekeeping commits to follow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The [fatfs] / [diskio] / [fatfs-test] traces and RunFatFsSelfTest were instrumentation added during the S08.05 slice 6 pause to surface the dual-FIL hazard root-caused in PR #354 (S27.01). With the refactor merged and the FF_FS_LOCK=4 workaround walked back, the FatFs layer's behaviour is no longer the suspect — the next iteration of slice-6 debugging is at the store-and-forward drain level, where this noise just masks the interesting bits. The [solidsyslog] mount-failed error line stays — that's a genuine failure path the integrator wants to see. Cross-built clean against the freertos-cross preset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: S08.05 slice 1 plumb FatFs host TDD build
Plumbing-only scaffolding for the FatFs adapter slices to come. No
production code yet — first SolidSyslogFatFsFile content lands at
slice 2.
Adds Platform/FatFs/ as a peer to Posix / Windows / FreeRtos / OpenSsl,
shaped as an INTERFACE library so each consumer recompiles the
(future) adapter sources with its own ffconf.h on the include path —
header-configured platform pattern, same as Platform/FreeRtos/.
Adds Tests/Support/FatFsFakes/ holding a host-suitable ffconf.h
(FF_FS_REENTRANT=0, FF_USE_LFN=0, FF_FS_NORTC=1, single 512-byte-sector
volume, FF_USE_MKFS=0) so adapter unit tests can compile against ff.h
without pulling in the real ff.c or a RAM-disk diskio. The FatFsFake.h
control surface and FatFsFake.c bodies land per-slice as adapter
behaviour drives new f_* calls.
Adds Tests/FatFs/SolidSyslogFatFsFileTest with one plumbing assertion
(FfConfRevisionMatchesFatFsHeader) that exercises the include-path
contract ff.h enforces with #error: FatFsFakes/Interface/ffconf.h is
found first, \$FATFS_PATH/source/ff.h is reachable, and the FFCONF_DEF
/ FF_DEFINED revisions agree (80386 ↔ R0.16). This test is removed at
slice 2 when the first real TEST_GROUP(SolidSyslogFatFsFile) lands.
Both Platform/FatFs and Tests/FatFs are gated on \$ENV{FATFS_PATH}
being set, matching the existing Platform/FreeRtos gating on
FREERTOS_KERNEL_PATH.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style: S08.05 slice 1 clang-format ffconf.h
clang-format collapses the tab-aligned columns the upstream FatFs
ffconf.h uses (which mine inherited verbatim) into single-space
delimited #define lines. Values unchanged — purely whitespace.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 2 SolidSyslogFatFsFile Create + Open/Close
Adds the FatFs-backed SolidSyslogFile adapter with Create/Destroy
lifecycle and the Open/Close/IsOpen portion of the vtable. Read/Write/
SeekTo/Size/Truncate land in slice 3, Exists/Delete in slice 4.
Public surface (Platform/FatFs/Interface/SolidSyslogFatFsFile.h)
mirrors SolidSyslogPosixFile.h: SolidSyslogFatFsFileStorage opaque
storage struct, SOLIDSYSLOG_FATFS_FILE_SIZE sized to hold the internal
struct (FIL + base vtable + isOpen flag) on 64-bit hosts, Create takes
a storage pointer and returns &storage->base.
Open uses FA_READ | FA_WRITE | FA_OPEN_ALWAYS, matching PosixFile's
O_RDWR | O_CREAT (read/write, create if absent). Open state is tracked
explicitly via a bool field rather than reading FatFs's internal
fp.obj.fs — cleaner and doesn't depend on FatFs internals.
Destroy delegates to Close; Close is idempotent (guarded by isOpen),
so Destroy on a never-opened or already-closed file is a safe no-op
on f_close.
9 tests driven in ZOMBIES order against a new Tests/Support/FatFsFakes
fake of the FatFs API surface (f_open / f_close only at this slice),
not real FatFs — adapter unit tests verify the translation from
SolidSyslogFile contract to FatFs calls, not FatFs's own correctness.
Real FatFs only enters the build at slice 5 on QEMU. Tests use the
existing CALLED_FAKE / NEVER / ONCE assertion macros from
Tests/Support/TestUtils.h and a pair of CHECK_FILE_IS_OPEN /
CHECK_FILE_CLOSED macros local to the test file.
Static functions in the adapter follow the FatFsFile_* class-name
prefix convention (memory feedback_static_name_prefix), not bare
verbs like the older PosixFile precedent.
Storage size is set to sizeof(intptr_t) * 90 which fits the 64-bit
host TDD struct comfortably. The 32-bit FreeRTOS cross-compile sizing
is revisited at slice 5 when the BDD target first instantiates
storage on Cortex-M.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 3 FatFsFile Read/Write/SeekTo/Size/Truncate
Adds the remaining bulk-IO vtable slots to the FatFs adapter. After
this slice the SolidSyslogFile contract is fully implemented except
Exists / Delete (slice 4).
Mapping:
- Read(buf, count) -> f_read(fp, buf, count, &br);
returns true iff result == FR_OK && br == count.
- Write(buf, count) -> f_write(fp, buf, count, &bw); same shape.
- SeekTo(offset) -> f_lseek(fp, offset).
- Size() -> reads fp->obj.objsize via f_size macro.
The macro dereferences FIL state directly so the
FatFsFake captures the FIL pointer at f_open and
exposes FatFsFake_SetFileSize to populate it for
tests.
- Truncate() -> truncates to zero length:
f_lseek(fp, 0); f_truncate(fp);
FatFs's f_truncate alone would truncate at the
current fptr, which doesn't match PosixFile's
"ftruncate(fd, 0)" semantics.
9 tests added (ZOMBIES order):
- TruncateSeeksToZeroAndCallsFTruncate
- SeekToCallsFLseekWithGivenOffset (triangulation built in)
- SizeReturnsFileObjectSize
- ReadCallsFReadWithCorrectDefaults + partial + FRESULT failure
- WriteCallsFWriteWithCorrectDefaults + partial + FRESULT failure
Tests use new intent-naming CHECK_* macros:
CHECK_LSEEK_OFFSET, CHECK_READ_BUF, CHECK_READ_COUNT,
CHECK_WRITE_BUF, CHECK_WRITE_COUNT.
Production-side DRY: a Self() inline helper extracts the
(struct SolidSyslogFatFsFile*) cast that every vtable function needs;
READ_WRITE_OR_CREATE names the FA_READ | FA_WRITE | FA_OPEN_ALWAYS
flag combination passed to f_open.
FatFsFake state now grouped by FatFs function (f_open / f_close /
f_lseek / f_truncate / f_read / f_write) — each ff function has its
own state block and Reset() walks them top-down.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style: S08.05 slice 3 bracket comparison expressions in && per MISRA 12.1
FatFsFile_Read and FatFsFile_Write returned
return result == FR_OK && br == count;
MISRA C:2012 Rule 12.1 wants explicit precedence around comparisons
inside &&, so this becomes
return (result == FR_OK) && (br == count);
CLAUDE.md's "no parens needed for plain && between bool-typed operands"
covers bool *variables* / bool-returning function calls; comparison
expressions are a different case and get parens.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 4 FatFsFile Exists/Delete
Completes the SolidSyslogFile vtable on the FatFs adapter. Exists and
Delete are stateless with respect to the file handle — neither uses
self — so they wrap f_stat and f_unlink directly with the caller's
path.
- Exists(path) -> f_stat(path, NULL) == FR_OK
NULL FILINFO is documented as supported by FatFs
when only the existence check is needed.
- Delete(path) -> f_unlink(path) == FR_OK
6 tests added:
- ExistsCallsFStatAndReportsTrue (call shape + path + CHECK_TRUE)
- ExistsUsesPassedPath (triangulation)
- ExistsReportsFalseWhenFStatFails (FR_NO_FILE)
- DeleteCallsFUnlinkAndReportsTrue (call shape + path + CHECK_TRUE)
- DeleteUsesPassedPath (triangulation)
- DeleteReportsFalseWhenFUnlinkFails (FR_DENIED)
FatFsFake grows f_stat and f_unlink wrappers with the same shape as
the other ff functions (set-result + call-count + last-path).
After this slice, the full SolidSyslogFile contract is implemented on
FatFs. Slice 5 wires the BlockStore composition into the FreeRTOS BDD
target main.c and adds the QEMU semihosting diskio.c that makes real
FatFs run on the target.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style: S08.05 clean code — Fp() helper and CHECK_*_PATH/MODE macros
Two extractions surfaced in the end-of-slice review pass:
1. Production — Fp(self) inline helper that returns &Self(self)->fp.
Five vtable functions (Read, Write, SeekTo, Size, Truncate) need
only the FIL*, not the wider SolidSyslogFatFsFile*. Replacing the
per-function
struct SolidSyslogFatFsFile* fatfs = Self(self);
... &fatfs->fp ...
pair with Fp(self) drops one local variable per function and turns
Size/SeekTo into true one-liners. Open and Close still use the
wider Self() because they also touch isOpen.
2. Tests — CHECK_OPEN_PATH, CHECK_OPEN_MODE, CHECK_STAT_PATH,
CHECK_UNLINK_PATH macros. Match the existing intent-naming pattern
(CHECK_FILE_IS_OPEN / CHECK_LSEEK_OFFSET / CHECK_READ_BUF etc.) and
replace eight bare STRCMP_EQUAL / LONGS_EQUAL invocations across
the Open / Exists / Delete tests.
No behaviour change; all 24 tests / 55 checks stay green.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 5 QEMU semihosting diskio + FatFs store wiring
Wires FatFs into the FreeRTOS BDD target so `set store file` over the
UART tears down NullStore and rebuilds SolidSyslog with FatFsFile +
FileBlockDevice + BlockStore on top of a semihosting-backed FAT image.
Integrator glue under Bdd/Targets/FreeRtos/:
- ffconf.h with FF_FS_REENTRANT=1, FF_USE_MKFS=1, FF_VOLUMES=1, FF_FS_NORTC=1
- ffsystem.c vendored from upstream with OS_TYPE 3 (FreeRTOS,
xSemaphoreCreateMutex). FF_FS_REENTRANT is on even though only the
Service task touches the store today — a future reentrancy stress
test exercises the production lock path rather than a no-op
- diskio.c implements disk_initialize/status/read/write/ioctl via
BKPT 0xAB semihosting traps against solidsyslog-disk.img (~1 MiB
sparse, 2048 x 512 B). disk_initialize creates the image on first
open so f_mount returns FR_NO_FILESYSTEM and FatFs falls through to
f_mkfs
CMake stages ff.c / ff.h / diskio.h alongside our ffconf.h in the
build dir via configure_file, so ff.h's `#include "ffconf.h"` resolves
to our integrator config — works around GCC's "current file's
directory first" rule without vendoring 7000+ lines of upstream FatFs
into the repo.
main.c lifts SolidSyslog config + state to file scope and adds a
SolidSyslogMutex around the Service task vs the rebuild path. The new
`set` keys max-blocks / max-block-size / discard-policy / halt-exit
update pending globals; `set store file` is the rebuild trigger.
Halt-exit fires SYS_EXIT (0x18) via semihosting to terminate QEMU
deterministically, mirroring Linux's _exit(2).
target_driver.py adds five flag translations, sorts emission so
`--store` lands last, supplies `-semihosting-config enable=on,
target=native` to QEMU, and synthesises a `"1"` value for bare
`--halt-exit` so the UART NAME VALUE protocol stays honest.
environment.py removes solidsyslog-disk.img in after_scenario.
SOLIDSYSLOG_FATFS_FILE_SIZE bumped to sizeof(intptr_t) * 180 — real
FatFs FIL with FF_MAX_SS=512 + FF_FS_TINY=0 is ~620 B, the previous
90-intptr constant was tuned to the smaller fake FIL. Tunable
follow-up tracked for S21.03 under E21.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 6 mount FatFs and run @store features on FreeRTOS
Lights up store_and_forward / power_cycle_replay / store_capacity on
the FreeRTOS BDD target. The slice-5 wiring landed BlockStore plumbing
but every Store_Write was silently a no-op: FatFs requires an explicit
f_mount before any file operation, and slice-5 relied on a non-existent
auto-mount. With NullStore destroyed and the file-backed BlockStore
unable to write, every message fell through DrainBufferIntoStore's
direct-send fallback — fine while the oracle was up (first scenario
message reached it) but lossy as soon as the oracle went down (the
other 5 scenarios saw "1 of N" deliveries).
Fix: RebuildWithFileStore now calls EnsureFatFsMounted() before tearing
down the existing store. f_mount runs with opt=1 to surface
FR_NO_FILESYSTEM eagerly; on a fresh disk image f_mkfs lays down a
FAT12 volume and re-mounts. A mount failure leaves the target on its
original NullStore (zero-disruption) and the OnSet "store" branch
reports false so the harness surfaces the error rather than silently
degrading. Verified locally: `set store file` now leaves a 1 MiB image
with a real FAT12 boot sector (eb fe 90 MSDOS5.0, 55 aa at 510).
Also lands the wiring the three @store features need:
- target_driver.py: `--no-sd` joins the FreeRTOS translation table as
a bare flag (synthetic "1" value), matching --halt-exit. store_capacity
scenarios couple --no-sd with --store file so the sort order keeps
--store last and the rebuild path picks up the final no-sd setting.
- main.c: `set no-sd N` flips g_pendingNoSd; both initial Setup and
RebuildWithFileStore re-honour it via sdCount = 1 (just metaSd) or
3 (full SD list).
- ci/docker-compose.bdd.yml: `not @store` lifted from behave-freertos
so store_and_forward, power_cycle_replay, and store_capacity all run.
capacity_threshold stays out via per-feature @freertoswip until the
threshold-marker plumbing has a semihosting equivalent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: S08.05 slice 6 diagnostic patch for docker BDD failures
Locally the slice-6 f_mount path creates a valid FAT12 disk image and
the first `send 1` succeeds. In David's WSL docker compose run none of
that happens — no disk image is left after a scenario and every first-
message step fails (0 of 1 received), which is a regression from the
previous run where the first message at least reached the oracle via
direct-send fallback. Need visibility into the FreeRTOS guest's UART
output to find the divergence.
Adds:
- [fatfs] traces in main.c around f_mount / f_mkfs (entry, return codes,
mount-complete confirmation).
- [diskio] traces in diskio.c around disk_initialize, the semihosting
open(r+b) / open(w+b) handle returns, and Flen result.
- target_driver.py: route QEMU stderr to the parent process's stderr on
FreeRTOS so semihosting / QEMU runtime errors surface in docker
compose's behave output.
- syslog_steps.py: tee the FreeRTOS guest's UART output (stdout) into a
16 KB sliding buffer maintained by the existing reader thread.
- environment.py after_step hook: on step failure, dump that buffer to
the behave log so we see the [fatfs] / [diskio] / SolidSyslog ErrorHandler
lines that ran during the failing step.
Once the docker run reveals the divergence, the diagnostic printfs and
target_driver stderr passthrough get reverted; the after_step buffer
dump is general enough to keep.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: revert stderr passthrough — behave wraps sys.stderr without fileno()
The slice-6 diagnostic patch routed QEMU stderr to sys.stderr on the
FreeRTOS target so semihosting errors would surface in docker compose
output. behave's runner wraps sys.stderr with an internal stream that
doesn't support fileno(), so subprocess.Popen raised
io.UnsupportedOperation on every spawn — breaking all FreeRTOS scenarios,
not just the failing @store ones.
Revert to stderr=subprocess.PIPE everywhere. The sliding-buffer +
after_step dump (added in the same diagnostic chore) still gives us
visibility into [fatfs] / [diskio] / [solidsyslog] guest UART output
on step failure, which is the actual diagnostic we want.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 slice 6 — CodeRabbit fixes + FatFs end-to-end self-test
Bundles the major CodeRabbit finding fix, three minor cleanups, and the
focused FatFs self-test David suggested to isolate the docker BDD
regression.
ServiceTask race on teardown (CodeRabbit major, also the slice-5
smoke-test hard-fault root cause): InteractiveTask's quit-time teardown
destroyed g_lifecycleMutex and NULLed the pointer; ServiceTask's next
iteration unconditionally locked it, dereferencing NULL or use-after-
free freed kernel state — `qemu: fatal: Lockup` on every quit.
Fix: add a g_solidSyslogTeardown flag, set inside the lifecycle-mutex
critical section so Service observes it atomically with the SolidSyslog
destroy. Service unlocks, vTaskDelete(NULL) before any further mutex
access. Teardown then vTaskDelay(20ms) — generous against Service's
worst-case iteration — before destroying the mutex. Local smoke test:
`quit` now prints the stack-hwm report and exits cleanly with no
HardFault escalation.
Three minor CodeRabbit findings:
- main.c: `set store null` returns true unconditionally now (was
!g_currentStoreIsFile, asymmetric truth value). The harness always
emits --store last so the rebuild ordering is the real guard.
- environment.py: hoist `import sys` to the module top-level block
(was repeated inside after_step).
- target_driver.py: apply_extra_args fails fast when a key/value flag
is followed by another flag instead of a value — `--facility
--severity 6` would have silently treated `--severity` as facility's
value.
FatFs self-test (David's suggestion in lieu of broader instrumentation):
runs the exact sequence BlockStore + FileBlockDevice will use —
open(W), write, close, stat-exists, open(R), read, close, unlink,
stat-not-exists — immediately after EnsureFatFsMounted reports
success. Each step prints [fatfs-test] with its FRESULT. Locally all
nine steps return 0 / FR_NO_FILE=4 as expected. If docker shows any
non-zero, we have the exact failing operation. If docker shows the
same all-pass, the FatFs layer is sound and the regression lives
above it (BlockStore / Sender / Service).
Diagnostic printfs from the prior chore commit stay in place pending
the docker investigation; cleanup is tracked.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: align executable include path to FATFS_STAGE_DIR (CodeRabbit major)
The OBJECT lib (containing ff.c + our ffsystem.c) compiles against
FATFS_STAGE_DIR, where our integrator ffconf.h is colocated with the
staged ff.h. But the executable target (main.c, diskio.c,
SolidSyslogFatFsFile.c) pointed at ${FATFS_PATH}/source. When those
.c files include "ff.h" from that path, ff.h's `#include "ffconf.h"`
resolves to upstream's sample in the same directory before searching
-I paths — so the executable's translation units saw FF_FS_REENTRANT=0
(upstream default) while ff.c saw FF_FS_REENTRANT=1 (ours).
Any ffconf.h-conditional layout (FF_FS_REENTRANT controls the sync
object, FF_USE_LFN controls lfnbuf, FF_MAX_SS vs FF_MIN_SS controls
ssize, etc.) would then differ between translation units, opening the
door to silent FATFS struct corruption. Locally the specific combo
happens not to trip an observable failure, but the divergence is wrong
on its face and is a plausible candidate for the docker BDD regression
(where the disk image is created and FAT is laid down correctly but
oracle receives 0 messages).
Fix: point the executable at FATFS_STAGE_DIR too, with a comment
explaining why it must mirror the OBJECT lib's include path. Local
smoke test re-verified: build clean, every FatFs self-test step
passes (return 0 / FR_NO_FILE=4 as expected), `quit` exits without
HardFault.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: enable FF_FS_LOCK=4 — FileBlockDevice opens two FILs on the same path
Suspected docker BDD regression root cause. FileBlockDevice
(Core/Source/SolidSyslogFileBlockDevice.c) caches readHandle and
writeHandle independently, each holding an underlying FIL open between
calls. When the store has a single active block (the common case
during a short outage burst), BOTH handles target the same path:
writeHandle has STORE00.LOG open from the first Append, readHandle
opens STORE00.LOG for the first SendOneFromStore read. With
FF_FS_LOCK=0 in ffconf.h, the FatFs manual explicitly warns:
"0: Disable file lock function. To avoid volume corruption,
application program should avoid illegal open, remove and rename
to the open objects."
Two concurrent FILs on one path is "illegal open" — undefined
behaviour, can silently corrupt the volume's FAT chain. On POSIX (the
Linux BDD target) two fds on one path are fine; the FileBlockDevice
contract was designed against that, so the same dual-handle pattern
silently breaks on FatFs+FF_FS_LOCK=0 while passing on Linux.
Setting FF_FS_LOCK to a positive value enables FatFs's internal open-
file tracking, which permits multiple FILs on one path with correct
shared-write semantics. 4 gives headroom over the 2 FileBlockDevice
holds (in case rename/delete opens a third FIL transiently inside
f_unlink). Memory cost: ~12 B per slot = 48 B static, trivial against
our 96 KB heap.
Local smoke test (single-FIL self-test) unchanged — sequential ops
weren't hitting the lock path anyway. Validation comes from the BDD
@store scenarios which exercise FileBlockDevice's concurrent pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: pause S08.05 slice 6 at diagnostic state
S08.05 slice 6 is paused pending a store-layer refactor that removes the
"two file handles open on the same path" requirement. See memory entry
project_s08_05_slice6_status for the full architectural finding and the
next-session brief.
This commit snapshots the slice-6-diagnostic state so the branch is
clean across context switches:
- Bdd/Targets/FreeRtos/ffconf.h: clang-format fix that was queued for
the next slice-6-targeted commit. Standalone hygiene; not load-bearing.
- Bdd/Targets/FreeRtos/main.c: RunFatFsDualFilProbe — a boot-time probe
that walks the FileBlockDevice dual-handle open pattern and prints
FRESULT traces. Designed to surface FatFs's lock semantics (chk_share
at ff.c:970 rejects 2nd write-mode open with FR_LOCKED) without BDD
scaffolding. Keep as evidence; revert when slice 6 resumes and greens.
- Tests/Support/FatFsFakes/Source/FatFsFake.c: f_sync stub returning
FR_OK so FatFsFake-linked unit tests stay green if a future iteration
reintroduces f_sync calls. Harmless future-proofing.
When slice 6 resumes (post-refactor on main), revert this commit or
strip the probe + earlier [fatfs]/[diskio]/[fatfs-test] diagnostic
prints together as a cleanup commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 slice 6 — collapse FreeRTOS FatFs wiring to single SolidSyslogFile* per S27.01
The S27.01 refactor (PR #354, on main) dropped FileBlockDevice's
readHandle/writeHandle pair in favour of a single handle. The Linux and
Windows BDD targets were updated at that time; the FreeRTOS target lived
on this branch and missed the cascade.
This commit cascades:
- Replaces g_storeReadFile / g_storeReadFileStorage / g_storeWriteFile /
g_storeWriteFileStorage with a single g_storeFile / g_storeFileStorage.
- Updates SolidSyslogFileBlockDevice_Create and matching Destroys.
- Drops RunFatFsDualFilProbe and its caller — the probe walked
FileBlockDevice's dual-FIL pattern to surface the FatFs FF_FS_LOCK
rejection. With S27.01 the pattern is gone, the rejection can't
happen, and the probe has nothing left to prove.
Cross-built clean against the freertos-cross preset.
FF_FS_LOCK=4 walk-back, [fatfs]/[diskio]/[fatfs-test] diagnostic-print
strip, and the FatFsFake f_sync stub are separate housekeeping commits
to follow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Revert "fix: enable FF_FS_LOCK=4 — FileBlockDevice opens two FILs on the same path"
This reverts commit f72cc1a.
* chore: S08.05 slice 6 strip FatFs diagnostic prints
The [fatfs] / [diskio] / [fatfs-test] traces and RunFatFsSelfTest were
instrumentation added during the S08.05 slice 6 pause to surface the
dual-FIL hazard root-caused in PR #354 (S27.01). With the refactor
merged and the FF_FS_LOCK=4 workaround walked back, the FatFs layer's
behaviour is no longer the suspect — the next iteration of slice-6
debugging is at the store-and-forward drain level, where this noise
just masks the interesting bits.
The [solidsyslog] mount-failed error line stays — that's a genuine
failure path the integrator wants to see.
Cross-built clean against the freertos-cross preset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 slice 6 — bound FreeRTOS_connect with SO_RCVTIMEO
FreeRtosTcpStream set SO_SNDTIMEO before FreeRTOS_connect to bound the
blocking call, but upstream FreeRTOS_connect gates on the socket's
xReceiveBlockTime (SO_RCVTIMEO), not xSendBlockTime. The intended 200 ms
bound was silently ignored — actual wait was ipconfigSOCK_DEFAULT_RECEIVE_BLOCK_TIME
(5000 ticks, 50 s at our 100 Hz tick rate). When the oracle was paused for
the @store scenarios, the Service task blocked inside FreeRTOS_connect well
past the 10 s test deadline, so none of the buffered messages drained after
resume. store_and_forward on freertos-cross was 1 of 6.
Set both SO_SNDTIMEO and SO_RCVTIMEO before connect as belt-and-braces
against any future upstream change to the gating option. ClearTimeouts
already resets both to 0 after connect, so Send/Read keep their
non-blocking single-call contract.
store_and_forward.feature on freertos-cross now passes 6/6 in 4.7 s.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 7a FatFsFile f_sync after each write
Each successful f_write is followed by f_sync so a power-loss crash
never loses a record the BlockStore claims is stored, nor double-sends
a record after a crash-then-restart between MarkSent and shutdown.
FatFsFake gains a sync call counter and result override; Read/Write
fake capture switches from pointer-equality to content-equality
(SetReadSource / LastWriteBytes) so the tests actually verify the data
path. Drive-by tidy: br/bw -> bytesRead/bytesWritten, Self/Fp ->
FatFsFile_Self/FatFsFile_Handle per the Platform-adapter static-prefix
convention.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: S08.05 slice 7b graceful FatFs shutdown for power_cycle_replay
`set shutdown 1` on the UART tears down our objects (SolidSyslog +
file-store chain — the FatFsFile_Destroy → Close → f_close flushes
the dir entry), then f_unmount the FatFs volume, then SemihostingExit(0).
The destroy chain extracted out of RebuildWithFileStore as
DestroyCurrentStore (now called from both paths).
`step_client_is_killed` becomes target-aware: on freertos the kill is
mapped to `set shutdown 1` so the next session's f_mount finds the
STORE*.log directory entries up-to-date — testing our recovery code,
not FatFs's mid-write crash semantics (those are covered by the
unit tests added in slice 7a). Linux/Windows targets keep SIGKILL —
the kernel flushes page cache and file descriptors on process exit.
power_cycle_replay.feature now green on freertos-cross (15 steps, 8s).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: gitignore solidsyslog-disk.img FatFs semihosting artefact
QEMU's user-mode semihosting creates the disk image at its cwd (the
project root) per Bdd/Targets/FreeRtos/diskio.c::DISK_IMAGE_PATH.
after_scenario removes it, but an aborted run can leave it behind.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 FatFsFake f_read reports actual bytes copied
When SetReadSource programs fewer bytes than the caller requests, the
fake now sets *br = copyCount (bytes actually memcpy'd into buff) rather
than *br = btr (bytes requested). Production's
`(result == FR_OK) && (br == count)` partial-read check now sees the
short read instead of treating uninitialised buffer memory as valid data.
CodeRabbit catch on PR #352.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: BDD before_scenario removes stale FatFs disk image
after_scenario already removes solidsyslog-disk.img on the happy path,
but a Ctrl-C / OOM / debugger-detach during a prior run leaves the
image behind. diskio.c::DiskImageIsReady keeps any existing full-size
image, so a stale FAT with STORE*.log content carries silently into
the next scenario's mount. Idempotent FileNotFoundError-tolerant
remove in before_scenario is the belt to after_scenario's brace.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor: drop g_* Hungarian prefix in FreeRTOS BDD target
CLAUDE.md bans Hungarian notation and member-variable prefixes; the
FreeRTOS BDD-target main.c had crept past the rule (~30 file-scope
identifiers). Pure rename, contained to Bdd/Targets/FreeRtos/main.c —
no other file references the old names.
testMessage takes the special-case name to avoid shadowing
ErrorHandler's `message` parameter; everything else just drops g_.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor: S08.05 unify FreeRTOS BDD-target shutdown via TeardownAll
Previously, the `quit` command and `set shutdown 1` had two separate
exit paths. `quit` ran the full destroy chain (SolidSyslog + SDs +
counter + store + Buffer + Mutex + Senders + Streams + Datagram +
Resolver). `set shutdown 1` only ran a partial chain (SolidSyslog +
store + f_unmount), leaving the Senders / Streams / Datagram / Resolver
dangling — most importantly the TCP stream's open connection to
syslog-ng-freertos, which on SemihostingExit leaves the peer with an
ungracefully closed connection.
Single source of truth now: TeardownAll() does the full chain plus
f_unmount. Two callers:
- `quit`: BddTargetInteractive_Run returns → TeardownAll → vTaskDelete
- `set shutdown 1`: OnSet → TeardownAll → SemihostingExit(0)
The Setup-locals (resolver, datagram, tcpStream, tcpSender, buffer,
bufferMutex) become file-scope static so both entry points reach them.
No g_* prefix — convention CLAUDE.md mandates.
The standalone ShutdownGracefully is gone; DestroyCurrentStore remains
as a small helper shared by RebuildWithFileStore and TeardownAll.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 slice 7c ARP-prime FreeRTOS TCP connect
Mirrors the SolidSyslogFreeRtosDatagram ARP-prime that landed in
S08.03 slice 3b.1.5. Before slice 6's SO_RCVTIMEO fix, FreeRTOS_connect
defaulted to 50 s of patience and that absorbed the cold-start ARP
resolution delay invisibly; with the connect now bounded at 200 ms,
a cold first SYN gets dropped at the IP layer while ARP resolves and
the bounded timer expires before the retransmit-SYN cycle completes.
PrimeArpIfMissing runs before every connect: xIsIPInARPCache, on miss
fire FreeRTOS_OutputARPRequest then vTaskDelay 50 ms so the reply has
landed by the time SYN goes out. Symmetric with the Datagram path.
Hypothesis: this also explains why tcp_reconnect:7 and tcp_transport:9
regressed in CI between slice 6 baseline (8a23ac6) and slice 7's first
push (b217a38) — both single-shot cold-connect scenarios with no Service
retry to absorb the first dropped SYN. CI will confirm.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: BDD FreeRTOS extra_args guard checks known flags not hyphen prefix
The next-token-is-another-flag guard rejected any value starting with
'-', which would refuse legitimate hyphen-prefixed values like
`--message -hello`. Match against _FREERTOS_SET_TRANSLATION instead so
only actual known flag tokens trigger the ValueError.
CodeRabbit catch on PR #352.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 SemihostingExit propagates status via SYS_EXIT_EXTENDED
On AArch32 ARM Semihosting, SYS_EXIT (0x18) treats R1 as a *literal*
reason code, not a parameter-block pointer. Passing &{reason, status}
as R1 resolved to "unrecognised reason" → QEMU exit 1 regardless of
the status field. SYS_EXIT_EXTENDED (0x20) is the form that accepts
a {reason, subcode} parameter block and propagates subcode as the
exit status.
Caught by store_capacity.feature:37 — Halt stops asserts exit code 2
but kept seeing 1. CodeRabbit flagged this on the prior round as a
🔴 follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: S08.05 add BlockStore drain-ordering harness
Host-side integration test over the real BlockStore + BlockSequence +
RecordStore + FileBlockDevice stack with FileFake at the bottom. Motivated
by the discard-newest BDD failure on freertos-cross — oracle saw
[1, 11, 2, 3, 4, 5, 6] which on its face looks like a BlockStore drain
ordering bug.
The first reproducer test in this harness (Outage drain produces ascending
sequenceIds) PASSES — so BlockStore alone isn't the culprit. That moves
the search up one layer to the SolidSyslog_Service drain algorithm
(Buffer -> Store -> Sender) which is where the [1, 11, 2..] interleave
actually arises. Follow-up commits will add a Service-level test and the
fix.
The harness drives the Store interface directly (Write / HasUnsent /
ReadNextUnsent / MarkSent), parameterised by maxBlocks / maxBlockSize /
payloadSize / discardPolicy via a DrainTestConfig struct so future tests
can sweep configurations without rewriting the fixture.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: S08.05 add Service-level [1,11,2..] reproducer (RED, IGNORE_TEST)
Wires BufferFake-replacement (real CircularBuffer + NullMutex) + real
BlockStore + a local SenderSpy with sticky outage mode to drive
SolidSyslog_Service through the exact BDD discard-newest scenario shape
at unit-test speed.
When TEST'd (not IGNORE_TEST'd), the assertion fires at:
Send order descended: ids[2]=2 after ids[1]=11
Reproducing the BDD failure host-side: oracle log [1, 11, 2, 3] —
sequenceId 11 (newest, queued in CircularBuffer at the moment of
oracle recovery) bypasses the older stored 2, 3, ... via
DrainBufferIntoStore's fallback-to-Sender_Send path
(Core/Source/SolidSyslog.c:269-272). That path is correct for
NullStore configurations but breaks the discard-newest promise on
BlockStore: "newest discarded" turns into "newest delivered first."
Marked IGNORE_TEST so CI stays green until the next commit lands the
fix (Store_IsTransient vtable method gating the fallthrough). The
next commit flips this to TEST and the assertion goes from RED to
GREEN.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 BlockStore Write rejection no longer bypasses to sender
Adds Store_IsTransient to the Store vtable, distinguishing stores that
never retain (NullStore, NilStore) from those that retain durably
(BlockStore). Service's DrainBufferIntoStore now gates its fallthrough-
to-Sender_Send on transience: only transient stores can bypass to the
sender on Write rejection.
This fixes the BDD discard-newest failure on freertos-cross. The
previous Service algorithm fell through to direct-send whenever
Store_Write returned false. On BlockStore in discard-newest mode this
meant: when the store filled and rejected the *newest* buffered message,
the message escaped via direct send the instant the oracle recovered —
bypassing the older retained records that had been queued during the
outage. Oracle saw [1, 11, 2, 3, 4, 5, 6] when it should have seen
[1, 2, 3, 4, 5, 6] (with 11 discarded by the discard-newest policy).
The host-side reproducer test in the previous commit (RED, IGNORE_TEST)
is now flipped to TEST and goes green: successful-send order is
[1, 2, 3] — no bypass.
Vtable additions:
- NullStore: IsTransient = true (matches its "never retain" contract).
- NilStore (internal fallback before Create / on NULL config.store):
IsTransient = true (same role).
- BlockStore: IsTransient = false (rejection is the discard policy
speaking; messages do not escape).
- StoreFake (test): IsTransient = false (models a real store).
The existing test ServiceSendsDirectlyWhenStoreWriteFails pinned the
old fallthrough-on-StoreFake-rejection behaviour. Renamed and updated to
ServiceDoesNotBypassToSenderWhenNonTransientStoreRejectsWrite, asserting
the new contract. NullStore-side fallthrough is still covered by
ServiceSendsBufferedMessageWithNullStore.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: S08.05 drain-ordering test passes tidy / tunable-override
- Add [[nodiscard]] + const-qualify WriteMessage / DrainOne / DrainAll;
the test methods are pure observers of fixture state.
- NOLINTNEXTLINE for the SenderSpy reinterpret_cast (vtable downcast
pattern), the swappable {sequenceId, payloadSize} pair (distinct
concepts, both numeric is incidental), and the snprintf calls used
to build CppUTest FAIL messages.
- Drop the diagnostic printfs that were useful during bug investigation
but added vararg-call surface for tidy without buying anything for
the persisted test.
- Derive payloadSize from SOLIDSYSLOG_MAX_MESSAGE_SIZE so the Service-
level reproducer overflows the store on both default (MAX=2048) and
tunable-override (MAX=512) builds — the test pins drain ordering, not
a fixed byte count.
- Trim <string.h>, add <cstdio> for snprintf — IWYU hygiene.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: address CodeRabbit + IWYU on PR #352 drain-ordering harness
IWYU: drop unused SolidSyslogFile.h include from the test file — the
forward declaration that arrives via SolidSyslogFileBlockDevice.h is
enough since the test only holds the struct as an opaque pointer.
CodeRabbit accepts:
- Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp: extract a shared
DrainTestFixtureBase (TEST_BASE) holding the file / block-device /
null-security-policy fixture; both TEST_GROUPs derive from it,
eliminating ~14 lines of duplicated setup/teardown plumbing. Matches
the existing BlockDeviceTestBase pattern in SolidSyslogBlockStoreTest.cpp.
- Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp::
DiscardNewestDoesNotLetNewestBypassOldestOnRecovery: assert
!HasUnsent + successfulSends > 1 after the recovery drain window —
catches under-drain (ServiceTickUntilQuiet's fixed cap exhausting
before the store is empty) instead of silently passing on monotonic
order alone.
- Bdd/features/steps/syslog_steps.py::step_client_is_killed: wrap the
post-SIGKILL process.wait in try/except so a degenerate runtime that
blocks even on SIGKILL doesn't leak context.interactive_process.
CodeRabbit declines (replied on threads):
- SolidSyslogStore_IsTransient NULL guard — CLAUDE.md "trust internal
code; don't add fallbacks for scenarios that can't happen." None of
the other Store_* wrappers guard their vtable methods; adding it for
IsTransient alone would mask a wiring bug we want to surface loudly.
- 4-byte payload precondition CHECK_TRUE_TEXT guards — payloadSize is
derived from SOLIDSYSLOG_MAX_MESSAGE_SIZE (>= 512) or hardcoded 64;
the < 4 scenario can't reach the writer. Guards only add noise.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: S08.05 record FatFs/FreeRTOS in CLAUDE.md, DEVLOG, README + linked docs
CLAUDE.md: add SolidSyslogFatFsFile.h row under Public header audiences
(ChaN FatFs adapter, f_sync per write, integrator-supplied diskio.c).
DEVLOG: full S08.05 entry — design decisions (FatFs over Plus-FAT,
Platform/FatFs/ peer pack, f_sync-per-write, graceful shutdown for
BDD power-cycle, unified TeardownAll, ARP-prime on TCP connect,
SYS_EXIT_EXTENDED for AArch32 status propagation, Store_IsTransient
to keep discard-newest honest), the host-side BlockStore drain-
ordering harness that pinned the [1, 11, 2, 3, ...] bug, ChaN
license-compatibility note (BSD-style ⊂ PolyForm Noncommercial
1.0.0 — preserve ChaN's notice in ff.c), deferred items.
README: FreeRTOS support paragraph now reflects UDP+TCP and FatFs
store-and-forward; add SolidSyslogFreeRtosTcpStream + SolidSyslogFatFsFile
to the platform-helpers list; FreeRTOS BDD target row mentions the
new `set store file` / `set shutdown 1` UART commands and the
store_and_forward / power_cycle_replay / store_capacity scenarios
that now run on QEMU.
Bdd/Targets/FreeRtos/README.md: dedicated section on persistent
store-and-forward via FatFs over semihosting — covers the BDD
diskio.c shape, the production-integrator handoff (own diskio.c +
ffsystem.c), and the graceful shutdown contract.
Bdd/README.md + docs/bdd.md: FreeRTOS local-run and per-runner tag
filters caught up to what `ci/docker-compose.bdd.yml` actually uses
(`not @wip and not @freertoswip and not @rtc and not @windows_wip
and (@udp or @tcp)`).
docs/containers.md: cpputest-freertos-cross row now mentions the
FreeRTOS-Kernel / Plus-TCP / FatFs source mounts that the cross
image carries.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #353.
Summary
readHandle/writeHandlepair fromSolidSyslogFileBlockDeviceand keeps a singleOpenHandlethat gets re-pointed only on block-index change. Same-block runs (multi-Append, drain Reads, Append-then-WriteAt during MarkSent) reuse the handle, so the perf shape of the dual-handle design is preserved.SolidSyslogFileBlockDevice_Createtakes oneSolidSyslogFile*instead of two. Cascades through three test files and the two BDD targets.FileFakegains per-FileEntryopenOwnertracking — a second Open by a different instance on an already-owned path tripsTestAssert_Fail. This pins the S27.01 single-handle-per-path invariant as a permanent test-time regression guard.FileFakeTest.TwoInstancesShareFilesystemupdated to Close between the two instances' Opens; a new testOpenWhileAnotherInstanceHoldsPathOpenAssertsproves the assertion fires on violation.ReadFollowedByWriteOnSameBlockDoesNotOpenTwoHandlesmirrors the BlockStore steady-state pattern (write record → read record → write sent flag in place) on one block — would fault on the Read step if the dual-handle shape ever regressed.SolidSyslogStore's public contract is unchanged.Why
Core/Source/SolidSyslogFileBlockDevice.ccached two handles independently, so in BlockStore's steady-state Read↔Write pattern on one block both ended up open on the same path. Most platforms tolerate this; FatFs does not withoutFF_FS_LOCK ≥ 4, whichbfaa39chad to flip on as a workaround. The cleaner invariant — and the one E27 #345 should pin down for the storage layer — is at most oneSolidSyslogFile*open on any given block file path at any moment, by construction. Foundational for the S18.04 flash example and any future block-device driver where a logical block is a single-handle resource.Local checks
FileBlockDevice.cTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests