[ritzylab-dev]

The bug

The POSIX flash HAL's file reader returned "zero bytes" for every failed open and never checked for a read error, so hal_flash_read()
reported success with fabricated content, and hal_flash_write() / hal_flash_erase() — which read the whole store before rewriting it —
replaced an unreadable existing store with an empty one plus the new bytes. The NVM store's unreadable-flash protection therefore held only
against the test hook, not against a real read failure.

What breaks, and when

A transient filesystem error at boot, or a store the process cannot read, becomes a device that believes it has no configuration — and the
next write makes that permanent.

What changed and why it fixes it

An absent backing file is the one legitimate "empty" case (ENOENT). Every other open or read failure is HAL_ERR_IO, observed through the HAL
fault event; read returns it, and write and erase return it without touching the store. The read contract is now stated in the HAL header:
success means stored bytes (absent and erased regions read as the implementation's absent/erased value — callers detect "nothing stored" with
their own framing, as the NVM store does with magic and CRC), and HAL_ERR_IO means the store could not be read for any reason other than its
absence.

The write path checks flush, fsync and close as well as the write: a failure at any step up to and including the rename removes the temp
file and leaves the previous store in place. After the rename the containing directory is fsync'd, so the new directory entry is durable
too; the header now states this, and the error contract on either side of the publication point. A backing file larger than the
implementation holds is refused with HAL_ERR_INVALID instead of being rewritten truncated.

Ranges are validated by subtraction before any addition or pointer arithmetic, for read, write and erase alike: a wrapping interval such as
hal_flash_erase(1, SIZE_MAX) used to reach memset with SIZE_MAX and is now HAL_ERR_INVALID with nothing touched; an out-of-capacity read
is HAL_ERR_INVALID instead of silent zeros; and len == 0 within capacity is HAL_OK and does not rewrite the store. A store or temp path that
does not fit the path buffer is refused, never shortened: the HOME-derived default path is now checked like the explicit one, before any
directory is created.

A read never creates a directory. Every successful write or erase syncs the store file, the store's directory and that directory's parent,
in that order, before reporting success, so a directory created on first use (the default $HOME/.embediq, or an explicit path's parent) is
durable by the first write that succeeds; a failed sync returns HAL_ERR_IO and is simply performed again by the next nonzero write or erase,
whichever store was written in between. No record of which directories the HAL created is kept.

Evidence

A new HAL test suite reproduces the failures with a real unreadable store (a directory as the backing path; a permission-denied file) — no
injection hook — and pins the absent/erased/short-read semantics; an NVM-level test shows a real read failure now puts the store into its
unreadable state and recovers after flush. Further tests cover the wrapping and boundary ranges, the zero-length policy, both path
refusals (nothing created), and a directory-sync failure after the rename (a host-only hook): the write reports HAL_ERR_IO with exactly one
observation and the new store published. Full suite under ThreadSanitizer: 0 findings; Linux CI build and TSan jobs green.

Limits stated

The POSIX implementation holds the whole store in a 64 KiB stack buffer for read-modify-write; a store larger than that is now refused
rather than truncated, and the buffer itself is unchanged here. A flush failure (disk full) during a write is handled but not unit-tested.
A directory-sync failure after the rename returns HAL_ERR_IO with the new contents already visible and their durability not guaranteed; no
rollback is attempted; the same holds when the store directory's parent cannot be synced. These tests establish call order
and error handling, not a physical power-cut proof.

🤖 Generated with Claude Code

[ritzylab-dev]

Consolidated review at cb25e2e: hold merge. Read-error propagation, oversized-file refusal and NVM fail-closed recovery work in the focused local tests. Four closure items follow; the last two are pre-existing adjacent defects found during the requested whole-family audit, not newly introduced regressions.

1. [P1] Fix the Linux build failure in the new tests. test_hal_flash_posix.c:84–87: GCC sees a potentially 255-byte g_dir plus a suffix written into another 256-byte array. Both jobs in CI run 41964864490 fail with -Werror=format-truncation on all four constructions, before tests run. Derive destination capacities from the directory buffer plus the longest suffix and terminator, or construct/check paths with bounds GCC can verify. Keep warnings enabled. Audit the sibling test-path constructions in this commit. Closure: normal and TSan Linux builds/tests pass at the revised head.

2. [P1] Persist the renamed directory entry before reporting durable success. hal_flash_posix.c:178–189 syncs the temp file but returns OK immediately after rename. The public HAL/NVM durability promise is stronger than that: file fsync does not ensure the directory entry reached storage; the Linux fsync manual explicitly requires a directory fsync as well. The PR body discloses the limitation, but that does not satisfy the public contract. Obtain/check the containing-directory descriptor, sync it after successful rename, and account for any parent directory created by this operation (or require/prevalidate an already durable parent). Align the error contract: failures before publication preserve the old store; a directory-sync failure after rename returns ERR with durability uncertain and the new store possibly visible, rather than falsely promising that every error preserves the old contents. Do not attempt an unproven rollback after publication. Tests should verify operation order, exactly one HAL observation per failed operation, pre-rename failure preservation/cleanup, directory-sync failure returning ERR, and NVM retry behavior. These tests establish call/error handling, not a claimed physical power-cut proof.

3. [P1, pre-existing adjacent] Reject wrapped write/erase intervals before memory access. The write and erase checks add addr + len before validating. hal_flash_erase(1, SIZE_MAX) wraps needed to zero and reaches memset with SIZE_MAX. An independent probe against this exact source, instrumented with ASan, aborts with negative-size-param: (size=-1) in hal_flash_erase. Use subtraction-based checks (addr <= capacity, then len <= capacity - addr) before addition or pointer arithmetic, for both operations. Add regressions for overflow, last valid byte, exact end, out-of-capacity ranges and the chosen zero-length policy; invalid calls must return HAL_ERR_INVALID, observe once and leave the backing store untouched. This arithmetic predates the PR, but it is a confirmed memory-safety gap in the capacity-check family, not a speculative alternative design.

4. [P2] Apply the new path refusal to the default HOME-derived path as well. hal_flash_posix.c:114–122 uses precision-limited %.*s, so snprintf reports the shortened output length and s_path_invalid stays false. The default branch can create/write a different path. Independent macOS probe with a controlled /private/tmp HOME: home_len=1008 resolved_store_len=1019 path_invalid=0, followed by write_rc=0; the shortened-home store exists. Validate the complete requested HOME+suffix and temp path before any mkdir/open; do not silently shorten the input. Add boundary tests for explicit EMBEDIQ_NVM_PATH and default HOME paths, including a temp suffix that alone exceeds capacity, and assert invalid paths create no shortened store. The shortening behavior predates the PR; the newly advertised path-refusal mechanism currently covers only the explicit environment-path branch.

Verification completed: isolated macOS build succeeded; focused ctest 3/3 (HAL flash, NVM, HAL observation) passed. No claim of green Linux CI or a clean sanitizer suite: both current Linux jobs fail compilation, and the independent boundary probe intentionally exposes the overflow failure. Source and developer checkout untouched; no merge.

Please address these as one consolidated correction with the stated tests, then obtain exact-head green Linux build/TSan. No further optional redesign is requested here.

[ritzylab-dev]

All four items are addressed in 2593739. (1) The test paths are sized from their derivation: the temp-dir buffer is prefix + int pid + NUL, and the store paths add the longest suffix. They are built with a bounded %.*s, and every snprintf result is checked in setup. Linux CI run 41971926269 at 2593739 is green for both build and tsan; the only warnings in the log are the existing ones in vendored paho, and ctest is 35/35. (2) write_file now fsyncs the containing directory after the rename. The header states the error contract on both sides of the publication point: a failure before the rename preserves the old store and leaves no temp file; a directory-sync failure after it returns HAL_ERR_IO with the new store visible and durability uncertain, and nothing is rolled back. The durability promise assumes the parent directory already exists, since first-use creation is a convenience. A host-only hook test shows that path returning HAL_ERR_IO with exactly one observation and the new contents published. (3) read, write and erase all validate addr <= capacity && len <= capacity - addr before any arithmetic. Tests cover (1, SIZE_MAX), (MAX-1, 1), (MAX-4, 4), (MAX-3, 4), (MAX, 0) as a no-op without a rewrite, and (MAX+1, 0); each refused call emits one observation and leaves the store byte-identical. On cb25e2e the same test aborts under ASan with negative-size-param: (size=-1), and the fix is ASan-clean. (4) The default branch formats the whole HOME and is checked like the explicit path, with no %.*s; mkdir runs only after the check passes. Tests cover a HOME of PATH_MAX-20 chars (refused, nothing created) and an explicit path whose .tmp alone does not fit (refused). Your boundary-probe.c wasn't available to me, so I rebuilt an equivalent probe. Against cb25e2e it reproduces your result: home_len=1008 write_rc=0, with the store written under the shortened prefix. Against the fix it gives write_rc=-1 with nothing created, and erase(1, SIZE_MAX) rc=-1 with no abort. One gap: the NVM retry behaviour after a post-rename directory-sync failure is not tested, because test_fb_nvm.c was outside this round's scope.

[ritzylab-dev]

Recheck at 2593739. Linux compile failure, overflow-safe range checks and both path-length branches are resolved. Directory-sync errors after publication now return an honest error without an unsafe rollback. One part of the original durability finding remains: first-use directory creation. Hold merge for that item.

[P1] Do not return durable NVM success for a directory the HAL created without syncing its parent. hal_flash_posix.c:130–131 creates $HOME/.embediq even during the first read; ensure_parent_dir can also create an explicit-path parent. Later sync_store_dir only syncs the store’s own directory. A source-interposition probe using the real NVM calls and real filesystem records:

first_set=0 directory_syncs=1 home_parent_syncs=0

Here nvm__load_state() creates .embediq, then embediq_nvm_set() returns OK without syncing HOME. The file/entry inside .embediq is synced, but the entry naming .embediq is not. A power failure can therefore lose the directory holding an acknowledged first configuration. This is the first-use case explicitly identified in the prior review, not a new adjacent finding. The HAL header’s new pre-existing-directory assumption does not carry through the unconditional NVM success contract or the default first-use call path; ordinary callers neither provision nor establish durability of that directory.

Closure: make the parent entry of each directory created by the HAL durable before reporting successful persistence. Account for creation during resolve_paths/read as well as during write. Preserve correct retries: if mkdir succeeds but syncing its parent fails, a later EEXIST must not bypass the still-needed parent sync. Keep one HAL error observation per failed HAL call, and propagate failure to NVM. Avoid changing unrelated code.

Required targeted regressions: start with an existing HOME but no .embediq; run normal NVM load/set and verify the new directory’s parent is synced as well as the store directory, with success only after the necessary syncs. Repeat for an explicit path whose immediate parent is auto-created. Inject failure of the newly-created-directory parent sync: no false success; after recovery, retry must perform the needed sync even though mkdir now sees EEXIST, and preserve/reload the data. Keep pre-rename versus post-publication error semantics distinct. These check the durability protocol, not physical power-cut survival. Update the first-use disclaimer after implementation so the public contracts agree.

The omitted NVM retry case was independently verified; it is not an additional blocker: with an already existing directory, the first identity provision hits the directory-fsync hook, returns ERR, then provision retry writes once and the identity reloads correctly:

failed_provision=-1 retry=0 retry_writes=1 get_after_reload=0 identity_ok=1

Source of both probes is saved in the review workspace as reviews/pr-189/retry-and-first-use-probe.c; it wraps fsync only to count which directory is synced, then calls the real fsync. A durable in-repository regression is still worthwhile, but there is no demonstrated NVM retry logic failure.

Verification: focused local ctest 3/3 passed (HAL flash/NVM/HAL observation). Exact-head CI 41971926269: normal 35/35, TSan 35/35, 0 report files / 0 findings / 0 runtime aborts. No other blocking issue identified in this recheck. No source changes or merge performed.

[ritzylab-dev]

Added test_set_after_directory_sync_failure_retries_and_persists (test_fb_nvm.c). A directory-sync failure fails the persist with one NVM_PERSIST_FAIL carrying the HAL code, the published store already holds the new value, and the next persist carries the change, through set and through flush. It passes on the current head because it guards the contract rather than reproducing a regression.

[ritzylab-dev]

The recheck item is fixed in 7a06a72 (failing tests at 3d9c6ac). resolve_paths has no side effects any more: a read creates no directory, and the store's directory is created only by ensure_parent_dir in the write path. When that call's mkdir succeeds it records the directory as pending. After the rename and the store-directory fsync, a write to that store fsyncs the pending directory's parent. A failed parent sync returns HAL_ERR_IO with one observation and the new store visible, and the directory stays pending, so the next write to the same store syncs it even though mkdir now sees EEXIST. A pending directory that belongs to a different store is left for that store. Both disclaimers (hal_flash.h and the HAL file header) now state this contract. Your retry-and-first-use-probe.c, rebuilt against this checkout: on the previous head first_set=0 directory_syncs=1 home_parent_syncs=0; on the fix first_set=0 directory_syncs=2 home_parent_syncs=1; the retry case is unchanged (failed_provision=-1 retry=0 retry_writes=1 get_after_reload=0 identity_ok=1). New tests: test_first_use_read_creates_nothing_and_write_syncs_parent, test_first_use_parent_sync_failure_then_retry, test_explicit_path_autocreated_parent_is_synced and test_pending_directory_belongs_to_its_store (HAL suite, 67/67), and test_first_configuration_is_durable_only_after_parent_sync (NVM suite, 34/34). All of them fail on the previous head. Linux CI run 41975010526 at 7a06a72 is green for build and tsan (35/35 each, no warnings outside vendored paho). Locally, TSan -j8 ×3 gave 0 reports and ASan on both binaries is clean.

[ritzylab-dev]

Recheck at 7a06a72: normal first use, same-store retry and NVM retry are now addressed, and exact-head CI is green. One confirmed path-switching defect remains in the new pending-directory bookkeeping; hold merge.

[P1] Creating a second directory must not discard the first directory’s unsatisfied sync. hal_flash_posix.c:111–112 unconditionally overwrites the sole s_pending_dir whenever mkdir succeeds. Reproduction with A and B under different parents:

1. Write A at <parentA>/newdir/store, with its parent sync injected to fail. A returns HAL_ERR_IO and is pending.
2. Write B at <parentB>/newdir/store, where B’s directory also does not exist. Its mkdir replaces the pending path; its successful parent sync clears the slot.
3. Retry A. mkdir sees EEXIST; A returns HAL_OK without ever syncing parentA.

Independent public-HAL probe against this exact head:

A_failed=-4 A_pending=1 B_new_write=0 pending_after_B=0 A_retry=0 A_retry_parent_syncs=0

A’s directory entry remains unconfirmed even though its retry reports durable success. Using different parents is essential: syncing a shared parent could incidentally make both entries durable. The added test uses B’s already-existing directory, so its mkdir never replaces the pending slot and does not catch this case.

Recommended bounded fix: avoid pending-path bookkeeping by syncing the current store directory’s parent before each successful nonzero write/erase returns, in addition to syncing the store directory. This extra sync removes the lost-slot problem and dependence on the current lexical path matching an in-memory record; it needs no unbounded table or allocation. Update the test-only introspection and implementation comments to match the chosen protocol. If retaining pending state instead, it must never overwrite/evict an unsynced entry; inability to retain it must fail safely, not produce false success.

Closure regression: extend path-switching coverage to newly created A and B under distinct existing parents. Fail A’s parent sync, successfully create/write B, then retry A; prove A’s own parent is synced before HAL_OK. Also cover both parent syncs failing and subsequent retries in both orders if the implementation retains bookkeeping. Keep assertions about actual sync targets, one observation per failed call, unchanged no-side-effect reads and the existing NVM/retry coverage. Do not merely assert a global pending boolean.

Verification: independent focused local ctest 3/3 passed (HAL flash, NVM, HAL observation). CI 41975010526: normal Linux 35/35, TSan 35/35, 0 report files / 0 findings / 0 runtime aborts. Those checks omit the failing sequence above. The other prior review items remain closed; no additional blocker identified in this recheck. No source edits or merge performed.

[ritzylab-dev]

Recheck of 7a06a72 addressed at f43668f by removing the state rather than repairing it: every successful nonzero write or erase now syncs the store file, the store's directory and that directory's parent (computed with dir_of), in that order, before reporting HAL_OK; a failed parent sync returns HAL_ERR_IO with one observation and the new store visible (no rollback), and is simply performed again by the next write or erase to any store. The pending slot, its recording in ensure_parent_dir and the hal_flash__dir_entry_pending hook are gone; no static storage or allocation was added, and since no bookkeeping is retained, the "both parent syncs failing / never evict" coverage does not apply. Your probe against 7a06a72 reproduced A_failed=-4 A_pending=1 B_new_write=0 pending_after_B=0 A_retry=0 A_retry_parent_syncs=0; against f43668f (pending lines removed, sync target added) it gives A_failed=-4 B_new_write=0 A_retry=0 A_retry_parent_syncs=1 A_parent_synced=1. New test test_two_created_directories_under_distinct_parents_both_become_durable follows your sequence with newly created pa/newa and pb/newb and asserts the actual sync target (last_parent_synced == pa on A's retry, then the mirror for pb); it failed on 7a06a72 (A retry=0 parent_syncs=0 synced_pa=0) and passes now. test_every_write_syncs_store_dir_and_parent pins two writes → two parent syncs, a read → none, an erase → one. The path-switching test that used an existing B is superseded and removed; the first-use, retry and NVM assertions are kept. HAL flash 74/74, NVM 34/34, HAL obs 19/19; TSan ctest -j8 ×3 0 findings; ASan clean; CI 42167032138 build and TSan 35/35.

[ritzylab-dev]

Final recheck at f43668f: no blocking findings; ready to merge. All prior review findings are closed.

The write/erase helper now performs file sync, rename, store-directory sync and parent-directory sync before reporting successful persistence, without retaining a pending-directory slot. This removes the path-switching loss of retry state. Post-publication sync failure still returns HAL_ERR_IO with one observation and no unsafe rollback; a later nonzero write/erase repeats the protocol. Reads remain free of directory-creation/sync side effects, and the previously reviewed range/path/read-error protections remain intact.

Independent reproduction of the previously failing distinct-parent sequence against the exact head now passes:

A_failed=-4 B_new_write=0 A_retry=0 A_retry_parent_syncs=1 A_parent_synced=1

The regression tests use distinct parents and verify the actual sync target in both directions; the every-write/erase test verifies reads do not increment the count. Using the post-write count as the read baseline is correct. Additional multi-pending-slot tests are no longer a closure requirement because that bookkeeping has been removed; the per-call failure/retry paths remain covered.

Independent isolated local checks: focused ctest 3/3, HAL flash 74/74 assertions, NVM 34/34, HAL observation 19/19. Exact-head CI 42167032138: normal Linux 35/35, TSan 35/35, 0 report files / 0 distinct findings / 0 runtime aborts. No independent full-suite, sanitizer or physical power-cut rerun claimed this turn. These tests verify the sync protocol and error handling within the supported filesystem guarantees.

No further revision requested for #189. Developer checkout untouched; no merge performed.