Guard rebuildForLocation against synchronous re-entry via the poll pump - #2
Merged
Merged
Conversation
rebuildForLocation() holds a blocking HTTP fetch open while pumping
s_poll_fn (wifiLoop -> WiFiManager::process()) to keep the portal alive.
That poll synchronously dispatches web handlers, including
POST /api/map/rebuild (handleMapRebuild), so a second rebuild request
arriving mid-fetch can nest a second rebuildForLocation call inside the
first. Worst case today is a redundant fetch and tmp-file churn (release/
init are idempotent), but there was no guard against it. Flagged by Opus
review 2026-07-06 as a pre-existing, unguarded re-entrancy hole.
Adds services::map::RebuildLock (include/services/rebuild_guard.h), a
tiny Arduino-free RAII non-recursive busy flag, same idiom as
ui/color_blend.h, so it's natively testable. Only the lock instance that
actually acquired the flag clears it on destruction -- a failed nested
lock is a no-op on destruct, which is what makes the outer call the sole
owner of the flag across all of rebuildForLocation's ~10 return paths.
rebuildForLocation() now takes the lock first thing and returns the new
RebuildResult::kBusy if already held. web_app.cpp's handleMapRebuild
treats kBusy exactly like kBuilding in the JSON response
({"ok":false,"building":true,...}), so the webapp's existing auto-poll
handles it with zero webapp changes.
New native test (test/native/test_rebuild_guard) covers: first lock
acquires; nested lock fails while held; a failed nested lock's
destruction does not clear the flag (the regression this design
prevents); and the outer lock's destruction clears the flag for a
fresh acquire. Header-only, no build_src_filter change needed.
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
1 participant
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
services::map::rebuildForLocation()blocks on HTTP while pumping the injected poll fn (wifiLoop→ WiFiManagerprocess()), which synchronously dispatches web handlers mid-rebuild — including POST /api/map/rebuild itself — nestingrebuildForLocationinside itself. Pre-existing; flagged by the Opus review of PR #1 (2026-07-06). Worst case was a redundant fetch + tmp-file churn (release/init are idempotent), but it deserved a guard.Fix
RebuildLockininclude/services/rebuild_guard.h(natively testable, same idiom asui/color_blend.h). A failed nested acquisition is a no-op on destruction — the outer lock alone clears the flag, on every return path.RebuildResult::kBusy; nested/concurrent calls return it immediately.handleMapRebuildmapskBusyto the existing{"ok":false,"building":true,...}shape, so the webapp's auto-poll retries with zero webapp changes.Verification
pio test -e native: 58/58 (54 baseline + 4 new), run independently by orchestrator and implementer.pio run -e supermini: SUCCESS (RAM 17.6%, Flash 42.3%).RebuildResultbranch mishandleskBusy, and the webapp poll loop terminates (bounded retries).🤖 Generated with Claude Code