fix main: pin hierarchical seam in worker-double tests + template pins (issue #175)

[espg]

Closes #175 — restores green test.yml on main (red since the issue #170 merge chain; two more failures from the template edits after it), then per review direction materializes the CloudWatch error-signal split the cold-every-time posture requires. Full forensics in #175.

Verdict first: the #170 group is a test artifact, not a regression

The six test_apriori/test_streaming failures all share one mechanism, and no production behavior changed:

• Those tests fake reads via monkeypatch.setattr("zagg.processing._read_group", fake) + H5Coro = lambda *a, **k: object(). That is the hierarchical backend's seam: HierarchicalIndex.read_group late-binds _processing._read_group specifically so monkeypatched fakes keep working (its own docstring, src/zagg/index/hierarchical.py).
• InlineIndex.read_group (src/zagg/index/inline.py) never calls zagg.processing._read_group — it routes through _planned_read_group / _read_group_full with a compiled read_fn that tries build_chunk_map(h5obj, …) and degrades to h5obj.readDatasets(…). Against the fake object() that raises ('object' object has no attribute 'readDatasets' / 'coordinates'), so the worker's per-group handler counts a read error and the issue gain_bias template reads 0 observations where tdigest reads 200k on the same shard (dem_h segment read) #116 accounting correctly reports "No data after filtering (N group reads raised)".
• The plain vs. "(N group reads raised)" split predates Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 (issue gain_bias template reads 0 observations where tdigest reads 200k on the same shard (dem_h segment read) #116, src/zagg/processing/worker.py) and is byte-identical on both backends for real callers; the only thing Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 changed here is which backend an unpinned test config exercises. Corroboration: test_apriori.py::TestWorkerSeam::test_granule_url_passed_when_enabled kept passing, because its chunk_boundaries source keeps the hierarchical default via the Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 carve-out in index_from_config. Independently reproduced by the adversarial review pass.

Fix: pin index: {backend: hierarchical} in the seam-testing configs — the same approach PR #173 took for the benchmark-config pins. Zero src/ behavior change in this group.

Phases

• Phase 1 — test-double pins (the Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 group, 6 failures). tests/test_apriori.py::TestWorkerSeam::test_flag_off_call_signature_unchanged and tests/test_streaming.py::_config() pin index: {backend: hierarchical} with comments explaining the seam.
• Phase 2 — template pins + doc truth (2 failures). test_self_recycle_env_defaults now pins RecycleMaxInvocations default "1" — the "1" default is espg's explicit choice (commit 260221b): recycle after every invocation = cold-every-time posture. Template description, deployment/aws/lambda_handler.py docstring, and docs/deployment/lambda.md (which said "template 8") updated to match. (Phase 2 also narrowed the role's bucket grant; superseded by phase 3.)
• Phase 3 — role grant per espg's direction (comment): Get/Put/Delete on arn:aws:s3:::sliderule-public-cors/* is deliberate (virtual-index write-back under zagg-index/* and worker-written output zarr stores, e.g. zagg-examples/*). Implemented in template.yaml, brought the admin copy deployment/aws/execution_role.yaml into lockstep (it still had the narrow Virtual index entry point: pluggable chunk-index backends for the read path #160 form), and re-scoped the pinning test → test_execution_role_grants_public_cors_bucket.
• Phase 4 — CloudWatch error-signal split. With RecycleMaxInvocations=1, every async invocation self-exits and lands in Lambda's raw Errors metric — 100% noise. Four AWS::Logs::MetricFilter resources (both functions' log groups) publish namespace zagg/lambda: Process/ExtractSelfRecycleCount (the ZAGG_SELF_RECYCLE line) and Process/ExtractWorkerErrorCount (genuine signatures only: [ERROR], Traceback (most recent call last), Task timed out, Runtime.OutOfMemory, Runtime exited with error). Patterns verified disjoint with the recycle path: the handler logs the recycle at [INFO] and exits 0, which the runtime reports as Runtime exited without providing a reason — matched by neither error term (a real nonzero exit is Runtime exited with error). docs/deployment/lambda.md now says "alarm on WorkerErrorCount, never raw Errors" and warns against async OnFailure destinations under this posture.
• Fold — adversarial-review findings (pass 1): backend-seam streaming test (test_streaming_via_backend_seam_unpinned_config, the index_from_config stub idiom from test_processing.py) guarding that the streaming mechanics don't depend on the config pin — the stub intercepts before backend resolution, so compiled-inline integration coverage remains the follow-up gap noted in main red after #170 merge + template hammer edits (test_apriori/test_streaming under the inline default; template pins) #175; docs line rewrap.
• Fold — adversarial-review findings (pass 2): the finish_granule tolerated-path warning now inlines the reason instead of exc_info=True (a folded traceback would have tripped WorkerErrorCount on a path that never fails the read — test strengthened to pin no-exc_info); the pattern evaluator rejects mixed ?/plain term lists (outside the CloudWatch subset it models).

Design note: MetricFilters reference the log groups by name

The /aws/lambda/<fn> groups are created lazily by Lambda on first invocation. Owning them as AWS::Logs::LogGroup resources would fail stack CREATE/UPDATE with "already exists" on any deployed stack (the adoption landmine). Instead the filters reference the groups by name and are gated on a CreateLogMetricFilters parameter (default "true"): existing deployments (groups already present) update cleanly; a fresh stack creates with "false", invokes once, then updates with "true" (documented in the parameter description and docs/deployment/lambda.md).

How it was tested

• Full pytest -q locally on the branch: green except test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds, a local-environment artifact only (the build script's pip resolves against a local Python 3.10, so zarr>=3.1.5 has no candidate; it passes on CI in both red runs and is untouched here).
• Same suite on 260221b (current main) reproduces exactly the 8 CI failures listed in main red after #170 merge + template hammer edits (test_apriori/test_streaming under the inline default; template pins) #175 plus that same environmental one.
• New tests: template pins for the four filter resources; a pattern evaluator asserting the recycle/error filters are disjoint against realistic log lines (handler ZAGG_SELF_RECYCLE line, clean-exit runtime report, [ERROR]/traceback/timeout/OOM/nonzero-exit signatures); the backend-seam streaming test.
• ruff check clean on all changed files; pre-commit hooks clean except findings that pre-exist on pristine main (see below).

Questions for review

1. Was the IAM broadening intentional? Resolved: deliberate per espg's comment; implemented as directed in phase 3. One standing review-bot note (r3532665667): whole-bucket Delete also covers the Lambda distribution artifacts (versions.json, layer zips) in the same bucket — a two-prefix statement (zagg-index/* + zagg-examples/*) would exclude them, if ever desired.
2. Metric naming chose per-function metric names (ProcessWorkerErrorCount / ExtractWorkerErrorCount) over a shared name + dimension: static dimensions aren't expressible in MetricTransformations (dimension values must come from pattern fields, and these are term filters). Flagging in case a different naming scheme is preferred for dashboards.
3. Pre-existing, deliberately not fixed here (per convention): ruff N818 on src/zagg/registry.py::UnknownCapability (present on pristine main; CI lint selects E,F,W,I only); ruff format --check under local ruff 0.15.20 flags 7 files the pinned pre-commit ruff v0.14.10 accepts (version skew, also on pristine main); pre-commit check-yaml cannot parse CloudFormation !Equals tags (pre-existing); mypy types-PyYAML stub missing in the hook env (pre-existing).

🤖 Generated with Claude Code

[espg]

the Resource: arn:aws:s3:::sliderule-public-cors/zagg-index/* with delete permissions enabled was deliberate, and should be put back. It's done this way so that the lambda can read/write to to sliderule-public-cors : both for write cache index, and for output zarr arrays.

[espg]

🤖 from Claude (review)

Non-blocking (coverage): after this pin, streaming worker-integration coverage runs exclusively on the non-default backend.

The pin itself is correct — verified the mechanism locally: an unpinned _config() resolves to InlineIndex (index_from_config, src/zagg/index/__init__.py), whose read_group never touches zagg.processing._read_group and instead raises against the object() h5obj stub (KeyError: 'coordinates' on this config), which the worker counts as a read error → the "(N group reads raised)" suffix from the issue #116 accounting (src/zagg/processing/worker.py). That reproduces exactly the 5 test_streaming failures in the red main run on 260221b. Test artifact, not regression: confirmed.

Two follow-on observations, neither blocking:

1. Streaming × inline (the package default since issue Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170) is now covered nowhere. tests/test_index.py exercises process_shard under the inline default with a real _FakeH5 stub, but only the pooled path; every aggregation.streaming worker test here is pinned to hierarchical. The streaming seam is backend-agnostic (buffered.add_read(chunk) consumes whatever index_backend.read_group returns), so risk is low — but one cheap hedge would be a single streaming test that fakes at the backend seam instead (monkeypatch zagg.processing.worker.index_from_config to a stub VirtualIndex yielding the granule DataFrames), proving streaming does not depend on the hierarchical delegation specifics.
2. Two pin styles now coexist for the same problem: the issue Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 PR fixed tests/test_processing.py by monkeypatching zagg.processing.worker.index_from_config → HierarchicalIndex(), while this PR pins via the config block. The config pin is arguably stronger (it exercises real resolution + validation), so no change requested — just noting the divergence so a future sweep can align on one idiom.

[espg]

🤖 from Claude

Folded in 82296ea: added test_streaming_via_backend_seam_unpinned_config — config left unpinned (inline default) with the backend stubbed at the worker's index_from_config seam, matching the test_processing.py idiom you cite. Exercising the compiled inline decode itself needs real HDF5 fixtures; noted as a follow-up gap in issue #175.

[espg]

🤖 from Claude (review)

Non-blocking: IAM restore verified as an exact reversal of the hammer broadening — with one operational caveat for the standing question.

Checked against git show 5c9b1e2: that commit changed exactly Get/Put on arn:aws:s3:::sliderule-public-cors/zagg-index/* → Get/Put/Delete on sliderule-public-cors/* (and, in the same commit, RecycleMaxInvocations "8" → "0", which 260221b then set to "1" — consistent with this PR's forensics). This hunk restores the issue #160 scope byte-for-byte. Corroboration for the PR's Questions-for-review item (1):

• nothing in-tree issues s3:DeleteObject (grepped src/, deployment/, docs/ — the only sliderule-public-cors/* grant elsewhere is the benchmark-CICD instance role in docs/deployment/benchmark-cicd.md, a different principal);
• the admin copy deployment/aws/execution_role.yaml:83 still carries the scoped ARN, so the two role copies are back in lockstep, and test_execution_role_grants_zagg_index_store pins both.

The caveat: if the live stack was updated from the broadened template during the hammer runs and Delete was used operationally (e.g. clearing zagg-index manifests between cold-start baselines), merging this narrows the deployed role on the next stack update and that workflow silently loses the permission. That is exactly the PR's open question — leaving it standing for the repo owner; no change requested here.

[espg]

🤖 from Claude

Superseded by phase 3 (55ab362): espg confirmed the whole-bucket Get/Put/Delete grant is deliberate; both role copies now carry it in lockstep.

[espg]

🤖 from Claude (review)

Nit: re-wrap this line. The edit joined two previously wrapped lines into one ~120-char source line mid-bullet, while the rest of this paragraph wraps at ~76 columns. Rendered output is unaffected; source-only consistency nit.

Content-wise the claim checks out: _maybe_self_recycle (deployment/aws/lambda_handler.py) reads ZAGG_RECYCLE_MAX_INVOCATIONS via _recycle_limit, and _INVOCATIONS_SERVED increments at handler entry, so gen_limit=1 → generation >= 1 is true after every invocation — recycle-after-every-async-invocation is exactly what a "1" default does, and the sync/failed-mirror carve-outs stated here match tests/test_lambda_handler.py::test_sync_path_never_recycles / test_failed_mirror_skips_recycle.

[espg]

🤖 from Claude

Folded in 82296ea — rewrapped.

[espg]

🤖 from Claude

Acknowledged — restored your scope in 55ab362 (phase 3): the role statement is back to Get/Put/Delete on arn:aws:s3:::sliderule-public-cors/* in template.yaml, the admin copy execution_role.yaml is brought into lockstep (it still had the narrow #160 form — the two had diverged), and the pinning test is renamed/re-scoped to test_execution_role_grants_public_cors_bucket, asserting the whole-bucket Get/Put/Delete grant in both copies. The adjacent comments now document the intent (index write-back under zagg-index/* and worker-written output zarr stores).

One note, once: prefix-scoped statements (zagg-index/* + the output-store prefixes, e.g. zagg-examples/*) would be the least-privilege shape if the output prefixes are enumerable — but whole-bucket is your infra call and is what's implemented.

[espg]

🤖 from Claude (review)

Non-blocking (phase 3): confirm whole-bucket was the intended reading, and note the blast radius.

This supersedes my earlier comment on the phase-2 restore (that hunk is gone; net-vs-main the grant is unchanged and only this comment block moved). Two things for the record:

1. Interpretation fork in the directing comment. The authorizing PR comment quotes the prefix-scoped ARN (arn:aws:s3:::sliderule-public-cors/zagg-index/*) "with delete permissions enabled ... should be put back", but the stated rationale (output zarr arrays as well as the index cache) requires objects outside zagg-index/, and "put back" most plausibly means the hammer-commit form (5c9b1e2: Get/Put/Delete on sliderule-public-cors/*) — which is what phase 3 keeps. Well-grounded reading; flagging only because the literal one ("zagg-index/* plus Delete") is narrower, and permission breadth deserves an explicit confirm rather than an inferred one.
2. Blast radius of whole-bucket Delete on the worker role. sliderule-public-cors is also the Lambda distribution bucket: distribute_zips.sh publishes the layer/function zips under <minor>/ and maintains versions.json, and stand_up.sh bootstraps stacks from those objects. With this statement every worker invocation can delete (or overwrite via Put) those distribution artifacts — a buggy output-store path or a hostile payload could take down the standup path for future deploys. If output stores settle on a stable prefix convention (this comment names zagg-examples/*), a two-resource statement (zagg-index/* + the output prefix) would keep the deliberate read/write/delete capability without covering the dist zips. Deliberate scope per the directing comment, so nothing blocking — just making the tradeoff explicit.

[espg]

🤖 from Claude (review)

Non-blocking — the Traceback term matches a tolerated warning path, so WorkerErrorCount can fire on invocations that return 200. src/zagg/processing/worker.py:307 logs the per-granule backend hook failure at WARNING with exc_info=True, and the comment right above it says "A failure here never fails the read":

try:
    index_backend.finish_granule(h5obj, s3_url)
except Exception:
    logger.warning(f"  index backend finish_granule failed for {s3_url}", exc_info=True)

Under the Lambda text-format runtime a single record's newlines are folded to \r, so this becomes one CloudWatch event that starts [WARNING] but contains Traceback (most recent call last): — the OR filter matches it. That undercuts the "genuine failure signatures only" claim in the resource comment / docs / PR body: a shard that completes successfully (statusCode 200) with a flaky finish_granule still increments ProcessWorkerErrorCount.

Two cheap fixes, either fine: (1) drop exc_info=True there and fold {e!r} into the message (the other two tolerated warnings at worker.py:298/314 already do exactly that, so this also restores consistency); or (2) keep it and soften the "genuine failure signatures only" wording in the docs to "failure signatures (including tolerated per-granule backend-hook tracebacks)". Option (1) is the one that keeps the alarm semantics clean.

Everything else about the pattern checks out: the ?"..." OR-list is AWS's own documented Lambda idiom (?"[ERROR]" ?"Task timed out"), the >- folded scalar parses to a valid single-line pattern (verified: 117 chars, single spaces), quoted "[ERROR]" treats the brackets literally, and the recycle path is genuinely disjoint — handler logs at [INFO] (lambda_handler.py:264) and the clean os._exit(0) yields "Runtime exited without providing a reason", which no term matches.

[espg]

🤖 from Claude

Folded in c35d633: the finish_granule warning now inlines the reason (: {e}, the sibling tolerated-warning style) instead of exc_info=True, and tests/test_index.py::test_finish_granule_failure_never_fails_the_read is strengthened to pin the inlined reason + exc_info is None. The "genuine failure signatures only" docs claim now holds.

[espg]

🤖 from Claude (review)

Nit — the evaluator silently mis-models a mixed pattern. For the two patterns the template actually ships (all-? and single required term) this is a faithful model of CloudWatch semantics: quoted-phrase matching is case-sensitive substring over the whole event, uniform ? terms are OR. But if a future edit produced a mixed pattern (some terms ?-prefixed, some not), any_mode goes False and the evaluator quietly demands ALL terms — whereas CloudWatch does not support combining required and optional terms in one pattern at all (the ? terms change meaning / the pattern is not doing what it looks like). Since the guard already asserts parseability, one more line would turn that foot-gun into a loud failure:

qs = {q for q, _ in terms}
assert len(qs) == 1, f"mixed required/optional terms unsupported by CW: {pattern!r}"

(Then any_mode = qs == {"?"}.) Verified the rest is load-bearing, not tautological: weakening "Runtime exited with error" to "Runtime exited" flips recycle_lines[1] into a match and the test fails, which is exactly the regression this guards.

[espg]

🤖 from Claude

Folded in c35d633: the evaluator now asserts term-flag uniformity (mixed ?/plain terms fail loudly) before choosing any/all mode.

[espg]

🤖 from Claude (review)

Nit — scope of what this actually covers, vs the PR-body phrasing. The test itself is correct and its comment is honest: PipelineConfig is a plain dataclass (no construction-time validation), so del cfg.data_source["index"] is equivalent to building without the key, and the monkeypatch target is right (worker.py imports index_from_config at module level, line 35). What it adds over the pinned twins is real but narrow: it proves the worker's streaming path resolves the backend only through the index_from_config seam — nothing else KeyErrors on the missing data_source["index"].

But the PR-body fold bullet says it covers "the unpinned-config route", which slightly overstates: the stub intercepts before index_from_config runs, so the inline-default resolution path (ds.get("index") is None → read-plan carve-out → InlineIndex) is never executed here — only tests/test_index_backends.py-style unit coverage (if any) or real-HDF5 fixtures would exercise it, as the in-test comment already concedes. Suggest tightening the PR-body wording to match the comment ("stubs the seam under an unpinned config") so nobody later reads it as inline-decode coverage. No code change needed.

[espg]

🤖 from Claude

Agreed — wording tightened in the PR body: the stub intercepts before index_from_config runs, so the test guards the streaming mechanics against seam coupling, not the inline-default resolution itself; compiled-inline integration coverage stays the follow-up gap noted in issue #175.

[espg]

🤖 from Claude (review)

Adversarial review of the phase-4 + fold commits (84beedf, 82296ea; prior pass covered through 55ab362). Two nits that live outside the diff, then what was verified clean.

Nit — every unhandled crash counts 2 (sometimes 3) in WorkerErrorCount. deployment/aws/lambda_handler.py:991-992 logs the same failure twice back-to-back:

logger.error(f"Unhandled exception in Lambda handler: {e}")
logger.exception(e)

Two CloudWatch events, both starting [ERROR] (the second also carrying the traceback) — the OR filter matches each event once, so one crash increments the metric by 2; a crash whose exception message spills a second [ERROR] line makes 3. Harmless for a > 0 alarm, but it skews any dashboard comparing WorkerErrorCount against invocation counts. Pre-existing handler code, not this diff — either dedupe to a single logger.exception(...) call in a later pass, or note in docs/deployment/lambda.md that the metric counts matching log events, not failed invocations.

Nit — the Runtime.OutOfMemory term is likely inert under this template. The template sets no LoggingConfig, so the Python runtime emits text-format logs, where an OOM kill surfaces as Error: Runtime exited with error: signal: killed Runtime.ExitError — already caught by the "Runtime exited with error" term. The Runtime.OutOfMemory string appears in the JSON-format platform logs this stack does not enable. Zero cost to keep (a dead OR term matches nothing), just flagging so nobody assumes it is the load-bearing OOM catch.

Verified clean:

• Pattern syntax: ?"phrase" OR-lists are valid and are AWS's own documented Lambda-error idiom; the >- folded scalars parse to clean single-line patterns (checked by loading the template: 117 chars, single spaces); quoted "[ERROR]" is a literal match, brackets inert.
• Disjointness for real: the recycle line is logged at INFO (lambda_handler.py:264-265, format matches the test's sample incl. threshold=%g → threshold=1), _maybe_self_recycle fires only after a successful result mirror, and the clean exit produces "Runtime exited without providing a reason" — no error term matches. No logger.error/exception on the successful async path.
• Template wiring: log-group names match the functions (ProcessFn → !Ref FunctionName, ExtractFn → ${FunctionName}-extract); DefaultValue: 0 is a valid Double; Condition + DependsOn placement fine; ExtractFn env carries both ZAGG_RECYCLE_* vars so the Extract filters are meaningful; by-name group references correctly dodge the LogGroup-adoption trap.
• Docs: the OnFailure warning is accurate (both EventInvokeConfigs pin MaximumRetryAttempts: 0 with no DestinationConfig; under recycle-every-invocation each async invoke ends in Runtime.ExitError, so a destination would fire per invocation), and the fresh-stack false → invoke → true dance matches PutMetricFilter's missing-log-group failure mode.
• All three new tests pass locally (3 passed); the disjointness test is load-bearing (weakening the "Runtime exited with error" term breaks it).

One inline finding (the Traceback term matching the tolerated finish_granule warning at src/zagg/processing/worker.py:307) is the only one worth a code change before merge — see r3532703119.

[espg]

🤖 from Claude

Re the pass-2 general findings (comment): (a) the crash-path double log (logger.error + logger.exception) is left as-is — WorkerErrorCount over-counts a real crash by one, which is harmless for the >0 alarming the docs recommend, and touching the handler crash path is beyond this PR's minimal-fix scope; flagging for a follow-up if count-accurate dashboards matter. (b) The likely-inert Runtime.OutOfMemory term is kept deliberately: zero-cost, and it future-proofs against runtimes/report formats that do surface that error type string.