Live coverage robustness: re-add after remove, request resume on enable, tile dimension caps - #185
Merged
Conversation
added 12 commits
August 5, 2026 17:45
Clear sources_ via QObject::destroyed signal on layer removal
…ined tile-byte ceiling (#168)
Removing the live coverage layer left its sources_ entry set forever, so updateTopics() never respawned it — only a full camp restart brought it back (2026-07-23 field incident). Track spawned layers in a std::set and erase the entry from the layer's QObject::destroyed signal, factored into a protected trackSpawnedLayer() seam with a public isSourceTracked() accessor so the headless regression test exercises the real set and the real connect wiring (updateTopics() needs a live Node ancestor). Closes #168
…ile on enable (#169) The transient-local depth-1 catalog subscription delivers its latched sample exactly once; if it landed while the layer was disabled (the startup settings-restore race) it was discarded and — the boat's catalog being stable — never re-delivered, so tile requests never resumed until another camp restart (2026-07-23 field incident). Buffer every catalog in last_catalog_ regardless of enabled state and replay it from enableLiveCoverage() after warm-load/subscribe, so anti-entropy reconcile and the resulting requests fire on every enable (startup and in-session re-enable alike). Headless regression test delivers a prune-all catalog while disabled and asserts it takes effect on re-enable; verified to fail without the fix. Closes #169
A single oversized or corrupt coverage-tile message allocated width*height floats per band on the GUI thread straight from the message fields — an unbounded allocation on receipt, the most direct path to the 2026-07-23 operator-station crash (the ADR-0010 eviction budget only counts tiles after residency). handleTile now rejects, with a warning, tiles with zero/oversized edges (> kMaxImageEdge 4096), absurd band counts (> 64), or a combined footprint over 256 MiB (per-edge caps alone still admit ~4 GiB at 64 bands) — before any allocation or state mutation. GUI-thread load bounding (re-render rate-limit, eviction accounting amortization) is deferred to the #154/#155/#156 umbrella per the plan. Closes #170
…ng, replay burst) (#168 #169 #170) Comment-only: document that kMaxImageEdge doubles as the ingest ceiling and that the byte ceiling over-counts duplicate band names (rejection- safe); correct the catalog-resume test's prune-gate rationale (warm-load seeds reconciler version 0, not the tile stamp); note the deliberate full-heal re-request burst on each enable.
There was a problem hiding this comment.
🟢 Ready to approve
The production fixes are targeted, safety-motivated, and backed by new headless regression tests; remaining feedback is limited to minor test robustness improvements.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens the live-coverage subsystem in camp_map_ros following the 2026-07-23 field incident, addressing three failure modes: inability to re-add a removed layer, missed restart/re-enable tile requests due to a latched catalog arriving while disabled, and unbounded allocations from oversized tile messages.
Changes:
- Fix
SonarLiveCacheManagersource tracking so entries are cleared on layer destruction, allowing re-spawn after operator remove. - Buffer the last received catalog even while disabled and replay it on enable to ensure reconcile + requests fire every time.
- Validate tile dimensions/band counts/combined footprint in
handleTile()before allocation, and add headless regression tests for all three fixes.
File summaries
| File | Description |
|---|---|
src/camp_map/ros/live_coverage/sonar_live_cache_manager.h |
Switch spawned-source tracking to std::set, add tracking seam + accessor for testability. |
src/camp_map/ros/live_coverage/sonar_live_cache_manager.cpp |
Track layers and erase tracked bases on QObject::destroyed to permit respawn after removal. |
src/camp_map/ros/live_coverage/sonar_live_cache_layer.h |
Add last_catalog_ buffer to preserve transient-local catalog across disabled windows. |
src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp |
Replay buffered catalog on enable; reject absurd tile dimensions before any allocation/state mutation. |
test/test_sonar_live_cache_manager_respawn.cpp |
New headless test exercising real manager tracking and destroyed-signal erasure behavior. |
test/test_sonar_live_catalog_resume.cpp |
New headless test ensuring a catalog received while disabled reconciles/prunes on re-enable. |
test/test_sonar_live_tile_validation.cpp |
New headless test validating tile dimension/band/byte ceiling rejection and legitimate acceptance. |
CMakeLists.txt |
Register the three new gtests with the same headless harness dependencies/linking as existing tests. |
.agent/work-plans/issue-168/progress.md |
Work-plan progress log capturing plan/review checkpoints for the bundle. |
.agent/work-plans/issue-168/plan.md |
Bundle plan documenting approach, ADR alignment, and test strategies for #168/#169/#170. |
Review details
Suppressed comments (1)
test/test_sonar_live_cache_manager_respawn.cpp:83
- If
makeManager()returnsnullptr(e.g., iftopLevelLayers()ever fails), the test will dereference it and crash. Add a fatal assertion after construction so the test fails with a clear message instead of segfaulting.
Map map;
TestManager* manager = makeManager(map);
const std::string base = "/cube_bathymetry";
auto layer = new QObject();
manager->trackSpawnedLayer(base, layer);
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+41
to
+47
| TestManager* makeManager(Map& map) | ||
| { | ||
| auto layers = map.topLevelLayers(); | ||
| EXPECT_NE(layers, nullptr); | ||
| auto host = new camp::tools::LayerManager(layers, "test_host_tool"); | ||
| return new TestManager(host); | ||
| } |
added 2 commits
August 5, 2026 16:02
…ld its harness (#168) Copilot triage: the non-void helper cannot ASSERT, so guard-return nullptr on a null LayerList and ASSERT_NE at both call sites.
There was a problem hiding this comment.
🟢 Ready to approve
The changes are targeted, directly address the stated incident failure modes, and are backed by focused headless regression tests covering the new behaviors.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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.
Closes #168
Closes #169
Closes #170
What
Live-coverage robustness bundle from the 2026-07-23 BizzyBoat deployment incident — three structural fixes, one atomic commit each:
2544c56):SonarLiveCacheManagernow tracks spawned sources in astd::setand erases the entry from the layer'sQObject::destroyedsignal (factored into atrackSpawnedLayer()seam with anisSourceTracked()accessor). Previously the write-only map made a removed layer un-respawnable until camp restart.09e7443): every catalog is buffered inlast_catalog_even while disabled (the transient-local depth-1 subscription delivers its latched sample exactly once), andenableLiveCoverage()replays it after warm-load/subscribe — so anti-entropy reconcile + tile requests fire on startup-race and in-session re-enable alike.ee21fd0):handleTilerejects (with a warning) zero/oversized edges (>4096), >64 bands, or >256 MiB combined footprint before any allocation or state mutation — closing the unbounded-allocation crash path. GUI-thread load bounding (re-render rate-limit, eviction amortization) deferred to the [Umbrella] camp resource self-monitoring + self-protection #154/ResourceMonitor core — self-sampled footprint + GPU-byte accounting #155/Resource self-protection + back-pressure (eviction budget, load-shedding, GUI watchdog) #156 umbrella per the plan.Test plan
deleteLaterpaths, white-box on the real tracking set), catalog resume (prune-all catalog delivered while disabled takes effect on re-enable — negative-verified to fail without the fix), tile validation (per-edge / band-count / byte-ceiling rejection + acceptance).Ship: recommended— two independent adversarial passes confirmed all three fixes; 4 comment-precision suggestions applied (16e6e53).Authored-By:
Claude Code AgentModel:
Claude Fable 5