D8 co-land: live node → draft; import_bag/batch_regen → processed + clearOverlappedDraft - #134
Merged
Conversation
Retarget cube_bathymetry writers from SourceLayer::Survey to Draft (live node) and Processed (store_import, batch_regen), with test updates and build-verification strategy against uma#313.
Address the two Plan Review must-fix items with the operator's checkpoint decisions: - MF1: draft clearing on the direct-write path uses the store's public BathymetryStore::clearOverlappedDraft (uma#308 1d8c3a8) via a scoped load->clear->save cycle in persistBathyTile (no persistent in-memory store on that path); batch_regen clears transitively through it. - MF2: the live node writes Draft but primes reads from Processed ∪ Draft (startup GeoMapSheet seed + disk-serve tile-version catalog + the disk-serve/eviction-reload scratch reads) so warm-start survives the legacy survey/->processed/ migration; writes stay Draft-only. Also: close Open Question #2 (Processed is freely writable; drop the ADR-0002 A2.1 caveat), commit to Build Verification Option A (local build + test against the built issue-308 store install), and record the confirmed store API (enum ordinals, layerDirName, clearOverlappedDraft overloads, save/loadWindow) read from the installed headers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…308) Retarget the three CUBE writers off the retired SourceLayer::Survey: - cube_bathymetry_node: WRITES the Draft layer (live on-boat product). Its READS prime from Processed union Draft (MF2): on a boat's first boot after co-land, load() auto-migrates a legacy survey/ dir to processed/, so the startup GeoMapSheet seed, the disk-serve tile-version catalog, the disk-serve scratch read, and the eviction-reload read all fuse both layers (Processed primed second so it wins on overlap). Without this the node would warm-start from an empty Draft and lose its seed + catalog across the migration boundary. - store_import (import_bag): WRITES the Processed layer (authoritative off-boat re-run) and, after each direct saveTile, clears the overlapped Draft cells via the store's public BathymetryStore::clearOverlappedDraft (MF1) so stale live draft never shadows the re-run. This path keeps no persistent store, so it loads the tile window, clears against the just- written processed tile, and persists only the dirtied draft tiles; clearing is an optimization (query resolves Processed > Draft), so a failure is logged and swallowed, never losing the processed write. - batch_regen: finalize output-layer check -> Processed; its writes go through the shared persistBathyTile, so they clear draft transitively. Backscatter store (marine_mbes_backscatter_store::SourceLayer::Survey) is untouched per ADR-0010 D8. Tests: node-path tests -> Draft; import/batch-regen tests -> Processed; backscatter test sites unchanged. Fix the layerDirName assertion test (now draft). Add ProcessedImportClearsOverlappedDraft covering the net-new direct-write draft clearing. Cannot build against the pre-split jazzy core; verified against the uma#308 feature/issue-308 store install (see progress). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Retargets cube_bathymetry’s bathymetry-store writers/read-prime paths to the ADR-0010 D8 Draft/Processed layer split (replacing legacy Survey), including deterministic Processed-over-Draft priming and safety checks that loudly abort on legacy survey/ persistence after migration refusal. This aligns the node/import/regeneration behavior with the store-side split (lockstep with the referenced unh_marine_autonomy change) and updates/extends tests to pin the new contracts.
Changes:
- Live node now writes Draft but primes/serves from Processed ∪ Draft, enforcing per-cell Processed-wins on overlap via masked priming.
import_bag/batch_regennow write Processed and trigger clearOverlappedDraft after directsaveTile()writes (plus “persisting survey/” abort paths).- Tests updated from
SurveytoDraft/Processed, with new coverage for overlap-clearing, fused-prime priority, and ambiguous/symlinked legacy-store aborts.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cube_bathymetry/src/cube_bathymetry_node.cpp | Write Draft; prime/serve fused Processed∪Draft with deterministic per-cell priority; fail configure loudly on refused legacy migration. |
| cube_bathymetry/src/store_import.cpp | Write Processed; clear overlapped Draft post-write; add/store-wide legacy survey/ persistence detection and abort behavior. |
| cube_bathymetry/src/batch_regen.cpp | Retarget output-layer checks to Processed; relies on shared Processed write path. |
| cube_bathymetry/include/cube_bathymetry/store_import.h | Expose new priming helpers and legacy-store detection API used across node/import/tests. |
| cube_bathymetry/test/test_import_eviction.cpp | Add/extend tests for overlapped-draft clearing, fused-prime Processed priority, and legacy-store aborts (both-dirs + symlink). |
| cube_bathymetry/test/test_store_import.cpp | Retarget import/load round-trip tests to Processed. |
| cube_bathymetry/test/test_batch_regen.cpp | Retarget bathy layer expectations to Processed. |
| cube_bathymetry/test/test_persistence.cpp | Retarget live-node-like persistence paths to Draft and update layerDirName assertions. |
| cube_bathymetry/test/test_anti_entropy_disk_serve.cpp | Retarget disk-serve/persistence expectations to Draft. |
| cube_bathymetry/test/test_tile_eviction_rss.cpp | Retarget layer path under test from Survey to Draft. |
| .agent/work-plans/issue-133/plan.md | Work plan documenting the layer retarget + safety/prime semantics and verification strategy. |
| .agent/work-plans/issue-133/progress.md | Implementation/review trail and verification notes for the lockstep change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+375
to
+385
| for (const auto & [tile_index, tile] : tiles) { | ||
| std::int64_t version = fallback_version; | ||
| struct stat st; | ||
| const std::string tile_path = layer_dir + "/" + | ||
| marine_bathymetry_store::tileFilename(tile_index); | ||
| if (::stat(tile_path.c_str(), &st) == 0) { | ||
| version = static_cast<std::int64_t>(st.st_mtim.tv_sec) * | ||
| 1000000000LL + st.st_mtim.tv_nsec; | ||
| } | ||
| catalog_builder_.update(tile_index, version); | ||
| } |
Comment on lines
+908
to
+910
| std::error_code dir_ec; | ||
| std::error_code link_ec; | ||
| return fs::is_directory(survey, dir_ec) || fs::is_symlink(survey, link_ec); |
added 4 commits
August 20, 2026 16:08
The seed_catalog lambda decomposed each map entry as [tile_index, tile] but only used tile_index; iterate the pair and take .first so the tile payload name is not bound. Trivial dead-code cleanup (Integrated Review, Copilot) -- no behavior change.
Two hardening fixes on the direct-write/prime seam (Integrated Review): - legacySurveyDirPersists ignored the error_codes from is_directory / is_symlink, so a genuine stat failure (EACCES/EIO/ELOOP) on the survey/ probe returned false -- letting callers treat a permanent ADR-0010 D8 migration refusal as a transient per-tile skip and silently degrade instead of the loud abort the function exists to guarantee (cross-confirmed: Copilot + Local Review R3). Now fails SAFE toward abort: is_directory does not set ec for a plain nonexistent path (not_found is not an error), so a set ec is a real failure -- checked BEFORE the symlink probe (is_directory short-circuits and follows the link) rather than OR-ing both ecs at the end. - Added assert(!mask || mask->index() == tile.index()) in primeFromTileSkippingMask to pin the same-GridIndex mask contract the cell-for-cell k indexing relies on (all current callers satisfy it) (Local Review R3).
Comment on lines
+250
to
+258
| // A non-empty output processed layer means batch-regen is rebuilding over a | ||
| // populated store. The gather forces from-scratch (skip_survey_seed below) so it | ||
| // never blends onto the tiles it rebuilds, but tiles NOT touched by this run stay | ||
| // behind as stale processed data mixed with the fresh rebuild -- warn so the | ||
| // operator can point -o at an empty directory for a clean exact rebuild. | ||
| if (!cfg_.store_dir.empty()) { | ||
| const std::string survey_dir = cfg_.store_dir + "/" + | ||
| const std::string processed_dir = cfg_.store_dir + "/" + | ||
| marine_bathymetry_store::layerDirName( | ||
| marine_bathymetry_store::SourceLayer::Survey); | ||
| marine_bathymetry_store::SourceLayer::Processed); |
…clean (#133) On libstdc++ fs::is_directory/is_symlink(p, ec) set ec=ENOENT (or ENOTDIR) for a missing path, contrary to an earlier revision's assumption that not_found leaves ec unset. The '|| dir_ec' fail-safe therefore returned true for EVERY store without a survey/ layer -- refusing all clean stores. The bug stayed latent because the only callers were catch-block guards (seedNewTile / reloadEvictedTile / on_configure) that run after a real loadWindow throw; a new happy-path caller would abort on every fresh store. Classify the error_code instead: ENOENT/ENOTDIR means survey/ genuinely does not exist (clean store -> false); any OTHER error (EACCES/EIO/ELOOP) still fails safe toward the ADR-0010 D8 loud abort. Symlinked/both-dirs refusal detection is unchanged (covered by existing ImportEviction abort tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ny write (#133) batch_regen is a write-path tool that never load()s the output store, so the ADR-0010 D8 survey/->processed/ auto-migration never fires here. If the output store still carries a legacy survey/ (a pre-D8 store, or one whose migration was refused), the first processed write in finalize() would create processed/ alongside the surviving survey/ -- the ambiguous both-dirs state every future load() then permanently refuses (a nightly regen would silently brick the store for all loaders). Refuse loudly at construction, before a single tile is scattered or written, reusing legacySurveyDirPersists(); the error tells the operator to migrate first (run a load-path tool, or rename survey/ manually). Write-path tools do not migrate (documented invariant). Adds BatchRegen.LegacySurveyStoreRefusedBeforeAnyWrite: a store with a populated survey/ makes construction throw and no processed/ is created. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 #133. Lockstep co-land with rolker/unh_marine_autonomy#313 (ADR-0010 D8 quality-axis re-split, store side) — part of the world-model arc (rolker/unh_marine_autonomy#86).
What
Retargets all cube_bathymetry store writers for the
Survey→Draft/Processedsplit:cube_bathymetry_node.cpp): writes toDraft; startup prime (GeoMapSheet seed + disk-serve tile catalog) reads Processed ∪ Draft with per-cell Processed-wins resolved before CUBE's hypothesis machinery (skip-Draft-where-Processed-populated — seeding order can't deliver the priority throughchooseHypothesistie-breaks), so warm-start survives the boat-sidesurvey/→processed/auto-migration boundary.store_import(import_bag) /batch_regen: writeProcessedand invoke the store's sharedclearOverlappedDraftafter their directsaveTile()writes, so D8's "regeneration clears overlapped draft" holds on the non-GeoTIFF path too.survey/after a load throw (both-dirs ambiguity or symlink refusal) aborts imports loudly and fails the live node'son_configureexplicitly — no silent near-empty stores or cold starts.Merge together with rolker/unh_marine_autonomy#313 (this branch builds only against its store; the pre-split store breaks this build and vice versa), followed by the gabby rebuild.
Test plan
Full local build +
colcon testagainst the built feature/issue-308 store overlay: 553+ tests, 0 failures across three review rounds. New coverage: conflicting-depth Processed∪Draft prime priority (pins the per-cell contract), persisting-survey/abort/rethrow paths (both-dirs + symlink),ProcessedImportClearsOverlappedDraft.Review trail
.agent/work-plans/issue-133/progress.md: Issue Review → Plan (+revision per Plan Review's 2 must-fix: shared clear API, Processed∪Draft prime) → Implementation → R1 changes-requested (fused-prime priority must-fix + 3 suggestions, all fixed) → R2 approved (4 hardening suggestions, all fixed) → R3 approved, Ship: recommended.Authored-By:
Claude Code AgentModel:
Claude Fable 5