leaf_column rename, granule-id sibling objects, refusal manifest (issue #388 follow-up trio) - #403
Conversation
| # 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 |
There was a problem hiding this comment.
🤖 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 NoneSo 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).
There was a problem hiding this comment.
🤖 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 NoneBoth halves are pinned:
test_semantic_mismatch_with_equal_sets_rewrites_not_refusesnow assertsself.loads == 0— the 2,721 needless CA o9 GETs are gone.- New
test_semantic_mismatch_without_a_sibling_is_not_unrecorded: the same quadrant with the sibling absent returnssemantic-mismatch, notunrecorded-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 no longer reports 100%cells_unrecorded.
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.
| """ | ||
| import logging | ||
|
|
||
| units = [] |
There was a problem hiding this comment.
🤖 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:
MemoryErrorbuildingunitsfor 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. AMemoryErrorinsidejson.dumpsis caught and degrades; the identical failure one statement earlier is not.TypeErrorfromfor g in (meta.get("missing_granules") or [])on a malformed meta — a shape the function explicitly anticipates one line above withif 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".
There was a problem hiding this comment.
🤖 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: aMemoryErroron an unbounded refusal set, or a malformedmissing_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→ theTypeErrorout offor g in (meta.get("missing_granules") or [])→None.- a truthy list subclass whose
__iter__raisesMemoryError→None.
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.
| # 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) |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 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() - _t0and 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.
| granule_ids_key(name, spec), | ||
| json.dumps( | ||
| { | ||
| "schema_version": SCHEMA_VERSION, |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 7b04101 — GRANULE_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.
|
🤖 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:
So: this PR is ready to merge. All three rulings implemented, review folded (5 findings), CI green on Tracked in #402. |
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
columnkey and the record-bornegranule_idsexist onmainonly between3f13af2and 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
columnrenames toleaf_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, andtools/generate_spec_fixtures.py's read of the seam's metadata. Two reasons, both recorded at the key:columnis SQL-reserved in DuckDB and Trino/Athena — the unquotedWHERE column IS NOT NULLis a parse error, so every filter on the run parquet would needWHERE "column" IS NOT NULL— andleaf_columnreads correctly on the pyramid column's own record ("the column this LEAF carries", not "the column this record is"). The spec-fixture file keycolumnintests/data/spec/column.expected.jsonis the fixture schema's own name and is deliberately untouched (published; not the D20 record).(6)(c) —
granule_idsmoves off the record/envelope into a sibling object (c980446). The record now carries the catalog hash only; the id LIST lives ingranules.jsonbeside the stats sidecar, on the sidecar's own spec-keyed grammar (granules_{window}.json;{stem}.granules.jsonundermorton-hive/3), derived through one private_sibling_keyowner so the two names cannot drift.classify_leaf_identitytakes aload_recorded_idscallable and invokes it only when the recorded and plannedgranules_sha256DISAGREE — 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 foldc320eea; 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.)dedup.shard_status(one GET per shard across a whole shardmap inhas_run) andtelemetry.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.granules_sha256of the list it holds, anddedup.leaf_recorded_idsaccepts 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) #388unrecorded-idspath, exactly as ruled for question (4)(a).3f13af2..this-PR window (review fold9d7f65d): 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_idsfalls back torecorded["granule_ids"]when the sibling is absent or unusable, so those leaves stay guarded and self-heal on their next rewrite instead of classifyingunrecorded-idsforever. The sibling wins whenever both exist; the fallback pairs by construction (build_recordhashed the very ids it embedded).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 togranules_2020.json; the docs' footprint list and the.github/scripts/bench_objects.pyobject model both gain it (see the note under Testing).(9)(c)+(a) — a store-root refusal manifest (
b462ef2). A run that ends withcells_refused > 0writes ONE small JSON at the store root,refusals_{ts}_{run_id}.json— timestamp-first likestats_*.parquetandsweep_stats_*.json, and outside thestats_*.parquetglob the sweep's run-record discovery scans. It carries the run context needed to act (run_id, timestamp, the run'ssemantic_hash, zagg version) and one entry per refused unit:shard_key,window, the classification (contraction/mixed),n_missing, and the completemissing_granuleslist — 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.coverage.mocunion andtouch_store_root—runner._write_refusals, withc71198f'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.(shard, window)unit (leaf_identity_gate(window=...)), because on a windowed store the shard alone is ambiguous.refusal_manifest_path(local summaries only,Nonewhen 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.str(shard_key), i.e. lexicographically —10precedes9. 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:
c320eea— the 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 provablysemantic-mismatchregardless 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-mismatchnow short-circuits before any load, pinned atloads == 0. Second-order fix: with the sibling absent that quadrant used to degrade tounrecorded-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.9d7f65d— back-compat read of the record-bornegranule_idsfor the3f13af2..this-PR window (see the (6)(c) bullet above).5a7e3eb—write_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 aMemoryErroron question (3)'s unbounded case or aTypeErroron malformedmissing_granulesused to destroy the run record of a run that had already done all its work.write_granule_idsalready had the whole body inside its guard; these now match.45bb7eb— the sibling PUT moved inside the write-phase bracket on both leaf families. It sat one line after_write_elapsed +=(and afterwrite_s +=on the raster seam), sophase_timings["write"]→phase_writeunder-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.7b04101— the granule-id sibling got its ownspec: "zagg-granule-ids/1"marker instead of borrowing the D20 record'sSCHEMA_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.mdfollows 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 inflatescells_unrecorded.Testing
uv run pytest -qon this branch (three ruling commits + five review folds): 3,671 passed / 38 skipped / 1 failed — the failure is the pre-existing, environment-dependenttest_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 cleanmain.uv run ruff check src tests: clean on the diff; the pre-existingN818atsrc/zagg/registry.py:64is flagged, not fixed.uv run ruff format --check src tests: clean on the diff; the pre-existingtests/data/benchmark/README.mdreformat is flagged, not fixed.test_column.py's 12 seam assertions follow.metadata["phase_timings"]["column"]is a phase name, not the record key, and is deliberately unchanged.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);TestClassifyLeafIdentityrebuilt around a counting loader (0 calls on the fast path, exactly 1 on mismatch,Noneloader →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 nogranule_idsand the sibling has them.TestClassifyLeafIdentity::test_semantic_mismatch_with_equal_sets_rewrites_not_refuses(nowloads == 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(malformedmissing_granules+ aMemoryErrorraiser);test_hive.py::test_write_phase_covers_the_granule_id_sibling.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.pyvector with the exact dropped id,test_raster_runner.pyraster in the STAC-item id space), therefusal_manifest_path is Nonepin on the--allow-contractionrerun, the CLI pointer test, the summary key-set pin, and the windowed unit-identity pin.src/testschanged:.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_maxoff 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. Besidestests/test_benchmark_objects.py, it is imported by.github/scripts/run_benchmark.pyandrun_full_aoi_benchmark.py, executed bylambda-benchmark.ymlandlambda-benchmark-fullaoi.yml, and base-pinned frommainbylambda-benchmark-command.yml. The deploy coupling was checked and is safe: the command workflow never redeploys, and the push-to-mainpath 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, andtests/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_VERSIONstays 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.jsonis a sibling of a sidecar the spec does not normatively describe beyond its naming grammar, and the refusal manifest is per-run root telemetry likestats_*.parquet/sweep_stats_*.json. Both carry a version marker of their own so a future reader has a versioning handle: the manifestspec: "zagg-refusals/1"plusschema_version(it is run-record-class), and the siblingspec: "zagg-granule-ids/1"alone (review fold7b04101— as first written it borrowed the D20 record'sSCHEMA_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 ruledunrecorded-idsfallback rather than raising.Questions for review
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'ssemantic_hashstamp; putting the sibling at the dispatcher sites would have un-banked the catalog half, leaving every fleet-written leafunrecorded-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 asplanned_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 onatl03_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 tworunner.pysidecar 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/:1860pass neithersidecar_specnorwrite_sidecar(spec=), so the fleet writes both the sidecar and the sibling withspec=None— on amorton-hive/3store both land under the legacy name; (b) on the skip path, an absent sibling costs one extra S3head_objectper skipped unit intouch_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.refusal_manifest_pathis on the two LOCAL summaries only (always present,Nonewhen nothing refused), not on the Lambda ones, because there is no fleet writer at all — unlikerun_stats_path, which both backends produce. That is an asymmetric summary key set; the alternative (always-Noneon the fleet) reads as "no refusals" rather than "not recorded". Left as-is; say the word if you want the key mirrored.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
5a7e3ebat least made the worst case degrade: the unit composition is now inside the same fail-open guard as the PUT, so aMemoryErrorhere costs the manifest, not the run record.)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_sha256andsemantic_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.