sweep handler: forward the partition and families blocks (issue #527)

[espg]

Closes #527. Blocks the 0.51.0 release alongside #523 — both remaining production runs of the CA campaign depend on this.

What this changes

deployment/aws/lambda_handler.py::_handle_sweep forwards event["partition"] and event["families"] to zagg.sweep.run_sweep, which has accepted both since issue #377/#520:

summary = run_sweep(
    event["store_path"], leaves, store_kwargs=store_kwargs,
    families=event.get("families"), partition=event.get("partition"),
)

Absent keys forward as None, so an unpartitioned invoke is behaviourally identical to the pre-fix call (the call shape differs — the kwargs are now always passed, at their defaults): this adds reach, never new behaviour. The docstring now states the contract and why it matters.

The handler also validates the partition block with normalize_partition before discover_leaves runs, so a malformed block fails before paying a store-root LIST plus a parquet read per run record — the guard python -m zagg.sweep already carries for argv (src/zagg/sweep.py:1331-1335, a prior #377 review finding).

Why it was invisible

Every partition test in tests/test_sweep_partition.py drives run_sweep directly, where the contract holds. The transport was the only untested link, so a dropped block looked exactly like a correct system from the test suite's side.

What it cost, live

• The CA ATL03 toc/overview sweep (2026-08-25), dead. On the discover transport, each of 16 "partition" workers derived the whole store's work set and swept all 2,726 leaves; at the SERC probe's measured ~60 s/leaf that is ~45 h of work in a 900 s Lambda. All 16 hit the wall, zero records written. The 4-shard SERC probe passed the same code path only because a full sweep of 4 leaves fits.
• The disjointness invariant, latent. The runner's _invoke_lambda_sweep splits the work set client-side and sends each bucket inline — but with the block dropped, each worker still walked to the ROOT with only its subset and ran finish(), so coarse nodes above the split order were written from partial data by racing workers. Unreached in production only because the runner tail defaults to partitions=1.
• families was unreachable worker-side, so the columns backfill family (column backfill for pre-column stores: the /1 -> /2 upgrade bridge (columnize pass + /2 retrofit declaration) #520 / PR column backfill for pre-column stores: the /1 -> /2 upgrade bridge (#520) #524) could never run on the fleet — the observation that surfaced this bug, from that PR's standing question 3. This PR makes the receive half runnable; the send half is still missing (see "What this does NOT fix" below), so column backfill for pre-column stores: the /1 -> /2 upgrade bridge (columnize pass + /2 retrofit declaration) #520 should not be closed on this PR.

Operator notes — read before the next CA run

This PR is the difference between "the bug is fixed" and "the run succeeds". Two things stand between the two, neither of them in the diff. Both are now stated in the _handle_sweep docstring as well as here.

1. Pick a width that fits the 900 s wall

A partition does 1/N-th of the work but faces the same timeout, so a width that is merely narrower than the whole store still dies — N ways instead of one, each worker having done 1/N-th of the fold. The width must satisfy leaves × s_per_leaf / N < 900 and be a power of four (partition_split_order splits on whole morton digits).

For the CA ATL03 store:

work	2,726 leaves × ~60 s/leaf = 163,560 s
minimum width	ceil(163560 / 900) = 182
first legal power of four	256 (4⁴) → ~639 s/worker — fits, no headroom
the width to actually use	1024 (4⁵) → ~160 s/worker
the 16 that died	~10,222 s/worker — 11× over the wall

Both 256 and 1024 are legal here: run_sweep refuses only split > shard_order (src/zagg/sweep.py:761), and k=4 / k=5 are well inside an o9 store.

2. A partitioned pass writes nothing above the split order — the root singletons are still owed

For a partitioned pass (min_order = k > 0):

• orders below the split are never walked — range(shard_order - 1, min_order - 1, -1) at src/zagg/sweep.py:892;
• MocFamily.finish (src/zagg/sweep.py:275-300) — the only writer of the store-root coverage.moc and its sibling coverage.toc — is skipped and reported as finish_deferred (src/zagg/sweep.py:902-910);
• zagg.sweep_overview likewise defers the manifest pyramid.materialized RMW (src/zagg/sweep_overview.py:1285-1298).

That deferral is exactly what keeps concurrent partitions disjoint — but nothing on this transport picks it back up. The coarse levels, coverage.moc, coverage.toc, and the manifest update are owed by a subsequent partition-less pass over the same work set. That pass is cheap: every rollup the partitions already wrote is skip-if-current, so it only pays the orders above the split.

There is no finisher arm in the handler's mode table (deployment/aws/lambda_handler.py:649-673 — setup / finalize / ping / coverage / sweep / stats / extract / process_raster). zagg.sweep_stages.run_finisher is called from exactly one place, run_stage_sweep (src/zagg/sweep_stages.py:534, the --stages CLI), and python -m zagg.sweep has no finisher-only flag. So on the fleet the partition-less follow-up invoke IS the finisher. A partitioned "toc sweep" that stops after the fan-out produces no toc.

What this does NOT fix — filed as issues

• nothing in zagg can send a sweep event's partition or families block (send half of #377/#520) #530 — nothing in zagg can send either block. _build_sweep_event has no families parameter at all, and all three _invoke_lambda_sweep call sites pass partitions=1; only a hand-rolled boto3 payload (or the private runner._invoke_lambda_sweep(..., partitions=N)) reaches the new capability. Carries this PR's former standing question about a size-derived partitions default — a behaviour change to every run's tail, so it wants its own decision.
• sweep_lease's "sweep || sweep is serialized per store" law is unenforced on the fleet transport #529 — the sweep lease's stated law is unenforced on this transport. sweep_lease.py:29-32 says sweep‖sweep is "serialized per store — by this module", but run_sweep never takes the lease; only run_stage_sweep does. The good news is the direct answer to why partitioning works: concurrent partitions contend on nothing, so the fan-out delivers genuine parallelism. The flip side is that before this PR partitioned fleet sweeps merely timed out, and now they will write — so the unenforced law starts to matter.
• sweep families: [] is a silent 200 that sweeps nothing #531 — families: [] is a silent 200 that sweeps nothing. src/zagg/sweep.py:766 treats [] as an explicit "no families"; newly reachable now that the block is forwarded. Refuse-or-document call.

Tests

TestHandlerForwardsThePartitionContract in tests/test_sweep_partition.py — three pins, each mutation-verified (reverting the one-line fix fails all three):

1. partition and families reach run_sweep verbatim through the handler;
2. absent blocks forward as None (the pre-sweep handler drops the partition and families blocks: every partitioned fleet sweep silently sweeps the whole store (0.51.0 blocker) #527 call shape);
3. end to end through the transport, partition 0 of 4 returns statusCode 200 and writes exactly its own rollup subtree (-3/1/1, -3/1/2, -3/1) and no rollup above the split order.

Scoping note on (3): the spy is on sweep._put_rollup, so the claim is about rollup keys only. The pass does also PUT one object at the store root — the run record sweep_stats_{ts}_p0of4.json (src/zagg/sweep.py:801-802, 836-838, record=True by default) — which is telemetry, not a tree node, and is disambiguated per partition so 1024 partitions write 1024 distinct objects with no clobber. And families=["stats"] never selects MocFamily.finish in the first place, so this test does not exercise the finish() deferral either way; that deferral is pinned elsewhere against run_sweep directly.

How it was tested

• pytest tests/test_sweep_partition.py tests/test_sweep.py — 147 passed.
• Mutation check re-run after the fold: fix reverted ⇒ 3 failed; restored ⇒ 3 passed.
• ruff check / ruff format --check clean on both touched files. Pre-existing and untouched per §4: N818 on zagg.registry.UnknownCapability, and a ruff format diff in tests/data/benchmark/README.md (both on clean main; the PR lint bot's --select=E,F,W,I sees neither).

Changelog

CHANGELOG.md gains an entry under [Unreleased] — the same 0.51.0 heading sibling blocker #523 used.

Merge order

This branch's merge-base was 419a17b7, which predated #523's merge, so the branch did not carry #523's [Unreleased] entry and GitHub reported the PR CONFLICTING. Resolved by merging main forward (51859bd3, a regular merge — no rebase, no force-push, §1). The sole conflict was the CHANGELOG.md insertion point, where both entries are siblings under the same [Unreleased] heading; both are kept, #528's above #523's (newest first, matching how #523 itself inserted). Everything else auto-merged. The PR is MERGEABLE again, and gates + the mutation check were re-run on the merged tree.

Scope note

§1: issue #527 names deployment/aws/lambda_handler.py explicitly, which is the authorization; nothing else under deployment/aws/ is touched. PR #526 (issue #519, stage transport) adds a new _handle_stage arm to the same file and does not touch _handle_sweep — whichever lands second merges forward trivially.

Questions for review

• The response body deliberately does not echo the partition block (the store-root sweep record carries it). Say the word if a fleet driver should be able to read it off the invoke.
• sweep families: [] is a silent 200 that sweeps nothing #531 (families: []) is a refuse-or-document call I did not want to make unilaterally inside a release-blocker fix.

[espg]

🤖 from Claude (review)

Adversarial review of #528 — fresh context, worktree at 4fd0c29, no AWS touched.

Verdict: the one-line fix is correct and I found no defect in it. Merge it. But do not treat it as "the CA sweep now works" — two things stand between this PR and a successful Monday run, and neither is in the diff.

What I verified holds

• The forward itself is right. families/partition reach run_sweep verbatim; absent keys are None; nothing else changes.
• Worker-side filtering composes with discover. discover_leaves derives the whole store's set, then select_partition (sweep.py:770) filters it. That is the designed order and it is correct.
• No double-filter / off-by-one on inline partitioned leaves. select_partition is a pure prefix predicate, so re-filtering an already-bucketed set is the identity with foreign_leaves == 0. The walk range(shard_order-1, min_order-1, -1) (sweep.py:892) writes order k inclusive, and an order-k node has exactly k digits so partition_index maps it to exactly one owner — the boundary is right.
• The manifest RMW fence is real. sweep_overview.py:1285 defers the store-root pyramid.materialized RMW under min_order, so 2^n workers do not race it. Good.
• record=True is not a stampede. Keys are disambiguated _p{index}of{of} (sweep.py:836-838), so 1024 partitions write 1024 distinct root objects with no clobber, and discover_leaves' stats_.+\.parquet fullmatch (sweep.py:1185) cannot mistake them for run records. Cost is root-listing growth only. (The handler cannot set record=False, so the 1024 objects are mandatory — noting, not objecting.)
• The mutation claim in the PR body is TRUE. I reverted the call to run_sweep(event["store_path"], leaves, store_kwargs=store_kwargs) in the worktree and ran the class: 3 failed, and test 3 failed loudly with 12 rollup keys including -3/2/1/... — the whole-tree write the issue describes. Restored: 3 passed. 147 passed on tests/test_sweep_partition.py tests/test_sweep.py; ruff check and ruff format --check clean on both touched files.
• The partition-0 expected key set is correct (worked out independently — see the inline comment on the test).

The lease question, answered

The lease does not serialize this path, because this path never takes it. run_sweep touches nothing in zagg.sweep_lease; the sole caller of acquire_lease in the tree is sweep_stages.run_stage_sweep. So N concurrent mode="sweep" partition invokes contend on nothing and the fan-out delivers genuine parallelism. Full reasoning, and the corollary risk (the lease's stated "sweep || sweep is serialized per store" law is simply unenforced on this transport), in the inline comment on the run_sweep( call.

Findings, ranked

#	Sev	Finding
S1	high	16 partitions still does not fit 900 s — by the issue's own numbers each worker needs ~2.84 h. First fitting width is 256 (4^4); 1024 has headroom.
S2	high	A partitioned invoke defers MocFamily.finish — the only writer of the root coverage.moc/coverage.toc — and there is no finisher on the fleet (run_finisher is --stages-only; the handler has no finisher mode). A "toc sweep" that partitions produces no toc.
S3	medium	Neither block can be sent by any zagg dispatcher: _build_sweep_event has no families param, and all three _invoke_lambda_sweep call sites use partitions=1. The #520 unblock is half-delivered.
S4	medium (pre-existing)	No lease on this path -> good for parallelism, but the documented sweep-vs-sweep serialization law is unenforced for the transport this PR switches on.
S5	low-med	families: [] is a silent 200 that sweeps nothing (verified). Newly reachable.
S6	low	Validation is sufficient (normalize_partition at sweep.py:755, verified against three malformed shapes) but lands after discover_leaves' bill — the exact guard a prior #377 review folded into sweep.py:1331.
S7	low	Test 3 never asserts statusCode == 200; the handler's blanket except means it can go green on a failed invoke.
S8	low	Two PR-body claims overstated: "byte-identical to the pre-fix call" (behaviourally identical, call shape differs), and "writes ... nothing above the split order" (_written is blind to the store-root sweep_stats_*.json this pass does write, and to finish(), which families=["stats"] never selects).
S9	low	No CHANGELOG.md entry. The sibling 0.51.0 blocker #523 added one under [Unreleased]; this one adds none. The branch also predates #523's merge (merge-base 419a17b7) — rebase before ready.

None of S1-S9 is a defect in the diff. S1 and S2 are the ones I would not let ship silently: they are the difference between "the bug is fixed" and "the run succeeds", and this PR is the only place either will be read before Monday. My recommendation is to land the code as-is and add (a) the fitting-width arithmetic and (b) the "the coarse levels and root singletons are still owed" sentence to the PR body and the _handle_sweep docstring.

[espg]

🤖 from Claude (review)

S1 (high, operational) — the fix is correct, but 16 partitions still does not fit the 900 s wall. This paragraph names the CA death as the thing the forward repairs, which is true of the cause but not of the outcome, and I think that gap will bite on Monday.

Taking the issue's own measurements: 2,726 leaves x ~60 s/leaf = 163,560 s (the ~45 h stated). Split 16 ways that is 10,222 s per worker = 2.84 h — still 11x the 900 s wall. Every one of the 16 invokes dies exactly as before; the only difference is that each now dies having done 1/16th of the work instead of duplicating all of it.

Fitting 900 s needs ceil(163560/900) = 182 partitions, and partitions must be a power of four (partition_split_order), so the first width that fits is 256 = 4^4 (~639 s/worker, before discover_s), with 1024 = 4^5 (~160 s/worker) the width with actual headroom. Both are legal here: run_sweep refuses only split > shard_order (src/zagg/sweep.py:761), and k=4/k=5 are well inside an o9 store.

Not a code change — but this PR is the release note for "partitioned fleet sweeps now work", and shipping it without stating the width that actually fits invites a second dead run with the same 16. Worth a line in the PR body (and ideally in this docstring) saying the width is ceil(leaves x s_per_leaf / wall) rounded up to a power of four.

[espg]

🤖 from Claude

Folded — 5140d1ac. The arithmetic is now in the _handle_sweep docstring and in the PR body (as a table, under "Operator notes — read before the next CA run").

Docstring paragraph as landed:

Pick a width that fits the wall. A partition does 1/N-th of the work but faces the same 900 s timeout, so a width that is merely narrower than the whole store still dies — N ways instead of one, each having done 1/N-th of the fold. The width must satisfy leaves x s_per_leaf / N < 900 AND be a power of four (partition_split_order splits on whole morton digits). For the CA ATL03 store that is 2,726 leaves x ~60 s/leaf (the SERC probe's measured rate) = 163,560 s of fold work, so ceil(163560 / 900) = 182 partitions minimum, rounded up to the first legal power of four: 256 (4^4, ~639 s/worker — fits, no headroom) or 1024 (4^5, ~160 s/worker — the width to actually use). The 16 that died was ~10,222 s/worker, 11x over the wall; forwarding the block does not change that, only the width does.

The "still dies at the wall, just 1/N-th of the way in" framing is stated explicitly in both places, since that is the part a reader skimming for "is 16 fine now?" would otherwise get wrong.

[espg]

🤖 from Claude (review)

S2 (high) — a partitioned mode="sweep" invoke writes no toc, and there is no finisher on the fleet to write one. This paragraph correctly says the deferral is what makes partitions disjoint. What it does not say is that on this transport nothing ever picks the deferred work up, and for the CA run the deferred work is the deliverable.

Concretely, for a partitioned pass (min_order = k > 0):

• zagg.sweep._sweep_family (src/zagg/sweep.py:902-910) takes the if min_order: branch and sets finish_deferred instead of calling fam.finish(...). MocFamily.finish (src/zagg/sweep.py:275-300) is the only writer of the store-root coverage.moc and the sibling coverage.toc on this path.
• sweep_overviews (src/zagg/sweep_overview.py:1285-1298) likewise sets manifest_deferred and skips the pyramid.materialized RMW.
• Orders < k are never walked (src/zagg/sweep.py:892, range(shard_order-1, min_order-1, -1)).

And the finisher: run_finisher is called from exactly one place, zagg.sweep_stages.run_stage_sweep (src/zagg/sweep_stages.py:534) — the --stages / zagg-pyramid/2 path. The handler's mode table (deployment/aws/lambda_handler.py:649-673) has setup / finalize / ping / coverage / sweep / stats / extract / process_raster and no finisher arm. python -m zagg.sweep has no finisher-only flag either.

So the outcome of a successful 256-way CA "toc/overview" sweep is: rollups + overview slabs at orders >= 4, no root coverage.moc, no coverage.toc, no manifest materialized update. docs/hive_layout.md:487-513 already documents the follow-up recipe for the JSON families and then says of the overview family "Prefer the finisher" — for a finisher that does not exist.

Ask: no code change needed in this PR, but please say it out loud — one sentence in this docstring ("the coarse levels and the root singletons remain owed to a separate unpartitioned pass; issue #377's finisher leg is unimplemented") and a line in the PR body naming the follow-up the operator still owes. Silently shipping "partitions work now" when the toc is the goal is the same class of surprise #527 was.

[espg]

🤖 from Claude

Folded — 2a01359b. Both the docstring and the PR body now say it out loud, and both name the finisher gap.

Docstring paragraph as landed:

A partitioned pass writes NOTHING above the split order, and the root singletons are still owed. Orders below the split are never walked (range(shard_order - 1, min_order - 1, -1) in :func:zagg.sweep._sweep_family); MocFamily.finish — the only writer of the store-root coverage.moc and its sibling coverage.toc — is skipped and reported as finish_deferred; and :mod:zagg.sweep_overview likewise defers the manifest pyramid.materialized RMW. That deferral is exactly what keeps concurrent partitions disjoint, but nothing on this transport picks it back up: a subsequent partition-less pass over the same work set is what writes the coarse levels, coverage.moc, coverage.toc, and the manifest update. It is cheap — every rollup the partitions already wrote is skip-if-current, so the follow-up only pays the orders above the split. Note there is no finisher arm in the mode table above: :func:zagg.sweep_stages.run_finisher is called from exactly one place, run_stage_sweep (the --stages CLI), so on the fleet the partition-less follow-up invoke IS the finisher. A partitioned "toc sweep" that stops after the fan-out produces no toc.

One deliberate difference from your suggested wording: rather than describe the finisher leg as unimplemented and leave the operator to infer a remedy, it names the remedy that exists today — a partition-less mode="sweep" invoke over the same work set — and says why it is cheap (skip-if-current below the split). That makes the PR actionable for Monday without needing #377's finisher leg to land first.

[espg]

🤖 from Claude (review)

S4 (medium, pre-existing) — answering the parallelism question directly: no, the lease does not serialize this path, because this path never takes the lease.

I traced it end to end. run_sweep does not import or call anything from zagg.sweep_lease; the only caller of acquire_lease/heartbeat_lease/release_lease in the whole tree is zagg.sweep_stages.run_stage_sweep (src/zagg/sweep_stages.py:442-479). So N concurrent mode="sweep" partition invokes contend on nothing — the fan-out delivers real parallelism, which is the good news and the answer to the question this PR exists to settle.

(Note the contrast: on the --stages path partitions= is swept serially in one process under one lease (src/zagg/sweep_stages.py:520, and its docstring "each of the 2^n partitions swept in turn UNDER THE SAME LEASE"), so there partitioning buys bounded memory, not speed. Two different meanings of the same flag — worth keeping straight when reading the docs, which describe the CLI meaning only.)

The flip side, and the reason I am flagging it rather than just answering: zagg/sweep_lease.py:29-32 states the law as "sweep || sweep is serialized per store — by this module". That law is not enforced for the transport this PR just switched on. A fleet partitioned sweep and a python -m zagg.sweep --stages (or a second fleet fan-out, or the runner tail's own end-of-run sweep) can run concurrently on one store and neither sees the other; the chimera-column hazard the lease was built for (sweep_lease.py:5-9) is reachable that way. Before this PR the exposure was theoretical because partitioned fleet sweeps just timed out; now they will write.

Pre-existing and out of scope for a one-line forward — but it should be an issue, and if #528 lands in 0.51.0 it should probably be filed alongside.

[espg]

🤖 from Claude

Filed as #529 rather than folded — it is pre-existing and the remedy is a design fork, not a patch.

The issue leads with your good news, since it is the direct answer to the question this PR exists to settle: run_sweep touches nothing in zagg.sweep_lease, the sole caller of acquire_lease is sweep_stages.run_stage_sweep (src/zagg/sweep_stages.py:442-479), so N concurrent partition invokes contend on nothing and the fan-out delivers genuine parallelism. Your --stages contrast (partitions swept serially under one lease, sweep_stages.py:520 — bounded memory, not speed) is captured too, since the docs only describe the CLI meaning.

The gap is then stated as you framed it: sweep_lease.py:29-32 claims serialization "by this module" for a transport that never takes the lease, and before this PR partitioned fleet sweeps merely timed out, whereas now they will write. Three options laid out for @ the maintainer — take a partition-aware lease in run_sweep, weaken the documented law to match reality, or advisory-only detection — with no recommendation, because taking the lease naively would serialize the 256-way fan-out and destroy exactly the parallelism above.

The PR body links #529 under "What this does NOT fix".

[espg]

🤖 from Claude (review)

S3 (medium) — nothing in zagg can actually send either block, so the send side of the #520 unblock is still missing.

• families: _build_sweep_event (src/zagg/runner.py:5475-5507) has no families parameter at all and never sets the key. Grepping the tree, "families" appears as an event key in exactly two places — this line and run_sweep's own summary (src/zagg/sweep.py:777). So no dispatcher, CLI, or config path can produce a mode="sweep" event carrying families.
• partition: _build_sweep_event does accept and validate it, but all three _invoke_lambda_sweep call sites take the partitions=1 default (src/zagg/runner.py:1638, src/zagg/runner.py:4274, src/zagg/client.py:1244), and partitions is not exposed on any public entry point.

Net: after this PR the only way to reach either feature on the fleet is a hand-rolled boto3 Payload, or calling the private runner._invoke_lambda_sweep(..., partitions=N) from a script. That is fine as an operator recipe, but the PR body's framing ("families was unreachable worker-side, so the columns backfill family could never run on the fleet") reads as though this PR makes it runnable. It makes the receive half runnable. Please say which half, so #520 does not get closed on it.

(Related: the PR's own "should _invoke_lambda_sweep's default change?" question is really this same gap — the missing piece is not just the default, it is that partitions/families have no caller.)

[espg]

🤖 from Claude

Filed as #530 — the send half is a real body of work (a families parameter on _build_sweep_event, a public surface for partitions, and the default question), not something to bolt onto a release-blocker one-liner.

The framing you asked for is fixed in the PR body. The families bullet now reads: "This PR makes the receive half runnable; the send half is still missing (see 'What this does NOT fix' below), so #520 should not be closed on this PR." Your three call sites (runner.py:1638, runner.py:4274, client.py:1244) and the "families appears as an event key in exactly two places" finding are both recorded in #530.

The PR's former standing question about _invoke_lambda_sweep's default moved into #530 as decision (3), with your note that a size-derived default for fleet-scale stores is a behaviour change to every run's tail and wants its own decision — plus the observation that deriving it automatically needs a per-leaf cost estimate the runner does not have today.

[espg]

🤖 from Claude (review)

S5 (low-medium) — families: [] is now a silent success that sweeps nothing. I ran it against the head commit:

event = {"mode": "sweep", "store_path": ..., "leaves": [...6 leaves...], "families": []}
-> statusCode 200, body families {} , n_leaves 6, zero _put_rollup calls

run_sweep does DEFAULT_FAMILIES if families is None else families (src/zagg/sweep.py:766), so an empty list is an explicit "no families" and the loop body never runs. The response is indistinguishable from a successful sweep except that families is {} — and on an Event invoke nobody reads the response at all.

Before this PR [] was dropped on the floor and the defaults swept, so this is newly reachable. It is the same silent-wrong-answer shape select_partition's docstring calls out for an out-of-range index ("would otherwise filter EVERYTHING out and read as a clean 'nothing to do'"), and that one is guarded. Suggest either refusing an empty families in run_sweep (loud, consistent with the partition guard) or normalizing [] -> None here. Low severity because only a hand-built payload can produce it today (see S3) — which is also exactly who will be writing these payloads on Monday.

[espg]

🤖 from Claude

Filed as #531 — a refuse-or-document call I did not want to make unilaterally inside a release blocker, so it is a question for @ the maintainer rather than a patch here.

The issue carries your reproduction verbatim (200 / families {} / n_leaves 6 / zero _put_rollup calls), the sweep.py:766 mechanism, the "newly reachable" point, and your select_partition-docstring precedent — the out-of-range index is guarded against exactly this shape and the empty family list is not. Three options: refuse in run_sweep (my read of the house style, consistent with the partition guard directly above), normalize [] -> None, or document it as an intentional no-op probe.

Your closing point is in there too, since it is the reason it is not lower than low-medium: only a hand-built payload can produce an empty list at all today (#530) — which is also exactly who will be writing these payloads for the CA runs. #531 is linked from the PR body and from its "Questions for review".

[espg]

🤖 from Claude (review)

S6 (low) — validation is sufficient, but it lands after the discovery bill on the discover transport.

Sufficiency first, since it was asked: normalize_partition runs as run_sweep's first real statement (src/zagg/sweep.py:755), before read_manifest, and select_partition re-validates worker-side (src/zagg/sweep_partition.py). I fired the three malformed shapes through _handle_sweep on this branch:

{"index": 5, "of": 4}   -> 500 "sweep partition index 5 is out of range for of=4"
{"index": 1, "of": 8}   -> 500 "sweep partitions=8 (2^3) splits a morton digit in half ... use 4 or 16"
{"index": 0, "of": 256} -> 500 "... splits at order 4, finer than the store's shard_order 2"

All named, all caught. No handler-side validation is needed for correctness — normalize_partition is sufficient and it runs in the right place.

The nit: on the discover branch, discover_leaves at line 1176 runs before this call, so a malformed block pays a full store-root LIST plus a parquet read per run record before failing. That is precisely the cost an earlier #377 review finding removed from the CLI — see the comment at src/zagg/sweep.py:1331-1335: "Validate the fan-out width from argv alone, BEFORE anything touches the store: discover_leaves is a LIST plus a parquet read per run record, and a mistyped width should not cost that (review finding, issue #377)." The handler is now the second entry point that accepts the block and it does not carry that guard. A normalize_partition(event.get("partition")) right after store_kwargs would restore parity in one line.

(Also worth knowing, though not this PR's doing: the failure is a return {"statusCode": 500, ...}, which on an InvocationType="Event" invoke is a successful Lambda execution — no retry, no DLQ, CloudWatch only. A 256-way fan-out that is entirely malformed looks, from the fleet, like 256 clean runs.)

[espg]

🤖 from Claude

Folded — 0543ff13. One line, exactly where you pointed, plus a comment naming the CLI parity:

    from zagg.sweep_partition import normalize_partition
    ...
        store_kwargs = _output_store_kwargs(event)
        # Validate the block BEFORE anything touches the store: on the
        # ``discover`` transport the next statement is a store-root LIST plus a
        # parquet read per run record, and a malformed partition should not
        # cost that. run_sweep re-validates (it is the authority); this is the
        # same guard ``python -m zagg.sweep`` carries for argv, issue #377.
        normalize_partition(event.get("partition"))
        t0 = time.perf_counter()

Placed before t0 so the discover_s timing still measures discovery alone. run_sweep remains the authority — this is a cheap early echo of it, not a second source of truth, which the comment says so nobody later "optimizes" it by removing the run_sweep call's validation instead.

Your parenthetical about statusCode: 500 being a successful Lambda execution on an Event invoke (no retry, no DLQ, CloudWatch only — a wholly malformed 256-way fan-out looks like 256 clean runs from the fleet) is not folded: it is a property of every handler arm in the file, not of this diff, and changing it would mean re-raising rather than returning 500, which reverses the deliberate fail-open contract stated in _handle_coverage's and _handle_sweep's docstrings. Left standing for @ the maintainer as a whole-handler question.

[espg]

🤖 from Claude (review)

S8a (low) — the "byte-identical to the pre-fix call" claim, which this test pins, is true of the behaviour and false of the call.

The pre-#527 handler called run_sweep(store_path, leaves, store_kwargs=...); the new one always passes families=None, partition=None. Identical in effect (both are the defaults), but the call shape differs, and this assertion pins the kwargs' presence — it fails on the old code with a KeyError, not with a wrong value. So it is an implementation pin dressed as a behaviour pin: it forbids ever refactoring back to conditional kwargs even though that would be behaviourally identical.

Not worth changing the test over (the pin is cheap and the mutation signal is real), but the PR body should say "behaviourally identical", not "byte-identical to the pre-fix call".

Minor style: assert a is None and b is None collapses two facts into one failure message — two asserts read better when one of them breaks.

[espg]

🤖 from Claude

Both halves folded.

The PR-body claim now reads "an unpartitioned invoke is behaviourally identical to the pre-fix call (the call shape differs — the kwargs are now always passed, at their defaults)". "byte-identical" is gone.

The style nit — d260e3c2. Split into two asserts, so a break names which block leaked:

        assert seen["partition"] is None
        assert seen["families"] is None

The pin itself is kept as-is, deliberately. You are right that it is an implementation pin — it fails the old code with a KeyError, not a wrong value — but that is the property that makes it a mutation detector for this fix, and the refactor it forbids (back to conditional kwargs) is one nobody has asked for. If someone does, the test is one line to relax and the comment above it already says what it is pinning and why.

[espg]

🤖 from Claude (review)

S8b (low) — scope the "nothing above the split order" claim to rollups; _written cannot see the rest.

_written spies sweep_mod._put_rollup only (tests/test_sweep_partition.py:140-153). Two things a partitioned pass writes are invisible to it:

1. The store-root sweep record. run_sweep defaults record=True and PUTs sweep_stats_{ts}_p0of4.json at the store root (src/zagg/sweep.py:801-802, 836-838). So this invoke does write an object above the split order — telemetry, not a tree node, and harmless, but the PR body's "writes exactly its own subtree ... and nothing above the split order" is literally false as stated.
2. finish() deferral is not actually exercised here. families=["stats"] means MocFamily.finish — the root coverage.moc/coverage.toc writer, and the thing S2 is about — never runs in this test either way, so the test cannot distinguish "deferred" from "not selected". The deferral is pinned elsewhere against run_sweep directly, which is fine; just do not let the PR body claim this test proves it.

Suggest rewording the PR bullet to "writes exactly its own rollup subtree", or extending the spy to obstore.put (as _overview_written already does) if you want the stronger claim.

Separately, the partition-0 key set itself is correct — I checked it independently. LEAVES maps to first-digit ranks -311->0, -312->0, -321->1, -341->3, 141->3, 142->3, so partition 0 of 4 owns {-311, -312}; k=1, shard_order=2, and the walk range(1, 0, -1) writes order 2 then order 1 and stops. -3/1/1, -3/1/2, -3/1 is exactly right, and -3 (order 0) is correctly absent.

[espg]

🤖 from Claude

Folded into the PR body, no code change — both of your points, plus the correction you did not have to ask for twice.

The tests section bullet 3 now says partition 0 of 4 "writes exactly its own rollup subtree ... and no rollup above the split order", and a scoping note follows it:

Scoping note on (3): the spy is on sweep._put_rollup, so the claim is about rollup keys only. The pass does also PUT one object at the store root — the run record sweep_stats_{ts}_p0of4.json (src/zagg/sweep.py:801-802, 836-838, record=True by default) — which is telemetry, not a tree node, and is disambiguated per partition so 1024 partitions write 1024 distinct objects with no clobber. And families=["stats"] never selects MocFamily.finish in the first place, so this test does not exercise the finish() deferral either way; that deferral is pinned elsewhere against run_sweep directly.

I did not extend the spy to obstore.put. Doing so would make the test assert over the run record too, which is timestamped and partition-suffixed — a brittler assertion for a claim the narrower spy already makes correctly once it is scoped honestly. The stronger claim you would get (that finish() really is deferred) is better pinned where it already is, against run_sweep with a family that actually selects MocFamily.

Thanks for the independent check of the partition-0 key set — the first-digit ranks and the range(1, 0, -1) walk. That is the assertion the whole test rests on, and it is good to have it verified by someone who derived it separately.

[espg]

🤖 from Claude (review)

S7 (low) — this test can go green on a failed invoke. _handle_sweep wraps everything in a blanket except that returns {"statusCode": 500}, so an exception raised after the three rollups are PUT (the record write, say, or a family added later) leaves keys exactly as asserted and the test passes on a broken invoke. The sibling test above checks response["statusCode"] == 200; this one drops the response entirely.

One line: response = self._handler()._handle_sweep({...}) then assert response["statusCode"] == 200 before the key assertion. While there, json.loads(response["body"])["foreign_leaves"] == 4 would pin that the filter fired (4 of 6 leaves dropped) rather than inferring it from the key set — that field is the one thing distinguishing "filtered correctly" from "was handed the right leaves".

[espg]

🤖 from Claude

Folded — 08e5f829:

        response = self._handler()._handle_sweep({...})
        # _handle_sweep swallows every exception into a 500, so without this
        # the key set alone would go green on an invoke that raised after the
        # last rollup PUT (review finding, issue #527).
        assert response["statusCode"] == 200
        assert sorted(keys) == [...]

The foreign_leaves half is not folded, for a factual reason rather than a scope one: the handler does not echo it. _handle_sweep's response body is {ok, mode, n_leaves, families, duration_s, discover_s, record} — run_sweep's summary carries foreign_leaves but the handler drops it. So json.loads(response["body"])["foreign_leaves"] would KeyError today.

The underlying want is right, though: the key set proves the result was filtered, not that the filter fired. Two ways to get there — assert n_leaves == 2 off the response (which the handler does echo, and which distinguishes "filtered 6 down to 2" from "was handed 2"), or add foreign_leaves to the echoed body. I left both alone rather than widen a release-blocker fix into a response-shape change; the second is really the same question as the PR's standing "should the response echo the partition block?" and belongs with it.

[espg]

🤖 from Claude

Adversarial review folded — all 9 inline threads answered, CI green, ready for review.

Six fold commits, one per finding, plus a merge-forward:

finding	commit	what changed
S1 (high)	5140d1ac	the fitting-width arithmetic in the _handle_sweep docstring (and a table in the PR body): 2,726 × ~60 s = 163,560 s ⇒ ≥182 ⇒ 256 fits, 1024 has headroom; 16 still dies, just 1/16th of the way in
S2 (high)	2a01359b	a partitioned pass writes nothing above the split order and skips finish(), so the root coverage.moc/coverage.toc and the manifest materialized update are owed by a partition-less follow-up pass — which on the fleet IS the finisher, there being no finisher arm in the mode table
S6 (low)	0543ff13	normalize_partition now runs before discover_leaves, restoring the CLI's parity guard
S7 (low)	08e5f829	assert response["statusCode"] == 200 in the subtree test
S8a (low)	d260e3c2	the absent-block assert split into its two facts; PR body says "behaviourally identical", not "byte-identical"
S8b (low)	—	PR body scopes the "nothing above the split order" claim to rollup keys, and names the store-root run record _written cannot see
S9 (low)	4b7e811b	CHANGELOG.md entry under the same [Unreleased] heading #523 used
merge	51859bd3	main merged forward (regular merge) — the branch predated #523 and the PR had gone CONFLICTING on the one CHANGELOG.md hunk; both entries kept, everything else auto-merged

Filed rather than folded (out of scope for a release-blocker one-liner, each linked from the PR body):

• sweep_lease's "sweep || sweep is serialized per store" law is unenforced on the fleet transport #529 — S4, the sweep lease's "serialized per store" law is unenforced on this transport. Leads with the good news it answers: concurrent partitions contend on nothing, so the fan-out delivers real parallelism.
• nothing in zagg can send a sweep event's partition or families block (send half of #377/#520) #530 — S3, nothing in zagg can send either block; this PR fixes only the receive half, so column backfill for pre-column stores: the /1 -> /2 upgrade bridge (columnize pass + /2 retrofit declaration) #520 should not be closed on it. Carries the partitions default question.
• sweep families: [] is a silent 200 that sweeps nothing #531 — S5, families: [] is a silent 200 that sweeps nothing. Refuse-or-document call.

Gates, re-run on the merged tree: ruff check / ruff format --check clean on both touched files; pytest tests/test_sweep_partition.py tests/test_sweep.py → 147 passed; mutation check still holds (fix reverted ⇒ 3 failed, restored ⇒ 3 passed). CI on 51859bd3: ruff, build (x86_64 + arm64), test 3.12 and 3.13 all pass.

Left standing for review rather than folded: the statusCode: 500-on-an-Event-invoke observation (a property of every handler arm, and reversing it would undo the deliberate fail-open contract), and pinning foreign_leaves in the subtree test (the handler does not echo that field — n_leaves or a response-shape change would be the two ways there, and the latter is the same question as this PR's standing one about echoing the partition block).

No AWS was touched.