Bound import_bag memory: tile eviction (#92) + correct-at-record Welford backscatter (#93)

.agent/work-plans/issue-92/progress.md

@@ -0,0 +1,71 @@
+---
+issue: 92
+---
+
+# Issue #92 — Bound resident-tile RAM in the offline importer (persist-then-drop eviction)
+
+## Local Review (Pre-Push)
+**Status**: complete
+**When**: 2026-06-29 08:53 +0000
+**By**: Claude Code Agent (Claude Opus)
+**Verdict**: changes-requested
+
+**Branch**: feature/issue-92 at `65dfd9f`
+**Mode**: pre-push
+**Depth**: Deep (reason: 1502/-319 over 17 files; cross-cutting resource-management + numerical + binary-serialization)
+**Must-fix**: 3 | **Suggestions**: 2
+**Round**: 1 | **Ship**: continue — three must-fixes, all mechanical doc/comment/ADR reconciliation (no algorithmic defect); fast convergence expected after addressing.
+
+### Findings
+- [x] (must-fix) `extractNodeRecord` docstring still says intensity is "emitted UNCORRECTED" and "per-beam raw set is retained so it is re-derivable when #15" — both false after the record-time correction + Welford change — `cube_bathymetry/include/cube_bathymetry/node.h:195-204`
+- [x] (must-fix) ADR-0007 (and ADR-0001's re-derivability notes) document raw `{raw_intensity, beam_angle}` sufficient-statistic retention + correct-at-extract for cube#15 re-derivability; this PR reverses that decision (record-time correction, O(1) Welford, raw samples discarded) without updating/superseding the ADR — `cube_bathymetry/docs/decisions/0007-mbes-backscatter-store-addendum-phase-b-transition.md`
+- [x] (must-fix) Reload-failure path silently drops this batch's soundings on the failed tile and the comment claims it "retries on the next revisit" (only *future* batches retry; these beams are lost permanently offline) — contradicts the headline "lossless" guarantee; fix the comment and log the dropped count (buffer+re-add optional) — `cube_bathymetry/src/store_import.cpp:1503-1535` (cross-confirmed by both adversarial passes)
+- [x] (suggestion) `Node::setSettledIntensityWelford` writes to `chooseHypothesis()` without the `number_of_samples > 0` gate its siblings (`chosenIntensityWelford`, `extractNodeRecord`) use — harmless today, add for symmetry/robustness — `cube_bathymetry/src/node.cpp:354-362`
+- [x] (suggestion) Scratch-dir name `cube_import_spill_<steady_clock ns>_<per-process atomic counter>` is not collision-safe across concurrent `import_bag` processes (counter resets per process; ns is the only cross-process distinguisher) → potential silent spill cross-corruption; add `getpid()`/`mkdtemp` as the test helper already does — `cube_bathymetry/src/store_import.cpp:1291-1300`
+
+## Implementation
+**Status**: complete
+**When**: 2026-06-29 09:15 +0000
+**By**: Claude Code Agent (Claude Opus)
+
+**Branch**: feature/issue-92 at `6b9a7d7`
+**Addressed**: Local Review (Pre-Push) — When 2026-06-29 08:53 +0000, at `65dfd9f` (Verdict: changes-requested, 3 must-fix + 2 suggestions)
+**Commits**: `31ced91`, `4b4caff`, `50c8b56`, `5334319`, `6b9a7d7`
+
+### Actions
+- [x] (must-fix) `extractNodeRecord` docstring no longer claims "emitted UNCORRECTED" / raw-set retained — rewritten for record-time correction + streaming Welford; the matching `intensity`-field docstring (node.h:56-59) corrected too — `cube_bathymetry/include/cube_bathymetry/node.h` (`31ced91`)
+- [x] (must-fix) ADR reconciled — added a "Phase B.2 addendum (cube#92/#93)" to ADR-0007 documenting the record-time-correction + Welford reversal and the lost in-place re-derivability; marked the now-false Context/Consequences/Tier-2 claims superseded inline, and updated ADR-0001's backscatter re-derivability trade-off note — `cube_bathymetry/docs/decisions/0007-…md`, `0001-…md` (`4b4caff`)
+- [x] (must-fix) Reload-failure path no longer mislabels the loss — `addBatch` now counts soundings centred in un-reloadable tiles and emits a `WARNING` before dropping; the "retries on the next revisit" comments/cerr reworded to state this batch's soundings are not re-processed offline; added `GeoMapSheet::gridIndexForSounding` to support the count — `cube_bathymetry/src/store_import.cpp`, `geo_map_sheet.{h,cpp}` (`50c8b56`)
+- [x] (suggestion) `setSettledIntensityWelford` now applies the `number_of_samples > 0` gate its siblings use — `cube_bathymetry/src/node.cpp:354-362` (`5334319`)
+- [x] (suggestion) Spill scratch dir now created with `mkdtemp` (atomic, retries on collision) — collision-safe across concurrent `import_bag` processes; dropped the steady-clock-ns + per-process counter naming and the now-unused `<atomic>` include — `cube_bathymetry/src/store_import.cpp:1291-1300` (`6b9a7d7`)
+
+**Quick checks**: `ament_cpplint` + `ament_uncrustify` clean on all five touched C++/header files. A full `colcon build` was **not** possible in this worktree — the lower layers (`underlay_ws`/`core_ws`/… providing `marine_autonomy`/`gggs.h`) have no install trees here, so the package cannot be configured. Changes are mechanical (doc/comment + a gate + an additive helper + a `mkdtemp` swap) and were reviewed by hand against current source.
+
+### Next step
+Lifecycle: **Implementation** → **review-code** (re-review the fixes). Hand off to a fresh-context sub-agent:
+
+    .agent/scripts/dispatch_subagent.sh --mode in-process --issue 92 --skill review-code
+
+## Local Review (Pre-Push)
+**Status**: complete
+**When**: 2026-06-29 09:30 +0000
+**By**: Claude Code Agent (Claude Opus)
+**Verdict**: approved
+
+**Branch**: feature/issue-92 at `dbe7b21`
+**Mode**: pre-push
+**Depth**: Deep (reason: ~2000 lines / 20 files; cross-cutting resource-management + numerical estimator + binary serialization)
+**Must-fix**: 0 | **Suggestions**: 5
+**Round**: 2 | **Ship**: recommended — re-review of round-1 fixes; all 3 must-fix + 2 suggestions confirmed addressed, no new must-fix, remaining items are doc-accuracy / observability robustness.
+
+### Findings
+- [ ] (suggestion) `restoreSpilledSamples` is silent on a missing/corrupt spill while `persistBackscatterTile` plain-overwrites the tile → out-of-band spill loss (e.g. /tmp reaper on a multi-day import) silently drops pre-eviction backscatter; add a WARN mirroring the bathy reload-failure WARN — `cube_bathymetry/src/store_import.cpp` (`restoreSpilledSamples`/`persistBackscatterTile`)
+- [ ] (suggestion) Comment claims "Atomic temp-then-rename via tile_io::saveTile"; verified false — `marine_tiled_raster_store/src/tile_io.cpp:132` writes directly to the final path via GDAL `Create` (no temp/rename). Runtime is still safe (throw keeps tile resident) but the crash-safety reasoning is wrong — fix the comment — `cube_bathymetry/src/store_import.cpp` (`persistBathyTile`)
+- [ ] (suggestion) Bolded "Backscatter is lossless under eviction." reads unconditionally; the real guarantee is scoped to a consistent re-survey (a depth-disambiguation divergence between visits could divert backscatter) — tighten the claim — `cube_bathymetry/include/cube_bathymetry/store_import.h`
+- [ ] (suggestion) Registries written only at finalize; a mid-pass crash leaves evicted tiles with a source_index but no registry.json — likely acceptable (re-runnable single pass) but undocumented at the eviction site; add a one-line note — `cube_bathymetry/src/store_import.cpp` (`persistBathyTile`/`finalize`)
+- [ ] (suggestion) `reloadEvictedTile`'s `loadWindow(sw,ne)` over the tile's own corners relies on loadWindow inclusivity to reload that exact tile (correct per source); add a targeted single-tile reload round-trip test to lock it in — `cube_bathymetry/src/store_import.cpp` (`reloadEvictedTile`)
+
+**Static analysis**: `ament_cpplint` + `ament_uncrustify` clean on all changed C++/headers/tests. A full `colcon build` was not possible here (lower layers provide `marine_autonomy`/`gggs.h` only via install trees absent in this worktree); the two highest-risk correctness paths were verified by hand against source: (1) the reload window equals the add window exactly (shared `boundsForSoundings` + identical `GridAreaIterator`), so no touched evicted tile is missed; (2) a reload-seeded hypothesis has `number_of_samples == 1`, so the Welford-restore gate never silently skips the restore.
+
+### Next step
+Lifecycle: **Local Review** (approved) → push / open PR → **triage-reviews**. The 5 suggestions are advisory (doc/observability); the operator/host decides whether to apply them before or after push.

cube_bathymetry/CMakeLists.txt

@@ -292,6 +292,17 @@ if(BUILD_TESTING)
     cube_bathymetry_store_import
     marine_bathymetry_store::marine_bathymetry_store
   )
+
+  # Bounded == unbounded equivalence for the offline import accumulator (#92):
+  # an eviction-forcing budget reproduces the unbounded bathy + backscatter store
+  # exactly (lossless persist-then-drop + reload-on-revisit), and a persist
+  # failure never drops a tile.
+  ament_add_gtest(test_import_eviction test/test_import_eviction.cpp)
+  target_link_libraries(test_import_eviction
+    cube_bathymetry_store_import
+    marine_bathymetry_store::marine_bathymetry_store
+    marine_mbes_backscatter_store::marine_mbes_backscatter_store
+  )
 endif()
 
 # The installed public header store_import.h (#include'd by downstream consumers

cube_bathymetry/docs/decisions/0001-tile-eviction-and-incremental-publish.md

@@ -190,6 +190,10 @@ dependency) rather than retained.
   the full multi-hypothesis / pre-filter / backscatter-sample state (out of
   scope; backscatter is re-derivable per cube#15). A revisited cell continues
   from the persisted best estimate, which is the intended behavior.
+  **(Updated by cube#92/#93 — see ADR-0007 § Phase B.2 addendum: the corrected
+  backscatter Welford `(n, mean, M2)` IS now spilled and restored losslessly on
+  eviction rather than dropped/re-derived; raw per-beam samples are no longer
+  kept, so a richer correction requires re-importing the bag.)**
 - A misconfiguration where `max_resident_tiles` < CA-window tile span degrades
   the live view (over-lethal holes). Mitigated by the on_configure WARN and a
   default pair chosen to satisfy the constraint.

cube_bathymetry/docs/decisions/0007-mbes-backscatter-store-addendum-phase-b-transition.md

@@ -23,6 +23,9 @@ now the node-output correction was a **Phase B no-op**: `corrected == raw` for
 every beam, with the per-beam pairs kept so the surfaced backscatter stays
 re-derivable once a real correction lands.
 
+> **Superseded (cube#92/#93):** the correction was later moved to record time and
+> the per-beam raw set is no longer retained — see the *Phase B.2 addendum* below.
+
 An empirical study of the `m3_dryrun` Portsmouth Harbor survey (`/detections`,
 2025 pings, ~445k beams) showed the earlier candidate model — a cos²θ Lambert
 correction — is wrong for this data: it explains only ~¼ of the observed
@@ -83,6 +86,9 @@ sonar. The `rx_angles` sign/zero convention was verified against
   reflect "uncorrected by default".
 - The per-beam `{raw_intensity, beam_angle}` retention is unchanged, so the node
   value stays fully re-derivable when a richer correction lands.
+  **(Superseded by the Phase B.2 addendum, cube#92/#93: raw samples are no longer
+  retained — only an O(1) corrected Welford — so a richer correction now requires
+  re-importing the bag rather than in-place re-derivation.)**
 - Tests: 6 ARA cases at `extractNodeRecord()` (interpolation, nadir identity,
   beyond-max identity, NaN-angle identity, mode-None identity, port/starboard
   symmetry) plus curve-loader/mode-parse tests; the 3 pre-existing default-path
@@ -120,7 +126,10 @@ Design decisions:
    verbatim and never recomputes α, so Python and C++ apply an **identical** TL by
    construction (a divergence would silently corrupt the correction). At 500 kHz /
    24 °C / fresh water α ≈ 0.049 dB/m.
-3. **Per-beam slant range R is a new sufficient statistic**, threaded exactly like
+3. **Per-beam slant range R is a new sufficient statistic** (Superseded by the
+   Phase B.2 addendum, cube#92/#93: R is now consumed by `correctBeamIntensity` at
+   record time and folded into the Welford, not retained per beam), threaded
+   exactly like
    `beam_angle`: `Sounding::slant_range` → `DepthAndUncertainty::range` (the
    pack(1) raster struct grows to 20 bytes; layout-safe because every raster read
    strides by `sizeof(DepthAndUncertainty)`) → `BeamIntensitySample::range`,
@@ -139,6 +148,46 @@ beam-pattern), which is the deferred tier-3. The TVG/absorption/frequency state
 ultimately belongs in `SonarInfo` (unh_marine_autonomy#240); α is computed
 locally meanwhile.
 
+## Phase B.2 addendum — record-time correction + streaming Welford (cube_bathymetry#92/#93)
+
+cube_bathymetry#93 moves the per-beam correction from **extract** to **record**
+time and replaces the per-beam `{raw_intensity, beam_angle, range}` retention with
+a streaming **Welford** `(n, mean, M2)` of the *corrected* intensity, kept on the
+winning depth hypothesis. cube_bathymetry#92 then makes tile eviction **lossless**
+by spilling and restoring that triplet.
+
+This **reverses** two earlier decisions recorded above:
+
+1. **Correction timing.** Decision 1 located the empirical-ARA (and the tier-2 TL)
+   correction at `Node::extractNodeRecord()`. It now runs at record
+   (`Hypothesis::recordBeam` → `correctBeamIntensity`, the per-beam math moved
+   verbatim to `angular_response_curve.cpp`); extract only reads the triplet. The
+   math is unchanged, so the node-output mean + estimate variance are **identical**
+   to the old correct-at-extract path — only the timing moved.
+
+2. **Raw-sample retention / re-derivability.** The Context and Consequences above
+   state the per-beam `{raw_intensity, beam_angle}` set is retained so the surfaced
+   backscatter "stays fully re-derivable when a richer correction lands." That is
+   **no longer true**: only the O(1) corrected Welford is kept (the cube#93
+   Massabesic OOM fix — per-cell intensity memory used to grow with total beam
+   count). A future richer correction (the deferred tier-3 GeoCoder /
+   cube_bathymetry#15) therefore requires **re-importing the source bag**, not
+   in-place re-derivation from stored raw samples. Per-beam slant range `R`
+   (Tier-2) is likewise folded into the correction at record and not retained.
+
+Why this is acceptable: the empirical ARA + tier-2 TL correction the store needs
+*today* is applied before the Welford folds each beam, so the persisted value is
+the final corrected backscatter; applying a *different* correction was always going
+to be a re-processing operation, and bounding live/offline RAM (the cube#92/#93
+motivation) is the harder constraint. The Welford triplet is a perfect sufficient
+statistic for the mean + estimate variance, so eviction spill/reload is
+bit-identical to never-evicting.
+
+This addendum **supersedes** the "retention is unchanged / re-derivable" statements
+in the Context, Consequences, and Tier-2 sections above, and the backscatter
+"out of scope; re-derivable per cube#15" trade-off note in
+`0001-tile-eviction-and-incremental-publish.md` (§ Negative / trade-offs).
+
 ## Deferred (explicit follow-ups)
 
 - **Full radiometric GeoCoder (tier-3)** — insonified-area, beam-pattern, TVG

cube_bathymetry/include/cube_bathymetry/angular_response_curve.h

@@ -74,6 +74,27 @@ namespace cube
 /// provenance). Equivalent to `loadAngularResponseCurveWithHeader(path).points`.
   std::vector < std::pair < float, float >> loadAngularResponseCurve(const std::string & path);
 
+/// @brief Apply the node-output backscatter correction to one raw beam
+///        (ADR-0007 D3, cube_bathymetry#81/#87).
+///
+/// Returns the corrected backscatter (dB) for a single beam, computed at RECORD
+/// time (cube_bathymetry#93) so the CUBE hypothesis can stream a Welford of the
+/// CORRECTED intensity instead of retaining every raw beam. This is the exact
+/// per-beam math that used to run inside `Node::extractNodeRecord`, factored out
+/// verbatim so correct-at-record reproduces correct-at-extract bit-for-bit:
+///   - tier-2 TL add-back (cube#87): when @c backscatter_tl_removed and the range
+///     is finite and > 0, add `40*log10(R) + 2*alpha*R` (alpha ==
+///     @c backscatter_absorption_db_per_m). A NaN / non-positive range skips it.
+///   - angular-response residual (cube#81): when the mode is Empirical with a
+///     non-empty curve and the beam angle is not NaN, subtract
+///     `curve(|beam_angle| in degrees)` (linearly interpolated, edge-clamped).
+/// Identity (returns @p raw_intensity) when the mode is None or the curve is
+/// empty, and for the angle term when @p beam_angle is NaN. The caller gates a
+/// NaN @p raw_intensity (a NaN beam is never a sample). Computed and returned in
+/// `double` to match the estimator's accumulation precision.
+  double correctBeamIntensity(
+    float raw_intensity, float beam_angle, float range, const Parameters & parameters);
+
 }  // namespace cube
 
 #endif  // CUBE_BATHYMETRY__ANGULAR_RESPONSE_CURVE_H_

cube_bathymetry/include/cube_bathymetry/geo_grid.h

@@ -127,6 +127,29 @@ namespace cube
   /// beam-angle/GeoCoder correction is deferred to cube#81).
     std::vector < NodeRecord > nodeRecords() const;
 
+  /// @brief Corrected-intensity Welford of every cell's winning hypothesis, keyed
+  ///        by `gggs::CellIndex` (cube_bathymetry#92/#93 lossless eviction spill).
+  ///
+  /// Iterates the SPARSE node map (not all 960x960 cells), flushes each node's
+  /// median pre-filter (`queueFlush`, like @ref nodeRecords) and emits the
+  /// @ref Node::chosenIntensityWelford for cells with intensity-bearing beams
+  /// (n > 0). The result is what must be persisted to a scratch spill so a tile
+  /// evicted from RAM can be reloaded with its backscatter accumulator intact
+  /// (the bathy tile stores only the depth summary; the intensity Welford lives
+  /// only here). Three numbers per cell -- O(1), the cube#93 memory fix.
+    std::map < gggs::CellIndex, IntensityWelford > nodeIntensityWelford() const;
+
+  /// @brief Restore a corrected-intensity Welford onto @p cell's winning
+  ///        hypothesis (cube_bathymetry#92/#93 lossless eviction reload).
+  ///
+  /// Forwards to @ref Node::setSettledIntensityWelford on the node at @p cell. A
+  /// no-op if no node exists there (the cell was not reseeded by the depth
+  /// reload). Must run AFTER @ref setSettledDepthAt (which lazy-creates the node)
+  /// and BEFORE the revisit's soundings are added, so those beams then continue
+  /// the Welford on the same reloaded hypothesis (bit-identical to never-evicting).
+    void setSettledIntensityWelfordAt(
+      const gggs::CellIndex & cell, const IntensityWelford & intensity);
+
 private:
     gggs::GridIndex index_;