Coverage MOCs: worker-owned leaf tier + end-of-run root (issue 200)#208
Conversation
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of phase 1 (head 6f092f8) against the amended docs/design/sparse_coverage.md §4 (PR #206) plus the O8 resolution on the issue #200 thread (phase 1 = box + envelope only; the sidecar bitmap is phase 2). Kernel semantics were verified against a mortie 0.9.0 clone and by executing the branch's morton_box on adversarial occupancy sets (mixed depth, complete quads, deep chains, whole-shard, southern shards). No blocking-correctness findings. 5 inline comments: 3 should-fix, 1 nit/question, 1 question-for-author.
Findings
- should-fix / question-for-author (
morton.py):morton_boxis the DCA-children construction, not the claimed minimal ≤ 4-member MOC — counterexample inline; tighten per-child or fix the wording before the mortie-side spec freezes. - should-fix (
tests/test_coverage.py): all box tests use negative-base shards; the northern branch ofanc_order = len(anc) - 1has zero coverage. - should-fix (
tests/test_coverage.py): the plan's "stamp byte-cost assertion" is missing — one line pins the fixed-width property. - nit/question (
hive.py):read_coverageignoresspec— pick the unrecognized-spec posture (my lean: gate on themorton-moc/prefix) before phase 2 addsencoding. - question-for-author (
hive.py): envelope drops the plan'sgenerated_at/source; defensible for the leaf tier, but confirm the tiered envelope contract for phase 3 root/ancestor payloads.
Verified sound (checked, not just read):
- Compress-first canonicalization: correct and necessary — an occupied ancestor absorbs descendants before the split, so
{parent, child}never drops the parent's remaining area; the "every compressed member is strictly deeper than the DCA" argument holds (a member at the DCA's order would equal it and absorb the set). Complete-quad → 1-member parent is area-identical and canonical (PR question 1: fine as-is, provided the spec wording lands per finding 1). - Worker seam: the sink covers all paths — pooled
cell_to_slicekeys and bufferedStreamingAggregator.countskeys (tailflush()runs before capture; K==1 and K>1 share the one capture point outside the chunk loop); the "no data" early return precedes the sink but also setserror, so no stamp; a failed chunk write raises out ofprocess_shard, so a partially-failed shard is never stamped — coverage can only over-report relative to written cells, never miss them. DefaultNonecosts oneis not Nonetest outside all loops — byte-identical claim holds. - Zero extra ops: coverage is merged into the same stamp dict, one
attrsPUT, no companion object; debris semantics inherited (torn-write test extended accordingly). - Subtree guard: string-prefix check is sound under D1 (one digit per level) for both signs; in-shard occupancy can never legitimately trip it (compression of in-shard cells tops out at the shard id itself).
- Conventions: no new deps (
mortie>=0.9.0already pinned on main and exports all three kernels); no CI/deploy changes (the Lambda handler is imported read-only by a test, same pattern as existing handler tests); module sizes fine (hive.py436,worker.py710); new 420-line test module ran green locally (37/37).
CI for 6f092f8 at submit time: ruff ✅, build ✅, build-x86_64 ✅, build-arm64 ✅, check ✅, test (3.12) ✅, test (3.13) still in progress — not yet concluded overall.
Generated by Claude Code
| def morton_box(values) -> np.ndarray: | ||
| """Tier-0 morton box: the minimal <= 4-member MOC covering ``values`` (issue #200). | ||
|
|
||
| The fixed-width tier of the coverage envelope (``docs/design/ | ||
| sparse_coverage.md`` §4): compact the occupied cells to their canonical MOC | ||
| via mortie's ``compress_moc`` (mixed order allowed; an occupied ancestor | ||
| absorbs its descendants, complete sibling quads merge), then — unless one | ||
| member already covers everything — split at the deepest common ancestor | ||
| (``common_ancestor``, the longest common decimal-string prefix) and coarsen | ||
| each member to that node's child level (``clip2order``). The result is the | ||
| ancestor's 2-4 children that intersect the occupancy, or the lone covering | ||
| cell: a conservative superset — no occupied cell escapes the box. | ||
|
|
||
| Accepts anything :func:`morton_words` does; returns sorted packed ``uint64`` | ||
| words. Raises ``ValueError`` on empty input (and, via mortie, on a set | ||
| spanning HEALPix base cells — a shard's cells are one subtree by | ||
| construction, so the hive path never triggers it). | ||
| """ | ||
| from mortie import clip2order, common_ancestor, compress_moc | ||
|
|
||
| words = morton_words(values) | ||
| if words.size == 0: | ||
| raise ValueError("morton_box requires at least one occupied cell") | ||
| occ = compress_moc(words) | ||
| if occ.size == 1: | ||
| return occ | ||
| # After compression every member is strictly deeper than the ancestor (a | ||
| # member at the ancestor's own order would contain the rest and compress | ||
| # to a single cell), so coarsening lands each on one of its 2-4 children. | ||
| anc = morton_decimal(common_ancestor(occ)) | ||
| anc_order = len(anc) - (2 if anc.startswith("-") else 1) | ||
| return np.unique(clip2order(anc_order + 1, occ)) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (or question-for-author): the box is not "the minimal ≤ 4-member MOC" — it is the DCA-children construction, which can be strictly looser.
Concrete counterexample (verified against this branch's code with mortie 0.9.0, southern shard -5112333):
occ = {"-5112333111", "-5112333112", "-51123332"}
morton_box(occ) # -> ["-51123331", "-51123332"]But ["-511233311", "-51123332"] is also a valid 2-member, ≤ 4-slot, mixed-order cover — with the first member at 1/4 the area (the two occupied cells both live inside -511233311). The implemented construction coarsens every member to the DCA's child level, so whenever the occupancy inside an intersecting child is confined to a proper descendant of that child, the box over-covers relative to the true minimal ≤ 4-member MOC. The docstring, hive.py's module docs, and the PR body all say "minimal".
The spec text cuts both ways: §4 states "the minimal MOC with ≤ 4 members (mixed order allowed)" but its existence argument is exactly the DCA-children construction ("a deepest common ancestor whose ≤ 4 intersecting children form a valid cover"). Two concrete resolutions:
- Tighten per child (stays on the same public kernels, same 2–4 member count, never looser): after the
clip2ordersplit, replace each child bycommon_ancestorof the compressed members inside it — one groupby + one reduction. This is canonical and strictly ≤ area of the current form. - Keep the construction, fix the wording: docstring/spec say "the DCA's 2–4 intersecting children (or the lone covering cell)" instead of "minimal", and the mortie-side frozen spec records that.
Note (2) has a knock-on: the #202 tier-0 baselines (e.g. SERC "box 25.5× over-coverage") were presumably measured with this same construction, so if (1) is adopted the recorded baseline numbers shift. Either way the superset invariant holds — this is over-coverage only, never a false negative — which is why this is should-fix rather than blocking. Flagging for a decision since the tier-0 shape is frozen alongside the mortie spec.
(For completeness: the complete-quad → parent canonicalization raised in PR question 1 looks right to me — area-identical, canonical via compress_moc, and the "strictly deeper than the ancestor" argument in the inline comment is sound: a compact MOC member at the ancestor's own order would equal the ancestor and absorb the rest.)
Generated by Claude Code
| def _random_occupancy(rng, max_cells=24, max_depth=4): | ||
| """Random occupied cells at mixed depths within the SHARD subtree.""" | ||
| n = int(rng.integers(1, max_cells)) | ||
| return [ | ||
| SHARD + "".join(rng.choice(list("1234"), size=int(rng.integers(1, max_depth + 1)))) | ||
| for _ in range(n) | ||
| ] | ||
|
|
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: every box/coverage test in this file uses southern (negative-base) shards — the positive-base branch of the order arithmetic is never executed.
morton_box computes the ancestor order sign-dependently (src/zagg/grids/morton.py line 148):
anc_order = len(anc) - (2 if anc.startswith("-") else 1)SHARD = "-5112333" here, and the backend-identity test uses -4211322 — both southern, so the - 1 (northern) branch has zero coverage in this suite. An off-by-one there would coarsen to the wrong child level on every northern-hemisphere shard while all 37 tests stay green. One parametrization of the box tests (or at least the randomized oracle/superset pair) over {"-5112333", "5112333"} closes it cheaply. Same for one build_coverage subtree-check case with a positive base, since the startswith prefix guard is also string-form-dependent.
Otherwise the test design is genuinely adversarial where it counts: the oracle is kernel-independent pure string arithmetic, comparison is by canonical compressed area (so the quad-collapse form can't hide dropped or extra area), and mixed depths/duplicates/ancestor-descendant pairs all arise in _random_occupancy. A box that dropped its 4th member would fail both the explicit 4-child unit test and the area-equality seeds.
Generated by Claude Code
| self._cov(SHARD[:-1] + "41") # sibling shard's cell | ||
|
|
||
|
|
||
| class TestStampCoverage: |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: the stamp byte-cost assertion promised in the plan is missing.
The phase-1 test list in the implementation plan (issue #200, plan comment) ends with "stamp byte-cost assertion", and the PR body's zero-extra-ops claim is about request count only. Nothing here bounds the payload size, which is the thing that matters for the leaf zarr.json every reader GETs (the #202 measurement showed exactly how attrs payloads can blow up that object). The tier-0 payload is ~100 B by construction, so this is one line, e.g. in TestStampCoverage:
assert len(json.dumps(cov)) < 256or asserting the stamped zarr.json grows by < N bytes vs the pre-coverage stamp. Cheap now, and it pins the "fixed width" property of the 4-slot design against future envelope creep (phase 2 adds fields to this same dict).
Generated by Claude Code
| def read_coverage(leaf_store) -> dict | None: | ||
| """The leaf's tier-0 coverage payload, or ``None`` when absent (issue #200). | ||
|
|
||
| Rides :func:`read_commit`: debris and absent leaves read ``None``, and so | ||
| does a committed pre-coverage stamp (issue #199 stores carry no | ||
| ``coverage`` key) — older stores keep reading fine. Box members are | ||
| decimal morton strings; parse one back with | ||
| :func:`zagg.grids.morton.morton_word`. | ||
| """ | ||
| stamp = read_commit(leaf_store) | ||
| if stamp is None: | ||
| return None | ||
| coverage = stamp.get("coverage") | ||
| return dict(coverage) if isinstance(coverage, dict) else None |
There was a problem hiding this comment.
🤖 from Claude (review)
nit / question-for-author: read_coverage returns any dict payload without checking spec — decide the forward-compat posture before phase 2 freezes it.
The accessor's degradation behavior is right for the past (debris, absent leaf, pre-coverage #199 stamp → None, never raises — verified). But toward the future it's permissive: a hypothetical morton-moc/2 payload, or a malformed dict (box missing, wrong types), is returned as-is to callers that will index cov["box"]. Since the O8 resolution says phase 2 adds encoding + order + sidecar pointer under this same envelope, the phase-4 reader primitives will grow consumers of this dict, and "what does a reader do with an unrecognized spec" should be decided while there's exactly one producer. Options: (a) return None unless coverage.get("spec") == COVERAGE_SPEC (strict — but then phase 2 must keep morton-moc/1 or all phase-1 stamps go dark); (b) gate on the "morton-moc/" prefix; (c) document that callers must check spec themselves. My lean is (b) plus a one-line docstring note; any of the three is fine as long as it's chosen deliberately rather than inherited.
Conversely, phase-1 stamps look forward-compatible with the ratified phase-2 shape as long as the phase-2 reader treats a missing encoding/pointer as "box only" — worth stating in the phase-2 checklist item so it's tested then.
Generated by Claude Code
|
|
||
|
|
||
| def stamp_commit(leaf_store, *, cells_with_data: int, granule_count: int) -> None: | ||
| def build_coverage(shard_key, occupied, cell_order: int) -> dict: |
There was a problem hiding this comment.
🤖 from Claude (review)
question-for-author: the envelope drops the plan's generated_at + source fields — the timestamp omission is argued in the PR body, the source omission isn't.
The phase-1 plan specced coverage: {spec, box, cell_order, generated_at, source}; what shipped is {spec, box, cell_order}. Reusing the stamp's written_at is a good call for the leaf tier (one clock, one writer, same PUT). But source: dispatcher|sweep|worker is load-bearing in the O7 staleness lean ("Envelope: generated_at, source...") — for the leaf payload it's trivially "worker" today, so omitting it is defensible, but the same morton-moc/1 envelope is slated to wrap the root coverage.moc (phase 3) and sweep-composed ancestors, where source/timestamp are exactly the staleness discriminators. Two things to confirm: (1) the frozen mortie-side envelope doesn't require these fields at every tier; (2) phase 3 either adds them for root/ancestor payloads under the same spec string (i.e. they're per-carrier-optional) or bumps the spec. A one-line note in the docstring recording the deliberate omission would keep the next phase from re-litigating it.
Generated by Claude Code
|
🤖 from Claude Round-1 review folded (review) in
Phase 2 summary (details in the PR body): exact cell-order occupancy as a zstd bitmap in the in-leaf Two judgment calls flagged under "Questions for review" in the body: the bitmap encodes at the store's Suite: 1716 passed, 28 skipped; Generated by Claude Code |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review, round 2, scoped to 126f160 (review fold) + 2e94e6c (phase 2 sidecar), head 2e94e6c — against §4 as amended (PR #206), the O8 resolution (issue #200 comment), and round 1's findings. All claims below were executed against this branch (mortie 0.9.0, zarr 3.2.1, numcodecs 0.16.5), not just read. No blocking-correctness findings. 5 inline comments: 3 should-fix, 2 nits.
Round-1 fold verification — all 5 genuinely folded, not just claimed
- Tightened box: folded (option 1). The counterexample now reproduces the reviewer's result exactly —
{S+111, S+112, S+2}→["-511233311", "-51123332"]on the southern shard and its northern mirror; docstring wording downgraded from "minimal" to "canonical, NOT always globally minimal" with the freeze note. The tightening preserves the superset property at mixed depths ("within that child" grouping operates on the compressed members, so an occupied ancestor absorbs its descendants before grouping): 3,000-trial randomized fuzz across 4 shards, depths to 6 — zero superset violations, always ≤ 4 members, never escapes the shard. The oracle was updated meaningfully (per-groupcommonprefix, not just re-asserted): 4,000-trial impl-vs-oracle fuzz by canonical compressed area — zero mismatches. - Northern shards: folded — box matrix, bitmap tests, and the subtree guard (both arms) now run on
5112333via the fixture. - Byte-cost pin: folded — box-only < 256 B, full sidecar-pointing envelope < 512 B, both asserted.
- Spec posture: folded, strict-equality (option a) — stricter than my round-1 lean, and sound because phase 2 keeps
morton-moc/1; unknown-spec and missing-spec tests added; phase-1 stamps don't go dark. - Envelope fields: folded —
source: "worker"added;generated_atomission recorded as deliberate with the per-carrier-optional posture in the docstring.
Verified sound this round
- Bit-order convention (the freeze): the two stated definitions genuinely coincide — bijective and monotone over the shard subtree on BOTH hemispheres. Executed: for shards
{-5112333, 5112333, -4, 4}× depths 1–3,_cell_ranksenumerates 0..4^d−1 in exact ascending-packed-word order. Structurally it must: mortie packswithin = base-4 digit tailinto the low2*orderbits of the word (mortie/morton_index.py_decimal_to_word), and all cells of one shard subtree at one order share base/order/prefix bits — including bit-63-set southern bases — so uint64 ascending ≡ digit-tail ascending, no sign wrinkle. Encoder output confirmed MSB-first (rank 0 →0x80). The convention is sound to freeze; what's missing is the golden-vector test that keeps it frozen (inline should-fix — round-trip tests pass unchanged under an LSB flip, demonstrated). - Sidecar semantics: ordering pin extended to
dense → ragged → sidecar → stamp; unstamped-sidecar-is-debris tested; and I verified empirically that the retry re-template (overwrite=True) does wipe a pre-existingcoverage.moc— plus the envelope gate makes even a hypothetically surviving stale sidecar unreachable (a box-only re-stamp carries noencodingkey). One nit inline on pinning the wipe; one nit on theZarrUserWarningthe foreign key triggers inmembers()/retry walks ("invisible to zarr readers" overstates slightly). - Envelope/back-compat: strict spec check keeps accepting phase-2 envelopes (same spec string); box-only and pre-coverage stamps read correctly;
nbytesasserted against the actual object size,raw_nbytes= ceil(4^depth/8) matchespackbitsoutput exactly. - numcodecs: genuinely transitive —
uv pip show numcodecs→Required-by: zarr; zarr 3.x hard-depends on it, no new direct requirement, matching the O8-ratified posture. - Conventions:
hive.pyat 611 lines, tests at 586 — fine; docs updated accurately (bar the "invisible" nit); no CI/deploy changes; commits title-only; local run of the two touched suites: 135 passed, ruff + format clean.
PR question 1 (bitmap at cell_order vs the literal "order 19") — verdict: the generalization is faithful; keep it. The worker's occupancy truth exists at the store's cell_order; encoding a constant 19 for coarser stores would fabricate resolution (and 4^(19−shard) sizing for an order-12 store would be pure padding). No hidden constant-19 assumption survives in the code: the size formula, the envelope's cell_order, and the reader all derive from the recorded order (verified at order 8 configs throughout the suite). Recommend the mortie-side frozen spec text say "at the store's cell order (order 19 for ICESat-2 products)" rather than the constant. The one real edge the generalization exposes is cell_order == shard_order (legal config, crashes the write path) — inline should-fix.
PR question 2 (zstd level 3) — verdict: fine. The level is a writer-only detail (any level decodes; the frame is self-describing), 3 is the zstd default and the right speed/ratio point for a per-shard one-shot. One caveat to record: "byte-identical across workers and backends" holds per zstd-library-build — identical in a homogeneous fleet (same Lambda layer), not guaranteed across numcodecs/zstd version bumps. Fine as long as nothing (dedup, ETag comparison) is ever built on cross-version byte equality; the determinism test pins convention, not eternity. The decompressed bytes are the invariant — which is also why the golden vector should pin raw, not compressed, bytes.
CI for 2e94e6c at submit time: ruff ✅, build ✅, build-x86_64 ✅, build-arm64 ✅, test (3.12) ✅, test (3.13) still in progress — not yet concluded overall; pr-preview/deploy skipped.
Generated by Claude Code
| shard = morton_decimal(shard_key) | ||
| depth = int(cell_order) - _decimal_order(shard) | ||
| raw = np.frombuffer(bytes(Zstd().decode(payload)), dtype=np.uint8) | ||
| bits = np.unpackbits(raw, count=4**depth) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: decode_coverage_bitmap silently accepts a wrong-sized raw payload — a truncated sidecar decodes to a partial occupancy set, i.e. false negatives.
np.unpackbits(raw, count=4**depth) does not validate that raw actually holds 4**depth bits. Verified on this branch:
short = bytes(Zstd(level=3).encode(b"\xff")) # 1 raw byte
decode_coverage_bitmap(short, morton_word("-5112333"), 9) # depth 3 needs 8 raw bytes
# -> silently returns the 8 cells of the first byte; the other 56 bits read as unoccupiedcount larger than the available bits zero-pads instead of raising, and an oversized payload is silently truncated the same way. That is exactly the failure D9 forbids ("readers degrade ... never to wrong answers"): a corrupt/foreign/truncated sidecar should read None or raise loudly, never return a plausible-but-short cell list. In normal operation the stamp gate + atomic PUT make this unreachable, but this decoder is the reference implementation of a convention that freezes in the mortie spec, and the envelope already carries raw_nbytes for precisely this check.
One line closes it — in decode_coverage_bitmap, after decompression:
if raw.size != -(-(4**depth) // 8):
raise ValueError(f"sidecar payload is {raw.size} B, expected {-(-(4**depth) // 8)} B")(or have read_coverage_bitmap compare against the envelope's raw_nbytes). Related, a smaller inconsistency worth a deliberate call: read_coverage_bitmap maps a missing sidecar to None (degrade) but lets a corrupt one raise (Zstd decompression error propagates as RuntimeError). Either posture is defensible — corrupt-raises-loudly is arguably right since the truth is intact in the leaf — but the docstring should say which is intended.
Generated by Claude Code
| shard = morton_decimal(shard_key) | ||
| depth = int(cell_order) - _decimal_order(shard) | ||
| if depth <= 0: | ||
| raise ValueError(f"cell_order {cell_order} is not below shard {shard}'s order") |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: child_order == parent_order is a legal config that phase 2 turns into a permanently unstampable shard.
HealpixGrid.__init__ rejects only child_order < parent_order (src/zagg/grids/healpix.py:77), so a degenerate one-cell-per-shard hive config (level_diff == 0) passes config load and worked under phase 1 (the box is fine at depth 0). Under phase 2, process_and_write_hive unconditionally calls encode_coverage_bitmap(shard_key, words, grid.child_order) whenever occupancy exists, and this depth <= 0 guard raises — after all chunks are written, before the stamp. The shard errors, the leaf stays unstamped debris, and every retry fails identically: a config-dependent hard failure with no workaround.
Cheapest fix, matching the box-only degrade the envelope already supports: in process_and_write_hive, skip the sidecar when grid.child_order == grid.parent_order (a 1-bit bitmap carries no information beyond the stamp itself) and stamp the box-only envelope. Alternatively reject level_diff == 0 for hive at config load — but that changes what phase 1 accepted, so the skip is the smaller change.
While in here, one bound worth recording (comment-level, not a code ask): the bits staging array in encode_coverage_bitmap is 4**depth bytes — 8x the raw bitmap (one uint8 per bit). At the design's o9-shard/order-19 point that's 1 MB (fine); a coarse-shard config (e.g. parent_order 4, child_order 19) reaches GB-scale in a Lambda worker. Nothing guards it; if such configs are out of scope, a one-line comment saying so keeps the next reader from wondering.
Generated by Claude Code
| @pytest.mark.parametrize("seed", range(6)) | ||
| def test_round_trip_property(self, shard, seed): | ||
| # Exactness: the bitmap decodes to exactly the occupied set — no | ||
| # over-coverage, no false negatives (property over random occupancy). | ||
| rng = np.random.default_rng(seed) | ||
| depth = 3 | ||
| occ = _words(*(shard + t for t in self._tails(rng, depth, int(rng.integers(1, 40))))) | ||
| cell_order = _order(shard) + depth | ||
| decoded = hive.decode_coverage_bitmap( | ||
| hive.encode_coverage_bitmap(morton_word(shard), occ, cell_order), | ||
| morton_word(shard), | ||
| cell_order, | ||
| ) | ||
| np.testing.assert_array_equal(decoded, np.sort(occ)) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: no golden-bytes vector — every bitmap test here passes unchanged with the bit order flipped, so the frozen convention isn't actually frozen by the suite.
Verified on this branch: an LSB-first variant (np.packbits(..., bitorder="little") + matching unpack) round-trips identically through test_round_trip_exact, test_round_trip_property, test_deterministic_bytes, and the backend-identity test — round-trip and determinism are symmetric under any self-consistent bit permutation. The docstrings say "ascending packed-word rank, MSB-first per byte" freezes with the mortie-side spec; the only thing that pins a wire format against a silent flip (a numpy default change, a mortie-side reimplementation, a refactor of _cell_ranks) is a fixed-vector test on exact bytes.
Two lines do it — pin the DECOMPRESSED payload (compressed bytes are zstd-library-version-dependent; raw bytes are the actual frozen convention):
def test_golden_raw_bytes(self, shard):
# tail "11" = rank 0 -> MSB of byte 0; tail "44" = rank 15 -> LSB of byte 1
word, order = morton_word(shard), _order(shard) + 2
assert Zstd().decode(hive.encode_coverage_bitmap(word, _words(shard + "11"), order)) == b"\x80\x00"
assert Zstd().decode(hive.encode_coverage_bitmap(word, _words(shard + "44"), order)) == b"\x00\x01"(I verified both expected vectors against the branch's encoder: rank 0 → 0x80 0x00, rank 15 → 0x00 0x01.) A slightly richer vector (a depth-3 multi-cell pattern) would also freeze the rank arithmetic, but even the two-byte pair above converts a wire-format break from invisible to red.
Generated by Claude Code
| #: In-leaf occupancy-bitmap sidecar object name (issue #200 phase 2, O8) — | ||
| #: the one recorded exception to the "vanilla zarr v3 leaf" claim: a foreign | ||
| #: key inside ``{full_id}.zarr/``, invisible to zarr readers. | ||
| COVERAGE_SIDECAR = "coverage.moc" |
There was a problem hiding this comment.
🤖 from Claude (review)
nit: "invisible to zarr readers" is not quite true — the sidecar makes zarr emit a ZarrUserWarning on member enumeration, including on every occupied-leaf retry.
Verified on this branch (zarr 3.2.1): plain zarr.open_group + attrs reads are clean, but group.members() / tree() on a leaf containing coverage.moc warns "Object at coverage.moc is not recognized as a component of a Zarr hierarchy" — and so does emit_shard_template(store, overwrite=True), because the overwrite walk enumerates the existing prefix. So the retry path in process_and_write_hive will log this warning for every re-run of a shard whose prior attempt got as far as the sidecar PUT, and downstream users who enumerate leaf members see it too.
Nothing breaks (the warning is cosmetic; opens, array reads, the wholesale wipe, and the stamp all behave), so this is a docs/UX call, not a bug: either soften the "invisible to zarr readers" wording here and in docs/hive_layout.md to "ignored by zarr readers (member enumeration warns and skips it)", or filter that specific warning around the re-template call. Worth deciding now because the same claim is about to be repeated in the phase-5 docs and the mortie-side spec text.
Generated by Claude Code
| # The coverage sidecar (issue #200 phase 2) lands BEFORE the stamp: | ||
| # the stamp stays the leaf's final write, so an unstamped prefix's | ||
| # sidecar is debris like everything else in it. | ||
| assert ops == ["dense", "ragged", "sidecar", "stamp"] |
There was a problem hiding this comment.
🤖 from Claude (review)
nit: the retry-wipes-the-sidecar claim is verified but not pinned.
write_coverage_sidecar's docstring claims "the wholesale retry re-template clears it", and I confirmed it empirically on this branch (emit_shard_template(..., overwrite=True) delete-dirs the prefix; a pre-planted coverage.moc is gone afterwards). The torn-write test pins the wholesale-wipe mechanism via the stale CSR subgroup, which covers the same delete_dir — but the sidecar is the one object in the leaf that zarr does not own, so it's the one most plausibly missed if the re-template ever changes from prefix-wipe to node-by-node rewrite (the exact drift PR #205's review flagged). Two lines in test_torn_write_leaves_debris_then_retry_succeeds — drop a sidecar into the debris before the retry, assert it's gone after — would make the docstring's claim red-testable alongside the CSR pin. Cheap now, load-bearing if the zarr overwrite semantics ever shift under a version bump.
Generated by Claude Code
|
🤖 from Claude Round-2 review folded (review) in
Phase 3 summary (details in the PR body): end-of-run root Suite: 1749 passed, 28 skipped; Generated by Claude Code |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review, round 3, scoped to eb21f65 (round-2 fold) + f9c4653 (phase 3: end-of-run root coverage.moc), head f9c4653 — against §4 as amended (PR #206), the O8 / O9 + transport/handler-mode resolutions, and round 2's findings. Claims below were executed against this branch (both test suites re-run green locally: 119 + 49; PR-bot-equivalent ruff clean), not just read. No blocking-correctness findings. 5 inline comments: 1 should-fix, 3 nits, 1 question folded into a nit.
Round-2 fold verification — all 5 genuinely folded
- Truncation guard: folded.
decode_coverage_bitmaprejects any decompressed size!= ceil(4^depth/8)— short and oversized — and the test uses exactly the round-2 case (1 raw byte where depth 3 needs 8) plus a 16-byte oversize. The missing-degrades / corrupt-raises fork is now recorded in both docstrings, with the D9 rationale. - Depth-0 config: folded end-to-end.
process_and_write_hiveskips the sidecar atchild_order == parent_order(guard placed beforeencode_coverage_bitmap, so no post-chunk-write raise → no permanently unstampable shard), stamps the box-only envelope, and the test drives the real write path and asserts the read side:read_coverage_bitmap→None, noencoding/sidecarkeys, no sidecar object. The staging-memory bound comment also landed. - Golden vector: folded, and it genuinely pins MSB-first — I re-ran the round-2 experiment on this head: patching the encoder to
bitorder="little"produces0x0100where the test asserts0x8000(and0x09…where it asserts0x90…), so an LSB flip now FAILS three assertions. The depth-3 vector also freezes the rank arithmetic (tails 111/114/241/444 → ranks 0/3/28/63 →90 00 00 08 00 00 00 01, verified by hand), and pinning decompressed rather than compressed bytes is the right call (zstd-version-independent). - Warning posture: folded — "invisible to zarr readers" softened everywhere it appeared (module docs, constant, docstrings,
docs/hive_layout.md), and the retry suppression is tightly scoped:catch_warnings()around onlyemit_shard_template(overwrite=True), message-anchored to"Object at coverage.moc"— all other warnings, and this warning anywhere else (e.g. usermembers()calls), stay loud. Not over-broad. (Untested, but a reword upstream fails open to harmless noise, so I'm not asking for a pin.) - Retry-wipes-sidecar: folded — the torn-write test now plants a sidecar in the debris and asserts it's gone after the retry, exactly the "goes red if re-template drifts to node-by-node rewrites" pin round 2 asked for.
Phase 3 verified sound (executed, not just read)
- Range semantics:
np.uniqueon packed words keeps each base cell's words contiguous and rank-ascending at fixed order on both hemispheres — fuzzed across 6 bases (incl. bit-63 southern) with zero interleavings, so the linear run-finder is operating on the order it assumes; thesame_runpredicate requires same base AND rank+1, so runs never span base cells (verified:-54/-61split) and a multi-base CONUS-style set round-trips exactly (78 ranges, 400 cells,root_coverage_words== input). Union is canonical: touching ranges from two writes collapse ([-511,-512]+[-513,-514]→[["-511","-514"]]) because the union is re-built throughbuild_root_coverage, not spliced. Decimal-string endpoints (the 2^53 trap) asserted, and the 50k-shard payload bound (< 4 KB vs the 256 KB async cap) is a real test, not a comment. - Completion source:
doneis exactly the stamped-leaf set on both backends — Lambda FunctionError/timeout results carrystatus_code=Noneand error results are filtered; local raised cells never reachreport.resultsand"No granules found"/"No data after filtering"metadata carrieserror(worker.py) so unstamped no-data shards are excluded. Traced through_invoke_lambda_cell's three return paths and_async_result_poll's two. - Fail-open: the entire build→serialize→invoke sequence is inside the
tryon the Lambda path (serialization errors included —json.dumpslives in_invoke_lambda_coverage), likewise build→write locally; the handler catches everything to a logged 500 that no Event invoke reads; unparsable/incompatible existing objects are logged and overwritten (tested;json.JSONDecodeErroris aValueErrorsubclass, so_read_json's parse failure is genuinely caught). One residual scope gap inline (nit). - Config surface: matches the O9 ruling as closely as the layout allows — default ON resolves via
get_store_layout(config) == "hive", and hive is healpix-only by prior validation (config.py:330), so default-on-for-hive ≡ default-on-for-healpix-templates; rect can't be hive, and both explicit-true rejections (flat, rect) have pointed messages with tests, plus non-bool and null-falls-back cases. Existing fixtures were updated, not weakened: the runner-wiring test's root listing tightened from[manifest]to[coverage.moc, manifest]. - Handler mode:
_handle_coveragedispatches early like setup/finalize, uses the same_output_store_kwargscreds resolution, and the root envelope carriessource/generated_at(the per-carrier staleness discriminators the round-1 envelope question reserved for exactly this tier). Flag-off is checked at the ops level on both backends (no invoke at all; local store listing pinned to manifest-only). - Conventions:
hive.py810 lines (fine); no changes underdeployment/aws/beyondlambda_handler.py, which the espg-approved plan names for the new mode;template.yaml/workflows untouched; no new dependencies; commits title-only. One module-size flag inline (tests file at 1013).
Transport rationale (espg's ask on plan question 3): present as a real pros/cons comment at the dispatch site — payload bounded by construction vs the 256 KB cap, one hop, no read-back race, status-channel replayability already owned by the sweep. The cons of the chosen option are honestly stated. Delivered as specced.
CI for f9c4653 at submit time: ruff ✅, build ✅, build-x86_64 ✅, build-arm64 ✅, test (3.12) and test (3.13) still in progress — not yet concluded; deploy/pr-preview skipped.
Generated by Claude Code
| durable truth and the §7 sweep is the authoritative rebuilder, so | ||
| merging with garbage would be worse than replacing it. Returns the | ||
| payload actually written. | ||
| """ |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: the GET-union-PUT is not atomic — two concurrent runs interleave and the last writer wins, silently dropping the other run's shards until the sweep rebuilds. Document it as accepted, here.
The docstring covers sequential incremental runs ("a parsable existing object … is UNIONED"), which is the shape §4 records ("the end-of-run write may union with a prior root object"). Nothing on this PR records the concurrent case: run A GETs, run B GETs, A unions+PUTs, B unions+PUTs — B's object lacks A's shards. The resulting hole is exactly the failure mode the O7 staleness lean already blesses ("a shard exists that the MOC doesn't list … degrades to 'reader doesn't see the newest run', never a wrong answer" — issue #200 comment), and the §7 sweep is the authoritative rebuilder, so no code change is warranted — S3 has no compare-and-swap to reach for anyway. But an unrecorded race is the kind of thing that gets "fixed" later with a lock nobody needs: one docstring sentence ("concurrent runs race benignly: last writer wins, the union may miss the loser's shards until the sweep or the next run re-unions — accepted under D9/O7") pins the posture. Worth mirroring in the docs/hive_layout.md root-object paragraph, which currently only says "incremental runs union with the existing object."
Generated by Claude Code
| # An OLD deployment silently ignores the unknown mode — safe | ||
| # under D9 (readers degrade to the sweep MOC or the walk), | ||
| # but say so, mirroring the PR #205 deploy-ordering note. | ||
| logger.info( | ||
| f"Dispatching root coverage.moc write ({len(envelope['ranges'])} " | ||
| f"ranges, fire-and-forget) — requires a redeployed function; an " | ||
| f"older deployment ignores mode=coverage (harmless under D9)" | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
nit: "an older deployment silently ignores the unknown mode" is not what actually happens — it falls through to _handle_process and returns a logged 400.
Verified against main's handler (what an old deployment runs): the dispatch chain checks setup / finalize / extract / process_event and everything else lands in the else: _handle_process(event, context) branch. A mode: "coverage" event has no shard_key/parent_order/granule_urls/store_path… wait, it does have store_path — but not the others, so _handle_process's required-params check returns {"statusCode": 400, "body": …"Missing required parameters: shard_key, parent_order, granule_urls, s3_credentials"} (with an error-level CloudWatch log line, not silence).
The outcome is exactly as claimed — no store writes, no result mirror (result_url absent), and no async redelivery (a returned 400 is a successful invocation to Lambda's retry machinery; only unhandled exceptions trigger the ×2 Event retry) — so "harmless under D9" holds and this stays a nit. But the mechanism wording here, in _handle_coverage's docstring framing, and in docs/hive_layout.md ("an older deployment ignores the mode") should say "an older deployment 400s it in the process handler (logged, no writes, no retry)" — otherwise the first person who greps old-deployment CloudWatch for the promised silence finds an ERROR line instead and starts debugging a non-bug.
Generated by Claude Code
| done = [ | ||
| r["shard_key"] | ||
| for r in report.results | ||
| if r.get("status_code") == 200 and not r.get("error") | ||
| ] | ||
| if done: |
There was a problem hiding this comment.
🤖 from Claude (review)
nit: the done comprehension sits outside the fail-open try on both backends — "fail-open everywhere" starts one statement too late.
r["shard_key"] (here) and m["shard_key"] (the local block at ~1338) are hard indexing on result dicts before the try opens. I traced every producer and it's safe today: all four Lambda result envelopes (_invoke_lambda_cell's three return paths and _async_result_poll's two) unconditionally carry shard_key, and the local path appends process_shard's ProcessingMetadata, which initializes "shard_key" up front (src/zagg/processing/worker.py:239). So this is theoretical, not a live bug. But the block's own comment promises "Fail-open everywhere: a failed build/invoke logs and the run result is untouched" — a future result-envelope refactor that drops/renames the key would turn a fully successful fan-out into a crashed run at the coverage step, the exact outcome the comment forswears. Hoisting the comprehension inside the try (both here and in _run_local) costs nothing and makes the fail-open scope match the claim.
(Everything else about the completion source checked out: failed shards genuinely excluded on both backends — Lambda via status_code == 200 and not error with FunctionError/timeout results carrying status_code=None, local via the accumulator dropping raised cells plus the error filter catching "No granules found"/"No data after filtering" metadata — so done is exactly the set of stamped leaves, and retried shards appear once via np.unique.)
Generated by Claude Code
| } | ||
|
|
||
|
|
||
| def root_coverage_words(envelope: dict) -> np.ndarray: |
There was a problem hiding this comment.
🤖 from Claude (review)
nit / question-for-author: root_coverage_words materializes every covered shard word through a per-cell Python loop — fine at coherent-run scale, minutes at the pathological end. Is that bound worth recording?
The union path (write_root_coverage, and thus the Event-invoke handler) calls this on both the existing and the new envelope: each range is expanded cell-by-cell via morton_word(base + _rank_tail(r, order)) in pure Python. At the scales this PR tests (50k contiguous shards → one range) it's milliseconds-to-seconds and clearly fine. The place it grows teeth is an accumulated root object on a large store: a full-sphere order-9 product is ~3.1M shard words expanded twice per incremental run inside the worker; order-11 is ~50M, which is minutes of Lambda compute for what is conceptually an interval union. A rank-space union (merge [base, lo_rank, hi_rank] interval lists directly, never materializing words) would be O(ranges) instead of O(cells) and would also drop the morton_word round-trip.
Not asking for the optimization now — the design point (coherent regional runs, shard order ≤ 11) is comfortably inside the envelope, and the fire-and-forget invoke means nobody waits on it. But the encode-side memory note you added at encode_coverage_bitmap sets the precedent: one comment line here saying "expansion is O(covered shards); interval-space union is the upgrade path if root objects ever reach continental-accumulation scale" would keep the next reader from discovering the bound in CloudWatch. Alternatively, if you'd rather treat full-sphere roots as out of scope, say that.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude (review)
nit: this file just crossed the ~1000-line module guidance (1013 lines at f9c4653) with two phases still on the checklist.
CLAUDE.md §4 says a module heading past ~1000 lines should be raised before continuing, and phases 4–5 (reader primitives, staleness, docs examples) will land their tests somewhere. The seams are already clean — phases 1–2 (box/bitmap/leaf wiring) vs phase 3 (# ── the root coverage MOC (phase 3) ── onward, ~370 lines) — so a tests/test_root_coverage.py split is mechanical whenever you take it. Flagging now so the phase-4 commit doesn't silently push a 1k-line guidance file to 1.5k; fine to fold into the phase-4 commit itself rather than a standalone churn commit.
Generated by Claude Code
|
🤖 from Claude Round-3 review folded (review) in
Phase 4 ( Phase 5 ( Suite: 1766 passed, 28 skipped (foreground); ruff/format clean on changed files; codespell clean on the docs. CI runs via Generated by Claude Code |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review, round 4 (final), scoped to ffeefaa (round-3 fold) + 13b6b0b (phase 4: zagg.coverage reader primitives) + c504445 (phase 5: docs), head c504445 — against §4 as amended (PR #206, now on main, plus this branch's living-doc updates), the O8/O9 resolutions, and round 3's findings. Claims below were executed against this branch (both coverage suites re-run green locally: 136 passed = the PR body's 91 + 45; adversarial scripts against refresh_root_coverage, warn_if_stale, and each intersection helper), not just read. No blocking-correctness findings. 5 inline comments: 2 should-fix (one docs, one warning-wording), 3 nits (one with a question folded in).
Round-3 fold verification — all 5 genuinely folded
- Union race: folded —
write_root_coverage's docstring records the concurrent GET-union-PUT posture verbatim (last writer wins, accepted under D9/O7, explicit "do NOT add a lock"), and thedocs/hive_layout.mdroot-object paragraph mirrors it. - Logged-400 wording: folded everywhere it appeared — the runner dispatch comment, the
logger.infoline ("an older deployment 400s mode=coverage in its process handler (logged, no writes, no retry)"), and both places indocs/hive_layout.md(Coverage deploy note + reader bootstrap paragraph). Matches whatmain's handler actually does. doneinside the fail-opentry: folded on both backends, each with a comment tying placement to the claim. (Trivial residue:_run_localleaves thefrom zagg.hive import ...one statement outside thetrywhile the Lambda path hoisted it inside — an import of an already-imported module can't realistically fail, so this is asymmetry, not exposure.)- Scale bound: folded — the O(covered shards) note with the interval-space-union upgrade path sits on
root_coverage_words, androot_coverage_andcross-references it. - Test split: folded and verifiably mechanical — I diffed sorted test/class names across the seam (identical) and counted
assertoccurrences (107 before, 107 after); 646 + 634 lines, both files back under guidance.
Phase 4 verified sound (executed)
- Module org:
coverage.pyat 237 lines,hive.pyat 823 — the split honors the ~1000-line guidance and the write/read seam is clean; imports are one-directional (coverage→hive, nothing imports back — no cycle);__all__is tidy; staying out ofzagg/__init__'s eager re-exports matches the lazy-orchestrator pattern there. - Intersections: boundary cases executed — AOI exactly equal to a box member hits; the adjacent-but-disjoint sibling misses; a coarser-than-covered AOI (base cell vs shard ranges, shard vs cell-order bitmap) resolves via mortie's
moc_andmixed-order containment (rust kernel, sorted/compacted output);box_andskips the null pad slots;bitmap_and's empty-vs-Nonedistinction (definitive miss vs box-only fallback) is real and tested through the actual worker seam. Bit-rank arithmetic on the decode side is the same frozen convention the round-2 golden vectors pin on the encode side. load_coverage: tolerant exactly as claimed — garbage JSON, missing object, unknown spec/encoding all readNonewith a debug log; note it deliberately does NOT validaterangesshape, so a well-enveloped-but-corrupt payload surfaces later asroot_coverage_words' loudValueError— consistent with the corrupt-cache posture, worth knowing when wiring §5.refresh_root_coverage: walker honors the D5 node invariant precisely; stamp-gated leaf collection excludes debris; genuinely off the hot path (zerosrc/callers). Two docstring-vs-behavior gaps inline (mixed-order stamped leaf makes the escape hatch raise; delete-on-empty parenthetical).warn_if_stale: once-per-store latch works and tests isolate it (autouse fixture clears_stale_warnedboth sides). Two leans inline: the warning's cause list omits the benign in-flight-run case (should-fix, wording), and refresh never resets the latch (nit).
Phase 5 docs: one real find — the root-envelope example's ranges are at orders 7 and 2 under "order": 6, so this branch's own reader rejects the doc's reference example (inline, should-fix). Everything else cross-checked clean: leaf envelope example self-consistent (box members are descendants of the shard; raw_nbytes arithmetic exact), bit-convention wording matches _cell_ranks, corrupt-raises/missing-degrades matches the code, reader flow matches zagg.coverage's actual API names, and the design-doc updates are faithful — §4 tier-1 rewritten to the ratified bitmap shape with the supersession rationale, O8/O9 marked resolved with the right outcomes and permalinks, O7 annotated with the implemented shape + the accepted union race, no contradiction with what PR #206 landed. Status bullet lags its own document by one phase (inline nit).
Whole-PR coherence: all five checklist phases genuinely landed; the body's test description matches the suites (91/45 split, golden vectors, race/refresh/staleness cases all present); the four "Questions for review" are accurate and still open judgment calls (my round-2/3 verdicts on questions 1–2 stand; question 3 is serializer-only as stated; question 4's pre-existing-lint claim matches main).
CI for c504445 at submit time: ruff ✅, build ✅, build-x86_64 ✅, build-arm64 ✅, test (3.12) ✅, test (3.13) still in progress — not yet concluded; deploy/pr-preview skipped.
Generated by Claude Code
| { | ||
| "spec": "morton-moc/1", | ||
| "encoding": "ranges", | ||
| "order": 6, | ||
| "source": "dispatcher", | ||
| "generated_at": "2026-07-10T22:00:00+00:00", | ||
| "ranges": [["-42113221", "-42113224"], ["511", "511"]] | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix: the root-envelope example is malformed by the implementation's own validation — both ranges would be rejected.
The example declares "order": 6, but neither endpoint is an order-6 decimal:
"-42113221"/"-42113224"are order 7 (base-4+ 7 tail digits) — these were copy-pasted from the leaf example's box members above, which legitimately sit below shard order;"511"is order 2 (base5+ 2 tail digits).
Fed to this branch's own readers, zagg.hive.root_coverage_words raises ValueError: malformed coverage range ... at order 6 on both ranges (the _decimal_order(lo) == order check at src/zagg/hive.py:591), and zagg.coverage._ranges_contain never matches them. Since this example is the field reference a third-party reader (and the frozen mortie-side spec text) will be written against, it should round-trip through root_coverage_words. Order-6 endpoints matching the doc's running example would be e.g.:
"ranges": [["-4211321", "-4211324"], ["5112333", "5112333"]](Everything else in the two envelope examples checks out — I verified the leaf example's raw_nbytes: 512 = ceil(4^6/8) for shard order 6 / cell order 12, and the box members are genuine descendants of -4211322.)
Generated by Claude Code
| if store_root not in _stale_warned: | ||
| _stale_warned.add(store_root) | ||
| warnings.warn( | ||
| f"commit-stamped shard {decimal} is not listed by {store_root}/" | ||
| f"{ROOT_COVERAGE_NAME} — the root MOC is stale (crashed run, concurrent-run " | ||
| f"union race, or out-of-band writes); regenerate with " | ||
| f"zagg.coverage.refresh_root_coverage({store_root!r})", | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (wording only): the staleness warning omits its most common benign trigger — a run that is simply still in progress.
The root MOC is written at end of run, but leaves are stamped continuously as workers finish. A reader that computes leaf paths arithmetically (the D10-sanctioned pattern — it doesn't need the root MOC to find a shard) and opens a freshly stamped leaf during an in-flight incremental run lands exactly here: positive evidence, not listed. The warning then tells the user the root MOC "is stale (crashed run, concurrent-run union race, or out-of-band writes)" and to run refresh_root_coverage — every listed cause sounds pathological, and the suggested remedy is a full tree walk, for what is just normal mid-run lag. (Running refresh mid-run is harmless — the end-of-run write unions with it — but the user was sent debugging a non-problem, the same failure mode the round-3 logged-400 wording fix corrected on the deploy note.)
One phrase closes it, in three places: this message, the docstring's cause list (line 151), and the docs/hive_layout.md staleness paragraph — e.g. "(a run still in progress, a crashed run, the concurrent-run union race, or out-of-band writes)".
The mechanics themselves check out (executed on this branch): rank-space containment is correct for mixed-base envelopes (base equality + rank interval; the builder guarantees ranges never span bases, and a malformed cross-base range is skipped conservatively → stale, matching test_malformed_envelope_counts_as_stale); None reads as absence; the once-per-store latch behaves and the autouse fixture at tests/test_coverage_root.py:415 isolates it from cross-test pollution.
Generated by Claude Code
|
|
||
| #: Stores already warned about a stale root MOC in this process (O7: warn | ||
| #: once per store, trust silently otherwise, never auto-walk). | ||
| _stale_warned: set[str] = set() |
There was a problem hiding this comment.
🤖 from Claude (review)
nit: the warn-once latch is never reset by refresh_root_coverage, so the regen suggestion is a one-shot per process — even after the user follows it.
Verified on this branch: warn on store X → refresh_root_coverage(X) rebuilds the root object → a new staleness episode on X (another crashed run, another race) stays silent for the rest of the process, because _stale_warned still holds X. In a long-lived reader (notebook kernel, service) the second episode is exactly the one the O7 lean wants surfaced, and the user has demonstrated they act on the warning. One line in refresh_root_coverage — _stale_warned.discard(store_root) after the successful PUT — makes "once per store" mean "once per stale episode", which is what the docstring's "suggesting regen" implies.
Related key-normalization wrinkle, same line of code: the latch is keyed on the raw store_root string, so .../store and .../store/ warn independently (verified). refresh_root_coverage already normalizes with rstrip("/"); doing the same at the latch (and in the discard) keeps one store = one key.
Both are behavioral trims to the implemented O7 lean, not correctness bugs — the warning can only over-fire or under-fire, never change a query answer.
Generated by Claude Code
| if not keys: | ||
| try: | ||
| obstore.delete(store, ROOT_COVERAGE_NAME) | ||
| except (FileNotFoundError, NotFoundError): | ||
| pass | ||
| return None | ||
| envelope = build_root_coverage(keys, int(manifest["shard_order"]), source="refresh") | ||
| obstore.put(store, ROOT_COVERAGE_NAME, json.dumps(envelope, indent=1).encode()) | ||
| return envelope |
There was a problem hiding this comment.
🤖 from Claude (review)
nit / question-for-author: "recovers from anything" has one hole — a stamped leaf at a non-manifest order makes the escape hatch itself raise, with an error that doesn't say what to do; and the delete-on-empty parenthetical reads backwards.
Executed on this branch: plant one stamped order-5 leaf (old-config data surviving a manual partial clear, or a hand-copied leaf — ensure_manifest's overwrite guard can't prevent every path to this state) next to a legit order-6 leaf, and refresh_root_coverage dies with ValueError: shard keys ['-511233'] are not at shard order 6 out of build_root_coverage, leaving any stale cache in place. Raising is arguably the right posture — an order-6 ranges MOC cannot represent order-5 data, and silently dropping it would be the false negative D9 forbids — but (a) the docstring's "recovers from anything — crashed runs, the union race, garbage objects" should carve out this case, and (b) the error deserves refresh-level context ("found a stamped leaf at order 5 under a shard_order-6 manifest; the store is mixed-order — clear or re-shard before refreshing") instead of a bare builder message. A walk that already visited every leaf has everything needed to say that.
Separate small wording point on the empty branch: "None — deleting any existing root object — ... (an empty cache would be truthful, a stale one is not)" argues for writing an empty envelope, but the code deletes (and build_root_coverage rejects zero keys, so an empty envelope isn't currently representable). Delete is a fine choice — absence degrades to the walk, never a wrong answer — but the parenthetical should say what the code does, e.g. "(absence is truthful; a stale cache is not — and the ranges envelope has no empty form)".
The walk itself verified clean: delimiter-LIST per node via obstore.list_with_delimiter (the same API ensure_manifest already uses), root children gated by _is_base_component, non-root by the [1-4] single-digit rule, *.zarr prefixes treated as leaves and counted only when read_commit finds the stamp (debris excluded — tested), root-only objects (manifest, coverage.moc) never enumerated since only common_prefixes are walked, missing-object delete tolerated on both exception types, and no caller anywhere in src/ — genuinely off the hot path.
Generated by Claude Code
| - **Coverage ships** ([issue #200](https://github.com/englacial/zagg/issues/200) | ||
| phases 1–3): the tier-0 morton box on the commit stamp, the exact | ||
| zstd-bitmap `coverage.moc` sidecar inside each leaf, and the end-of-run | ||
| store-root `coverage.moc` (shard-order ranges MOC, `output.coverage_moc`, | ||
| default on for hive) for the one-GET bootstrap. |
There was a problem hiding this comment.
🤖 from Claude (review)
nit: the Status bullet says "Coverage ships (phases 1–3)" — but this same PR ships phase 4 too, and the section above already documents it.
The Reader flow and Staleness paragraphs (added in this commit) describe zagg.coverage's load_coverage/root_coverage_and/box_and/bitmap_and/warn_if_stale/refresh_root_coverage as existing, so the Status list is stale against its own document the moment it lands. One-line fix: "phases 1–4): ... for the one-GET bootstrap, plus the zagg.coverage reader primitives (intersection per tier, O7 staleness lean, explicit refresh)."
Generated by Claude Code
|
🤖 from Claude Round-4 review folded (review) in
Suite: 1770 passed, 28 skipped (foreground); ruff check + format clean on changed files; codespell clean. CI: previous head Standing for review (unchanged from the body's Questions): (1) bitmap order = store Generated by Claude Code |
|
🤖 from Claude Status: all five phases complete; four adversarial-review rounds run and every finding folded (final fold 77dadfe). Full suite 1770 passed / 28 skipped; CI fully green on the prior head c504445, and Lint/Docs already green on the final head (Tests/Lambda Build were finishing at last check — the tree is identical to the locally-green one apart from docs/tests). Applying
Also standing, deliberately untouched: the pre-existing Generated by Claude Code |
2 participants
Closes #200
Refs #198
All five phases of the coverage-MOC plan (implementation plan, answers, O8 resolution): the tier-0 morton box rides the hive commit stamp, exact cell-order occupancy ships as a zstd-compressed bitmap sidecar inside the leaf, the end-of-run store-root
coverage.moc(shard-order ranges MOC, default-on for hive) gives readers the one-GET bootstrap,zagg.coverageprovides the reader primitives + O7 staleness lean, and the docs (layout guide + design record) are updated. Design contract:docs/design/sparse_coverage.md§4 (amended via PR #206, now onmain; O7/O8/O9 registry entries updated to resolved-with-outcome in this PR — living doc). Coverage is hive-layout-only — flat stores and rect grids are untouched.What this does
Phase 1 — the tier-0 box on the commit stamp
zagg.grids.morton.morton_box(words)— the canonical ≤ 4-member MOC covering a set of occupied cells (mixed order allowed). mortie 0.9.0 public kernels only, no new dependencies:compress_moccanonicalizes (an occupied ancestor absorbs descendants),common_ancestor+clip2ordersplit the DCA into its 2–4 intersecting children, each tightened to the common ancestor of the occupancy inside it (round-1 finding 1). Documented as not always globally minimal; the construction freezes with the mortie-side spec.process_shard(..., occupied_out=...)— optional out-param sink (theragged_outpattern) delivering the distinct cell words holding ≥ 1 observation — exactly thecells_with_datacells (pooled:cell_to_slicekeys; buffered:StreamingAggregator.countskeys). Needed because the streamed carrier holds all template cells. DefaultNone: byte-identical.hive.build_coverage/stamp_commit(..., coverage=)/read_coverage— envelope{spec: "morton-moc/1", box: [4 slots, nulls trail], cell_order, source: "worker"}on the existing finalattrs.updatePUT (zero extra ops, debris semantics inherited); accessor strict on the spec;generated_atdeliberately omitted at the leaf (the stamp'swritten_atis the one clock).Phase 2 — exact occupancy bitmap as the in-leaf sidecar (the O8 shape)
hive.encode/decode_coverage_bitmap— bit field over the shard subtree atcell_order: bit i = the i-th subtree cell in ascending packed-word order (base-4 D1 digit tail, 1..4 → 0..3), MSB-first per byte, zstd at a fixed level → byte-identical sidecars for identical occupancy. Raw size deterministic (ceil(4^depth/8)) — the ragged-worst-case immunity the #202 measurement selected for. zstd vianumcodecs(already in the tree). The decoder rejects wrong-sized payloads (round-2 finding 1); corrupt raises, missing degrades.{leaf}/coverage.moc— the recorded exception to the vanilla-v3 leaf (zarr data reads unaffected; member enumeration warns and skips it; the retry re-template suppresses exactly that expected warning). Written before the stamp (dense → ragged → sidecar → stampordering pin); depth-0 configs (child_order == parent_order) skip it and stamp box-only (round-2 finding 2). Envelope growsencoding/sidecar/nbytes/raw_nbytesonly when the sidecar exists;read_coverage_bitmaptreats absence as "box only".Phase 3 — end-of-run root
coverage.moc(one-GET bootstrap)output.coverage_moc, default ON for hive (O9); explicittrueon flat/rect is a pointed validation error.hive.build_root_coverage— O1 envelope:encoding: "ranges", shardorder,source,generated_at, ranges as inclusive same-base digit-tail-rank runs with decimal-string endpoints (the 2^53 JSON float trap).write_root_coverageGET-unions-PUTs; unparsable/incompatible existing objects are logged and overwritten (regenerable cache; the §7 sweep is the authoritative rebuilder). Concurrent runs race benignly — GET-union-PUT is not atomic, last writer wins until the next re-union/sweep; recorded as accepted under D9/O7 (round-3 finding 1), do not add a lock.mode: "coverage"Event invoke with the pre-serialized envelope (transport rationale as a code comment at the dispatch site: payload bounded by construction — 50k coherent shards < 4 KB vs the 256 KB async cap; one hop, no status-channel read-back race; replayability owned by the sweep). Fail-open everywhere (completion-list derivation inside thetry, round-3 finding 3); flag-off dispatch byte-identical.mode: "coverage"event in its process handler — a logged CloudWatch error line, but no writes, no result mirror, and no async redelivery (a returned 400 is a successful invocation to Lambda's Event retry machinery) — fail-open by construction; the root object simply doesn't appear until a redeployed function, the sweep, or a refresh writes it. Mirrors PR Morton decimal shard ids + hive-partitioned output layout (issue 199) #205's deploy-ordering note (round-3 finding 2 corrected the earlier "silently ignores" wording).Phase 4 — reader primitives + staleness lean (O7)
src/zagg/coverage.py(237 lines) — placed in its own module rather than growinghive.pypast the ~1000-line guidance (hive.py is at 823; noted per review):hiveowns the write side and raw accessors,zagg.coverageowns consumption.load_coverage(store_root)— tolerant root-envelope reader: missing/unparsable/unknown-spec →None+ debug log (readers degrade to the walk, D9).moc_and:root_coverage_and(AOI vs root ranges, mixed order),box_and(AOI vs a leaf's ≤ 4-member box — the cheap reject on the stamp GET),bitmap_and(exact cell-level;Nonefallback for box-only leaves). No LISTs anywhere (D10).warn_if_stale(store_root, shard_key, envelope)— the O7 lazy-detection lean: called with positive evidence (an opened, commit-stamped leaf) the root MOC doesn't list, warns ONCE per store per process suggesting regen, never auto-walks; containment checked in rank space (O(ranges), no expansion).Noneenvelope is absence, not staleness.refresh_root_coverage(store_root)— the explicit escape hatch, documented as the sanctioned robustness path (not the hot path): delimiter-LIST walk of the digit tree (root{sign+base}children,[1-4]digits below,*.zarrleaves), collects commit-stamped leaves (debris excluded), REPLACES the root object withsource: "refresh"(the walk is ground truth — no union); zero stamped leaves deletes the stale cache and returnsNone.root_coverage_words(round-3 finding 4): expansion is O(covered shards) — fine at the coherent-run design point; interval-space union is the upgrade path if roots ever reach continental-accumulation scale.Phase 5 — docs
docs/hive_layout.md: full Coverage section — tier table (box / bitmap / exact truth / root), leaf + root envelope field references with examples, the frozen bit convention, sidecar zarr-warning note, corrupt-vs-missing posture, reader flow (root → box → bitmap →mortoncoordinate), union/race/staleness posture, refresh guidance, and the deploy note; the reader bootstrap now leads with the root MOC and falls back to the walk.docs/design/sparse_coverage.md(PR Design doc: hierarchical tiered coverage MOCs (issue 200) #206 is merged, so updated here as the living doc): §4 tier-1 bullet rewritten to the resolved bitmap shape, O8/O9 registry entries marked resolved with outcomes + permalinks, O7 annotated with the implemented shape and the accepted union race.Phases
coverage.moc(mode: "coverage", flag default-ON for healpix per espg)How it was tested
tests/test_coverage.py(91 tests — leaf tiers) andtests/test_coverage_root.py(45 tests — root MOC + readers; split at the phase-3 seam per round-3 finding 5):dense → ragged → sidecar → stampordering; both backends byte-identical through the real Lambda handler path.load_coveragepresent/missing/garbage/unknown-spec; each intersection helper hits/misses/boundary (incl. mixed-order AOIs and the exact-miss vs conservative-box distinction);warn_if_stalefires exactly once per store, only on mismatch, absence ≠ staleness, malformed envelope counts as stale;refresh_root_coveragerebuilds exactly the stamped set across both hemispheres, excludes debris, replaces a stale root without unioning, deletes the cache when nothing is stamped, and raises off a non-hive root.tests/test_hive.py: torn-write debris + sidecar wipe; ordering pin; local wiring root-object set.Local:
uv run ruff check+ruff format --checkclean on all changed files;uv run pytest -qfull suite green (1766 passed, 28 skipped); codespell clean on the docs; mypy viapre-commitatmain's baseline.Questions for review
cell_order(recorded in the envelope) — identical to the ratified shape whencell_orderis 19; a constant would fabricate resolution on coarser stores.hive._ZSTD_LEVEL) for byte-determinism; any level decodes.np.uniquepacked-word order fixes the serialized range order; a serializer-only change if the frozen mortie-side spec wants a specific base-cell ordering.ruff check src testsfails onmaintoo (N818UnknownCapability,src/zagg/registry.py:64), andruff format --checkwould reformat 6 files untouched by this PR (pre-existing). The PR lint bot runs--select=E,F,W,I, so CI does not see them.