Skip to content

leaf_column rename, granule-id sibling objects, refusal manifest (issue #388 follow-up trio) - #403

Merged
espg merged 8 commits into
mainfrom
claude/388-followup-trio
Aug 8, 2026
Merged

leaf_column rename, granule-id sibling objects, refusal manifest (issue #388 follow-up trio)#403
espg merged 8 commits into
mainfrom
claude/388-followup-trio

Conversation

@espg

@espg espg commented Aug 8, 2026

Copy link
Copy Markdown
Member

Refs #388. Refs #402 (items (5)/(6)/(9)). Follows PR #397 (merged as 3f13af2).

The three schema/artifact changes espg ruled pre-merge on the PR #397 thread (the ratification comment). PR #397 merged first, so they land here instead — unchanged in content. Timing still matters: these were ruled cheap because the schema is unreleased, and it still is. The D20 column key and the record-borne granule_ids exist on main only between 3f13af2 and this PR; landing before the next *.*.* tag preserves the "nothing published this spelling" premise the rulings rest on.

Rulings landed

  • (5)(c) — the D20 record/parquet key column renames to leaf_column (e00d7f4). Every site: the seam metadata stamp (hive.py), telemetry.build_record's key set, _EQ_OR_NONE_KEYS (both merge arms), _ROW_SCALARS/flatten_record, the parquet round-trip test, and tools/generate_spec_fixtures.py's read of the seam's metadata. Two reasons, both recorded at the key: column is SQL-reserved in DuckDB and Trino/Athena — the unquoted WHERE column IS NOT NULL is a parse error, so every filter on the run parquet would need WHERE "column" IS NOT NULL — and leaf_column reads correctly on the pyramid column's own record ("the column this LEAF carries", not "the column this record is"). The spec-fixture file key column in tests/data/spec/column.expected.json is the fixture schema's own name and is deliberately untouched (published; not the D20 record).

  • (6)(c) — granule_ids moves off the record/envelope into a sibling object (c980446). The record now carries the catalog hash only; the id LIST lives in granules.json beside the stats sidecar, on the sidecar's own spec-keyed grammar (granules_{window}.json; {stem}.granules.json under morton-hive/3), derived through one private _sibling_key owner so the two names cannot drift.

    • Identity equality never reads it. classify_leaf_identity takes a load_recorded_ids callable and invokes it only when the recorded and planned granules_sha256 DISAGREE — pinned by tests asserting the loader's call count is 0 on the skip path, 0 on the equal-hash/changed-semantic quadrant, and exactly 1 on a hash mismatch. The list exists to name the diff a refusal reports. (The narrower gate is review fold c320eea; as first written the loader fired whenever the fast path failed, which made a routine config-only rerun pay one sibling GET per shard for a verdict the hashes alone decide.)
    • The interaction check the ruling asked for holds: dedup.shard_status (one GET per shard across a whole shardmap in has_run) and telemetry.rows_from_status (one GET per mirrored envelope inside a 900 s worker) read the sidecar/envelope only — neither needs the sibling, and both got smaller: the ~4,600-id / ≈550 KB pole payload is off every one of those GETs and off the response envelope entirely.
    • Self-pairing: the sibling records the granules_sha256 of the list it holds, and dedup.leaf_recorded_ids accepts it only when that equals the sidecar's. A torn rewrite (either PUT lost, or a stale sibling beside a fresh sidecar) reads as unrecorded, not as ids that could name the wrong granules as dropped. Absent sibling on a mismatch → the pre-leaf skip-if-current: input-identity no-op with lifecycle touch (semantic_hash x granule-id set) #388 unrecorded-ids path, exactly as ruled for question (4)(a).
    • Back-compat over the 3f13af2..this-PR window (review fold 9d7f65d): leaves written after PR leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397 merged but before this PR carry the id list ON the record with no sibling. leaf_recorded_ids falls back to recorded["granule_ids"] when the sibling is absent or unusable, so those leaves stay guarded and self-heal on their next rewrite instead of classifying unrecorded-ids forever. The sibling wins whenever both exist; the fallback pairs by construction (build_record hashed the very ids it embedded).
    • Written by the leaf SEAMS, not by the dispatchers that write the sidecar — see "Questions for review" (1) below; this is the one place I did not take the obvious reading of "write sites". The adversarial review confirmed the implementation, the justification and the cost; the numbers are in that question now.
    • Lifecycle: touch_current_unit's footprint gains the sibling (a lifecycle rule reaping it would silently disarm the guard on an otherwise-fresh leaf), with the over-touch pin extended to granules_2020.json; the docs' footprint list and the .github/scripts/bench_objects.py object model both gain it (see the note under Testing).
  • (9)(c)+(a) — a store-root refusal manifest (b462ef2). A run that ends with cells_refused > 0 writes ONE small JSON at the store root, refusals_{ts}_{run_id}.json — timestamp-first like stats_*.parquet and sweep_stats_*.json, and outside the stats_*.parquet glob the sweep's run-record discovery scans. It carries the run context needed to act (run_id, timestamp, the run's semantic_hash, zagg version) and one entry per refused unit: shard_key, window, the classification (contraction/mixed), n_missing, and the complete missing_granules list — the composition of (6)'s sibling reads. Units sort by (shard, window) so two runs over one refusal set produce comparable objects. No D20 schema change and no synthesized unit rows — that was the point of (c) over (b). Ruled (9)(a): a pure-skip run writes nothing here and stays row-less.

    • D8: the write rides the same local wrap-up seam as the root coverage.moc union and touch_store_rootrunner._write_refusals, with c71198f's rationale mirrored ("the LOCAL dispatcher runs it in-process; this process is also the worker"). The fleet has no once-per-run worker-side seam, so the manifest is local-backend-only, exactly like the store-root touch; the docs now say both wait on question (10)'s resolution.
    • The gate's verdict now names the (shard, window) unit (leaf_identity_gate(window=...)), because on a windowed store the shard alone is ambiguous.
    • Operator surface: the summary gains refusal_manifest_path (local summaries only, None when nothing refused), the CLI prints it under the refusal remedy line, and the phase-4 docs gain a The refusal manifest section. The truncated-log caveat (phase-4 finding 4) now points at the manifest as the durable full list instead of telling operators to triage on the count.
    • Sort caveat, for anyone diffing two manifests: units sort by str(shard_key), i.e. lexicographically10 precedes 9. Deterministic and comparable across runs, which is all the ordering is for, but it is not numeric order.

Review fold

Five findings from the adversarial self-review, one commit each:

  • c320eeathe sibling read was not gated on the hash mismatch it is documented to be gated on. It also fired on the "hashes equal, semantic hash differs" quadrant, whose verdict is provably semantic-mismatch regardless of the id list — a routine config-only rerun therefore fanned out one sibling GET per shard (2,721 at CA o9, ~550 KB each on a pole unit). semantic-mismatch now short-circuits before any load, pinned at loads == 0. Second-order fix: with the sibling absent that quadrant used to degrade to unrecorded-ids, so a config-only rerun over a pre-leaf skip-if-current: input-identity no-op with lifecycle touch (semantic_hash x granule-id set) #388 store reported 100% cells_unrecorded — the counter that means "the guard was inert" — when the hashes had in fact proved no contraction.
  • 9d7f65dback-compat read of the record-borne granule_ids for the 3f13af2..this-PR window (see the (6)(c) bullet above).
  • 5a7e3ebwrite_refusal_manifest's fail-open now covers the unit composition, not just the PUT. It runs at _run_local:3212, before the summary and the run-stats parquet, so a MemoryError on question (3)'s unbounded case or a TypeError on malformed missing_granules used to destroy the run record of a run that had already done all its work. write_granule_ids already had the whole body inside its guard; these now match.
  • 45bb7ebthe sibling PUT moved inside the write-phase bracket on both leaf families. It sat one line after _write_elapsed += (and after write_s += on the raster seam), so phase_timings["write"]phase_write under-reported by one PUT per leaf in every run parquet and benchmark record. Pinned by a test that wraps the PUT in a 50 ms sleep and asserts the bracket sees it.
  • 7b04101the granule-id sibling got its own spec: "zagg-granule-ids/1" marker instead of borrowing the D20 record's SCHEMA_VERSION (see Spec assessment). The PR-body sentence that claimed both new objects already carried their own marker is corrected in the same pass — it was true only of the refusal manifest.

docs/hive_layout.md follows the first of these: the "fetched exactly once, only when the hashes disagree" invariant is now true as written, and the pre-#388 paragraph says granule-hash mismatch, with an explicit note that a config-only rerun is not one of these and never inflates cells_unrecorded.

Testing

  • uv run pytest -q on this branch (three ruling commits + five review folds): 3,671 passed / 38 skipped / 1 failed — the failure is the pre-existing, environment-dependent test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds, flagged not fixed. The other known-flaky one, test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries, failed on one earlier run of the same tree and passed here; it also fails on clean main.
  • uv run ruff check src tests: clean on the diff; the pre-existing N818 at src/zagg/registry.py:64 is flagged, not fixed. uv run ruff format --check src tests: clean on the diff; the pre-existing tests/data/benchmark/README.md reformat is flagged, not fixed.
  • New/changed tests, by ruling:
    • (5) the parquet round-trip (all-null + mixed, fastparquet and pyarrow), the metadata-rides-the-key pin, both merge arms, and the seam's stamp — all renamed in place; test_column.py's 12 seam assertions follow. metadata["phase_timings"]["column"] is a phase name, not the record key, and is deliberately unchanged.
    • (6) TestGranuleIdsSibling (key grammar across all three specs + unknown-spec raise, sibling path, sorted self-hashing round-trip, empty-set case, fail-open write, absent read); TestLeafRecordedIds (round-trip, absent, unpaired-hash rejection, four malformed bodies, per-window naming); TestClassifyLeafIdentity rebuilt around a counting loader (0 calls on the fast path, exactly 1 on mismatch, None loader → unrecorded-ids); the "pre-leaf skip-if-current: input-identity no-op with lifecycle touch (semantic_hash x granule-id set) #388 leaf" models on both seams now delete the sibling rather than nulling a record key, which is what a pre-leaf skip-if-current: input-identity no-op with lifecycle touch (semantic_hash x granule-id set) #388 leaf actually looks like; the lifecycle footprint counts and the S3 exact-key sets gain it; the end-to-end vector rerun asserts the sidecar has no granule_ids and the sibling has them.
    • review fold TestClassifyLeafIdentity::test_semantic_mismatch_with_equal_sets_rewrites_not_refuses (now loads == 0) and ::test_semantic_mismatch_without_a_sibling_is_not_unrecorded; TestLeafRecordedIds::test_record_borne_ids_are_read_when_no_sibling_exists, ::test_the_sibling_wins_over_a_record_borne_list, ::test_the_spec_marker_is_read_leniently; TestRefusalManifest::test_composition_is_fail_open_too (malformed missing_granules + a MemoryError raiser); test_hive.py::test_write_phase_covers_the_granule_id_sibling.
    • (9) TestRefusalManifest (key shape + traversal guard + not-a-run-record, full per-unit diff with sort order, nothing-refused-writes-nothing, fail-open); end-to-end refusal→manifest on both leaf families (test_hive.py vector with the exact dropped id, test_raster_runner.py raster in the STAC-item id space), the refusal_manifest_path is None pin on the --allow-contraction rerun, the CLI pointer test, the summary key-set pin, and the windowed unit-identity pin.
  • One file outside src/tests changed: .github/scripts/bench_objects.py, the benchmark object-count model. The per-leaf model had "TWO node-dir siblings"; with the granule-id list it is three, and without the update the in-tree tests fail (objects_other == 1, total_max off by one). Flagging it explicitly per CLAUDE.md §1, and correcting an earlier claim in this body that it is "test-support" — it is not. Besides tests/test_benchmark_objects.py, it is imported by .github/scripts/run_benchmark.py and run_full_aoi_benchmark.py, executed by lambda-benchmark.yml and lambda-benchmark-fullaoi.yml, and base-pinned from main by lambda-benchmark-command.yml. The deploy coupling was checked and is safe: the command workflow never redeploys, and the push-to-main path rebuilds the layer from the merged tree in the same run, so the object model and the fleet code it models land together. No workflow file is touched.

Spec assessment

No byte-contract change, consistent with PR #397's own assessment. docs/specification.md, tools/generate_spec_fixtures.py's fixture schema, and tests/data/spec/ are unchanged (the fixtures tool's one edit reads the seam's renamed metadata key; the emitted fixture JSON is byte-identical). SCHEMA_VERSION stays 1: the D20 record's keys are pre-release and rev in place per the constant's own rule, and this is a rename plus a key removal, both before publication. The two new object families are operational, where §4.7's informative scope-out already places the stats/sub-map families: granules.json is a sibling of a sidecar the spec does not normatively describe beyond its naming grammar, and the refusal manifest is per-run root telemetry like stats_*.parquet/sweep_stats_*.json. Both carry a version marker of their own so a future reader has a versioning handle: the manifest spec: "zagg-refusals/1" plus schema_version (it is run-record-class), and the sibling spec: "zagg-granule-ids/1" alone (review fold 7b04101 — as first written it borrowed the D20 record's SCHEMA_VERSION, welding a schema it deliberately is not). Readers must treat the sibling's marker leniently: the sidecar hash pairing is what decides, and an unknown marker degrades to the ruled unrecorded-ids fallback rather than raising.

Questions for review

  1. The granule-id sibling is written by the leaf SEAMS, not at the dispatcher-side sidecar write sites — the one place I read the ruling's "write sites" against its most literal sense, deliberately, and the one thing to overrule if you disagree. Reasons: (a) D8 — a store write belongs worker-side, and the seams already PUT the whole leaf; (b) fleet coverage with no deployment/aws/ edit — PR leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397 banked "fleet vector sidecars record the identity half with zero handler changes" via the seam's semantic_hash stamp; putting the sibling at the dispatcher sites would have un-banked the catalog half, leaving every fleet-written leaf unrecorded-ids (guard permanently inert on the fleet) until question (1)'s handler change lands. Via the seam, both leaf families record their input set from this release's deploy onward; (c) one source for the id space — the seam writes the very list the gate compares as planned_ids, instead of the dispatcher recomputing _resolve_urls(records, driver) in lockstep. The cost, stated plainly: the sibling PUT is unconditional on a committed leaf, so a caller that never opted into the gate (today's Lambda handler) is no longer byte-identical — it writes one extra small object per leaf. Both seam docstrings say so at the opt-in kwarg.

    The adversarial review measured this and the placement stands unless you overrule it. What it confirmed: the PUT is gated on a committed leaf (hive.py:1656 / processing/raster.py:1412), so it is one object per (node, window), never per shard-attempt. Object-count delta on atl03_tdigest_healpix_o9_hive.yaml: +9.1% objects/shard (11 → 12), +7.7% on _located, +5.3% on _strata. Request/cost delta at CA o9: +2,721 PUTs ≈ $0.0136 against $65.30 of compute — 0.02%. Size: ~110 B/id ⇒ ~22 KB on a typical shard, ~500 KB on a pole shard. Also worth recording: no test ever pinned the fleet's exact object set, so PR leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397's "byte-identical when unarmed" property was unpinned to begin with — this changes a real behavior, not a guarded one. If you would still rather the fleet stay byte-identical until the handler opts in, this moves to the two runner.py sidecar sites in one commit.

    Two facts for the handler recipe (which PR lambda_handler.py: six-key raster body fix + sidecar-clobber gate (skip-gate plumbing, NOT arming) — deployment/aws/lambda_handler.py #401 owns), both pre-existing and out of scope here under §1 — deployment/aws/ and the handler are not named by this PR: (a) lambda_handler.py:1687/:1860 pass neither sidecar_spec nor write_sidecar(spec=), so the fleet writes both the sidecar and the sibling with spec=None — on a morton-hive/3 store both land under the legacy name; (b) on the skip path, an absent sibling costs one extra S3 head_object per skipped unit in touch_current_unit (+2,721 HEADs on an all-skip CA o9 rerun over a pre-leaf skip-if-current: input-identity no-op with lifecycle touch (semantic_hash x granule-id set) #388 store). Neither is fixable in this PR; recording them so the handler change lands with both in hand.

  2. refusal_manifest_path is on the two LOCAL summaries only (always present, None when nothing refused), not on the Lambda ones, because there is no fleet writer at all — unlike run_stats_path, which both backends produce. That is an asymmetric summary key set; the alternative (always-None on the fleet) reads as "no refusals" rather than "not recorded". Left as-is; say the word if you want the key mirrored.

  3. The manifest is unbounded in size by design. A run where many pole shards refuse could write a large object (~550 KB of ids per refused pole unit). Truncating would defeat the entire reason (9)(c) exists — the docs now tell operators to triage from this object rather than from the truncated log — so it carries the full diff. Worth a size guard, or is "proportional to the refusal set" fine? (Review fold 5a7e3eb at least made the worst case degrade: the unit composition is now inside the same fail-open guard as the PUT, so a MemoryError here costs the manifest, not the run record.)

  4. A narrow pre-existing hole in the skip guard, inherited from PR leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397 — neither widened nor narrowed by this PR, and deliberately NOT fixed here. The sidecar PUT happens after the commit stamp, so if a rewrite dies after the stamp but before the sidecar PUT, the leaf holds run-2 data under run-1's sidecar. A later run planning run-1's id set then matches both granules_sha256 and semantic_hash, passes _leaf_is_committed, and skips a leaf whose contents do not match what the sidecar says. Three ways out, all yours to pick: (a) reorder — sidecar before stamp (costs the "sidecar only ever describes a committed leaf" property, and a torn pair the other way then reads as a stale record on a leaf that never landed); (b) carry an id-set digest in the stamp itself, so the commit and the identity claim land in one object and cannot tear apart; (c) accept it as narrow — it needs a crash inside a one-PUT window, and the failure mode is a stale leaf, not corrupt data. My read is (c) is defensible and (b) is the real fix, but this is a D4 change and belongs on its own issue rather than in a follow-up trio.

Comment thread src/zagg/dedup.py
# The ONE read the fast path exists to avoid: the recorded id list lives
# beside the sidecar, not in it (issue #388's ruling on question (6)), and
# is fetched only now that the hashes disagree.
rec_ids = load_recorded_ids() if load_recorded_ids is not None else None

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[medium] The sibling read is not gated on the hash mismatch it is documented to be gated on — a semantic-only rerun fans out one large GET per shard that provably cannot change the verdict.

The loader fires whenever the fast path fails, and the fast path is rec_hash == granules_sha256(planned) AND semantic_match:

if rec_hash is not None and rec_hash == granules_sha256(planned) and semantic_match:
    return {"action": "skip", "classification": "equal", "missing": []}
rec_ids = load_recorded_ids() if load_recorded_ids is not None else None

So the "hashes equal, semantic hash differs" quadrant — the routine same inputs, changed config rerun, which is the ordinary reason to rerun a store with the gate armed (runner.py: skip_if_current=not overwrite, i.e. on by default locally) — pays the sibling read. Verified on this branch:

classify_leaf_identity(rec, semantic_hash='b'*64, planned_ids=ids, load_recorded_ids=load)
-> {'action': 'rewrite', 'classification': 'semantic-mismatch', 'missing': []}   loads = 1

Equal hashes mean an identical sorted id multiset (granules_sha256 hashes "\n".join(sorted(ids))), so missing is necessarily empty and set(planned) == recorded_set necessarily holds — the only reachable outcome is semantic-mismatch, which is returnable without reading anything. At CA o9 that is 2,721 needless GETs; on a pole unit each one is the ~550 KB object the split exists to keep off the per-shard path. docs/hive_layout.md states the invariant as "fetched exactly once, only when the hashes disagree" — the code's trigger is broader than the doc's.

Second-order, same line: when the sibling is absent (a pre-#388 leaf, a lost PUT, a torn pair) the same quadrant returns

-> {'action': 'rewrite', 'classification': 'unrecorded-ids', 'missing': []}   loads = 1

cells_unrecorded is the counter _identity_counts documents as "the guard was INERT for them" and the phase-4 docs tell operators to treat specially — but with the hashes equal the guard is not inert, it has proved no contraction. Every config-only rerun over a pre-#388 store now reports 100% cells_unrecorded, which is the one signal that is supposed to shrink as leaves get rewritten under this release.

Both fall out of one guard: call the loader only when rec_hash != granules_sha256(planned).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in c320eea — the loader is now gated on the granule hashes DISAGREEING, and the equal-hash/changed-semantic quadrant returns semantic-mismatch before any read:

hashes_match = rec_hash is not None and rec_hash == granules_sha256(planned)
if hashes_match and semantic_match:
    return {"action": "skip", "classification": "equal", "missing": []}
if hashes_match:
    # Equal hashes mean an identical sorted id multiset, so the diff is
    # provably empty and the ONLY reachable verdict is semantic-mismatch ...
    return {"action": "rewrite", "classification": "semantic-mismatch", "missing": []}
rec_ids = load_recorded_ids() if load_recorded_ids is not None else None

Both halves are pinned:

id-multiset-drift still needs the read (hashes differ, sets equal) and is unaffected — test_duplicate_drift_with_equal_sets_rewrites still passes.

Docs followed: the load_recorded_ids parameter docstring now says "ONLY when the recorded and planned granules_sha256 DISAGREE", and docs/hive_layout.md's pre-#388 paragraph now says granule-hash mismatch, with an explicit note that a config-only rerun is not one of these and never inflates cells_unrecorded. The "fetched exactly once, only when the hashes disagree" invariant is now true as written.

Comment thread src/zagg/telemetry.py Outdated
"""
import logging

units = []

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[medium] write_refusal_manifest's fail-open covers only the PUT — the unit composition sits outside the try, so the unbounded-size case question (3) raises kills the run instead of degrading.

Everything from units = [] down through body = {...} runs before try:, and _write_refusals does not wrap the call either:

    return write_refusal_manifest(
        store_path,
        [m for m in metas if isinstance(m, dict) and m.get("refused")],
        ...
    )

The docstring three lines up promises the opposite: "Fail-open (D9 telemetry class, the sweep run record's posture): a failed PUT logs and returns None — the run's exit status and its cells_refused count are unaffected." Contrast write_granule_ids in this same commit's sibling helper, which puts the whole body (sorted(str(g) ...), granules_sha256, json.dumps) inside its try.

Why it matters concretely: in _run_local the call is at line 3212, before the summary dict is built and before the run-stats parquet write — the same ordering RasterStrategy documents as deliberate ("Run-level stats parquet … BEFORE the all-failed raise below so the failure evidence persists at the store root"). A raise in the composition therefore destroys the run summary and the stats_*.parquet of a run that has already done all of its work.

The realistic raisers are exactly the ones question (3) is about:

  • MemoryError building units for a full-fleet refusal — 2,721 CA o9 units at pole-density recorded sets (~4,600 ids each) is ~1.25e7 strings and a >1 GB JSON body. A MemoryError inside json.dumps is caught and degrades; the identical failure one statement earlier is not.
  • TypeError from for g in (meta.get("missing_granules") or []) on a malformed meta — a shape the function explicitly anticipates one line above with if not isinstance(meta, dict): continue, so the defensive posture is half-applied.

Moving the composition inside the try (or wrapping the call in _write_refusals) makes the docstring true and turns question (3)'s worst case into "no manifest" rather than "no run record".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 5a7e3eb — the whole body of write_refusal_manifest (unit composition, sort, body dict, key, PUT) is now inside the one try, matching write_granule_ids's posture in the same PR. The if not units: return None early return moved inside with it, so the nothing-refused case is unchanged.

The docstring now states the widened guarantee and why the ordering makes it matter:

The COMPOSITION is inside the same guard as the PUT (write_granule_ids's posture), because this runs before the summary and the run-stats parquet: a MemoryError on an unbounded refusal set, or a malformed missing_granules, must cost the manifest, never the run record of a run that already did all its work.

New TestRefusalManifest::test_composition_is_fail_open_too pins both raisers you named, at the exact statements:

  • missing_granules=7 → the TypeError out of for g in (meta.get("missing_granules") or [])None.
  • a truthy list subclass whose __iter__ raises MemoryErrorNone.

Left _write_refusals unwrapped: after this its only work is the identity["cells_refused"] lookup and the import, and wrapping it too would hide a genuine programming error at the seam without covering anything the helper now doesn't.

Comment thread src/zagg/hive.py
# Fail-open inside (telemetry class, D9).
from zagg.telemetry import write_granule_ids

write_granule_ids(leaf_path, granule_urls, spec=sidecar_spec, **store_kwargs)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] The sibling PUT lands outside the write-phase bracket, so phase_timings["write"] stops accounting for every write the seam performs.

        _write_elapsed += time.time() - _t0
        # The recorded granule-id list, as this leaf's own sibling object ...
        from zagg.telemetry import write_granule_ids

        write_granule_ids(leaf_path, granule_urls, spec=sidecar_spec, **store_kwargs)

The accumulator closes one line before the PUT. Same shape on the raster seam (processing/raster.py: write_s += time.time() - _t0 immediately precedes its write_granule_ids(...)), so both leaf families are affected. Everything else the bracket covers — the coverage.moc sidecar PUT and stamp_commit — is inside it, and the docstring's pinned order ("dense … -> ragged -> coverage sidecar -> stamp") is now one object longer than the bracket.

Consequence is measurement, not correctness: phase_timings flattens to phase_write on the D20 record (telemetry.flatten_record) and lands in every run parquet and every benchmark record, so from this release on the write bucket silently under-reports by one PUT per leaf and no longer decomposes the seam's wall. Reusing _t0 and moving the _write_elapsed += after the call costs nothing and keeps the bracket honest.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 45bb7eb, on both leaf families — the accumulator now closes AFTER the sibling PUT, reusing the same _t0:

        write_granule_ids(leaf_path, granule_urls, spec=sidecar_spec, **store_kwargs)
        _write_elapsed += time.time() - _t0

and the raster seam's write_s += time.time() - _t0 moved below its write_granule_ids(...) identically.

The three pinned-order comments that stopped at the stamp now name the sibling, so the docstring and the bracket agree again: hive.py's "dense … -> ragged -> coverage sidecar -> stamp -> granule-id sibling (issue #388; after the stamp, inside the bracket)", and both raster sites ("dense slabs -> coverage sidecar -> stamp -> granule-id sibling"; the write-out gloss is now "template + slabs + sidecar + stamp + sibling").

Pinned by a new test_write_phase_covers_the_granule_id_sibling: it wraps telemetry.write_granule_ids in a 50 ms sleep and asserts meta["phase_timings"]["write"] >= 0.05, so a future move back outside the bracket fails rather than silently under-reporting phase_write in every run parquet.

Comment thread src/zagg/telemetry.py Outdated
granule_ids_key(name, spec),
json.dumps(
{
"schema_version": SCHEMA_VERSION,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] The granule-id sibling carries no spec marker of its own and borrows the D20 record's SCHEMA_VERSION — the PR body's "Both carry a schema_version/spec marker of their own so a future reader has a versioning handle" is only true of the refusal manifest.

json.dumps(
    {
        "schema_version": SCHEMA_VERSION,
        "granules_sha256": granules_sha256(ids),
        "granule_ids": ids,
    }
).encode(),

SCHEMA_VERSION is declared as "Version stamped into every record (issue #297). The bump-on-key-change rule applies once the schema is released; while unreleased, pre-release key changes (e.g. the D19 rev 2 / D20 record) rev in place and this stays 1." — it versions the run record, which this object is deliberately not (that separation is the whole point of ruling (6)(c)). Welding them means a future D20 rev bumps the sibling's declared version without the sibling changing, and a sibling format change cannot be versioned without revving the record.

The refusal manifest in this same PR got the shape right — REFUSAL_SPEC = "zagg-refusals/1" plus schema_version — and the _sibling_key refactor was justified precisely as "so the two siblings are named by ONE rule and cannot drift"; the version marker is the other half of that identity and it drifted. Note also that dedup.leaf_recorded_ids never reads either field, so today this is a write-only marker owned by the wrong schema: a reader that one day does branch on it would branch on D20's number.

A GRANULE_IDS_SPEC = "zagg-granule-ids/1" beside GRANULE_IDS_NAME, emitted alongside the existing key, matches the manifest's own precedent and costs one line.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 7b04101GRANULE_IDS_SPEC = "zagg-granule-ids/1" now sits beside GRANULE_IDS_NAME, and the object carries it:

{
    "spec": GRANULE_IDS_SPEC,
    "granules_sha256": granules_sha256(ids),
    "granule_ids": ids,
}

One judgement call worth flagging: I replaced schema_version rather than emitting the marker alongside it. Keeping SCHEMA_VERSION would have preserved the exact defect you identified — a D20 rev would still bump the sibling's declared version — and the sibling has no second version axis to carry, so spec is the whole handle. Say the word if you want both keys for symmetry with the refusal manifest; it is one line. (The manifest keeps both because it is run-record-class root telemetry.)

Lenient read, as asked: leaf_recorded_ids never branches on the marker — the hash pairing decides — and its docstring now says so explicitly, so an unknown zagg-granule-ids/N that keeps these two keys stays readable and an unrecognized one degrades to the ruled unrecorded-ids fallback rather than raising. Pinned by TestLeafRecordedIds::test_the_spec_marker_is_read_leniently (marker rewritten to zagg-granule-ids/99 and to None, both still round-trip the ids); test_write_read_roundtrip_sorted_and_self_hashing pins spec == "zagg-granule-ids/1" and the absence of schema_version.

The PR body's Spec-assessment sentence is corrected in the same pass — it now says the manifest carries spec + schema_version and the sibling carries its own spec, instead of claiming both carry a schema_version/spec pair.

@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Question triage — three of the four are defaults, not decisions. Merging this PR accepts them.

Reviewing the four questions against what is actually at stake, only one warranted espg's time, and it is now filed separately so it does not gate this merge:

  • (1) seam vs dispatcher placement — RECOMMENDED: keep, merging accepts. This is not an open question any more; it has data. The adversarial review verified the implementation, the justification, and the cost: gated on a committed leaf, one object per (node, window), +9.1% objects/shard (11→12 on the o9 hive config), +2,721 PUTs ≈ $0.0136 against $65.30 compute = 0.02%, ~110 B/id. It also established that no test ever pinned the fleet's exact object set, so leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397's "byte-identical when unarmed" property was unpinned to begin with. The alternative leaves the contraction guard permanently inert on the fleet.
  • (2) refusal_manifest_path on local summaries only — RECOMMENDED: leave as-is. A coin flip with a stated rationale (an always-None fleet key reads as "no refusals" rather than "not recorded"). Mirroring it is one line whenever a fleet writer exists.
  • (3) manifest unbounded — RECOMMENDED: leave as-is. Proportional-to-the-refusal-set is the point of (9)(c); truncating would defeat it, and fold 5a7e3eb already made the worst case degrade safely (composition moved inside the fail-open guard, so a MemoryError costs the manifest, not the run record). Revisit if a real run produces an unwieldy object.
  • (4) the torn-rewrite hole — SPLIT OUT to Skip gate can pass on a torn rewrite: leaf stamped by run 2, sidecar still run 1's #407. This is the one genuine item: pre-existing (inherited from leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397, neither widened nor narrowed here), a correctness question, and a D4 write-discipline change that touches the spec — so it belongs in its own PR with a fixture pass, not in a three-ruling follow-up trio.

So: this PR is ready to merge. All three rulings implemented, review folded (5 findings), CI green on 7b04101, MERGEABLE. Nothing above blocks it; (1)–(3) are accepted by merging, and (4) lives on #407.

Tracked in #402.

@espg
espg merged commit 4edbf0e into main Aug 8, 2026
13 checks passed
@espg
espg deleted the claude/388-followup-trio branch August 8, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant