leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) - #397
Conversation
|
🤖 from Claude Phase 4 pushed (8304b04, docs-only): |
| `kms:Decrypt` + `kms:GenerateDataKey`. | ||
|
|
||
| **Known gaps** — plan lifecycle rules around them, they are not promised | ||
| away: the §7 sweep's **ancestor overviews and `pyramid.json` envelopes |
There was a problem hiding this comment.
🤖 from Claude (review)
[high] pyramid.json names no object zagg writes — the gap list points an operator at a filename that does not exist, and names one of four aging families.
grep -rn "pyramid\.json" src tests docs deployment returns exactly two hits: this line and the zagg.lifecycle module docstring it was drafted from (src/zagg/lifecycle.py:67). The sweep's per-node envelopes are {family}.rollup.json — sweep.py:105 (rollup_name), and the module docstring at sweep.py:39: "Rollup objects are JSON sidecars at digit nodes, named {family}.rollup.json". One per family in DEFAULT_FAMILIES = ("stats", "moc", "submap", "overview") (sweep.py:61), plus a root sweep_stats_{ts}.json per pass (sweep.py:689). Nothing anywhere is called pyramid.json.
Two knock-on inaccuracies in the same sentence:
- it scopes the gap to "ancestor overviews and
pyramid.json" — butstats.rollup.json,moc.rollup.jsonandsubmap.rollup.jsonat every digit node are equally outside the unit footprint and equally untouched by a skip. A skip-only store ages its whole ancestor rollup layer, not just the pyramid. - the workaround an operator will reach for — "just run the sweep periodically to keep them alive" — does not work, and the section does not say so: "A second sweep over an unchanged tree recomputes but PUTs nothing" (
sweep.py:33). For a paragraph whose whole purpose is "plan lifecycle rules around them", that is the load-bearing half.
If pyramid.json was instead meant as the manifest's pyramid block: that block lives inside morton_hive.json, which is in the store-root trio lifecycle.touch_store_root refreshes — so under that reading the sentence names a non-gap. Either way the name needs to change here and at lifecycle.py:67.
There was a problem hiding this comment.
🤖 from Claude
Folded in c3ae8b4. (Restating the finding here — the comment above posted as a literal scratchpad path rather than its contents: pyramid.json names no object zagg writes, the gap was scoped to one of four aging families, and the "run the sweep to keep them alive" workaround silently no-ops.)
Confirmed all three: grep -rn "pyramid\.json" src tests docs deployment now returns zero hits — it previously returned exactly the two the finding names (docs/hive_layout.md:699 and the src/zagg/lifecycle.py:67 docstring it was drafted from), and both are fixed in the same commit. The lifecycle change is docstring-only, no behavior diff.
The gap paragraph now names the real artifacts and scopes the aging honestly:
That is the whole ancestor layer, not the overviews alone — all four rollup families age identically (
stats.rollup.json,moc.rollup.json,submap.rollup.json,overview.rollup.jsonat every digit node, one persweep.DEFAULT_FAMILIESentry), plus each pass's rootsweep_stats_{ts}.jsonrun record. And re-running the sweep does not refresh them: a second sweep over an unchanged tree recomputes but PUTs nothing, so the obvious "just sweep periodically to keep them alive" workaround silently no-ops.
The sweep.py:33 sentence was the load-bearing half you flagged and it is now stated in both places — the doc paragraph above and the zagg.lifecycle module docstring ("A repeat sweep does not refresh them either (an unchanged tree recomputes but PUTs nothing)").
| **local backend only** (D8 keeps the Lambda dispatcher from writing to the | ||
| store, and the handler has no root-touch mode yet — PR #397 question (10)). | ||
| Until those close, do not scope a hard-expiry rule over the whole store | ||
| prefix and rely on skip re-runs alone to keep it alive: scope expiry to the |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The lifecycle-rule prescription is not implementable with an S3 lifecycle rule, and an operator following it verbatim expires the very objects the previous sentence flags.
"scope expiry to the leaf data planes" presumes leaves are separable from ancestor overviews by a lifecycle filter. They are not:
docs/specification.md§4.1: an overview zarr is written "at an ancestor digit node of the hive tree" — inside the same{sign+base}/{d1}/…/prefix tree as the leaves it summarizes, just shallower.- §4.2: "Nothing about the name distinguishes an overview from a leaf" — both are
{window}.zarr/all.zarr; classification is by root-group attrs (§4.3), which no lifecycle engine can read. - S3 lifecycle filters key on prefix, object tags, and size only — no suffix, no depth — and zagg's writers set no object tags. The only prefix that selects leaves while excluding ancestor overviews is a per-shard-node prefix, i.e. one rule per leaf node against a 1,000-rule-per-bucket cap.
So the rule an operator actually writes is over the digit tree, which sweeps up the ancestor overviews this same paragraph just said a skip never touches. The second alternative has the same defect: "give root and pyramid objects their own (longer) retention" is expressible for the root trio (three named objects at a known prefix) and not expressible for the pyramid, for the reason above.
Not data loss — §4.1 makes overviews "regenerable caches, never load-bearing", so the cost is a full pyramid rebuild via the sweep — but the paragraph presents this as the safe configuration, which is the opposite of its intent. Suggest replacing the prescription with the constraint: there is no prefix or tag that separates leaves from ancestor overviews today, so a hard-expiry rule over the digit tree costs the pyramid (rebuildable by a sweep) and the root trio must be excluded by name.
There was a problem hiding this comment.
🤖 from Claude
Folded in f9b6ffd. (Restating — the comment above posted as a literal scratchpad path: the lifecycle-rule prescription is not implementable as an S3 rule, and an operator following "scope expiry to the leaf data planes" verbatim expires the pyramid the previous sentence just flagged.)
The prescription is gone. It is replaced by the constraint plus three honest options:
There is no lifecycle rule that separates leaves from the ancestor artifacts above them. An S3 lifecycle filter keys on prefix, object tags, and size only; zagg's writers set no object tags; and per the specification §4.1–§4.2 an overview lives at an ancestor digit node inside the same prefix tree as the leaves it summarizes, where "nothing about the name distinguishes an overview from a leaf" (classification is by root-group attrs, which no lifecycle engine reads). So "scope expiry to the leaf data planes" is not writable as a rule: the only prefix that selects leaves while excluding ancestors is a per-shard-node prefix, against a 1,000-rule bucket cap.
Then (1) no expiry rule on a pyramid-declaring hive store; (2) expiry over the digit tree accepted with the ancestor artifacts named as regenerable caches per §4.1 — a sweep rebuild, not data loss — and the store-root trio excluded by name because morton_hive.json is REQUIRED reader-facing schema; (3) the levers named as not existing yet: write-time object tags (nothing in zagg writes any) and a scheduled ancestor-layer refresher, with issue #384's staged sweep called "the obvious candidate, but nothing about it promises a refresh today" and question (10) cross-referenced for the fleet-side root touch. No tagging invented, no #384 behavior promised.
| it entirely (the operator's unconditional-rewrite hammer — it does not | ||
| acknowledge a contraction, it bypasses the guard). The **deployed Lambda | ||
| handler has not yet opted in**: the seams default off, so fleet re-runs | ||
| still rewrite unconditionally today, and fleet sidecars written by current |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] "fleet sidecars … feed the gate only after the handler enablement lands" is wrong in both directions — vector sidecars start feeding it on redeploy with no handler change, and raster sidecars never will, including for LOCAL re-runs where the gate is armed.
Vector — the claim is too pessimistic. deployment/aws/lambda_handler.py:1842-1850 passes the seam's own metadata dict straight to build_record (with granule_ids=event.get("granule_urls") already riding), and the seam now stamps metadata["semantic_hash"] from the run value or its own config (hive.py:1396-1400, hive.py:1589-1590), which build_record's validated fallback picks up (telemetry.py:183-186). So the moment this release is deployed — gate still off fleet-side, zero handler changes — fleet vector sidecars record both identity halves and do feed the gate (a local re-run over a fleet-built vector store skips). The PR body says exactly this: "fleet vector sidecars stop recording null for the identity half with zero handler changes". The doc contradicts its own PR body.
Raster — the claim is too optimistic, and this is the operator-visible half. The raster branch rebuilds body from an explicit key list (lambda_handler.py:1355-1370) and never copies semantic_hash, so raster fleet sidecars record semantic_hash: null even after this release ships. In classify_leaf_identity that makes semantic_match False (dedup.py:242), the fast path fails, granule_ids is present (raster_granule_ids(event["granules"]), lambda_handler.py:1396), the id sets are equal, and it lands on semantic-mismatch → rewrite (dedup.py:265-270). Every raster leaf, every run, no self-heal until that leaf has been rewritten under this release.
The consequence this section owes the reader: an operator re-running a fleet-built raster store locally — where the gate is on by default per the previous sentence — gets cells_current: 0 forever, and nothing here explains why. That is question (1)'s six-key requirement leaking into local behavior, not just fleet behavior.
There was a problem hiding this comment.
🤖 from Claude
Folded in 68ec5f1. (Restating — the comment above posted as a literal scratchpad path: the "fleet sidecars feed the gate only after the handler enablement lands" sentence is wrong in both directions — vector needs no handler change, raster never self-heals, and the raster consequence is visible on local re-runs.)
Verified both halves against the tree before rewriting: the vector branch hands the seam's own metadata dict to build_record (lambda_handler.py:1842-1850), the seam stamps metadata.setdefault("semantic_hash", ...) (hive.py:1589-1590), and build_record's validated fallback picks it up (telemetry.py:183-186); the raster branch rebuilds body from a closed key list (lambda_handler.py:1355-1370) with no semantic_hash in it.
The single sentence is now a two-bullet split. Vector: "from this release's deploy onward — gate still off fleet-side, zero handler changes — fleet vector sidecars carry both identity halves, and a later local re-run over that store can skip", which is what the PR body already asserted. Raster: semantic_hash: null → fast path fails → semantic-mismatch rewrite every run, "with no self-heal until the leaf has been rewritten under a handler that carries the key", plus the operator consequence you asked for:
The consequence is local, not merely fleet-side: re-running a fleet-built raster store locally — where the gate is on by default — reports
cells_current: 0indefinitely until question (1)'s key-list fix lands.
| | verdict | when | what happens | | ||
| |---|---|---| | ||
| | **current** | both halves match, leaf stamped, column agrees with the declaration | fold no-ops; the unit writes **nothing** (no arrays, no stamp, no sidecar, no sub-map, no column — zero sweep dirtiness); the lifecycle touch below runs; counted as `cells_current` | | ||
| | **refused** | the planned set drops recorded ids: `recorded ∖ planned ≠ ∅` — deliberately *not* strict-subset, so a shardmap that grew while silently dropping old granules (an upstream purge behind a fresh catalog query) still trips it | the unit refuses, names the missing ids in the log, writes nothing, and counts as `cells_refused` — never as an error. `--allow-contraction` (`agg(allow_contraction=True)`; on Lambda an `allow_contraction` event field) turns it into a normal rewrite | |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] "names the missing ids in the log" — the log names at most five of them.
hive.py:1294-1297 truncates before the warning:
shown = ", ".join(missing[:5]) + (f", +{len(missing) - 5} more" if len(missing) > 5 else "")
logger.warning(
f"shard {shard_key}: REFUSED — planned inputs drop {len(missing)} recorded "
f"granule id(s) ({shown}); pass allow_contraction to rewrite (issue #388)"
)The full set rides only metadata["missing_granules"], and a refused unit writes no run-record row (question (9)), so on a real contraction the dropped ids are not recoverable from the log or from any durable artifact — the operator sees five names and a count. Worth saying "names the first few missing ids and their total count", since the table row is what an operator will plan a triage step around.
There was a problem hiding this comment.
🤖 from Claude
Folded in 7c62ae6. (Restating — the comment above posted as a literal scratchpad path: "names the missing ids in the log" overstates, since hive.py:1294 truncates to missing[:5] + a count.)
The refused table row now reads:
the unit refuses, logs the first five missing ids and their total count (the full list rides only the in-process
missing_granulesmetadata, and a refused unit writes no run-record row — PR #397 question (9) — so plan triage around the count, not around recovering the set from durable artifacts), writes nothing, and counts ascells_refused— never as an error.
That states both halves you flagged: the truncation, and the recoverability limit (no durable artifact carries the full set, because the refusal writes no row — cross-referenced to question (9), where the refusal-row shape is the standing decision).
| current too, verified by reading them, never trusted from the record: the | ||
| leaf must carry its commit stamp, and the [leaf | ||
| column](#leaf-columns-zagg-column1) must agree with the run's declaration | ||
| (the §4.6 config-decides gate: the declaration moves *neither* identity |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] Bare §4.6 here and bare §7 at line 699 resolve to two different documents, and specification.md's own §7 is an unrelated section.
§4.6 is docs/specification.md §4.6 (Leaf column artifacts, zagg-column/1). §7 at line 699 ("the §7 sweep") is docs/design/sparse_coverage.md §7 — docs/specification.md §7 is "Conformance fixtures". A reader who resolves the first reference to the spec and carries that document 67 lines forward lands on the wrong section.
This doc otherwise qualifies its spec references: "the specification §4.4–§4.5" (line 308), "Byte grammar: specification.md §4.6." (line 375), "design §4" (line 519). Matching that convention here is one link. (Bare "§7 sweep" is pre-existing at line 793 and fine on its own — it is the collision with an unqualified spec § inside the same new section that misleads.)
There was a problem hiding this comment.
🤖 from Claude
Folded in d4a65bf. (Restating — the comment above posted as a literal scratchpad path: bare §4.6 and bare §7 67 lines apart resolve to two different documents, and specification.md §7 is the unrelated "Conformance fixtures".)
Both are qualified per the doc's own convention (lines 308/375/519):
§4.6→ "the specification §4.6 config-decides gate"- "the §7 sweep" → "the design §7 sweep"
The follow-on fold of finding (2) added two more qualified refs in the same section ("the specification §4.1–§4.2", "specification §4.1"), so every § in the new section now names its document. The pre-existing bare "§7 sweep" at line 793 is left alone as you noted — it carries no adjacent spec § to collide with.
|
🤖 from Claude All four phases complete, all four adversarial-review cycles folded (26 findings total), CI green on |
|
🤖 from Claude espg rulings (in-session, 2026-08-07): questions (5)/(6)/(9) are decided per the recorded leans — (5)(c) the record/parquet key renames to |
|
🤖 from Claude Follow-up bookkeeping now lives outside this thread: #402 tracks all ten review questions (rulings, landing sites, sequencing); #401 is the espg-named handler-plumbing issue for question (1a); the D19 hash-epoch companion ((7)(b)+(8)(c)) is recorded on #384 plan delta 2. The (5)/(6)/(9) pre-merge trio is in flight on this branch. |
Closes #388. Refs #381 (point (11)).
Blocked by #391Unblocked 2026-08-07: #391 merged intomain(ad8aa30); base retargeted tomainandorigin/mainmerged into the branch (stacking order #389 → #391 → this PR held).What this does
Leaf-level skip-if-current: a re-dispatched unit whose planned inputs and semantics match what its leaf already records no-ops the fold, reports
currentin the run stats, and touches every object under the leaf's prefix footprint (S3 self-copy,MetadataDirective=REPLACE) so bucket lifecycle purges see actively-used products. Identity is the ruled PAIR — the run'ssemantic_hash(D19, what/how) × the unit's planned granule-id set (over what) — compared per (shard, window) unit against the leaf's D20 sidecar; whole-set underall/ad-hoc. Both leaf families ship it (espg ruling on the issue): the vector seam (zagg.hive.process_and_write_hive) and the raster seam (zagg.processing.raster.process_and_write_raster_hive), shared by both backends.granules_sha256compare (+semantic_hashequality). The id-set difference runs only on hash mismatch.recorded ∖ planned ≠ ∅, so the mixed add-and-drop signature trips it): the unit refuses, names the missing granule ids, and counts in its own run-stats counter — never folded into failures — unlessallow_contractionrides the run (anagg()kwarg + a worker event field; deliberately NOT a config knob, which would pre-approve contraction store-wide against the ruling's intent)..zarrtree, its stats sidecar and D22 sub-map, and the pyramid v2: leaf-worker column writes (issue #383) #391 column artifact + its sidecar. Touched-object count rides the run stats. Local stores touch viaos.utime; S3 via aboto3self-copy (already a dependency — obstore'scopyhas no metadata-directive control, and S3 rejects an identity self-copy withoutMetadataDirective=REPLACE).Phase 2 (what landed)
The gate lives in one shared helper,
hive.leaf_identity_gate(sidecar read →dedup.classify_leaf_identity→ skip/refuse/proceed verdict + the early-return unit metadata), called by both seams behind new opt-in kwargs (skip_if_current/allow_contraction/semantic_hash/sidecar_spec, all defaulting to today's behavior — the deployed Lambda handler is byte-identical until it opts in, per Questions (1)). The local backend arms the gate by default (skip_if_current=not overwriteon both the aggregation and raster paths —overwrite=Truestays the operator's unconditional rewrite, per Questions (2)), passing the RUN config's D19 hash and the manifest spec in effect. A skipped/refused unit returns before any read or write and the dispatcher writes no record, no sidecar, no sub-map, no run-parquet row for it (the sidecar-clobber gate from Questions (1), now dispatcher-side on the local path). Both seams also now stampmetadata["semantic_hash"](run-passed, or their own config's hash as fallback), which lights up phase 1's validatedbuild_recordfallback: fleet vector sidecars stop recordingnullfor the identity half with zero handler changes (the pinned handler test now asserts the hash, not the null). Summaries gain three always-present counters —cells_current,cells_refused,cells_unrecorded(the inert-guard rewrites, counted apart) — on all four paths (spatial/raster × local/lambda; the lambda ones read worker envelopes and stay zero until the handler opts in).agg(allow_contraction=...)threads through every strategy; the lambda dispatchers add theallow_contractionevent key only when true (both the_build_cell_eventconstruction site and the raster event builder), so default event payloads stay byte-identical.Phase 3 (what landed)
The touch lives in a new small module,
src/zagg/lifecycle.py(hive.pywas already past the §4 line aim, so the feature gets its own file, thecolumn.pyprecedent):touch_current_unitassembles ONE unit's footprint — the leaf.zarrtree (commit stamp, arrays, and the in-leafcoverage.mocride the prefix walk), the D20 stats-sidecar and D22 sub-map siblings via the spec-keyed naming seams (telemetry.sidecar_key/sweep.submap_key), and the #383 column tree plus its own stats sidecar when the run declares one — andtouch_unit_footprintrefreshes every object'sLastModified. Local stores useos.utime; S3 uses the boto3 self-copy (CopyObjectonto itself,MetadataDirective=REPLACE— one LIST per leaf/column tree, one copy per object). Both seams call it on the skip arm only (a refusal touches nothing — only a certified-current unit resets the purge clock), and only after the fold's artifact checks passed, so the touch cannot resurrect thecolumn-driftambiguity: the column is touched exactly when declared-and-committed. Fail-open is structural at three layers — per-object (count + log; an ABSENT sibling like a never-written sub-map is neither touched nor failed), per-footprint (assembly or LIST faults count one failure and abandon the remainder), and at the seam (try/except— a raising touch still returnscurrent: True). Counts ride the unit metadata (touched_objects/touch_failed) and roll up into two new always-present summary keys,objects_touched/touch_failures, via the same_identity_countsseam as the phase-2 counters (never folded intocells_error). Documented caveats, not solved: versioned buckets mint a new version per touch; a ≥5 GB object would need multipart copy and simply counts failed; the self-copy REPLACEs user metadata with none (zagg's writers attach none).The #391 handoff (deferred run-record key)
The 2026-08-07 scope-note ruling (issue comment) is discharged by phase 1 —
metadata["column"]enters the run-record schema via thecolumnkey below (with both merge arms and the parquet round-trip pinned in the phase-1 fold), so no further phase-2/3 work item exists for it; only the rename question (5) stays open.PR #391 deferred "should
metadata["column"]enter the run-record schema" to this PR (the fold reply). Landed here in phase 1: the D20 record gainscolumn(the artifact basename — a scalar string, in_EQ_OR_NONE_KEYSand_ROW_SCALARS, so D22 run-record discovery sees column-bearing leaves without a tree listing) andgranule_ids(the recorded id list behindgranules_sha256, in the hash's own canonical order — what a contraction refusal diffs against to name dropped ids; record + sidecar only, never a parquet column). The column's resolution set deliberately stays in the artifact's ownzagg_columnattrs — the skip gate verifies column currency by reading the artifact (the spec's "read the columns, not the manifest" posture), not by trusting a record that can lag it.Phases
zagg.dedup.classify_leaf_identity: equal / expansion / pure-contraction / mixed, every ambiguity → rewrite) + the D20 record keys above + thesemantic_hashmetadata fallback (the shared seams will stamp it, so Lambda-written sidecars finally record the identity half — today the handler passes none and fleet vector sidecars carrynull) + testsunknown-plannedsplit from the empty planned set; the one-sided-conservatism correction on theunrecorded-idsrationale; D19-shape validation of themetadatasemantic-hash fallback; thecolumn-is-null meaning pinned; both merge arms + awrite_run_parquetround-trip forcolumn), 2 standing as design forks under Questions for review (5) and (6)cells_current/cells_refused, plusunrecorded-idscounted apart from ordinary rewrites),allow_contractionplumbing (run kwarg + event field) + tests (windowed included)currentforever —bac97ec); the pyramid v2: leaf-worker column writes (fleet side) #383 leaf column is verified as an artifact rather than trusted from the record, so enabling/droppingoutput.pyramidover an existing store rewrites instead of skipping (5ab707d); a valid-JSON non-object sidecar degrades to rewrite instead of failing the unit (6967258); the CLI gained the three guard counters and--allow-contraction(f3ba09b); the raster gate matrix + the raster dispatcher's event-key pin now mirror the vector ones (546dac8). The raster Lambda handler's droppedsemantic_hashis named at the seam and folded into question (1) (bebfe68) rather than fixed indeployment/aws/. Two halves stay standing as questions (8) and (9).c71198f); the self-copy preservesStorageClassinstead of silently demoting every touched object to STANDARD, with an exact ACL/KMS caveat list (b6caad1); the S3 client is cached process-wide per store kwargs and built under a lock through its ownSessioninstead of once per unit off the default session (7cc08cb);touch_failuresgets its own CLI line so a blanketAccessDeniedcan't render as unqualified success (3c7269f); the raster-handler seam note now names six dropped keys, not four (5ed8970); the column sidecar name comes fromcolumn._sidecar_nameand the over-touch direction is pinned (58f06fb). The fleet half of finding (1) is question (10) below.docs/hive_layout.mdoperator section (stating plainly that the contraction guard cannot protect leaves written before this release) + the spec assessment belowSpec assessment (flagged, not silently decided)
Phase-4 re-verification (final tree): the assessment above holds after phases 2–3 and both folds —
docs/specification.md,tools/generate_spec_fixtures.py, andtests/data/spec/are untouched across the whole branch; nospecmarker string moved anywhere undersrc/zagg;SCHEMA_VERSIONstays 1 (the two new D20 keys are additive and nullable, and the D20 design record never enumerates per-key additions — then_obs_read/spill_blocks_closedprecedent — so no design-doc edit is due either). The lifecycle touch is contract-invisible by construction: it changes onlyLastModified(plus the documented S3 system-metadata side effects — ACL/SSE-KMS; storage class is preserved), object content is byte-identical, the §5 content hashes are over decoded values, and no zagg reader keys on theLastModifiedof any store object (the poller reads it on.status/result objects only, which are never touched). The gate itself only READS existing artifacts, and its column check implements §4.6's already-ratified "config decides, read the columns not the manifest" posture — no new normative language needed. The operator-facing semantics land indocs/hive_layout.md(narrative docs, phase 4), where §4.7's informative scope-out already places the stats/sub-map families.No byte-contract change. The stats/sub-map families are scoped out of
docs/specification.mdas operational (§4.7 informative: "no new byte layouts — the stats rollup reuses the D20 sidecar schema"); the only spec-normative pieces of the sidecar are the §5.3content_hashesshape and the sidecar naming grammar, both untouched. The two new D20 record keys (column,granule_ids) are nullable, telemetry-class additions per the D20 design record (docs/design/sparse_coverage.md), consistent with hown_obs_read(#374) andspill_blocks_closed(#370) landed;SCHEMA_VERSIONstays 1 (additive nullable keys, no key changes). Nospecmarker moves, so no conformance-fixture regeneration.Testing
origin/main(post-pyramid v2: leaf-worker column writes (issue #383) #391) and phase 2:uv run pytest -q= 3,593 passed / 38 skipped / 1 failed — the failure is the pre-existing environment-dependenttest_lambda_build.py::TestFunctionBuild::test_function_build_succeeds(flagged not fixed; same set pyramid v2: leaf-worker column writes (issue #383) #391 reports — the flakytest_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retriespassed on these runs). Phase 2 adds 20 tests: the seam gate matrix (skip / off-by-default / contraction / mixed / flagged-contraction / expansion / semantic-mismatch / unrecorded-ids / no-sidecar / hash stamp) on both leaf families, the per-window gate on a windowed store, three end-to-end localaggrerun tests (identical→cells_currentwith fold-not-invoked + sidecar-byte-equal,overwritedisables, contracted→refuse-then-flag-rewrites), the twoallow_contractionevent-key tests, and the summary-key byte-identity pins extended with the three counters. One existing test updated deliberately:test_hive_event_writes_stats_sidecarpinnedsemantic_hash is Noneon Lambda-written sidecars — the null phase 1 exists to eliminate; it now pins the recorded hash.uv run pytest -q= 3,621 passed / 38 skipped / 1 failed (same pre-existing lambda-zip-build failure). The fold adds 28 tests: destroyed-leaf and unstamped-debris reruns on both seams, a parametrized non-object-sidecar matrix, six column-drift cases (declaration on/off/unchanged, per-window, and the premise that the D19 hash does not move), the six raster gate cases that were vector-only (off-by-default /no-sidecar/unrecorded-ids/ expansion / semantic-mismatch / mixed), three raster localaggrerun tests, the two rasterallow_contractionevent-key tests, and four CLI summary tests.uv run pytest -q= 3,637 passed / 38 skipped / 1 failed (same pre-existing lambda-zip-build failure). Phase 3 adds 16 tests:tests/test_lifecycle.py(local footprint incl. column family, absent-column/absent-sub-map neither-touched-nor-failed, failingutimecounted-not-raised, unresolvable footprint fail-open; mocked-S3 self-copy assertingMetadataDirective=REPLACE+ identityCopySourceover the enumerated footprint,NoSuchKey= absent not failed,AccessDeniedcounted, LIST-fault abort, store-kwargs→client keying, local-store-builds-no-client) plus seam/rollup coverage (both seams' skip tests now pin bytes-identical + every-mtime-advanced + exact touch counts, refusals pin NOT-touched, a raising touch never un-skips, the column family moves with the leaf intest_column.py, and the local rerun asserts theobjects_touched/touch_failuressummary rollup). The two phase-2 “wrote nothing” pins moved from mtime-equality to content-equality deliberately — the touch's entire purpose is to move mtimes on a skip.uv run pytest -q= 3,646 passed / 38 skipped / 1 failed (same pre-existingtest_lambda_build.py::TestFunctionBuild::test_function_build_succeeds). The fold adds 9 tests: thetouch_store_rootfamily (local three-object touch, absent root MOC neither touched nor failed, missing root fail-open, S3 exact-keys-no-LIST), the storage-class preservation pin (LIST/HEAD echo + STANDARD default when S3 omits it), the client-cache pins (same kwargs → one client across units, different kwargs → a second; 16 concurrent touches across 8 threads build exactly one), the two CLItouch_failurespins, and the OVER-touch pins (a node neighbour / sibling window / string-prefix-extension key is not touched, intest_lifecycle.pyand end-to-end intest_hive_windows.py::test_skip_if_current_is_per_window). Two existing reruns (test_hive.pyvector,test_raster_runner.pyraster) now also assert the store-root manifest andaggregation.yamlmtimes advanced on a skip.uv run pytest -q= 3,645 passed / 38 skipped / 2 failed — the same pre-existing pair flagged at the top of this section (test_function_build_succeeds; the flakytest_invoke_fault_burns_an_attempt_and_retries, which fails on cleanorigin/main, resurfaced on this run). No code changed in phase 4.uv run pytest -qat the merge commit (pre-phase-2): 3,573 passed / 38 skipped / 1 failed (same pre-existing failure) — the pyramid v2: leaf-worker column writes (issue #383) #391 merge broke nothing.uv run ruff check src tests/uv run ruff format --check src tests: clean on the diff; pre-existing N818 atsrc/zagg/registry.py:64and the format diff ontests/data/benchmark/README.mdflagged, not fixed.Questions for review
The Lambda handler enablement is deliberately NOT in this diff (
deployment/aws/is excluded from agent edits per CLAUDE.md §1 unless an issue names it by name). The seams are opt-in kwargs (default off), so the deployed handler's behavior stays byte-identical until it opts in. To light the feature up fleet-side the handler needs, in both leaf branches:skip_if_current=True, allow_contraction=bool(event.get("allow_contraction"))on the seam call, plus its sidecar/sub-map writes gated onnot metadata.get("current")— without that gate a Lambda skip would clobber the good sidecar with ann_obs=0record, which is exactly why the seam defaults off. The dispatcher already sends theallow_contractionevent field (only-when-true, so existing payloads stay byte-identical). Happy to draft that handler change as a separate espg-named follow-up.The raster branch needs more than the vector one. The aggregation branch returns the seam's own dict (
"body": json.dumps(metadata),build_record(metadata=metadata, ...)), so phase 1's metadata fallback fires there and fleet vector sidecars stop recordingnullwith no handler change. The raster branch does not: it rebuilds a freshbodyfrom an explicit key list and passes THAT tobuild_record, sosemantic_hash/identity/current/refusedare dropped on the floor (grep -n semantic_hash deployment/aws/lambda_handler.pyreturns one hit, a docstring). Consequences: raster fleet sidecars keep recordingnulltoday, and once the gate is armed anullrecord can never match a resolved hash, soclassify_leaf_identityfalls through tosemantic-mismatchand every raster leaf rewrites forever without self-healing;_identity_countsover the raster lambda envelopes also stays permanently zero. The follow-up must therefore addbody["semantic_hash"] = meta.get("semantic_hash")plus the three gate keys (identity/current/refused) and the two phase-3 touch keys (touched_objects/touch_failed) alongside the opt-in kwargs — six keys, not four. Without the last two the fleet raster rollup would reportcells_current: N, objects_touched: 0, touch_failures: 0, indistinguishable from a touch that never ran. The seam carries this note at the stamp site so the requirement is not only in a PR body (5ed8970).overwrite=Truedisables the skip — an operator forcing overwrite gets today's unconditional rewrite. Reasonable, or should skip still apply under overwrite?The recorded id space is driver-dependent on the vector path (resolved s3 vs https hrefs — the documented
dedup.shard_statustrap). A driver flip between runs reads as a full mixed contraction and refuses per leaf;allow_contractionis the escape hatch and the docs call it out. OK to leave as-is (consistent withgranules_sha256semantics) rather than normalizing ids?The contraction guard is inert on every leaf written before this release.
granule_idsarrives with this PR, so a pre-leaf skip-if-current: input-identity no-op with lifecycle touch (semantic_hash x granule-id set) #388 sidecar has no recorded set to diff and classifiesunrecorded-ids→rewrite— i.e. a contracted rerun over an old leaf performs the wholesale rewrite the guard exists to prevent, silently. Phase 1 gives it its own classification so phase 2 can count it apart from ordinary rewrites and phase 4 can say so plainly, which is the plan of record. The alternative is to makeunrecorded-idsrefuse withoutallow_contraction, which is safe but means essentially every fleet leaf refuses once until it has been rewritten under this release. Ruling wanted: (a) count-and-document (current), or (b) refuse-until-rewritten?Raised by the phase-1 self-review at this comment; folded the docstring-accuracy half, left the behavior fork here.
Rename the
columnrecord/parquet key? PR pyramid v2: leaf-worker column writes (issue #383) #391 deferred the key's existence into this PR; the name is the part still open, and it is cheap to change only while the schema is unreleased.columnis SQL-reserved and reserved in both engines this parquet targets (DuckDB, Trino/Athena), so every filter must quote it —WHERE "column" IS NOT NULL; the unquoted form is a parse error. It is also ambiguous on the sidecar: the pyramid column's own stats record carries"column": null(only the leaf record carries the basename), so it reads as "the column basename this unit wrote alongside its leaf", not "the column this record is". Pick one and it is a two-line change: (a) keepcolumn(comment already documents the quoting), (b)column_name, (c)leaf_column— which also resolves the sidecar ambiguity outright, (d)pyramid_column.Raised by the phase-1 self-review at this comment and this one; the inaccurate "plain column filter" comment is fixed, the rename is not taken on agent authority.
Where should
granule_idsride? (supersedes the earlier sizing question, which measured against the wrong cap). At the pole figure, ~4,600 ids ≈ 550 KB per unit. That is comfortably under the 6 MB sync-response cap, but the costly consumers are the fan-out readers, and neither reads the field:dedup.shard_statusGETs one sidecar per shard (has_runloops it over the whole shardmap) and reads onlysemantic_hash/granules_sha256/content_hashes;telemetry.rows_from_statusGETs every mirrored envelope through a thread pool inside a 900 s worker andflatten_recorddiscardsgranule_ids(deliberately not in_ROW_SCALARS). So a few-thousand-leaf pre-run dedup check could go from a few MB of GETs to the better part of a GB. Options: (a) keep as-is on the record (envelope + sidecar) and measure both paths before phase 2 lands; (b) sidecar-only augmentation at the write sites — keeps it off the transport entirely, but needs the handler edit from (1) to cover fleet leaves; (c) split the id list into its own sibling object next to the sidecar, so identity-check readers keep their small GET and only a contraction refusal pays for the list — this adds an object family to the leaf footprint, which phase 3's touch and phase 4's docs would both have to cover.Raised by the phase-1 self-review at this comment; left standing as an architecture decision rather than folded.
granule_workersis not inDATA_SOURCE_PACKAGING_KEYS, which makes the seam's fallback hash clamp-sensitive (found implementing phase 2). The seams'semantic_hash=Nonefallback hashes the config the WORKER received — but both dispatchers send a per-cell config whosedata_source.granule_workersis clamped tomin(K, n_granules)(runner._clamped_data_source, issue Follow-ons to #183: dispatcher-side shard-aware granule_workers, and function-size variants gated on utilization #184), andsemantics.DATA_SOURCE_PACKAGING_KEYS = ("reader", "driver", "read_plan", "anonymous")does not excludegranule_workerseven though the D19 docstring says "read machinery" is packaging. So a small shard's worker-side hash can differ from the run-level hash. The drift is one-sided by construction: a mismatch degrades to today's rewrite (or astaleindedup.shard_status), and any collision implies output-identical semantics — never a false skip. The local runner is immune (it passes the run hash into the seam), and phase 2 documents the caveat on the seam docstring. The real fix — addinggranule_workersto the packaging keys — changes every existing product's D19 hash (manifest refusals on rerun into existing named stores), so it is not taken on agent authority. Ruling wanted: (a) live with the documented one-sided drift (current), or (b) addgranule_workerstoDATA_SOURCE_PACKAGING_KEYSin a dedicated PR with a migration note.A second consequence weighs on the same fork:
semantic_hashis intelemetry._EQ_OR_NONE_KEYS, so a cross-leaf rollup collapses it toNonewhenever a run mixes clamped and unclamped shards. That is not a regression (the key already collapses on any genuine divergence), but it means the clamp can erase the run's identity half from every rollup record, which (b) would also fix.How far should the recorded identity chase the leaf's ARTIFACT SET? The phase-2 fold closed the concrete regression — a config that declares
output.pyramid.overviewswill not skip a leaf that has no committed column beside it, and vice versa (hive.leaf_column_expectation, verified by reading the artifact per the spec's §4.6 "read the columns, not the manifest" posture). But the general hole stands: the wholeoutputblock is outsidesemantics.semantic_coreandpyramidis deliberately not a frozen manifest key, so any output knob that changes what a leaf contains moves neither identity half.output.shardedis the checked example — flipping it over an existing store changes the leaf's object layout while the hash and the granule set both hold, so the gate readsequal. Options: (a) leave it — the column was the only knob that adds/removes an artifact, and ashardedflip over an existing store is already an operator-forced rewrite (--overwrite); (b) record a smalloutput-layout digest in the D20 sidecar (not D19 — moving D19 breaks manifest compat) and add it as a third identity half; (c) widensemantic_coreto include the leaf-shapingoutputkeys, which is the same migration cost as (7)(b). This is deliberately not taken on agent authority: (b) and (c) both change what a sidecar means.Should an all-skipped / all-refused run still write a run-record parquet? Today it does not, and the CLI half of that gap is now closed (the summary prints
cells_current/cells_refused/cells_unrecordedand names--allow-contractionwhen anything refused —f3ba09b). The durable half is left standing because it is a D20 schema decision, not an observability tweak: a skipped or refused unit writes nothing and produces nometa["stats"], sorowsis empty and both_write_run_statsand_dispatch_run_statsreturn before the PUT. Synthesizing rows would mean emitting D20 records for units that wrote no leaf — records whosen_obs,content_hashes, andcells_with_datadescribe nothing — which changes what a run-record row is, and it interacts with (6): a refusal row's useful payload is exactly thegranule_idsdiff that question is trying to keep off the transport. Options: (a) leave it (the counters are in the summary and in the worker logs, and the leaf sidecars are untouched by construction); (b) add a distinct refusal row shape to the run parquet (identity,missing_granulescount, no content fields), which is aSCHEMA_VERSIONconversation; (c) write a separate small refusal manifest at the store root whencells_refused > 0.Where should the FLEET's store-root lifecycle touch live? The phase-3 review found that an all-skip run never refreshes the store-root objects:
ensure_manifestaccepts a frozen-key-matching manifest with no second PUT (andaggregation.yamlis written only inside that PUT branch), and every skip-capable run isoverwrite=Falseby construction (skip_if_current=not overwrite). So{store}/morton_hive.json— REQUIRED reader-facing schema (D6) — keeps its originalLastModifiedacross arbitrarily many "keep this store alive" reruns and expires first under a bucket rule, bricking a store whose data objects are all pristine.Fixed for the local backend (
c71198f):lifecycle.touch_store_roottouches the manifest,aggregation.yamland the rootcoverage.moconce per run, gated oncells_current > 0, riding the same post-units wrap-up seam that already GET-union-PUTs the rootcoverage.moc— the seam whose own comment records why it is not a D8 violation ("the LOCAL dispatcher runs it in-process — this process is also the worker and already PUTs every leaf with these credentials").Not fixed for the fleet, deliberately. D8 is a standing ruling (the dispatcher never writes; all store writes are worker-side), and a lifecycle self-copy IS a write — so the Lambda dispatchers cannot make these three requests themselves, and no existing worker-side seam runs exactly once per fleet run. Options, all needing a ruling: (a) ride the
mode="sweep"worker invoke (the §7 sweep is already the D8-sanctioned "orchestrator needs a store write → send a worker" path) and have it touch the root when the run reportscells_current > 0; (b) a new one-shotmode="touch"worker invoke at finalize; (c) leave the fleet gap open and document it (the operator's bucket rule would need to exclude the root prefix). Until one lands, an all-skip fleet run refreshes every leaf and no root object.Same shape, second half: the §7 sweep's ancestor-node overviews and
pyramid.jsonenvelopes belong to no unit footprint and are not root objects either, and a skip produces zero sweep dirtiness by construction — so a skip-only store ages out its whole pyramid above the leaves while the leaves and the root survive. Touching them means either a store-wide walk (which this unit-scoped module deliberately does not own) or teaching the sweep to touch-not-rebuild the clean ancestors of a current leaf. Folded as a documented gap in thezagg.lifecyclemodule docstring rather than an invented walker.Raised by the phase-3 self-review at this comment.
Generated by Claude Code