Repository navigation
Fix: snapshot walk yield and row budget - #143
Merged
Merged
Conversation
…ading is not The path-table walk in refreshSnapshots() rests on ++ touching only the in-memory index, so a resumable row cursor can reach position N for one read rather than N. microStore is pinned to a branch, not a commit, and has already regressed that once, so assert it here — on TypedStore over a counting fake and on a real FileStore over a counting RAM filesystem. Red means upstream moved.
…it was starving The path-table walk's 400 ms budget was consulted only once the snapshot's rows were full, so the row half ran unbounded — and because a row is filled only by an entry that still has a receiving interface, a table of the dead entries the sweep exists to remove was walked to the end with no bound in force at all. The free step-over path jumped past the check too. The walk also fed the watchdog without ever yielding. esp_task_wdt_reset() answers for the running task only, and the task being starved was Arduino's loopTask: priority 1 against the RNS task's 3 on the same core, so it did not run at all while the walk did, and its watchdog is the one that fired. The walk now gives the core up every 100 ms of walk time — an elapsed-time cadence because per-record cost varies by a factor of five, and not the ring drains' kFeedEvery, which at 16 records is longer than the whole budget. Letting the budget stop the walk in the row half puts the stop position ahead of the sweep cursor, so the cursor is now monotonic within a cycle rather than being dragged to the front of the table. The per-position rule and both cadences move to src/rns/SnapshotWalk.h, which test/test_snapshot_walk drives directly: RnsTransport.cpp cannot be built for the host, so a rule that lived only there could not be pinned.
…ed goes Review of the bounded walk found three things. The header credited the yields with keeping loopTask alive. vTaskDelay(1) blocks until the next tick, so three of them are worth 0-3 ms a pass; what saves the task is the budget ending the walk well inside SNAPSHOT_INTERVAL_MS, which returns the RNS task to its 10 ms sleep hundreds of times a window instead of running passes back to back. That relation is now a static_assert rather than prose. The in-loop watchdog feed could not fire: sixteen records outlast the whole budget at every per-record cost measured here, and a slower filesystem — the case it was justified by — reaches it less, not more. Dropped; the pass is bounded and has a feed either side of it. A sweep cursor left past a table that shrank under it never recovered: nextSweepPos() only moves forwards, so the sweep latched part-way through and the cycle never closed. The cursor is now resumed against the table's size. The stall the bound leaves — a slow table publishing the same prefix and never advancing the cursor — is driven by a test that documents it, for the row cursor to flip.
The feed at the end of the snapshot walk fired before remove_paths(), so up to 64 store removals ran between two feeds — each one a tombstone flushed to flash plus an index entry flushed after it. Bounded by count is not bounded by time, which is the fallacy the walk's own budget exists to kill. Also corrects five comment claims that were false or overreaching, chiefly two: the sweep cursor is an ordinal, so an erase ahead of it does shift the tail; and nomadAddress() and lxmf() do enter RNS:: from other tasks, so the invariant narrows to "nothing another task calls mutates the store". Yield counts are driven by a test now rather than divided out, one of the quoted figures having been wrong.
… table Bounding the walk stopped the watchdog reboots and left a permanent stall: rows were re-collected from the front on every pass, so on a table slow enough to spend the budget before they filled, the same short prefix was read for ever — the cursor never advanced, the sweep cycle never closed and dead entries beyond the prefix were never found. Row collection now carries a cursor of its own, under the two rules the sweep cursor already had, and both are one definition rather than two. A pass cannot be stopped by the budget until it has read one record, so it always reaches its own cursor and always moves it forward — the defence against the prefix ever costing anything again. Only a whole list is published: a budget-stopped pass keeps its prefix in staging rather than showing four paths on a node holding eighty. The count beside it stays the table's own size and is exact every pass. The removal of dead entries is bounded by the same clock and budget as the walk, instead of running up to 64 flash write-and-sync pairs unwatched. New in the heartbeat and in /api/status `diag.tables`: the position the walk reached, how many passes the budget has ended, whether the published rows are a whole cycle, and the worst pass time, which was not served before.
The gap between two ring drains is a whole snapshot pass now, not the sweep half, so on a table too big for one pass it opens every five seconds and rx_drain_capped climbs for ever. Five surfaces called that climb the reading that is *not* expected, and pointed at the radio. They point at snap_budget_stops instead. Also: the Mesh list and the map said "nothing announced yet" on a node whose first cycle had not closed while Home said 500 peers; the API gave two opposite readings of one climbing counter; and snap_walk_pos was documented as "cut off" for the ordinary row-cap stop, which is neither cut off nor resumed from there. The row cursor's throw rule moves into SnapshotWalk.h as resumeRowCursor() so the host can drive it, the removal loop counts attempts rather than successes and runs back to front so a short budget spends itself on the entries the next pass will not re-read, and soak.py records the four snap_* fields it was fetching and discarding.
…eed share Two bounds the budget could not give. The budget is what this file chooses and is static_asserted at a quarter of the interval; what a pass *costs* is neither. It is the budget plus the record in hand, and — because a pass that has read nothing may not be stopped at all — the prefix it steps over plus one record if microStore's iterator ever loads on ++ again, which is what the abandoned branch ran on. Past a second or so the five-second gate is a no-op and passes run back to back, which is the shape that took a T-Beam down. Rns::nextIntervalMs() schedules the next pass at four times the measured cost of the last, capped at the minute between sweep cycles so a slow node's cleanup does not go with it. Inert at every per-record cost this tree has measured, which is the point: it is a guard, not a knob. Served as snap_interval_ms, on the heartbeat and in the soak CSV, because a backed-off node's readings are older than they look. The sweep had the remainder of the budget after the rows, and the rows restart from the front of the table every cycle while the sweep does not — so the cursor moved in one pass per row cycle, taking a sweep cycle from pass 35 to pass 115 on a 200-entry table, and not at all where reads per pass divide the row cap exactly. The rows now hand half the budget over whenever taking it all would leave the sweep nothing: first dead entry on pass 10, cycle closed on pass 23, and the cursor advances on every sweeping pass. Row collection carries its own stop position and its own "cut short" so a cycle the share ended stays a prefix instead of being published as a whole list.
… it too The rule was spelled out twice — UiDestinations.cpp and UiNav.cpp each wrote `!snap.tables.snapRowsWhole && snap.pathTotal` — and the surface that most needed it had neither copy: the web portal rendered "Transport node · 500 known paths" over an empty list with no hint. It is now Snapshot::stillReadingPaths(), asked by both display pages and mirrored in the portal off the same two fields it already receives. Also: the sweep's share guarantee now holds against mixed record costs and not only uniform ones, with both boundaries driven rather than argued; the back-off log no longer alternates warning and recovery every five seconds on a cost oscillating across the quarter; the sweep interval is SNAPSHOT_SWEEP_INTERVAL_MS in one place rather than 60000 in five; and docs/development.md lists test_snapshot_walk.
…s left RnsTransport::paths() lost its last two callers to snapshot() earlier in this branch: an exported copy loop under a lock, kept for nobody, on a board at 97 % of its OTA slot. Removed. The rest are statements that had drifted from what the code does. The console still called rx_drain_capped transient and blamed the sweep, where it is the whole walk now and climbs for as long as the table stays too big — it was missed when the other five surfaces were corrected, and on a solar node it is the surface in hand. api.md's example had tables.paths and transport.path_count disagreeing about a figure both read from one variable under one lock. The "no compaction on this store" argument stopped at "never called from src/" without saying why the library's own caller for that setter never runs. And two comments claimed figures that are narrative rather than asserted.
…mes its triggers Two statements this branch's own comments overstate. The path-table note claimed "no TTL is set", which made this sweep the only thing that ever removes a stored path. Every path carries a per-record TTL — Transport::inbound passes AP_PATH_TIME, ROAMING_PATH_TIME or DESTINATION_TIMEOUT to put() by the receiving interface's mode — and get() and exists() each drop an expired record from the index, so has_path() removes one too. What is not set is a store-wide TTL *policy*. The residual is real but smaller: expiry happens on access and nothing scans for it, so a destination nobody asks after again is never reached. The 70-90 B per path stays, now with its derivation. nextIntervalMs() called 562 ms against the 1250 ms quarter "comfortable" and named no route to its own branch. There are three. A budget of 1250 passes walkBudgetFitsInterval() and takes the margin to zero, so every budget-stopped pass would reschedule. The budget is read between records and the removal attempts one entry past it regardless, on a store that may be on the SD card. And walkMs is wall clock, so it is charged for the radio task preempting the pass at priority 5 on the RNS task's own core. The margin is now written as SNAPSHOT_INTERVAL_MS / kPassShareDiv - kWalkBudgetMs rather than as 850. Comments only; no code changed.
There was a problem hiding this comment.
🟡 Changes recommended
Snapshot state can be misreported, failed slow passes can retry immediately, and soak summaries omit important conditions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds bounded, resumable path-table snapshot walking to prevent task starvation while preserving complete published snapshots.
Changes:
- Introduces budget, yield, cursor, and scheduling policies.
- Exposes snapshot health through diagnostics, UI, documentation, and soak tooling.
- Adds extensive native and soak tests.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
Documents dependency-cache constraints. |
data/index.html |
Shows snapshot-reading status. |
docs/api.md |
Documents snapshot diagnostics. |
docs/development.md |
Lists new native tests. |
docs/troubleshooting.md |
Updates drain-cap guidance. |
platformio.ini |
Adds native microStore dependency. |
src/Config.h |
Defines sweep interval. |
src/main.cpp |
Expands snapshot logging. |
src/net/WifiManager.cpp |
Exposes snapshot diagnostics. |
src/rns/RnsTransport.cpp |
Implements resumable bounded walks. |
src/rns/RnsTransport.h |
Extends snapshot metadata API. |
src/rns/SnapshotWalk.h |
Defines walk policies. |
src/sys/Maintenance.cpp |
Updates status explanation. |
src/sys/RingDrain.h |
Separates drain and walk cadence. |
src/ui/lvgl/UiDestinations.cpp |
Distinguishes pending path rows. |
src/ui/lvgl/UiNav.cpp |
Adds pending-path plot caption. |
test/test_ring_drain/test_main.cpp |
Updates cadence test terminology. |
test/test_snapshot_walk/test_main.cpp |
Tests walk and scheduler behavior. |
test/test_typed_store_iterator/test_main.cpp |
Verifies lazy iterator reads. |
tools/soak.py |
Records and summarizes diagnostics. |
tools/tests/fixtures/soak-header.csv |
Extends expected CSV schema. |
tools/tests/test_soak.py |
Tests snapshot diagnostics. |
Review details
Suppressed comments (1)
tools/soak.py:745
- This detects only a transition during the CSV. If collection starts after the node has already settled at (for example) 30000 ms, all values are equal and the summary suppresses the interval even though it is the value needed to explain stale snapshot data.
snap_walk_max_mscannot substitute for the current schedule because it is a since-boot high-water mark. Report any non-default interval as well, and add a constant-backed-off test case.
gaps = [v for v in (num(r.get("snap_interval_ms")) for r in up) if v is not None]
if gaps and max(gaps) != min(gaps):
print(f" ⚠ snap_interval_ms up to {max(gaps):.0f} ms (from {min(gaps):.0f}) — this "
- Files reviewed: 22/22 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1860
to
+1864
| tb["snap_walk_max_ms"] = t.snapWalkMaxMs; | ||
| tb["snap_walk_pos"] = t.snapWalkPos; | ||
| tb["snap_budget_stops"] = t.snapBudgetStops; | ||
| tb["snap_rows_whole"] = t.snapRowsWhole; | ||
| tb["snap_interval_ms"] = t.snapIntervalMs; |
| // which point the gate is the only thing that can keep passes from running | ||
| // back to back — the budget bounds what this file chooses and not what a pass | ||
| // comes to (Rns::nextIntervalMs). | ||
| if (millis() - sSnapAtMs < sSnapIntervalMs) return; |
The handler asked tables() for diag.tables and snapshot() for the path rows four hundred lines later: two acquisitions of the snapshot lock with a refresh free to land between them, so one reply could carry snap_rows_whole from one pass beside path_count and rows from another — the exact pair Snapshot::stillReadingPaths() combines. One Snapshot now serves both halves.
sSnapIntervalMs was written only after the walk and the removal had both returned, so a pass that threw left the interval it was gated on standing — and a pass that had already outrun that interval was due again the instant it left. The scheduler is now asked on the way out whichever way the pass leaves, through the same Rns::nextIntervalMs(), and the gap it buys runs from where the pass stopped. And the flag that says the published path rows are a whole cycle no longer latches for the life of the run. The empty table a node boots with closes a cycle on the first pass, and that flag then called the empty list whole for ever: a node with two hundred paths part-way through a cycle read "nothing announced yet" on every surface. A closed cycle is now credited to the table it closed over and answers for that table only — unless the row cap ended it, since the cap and not the cycle bounds the list, which is what keeps a large node from reading "still reading" for most of its life.
It counts since boot, so a restart mid-run zeroed it, the last sample read zero and the whole finding disappeared; the first-to-last range it printed spanned the reset as if the counter had gone backwards. Segmented by boot_count now, through the same splitter the announce counter uses. The snap_rows_whole warning says what false means now that it is not a latch.
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.
No description provided.