Repository navigation
Keep future-dated records at init instead of sweeping them away - #16
Open
mclueppers wants to merge 2 commits into
Open
mclueppers wants to merge 2 commits into
mclueppers wants to merge 2 commits into
Conversation
init() calls sweep(), which deleted every record whose timestamp is ahead of microStore::time(). On a device without a real-time clock that clock is millis() plus an offset the application persists periodically (microReticulum writes it every 10 minutes), so after a reboot it resumes from the last saved offset. Every record written after that save is then "in the future" and is deleted at boot. Those are the newest records in the store: on an ESP32-S3 running microReticulum, the paths and known destinations learned in the last minutes before each restart, or all of them if the device restarts before the first offset is saved. Clamp those timestamps to the current time in the in-memory index instead of erasing the records. A record with an impossible timestamp now ages from the boot that noticed it, so TTL expiry stays bounded and nothing is lost because the clock went backwards. Nothing is written to flash, so crash safety is unchanged. Adds test_file_store_future_timestamps_survive_reload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
FileStore::init()deletes every record whose timestamp is ahead ofmicroStore::time(). On devices without a real-time clock that clock resumes from the application's last persisted offset after a reboot (microReticulum saves it every 10 minutes), so the records written since that save are deleted at every boot. Those are the newest records in the store; through microReticulum they are the most recently learned paths and destinations. A device that reboots before its first save loses everything. Fixes #.Solution
sweep()now clamps a future timestamp to the current time in the in-memory index instead of erasing the record. A record with an impossible timestamp ages from the boot that noticed it, so TTL expiry stays bounded and no data is lost because the clock went backwards.Trade-off: the on-disk timestamp is left as it was, so a record whose stored time is still ahead of the clock is clamped again on the next boot. Its TTL restarts at each boot until the clock passes the stored time. I preferred that to rewriting records at init, which would add flash writes and a crash-safety question to every boot.
The clamp runs at
init()only, between loading the index and themax_recsprune, so a clamped record counts as the newest when the cap picks what to evict. An index rebuilt mid-session (after compaction, or the in-session recovery added in bed1195) reloads the stored timestamps, so such a record holds its future timestamp until the next boot. That is safe:is_ttl_expired()already treats a future timestamp as unexpired, and the prune keeps it as one of the newest records.Note on 5ade7ff: that commit moved three tests' timestamps into the past to fit the deleting sweep. With this change those tests pass either way, and the past timestamps are kept as they are.
Compatibility
sweep()is private; its return value now counts clamped records)Testing
Platforms tested (and how):
pio test -e nativeon the branch with current master (d5282e6) merged in, 113/113. The newtest_file_store_future_timestamps_survive_reloadfails on master (Expected 2 Was 0) and passes on the branch.Platforms not tested, and why: nRF52 (no hardware). The embedded environments in this repo's
platformio.iniwere not built locally and are left to CI.Docs
docs/microstore_user_guide.mdupdatedCloses #15.