Skip to content

Force-cold Lambda dispatch option (issue #171)#172

Merged
espg merged 9 commits into
mainfrom
claude/171-force-cold
Jul 6, 2026
Merged

Force-cold Lambda dispatch option (issue #171)#172
espg merged 9 commits into
mainfrom
claude/171-force-cold

Conversation

@espg

@espg espg commented Jul 6, 2026

Copy link
Copy Markdown
Member

Closes #171.

What

Two mechanisms against the warm-container RSS ratchet (#171; forensics on the issue #169 thread: container RSS 959 → 1650 → 2029 → OOM at 2047 MB across four back-to-back fleet runs on the same 9 sandboxes), per the option analysis and espg's decisions in the plan comment:

  1. force_cold: bool = False on zagg.runner.agg (both Lambda paths) — the explicit certification-run tool. When requested, _force_cold_containers merges a per-run ZAGG_COLD_EPOCH=<uuid4> marker into the function environment via update_function_configuration and polls until this update applies (any configuration change invalidates every warm sandbox). Requires lambda:GetFunctionConfiguration + lambda:UpdateFunctionConfiguration on the caller and hard-fails (actionable message) rather than silently dispatching warm — a feature of an explicit request, which is why it is opt-in rather than the default.
  2. Worker-side container telemetry + self-recycle (phases B/C) — the durable, zero-new-IAM mechanism: workers report cold/warm, sandbox generation, and per-invocation RSS in the result envelope; bloated sandboxes destroy themselves after their result is safely mirrored to S3 (the async Lambda dispatch with S3-polled worker results (issue #151) #153 async result channel makes this safe: the runner polls the result object, not the Lambda response, and MaximumRetryAttempts: 0 means no zombie retry).

Design points (force_cold helper)

  • Existing environment variables are read first and preserved (the malloc tunables survive every run).
  • The poll accepts only this update's terminal states: the API is eventually consistent, so a Successful read immediately after the update can still describe the prior update — acceptance requires the polled environment to carry this run's marker (self-review finding, folded).
  • ResourceConflictException retries until the deadline (a concurrent force_cold run or in-flight deploy is not a permissions problem); a superseding concurrent update counts as success (Lambda serializes config updates, so its acceptance proves ours applied — warm sandboxes are invalidated either way); other failures raise with an actionable message (self-review findings, folded).
  • force_cold=False (the default): dispatch is byte-identical to pre-Dispatch option to force cold Lambda containers per run (warm-container RSS ratchet) #171.
  • Cost when enabled: one config-update round trip (seconds) + ~3.6 s cold init per container (measured on the SERC fleet 2026-07-06) — negligible against 180–300 s workers.

Phases

  • _force_cold_containers helper + plumbing through agg → strategies → both _run_lambda* preflights + tests
  • Fold self-review findings (marker-verified poll, conflict retry, superseded-success)
  • A — flip force_cold default back to False; reposition docstring as the explicit certification-run tool (per the plan decision); update tests
  • B — container telemetry in the worker result envelope (cold/warm sentinel, generation counter, per-invocation start RSS, sandbox id, init timestamp) + runner summary aggregation (cold/warm counts, per-generation max start-RSS)
  • C — worker self-recycle: after the result_url mirror succeeds, exit the sandbox when ZAGG_RECYCLE_RSS_MB / ZAGG_RECYCLE_MAX_INVOCATIONS thresholds are crossed (template.yaml defaults; structured ZAGG_SELF_RECYCLE log line; injectable exit seam; never on the sync path)
  • D — polish: docstrings (handler module docstring, agg() independence note), deployment-docs note (docs/deployment/lambda.md self-recycle section + CloudWatch metric filter; docs/deployment/benchmark.md container-regime/stratification note), final gates + review pass

Telemetry design (phase B)

  • Handler: module globals persist per sandbox, so _CONTAINER_INIT_TS (import time) and _INVOCATIONS_SERVED (incremented once per invocation, all modes — a setup/finalize/extract invoke warms the sandbox just like a shard) give cold/warm + generation for free; rss_start_mb is VmRSS at dispatcher entry (the ratchet signal — a dirty sandbox starts high); sandbox_id is AWS_LAMBDA_LOG_STREAM_NAME (unique per sandbox). Telemetry is merged into the per-unit response body at one dispatcher seam (_attach_container_telemetry), covering both per-unit handlers and every status branch (200/400/500) and therefore also the result_url mirror — setup/finalize/extract bodies stay byte-identical.
  • No rss_peak_mb field was added: the per-invocation peak already exists in the envelope as max_memory_mb (the issue Track per-invocation worker peak RSS, not container-lifetime ru_maxrss #141 sampled VmRSS peak) — extending, not duplicating, per the plan's coordinate-with-Track per-invocation worker peak RSS, not container-lifetime ru_maxrss #141 note; schema.py documents the relationship.
  • Runner: _container_telemetry_summary (shared by spatial + temporal paths) adds three additive summary fields — worker_cold_starts, worker_warm_starts, worker_rss_start_max_by_gen (max start-RSS per sandbox generation: flat = healthy, climbing = the End-to-end sponsor example: catalog → shardmap → dual aggregation → cached reads #169 ratchet). All None when no envelope carries telemetry (older deployed workers), so "no data" is distinguishable from "all warm". Existing keys untouched.

Self-recycle design (phase C)

  • Ordering is load-bearing: _maybe_self_recycle() runs only after _write_result (the Warm start lambda and memory freeing are *still* a problem #151/async Lambda dispatch with S3-polled worker results (issue #151) #153 async mirror) returns True — the orchestrator polls the result object, not the Lambda response, so at that point the invocation is operationally complete and os._exit(0) loses nothing; MaximumRetryAttempts: 0 guarantees no zombie retry. Pinned by test_result_mirror_completes_before_exit (records call order). A failed mirror skips the recycle (shard is recorded failed at the poll deadline anyway; don't churn the sandbox on a possibly transient S3 fault) — _write_result now returns a bool for that gate (its never-raises contract is unchanged). The sync path never reaches the gate (no result_url), pinned by test_sync_path_never_recycles.
  • Chosen defaults + rationale (template.yaml parameters RecycleRssMb/RecycleMaxInvocationsZAGG_RECYCLE_RSS_MB=1400, ZAGG_RECYCLE_MAX_INVOCATIONS=8): the End-to-end sponsor example: catalog → shardmap → dual aggregation → cached reads #169 ratchet retained ~700–1100 MB per heavy invocation against the 2047 MB OOM ceiling, so 1400 catches a sandbox after roughly one heavy retention while leaving the triggering invocation ~650 MB of headroom to finish; the generation cap of 8 is belt-and-suspenders for retention the RSS read misses (and the only check that can fire where /proc/self/status is unavailable). Either knob absent/empty/0 disables that check, so a stack without the new parameters behaves exactly as pre-Dispatch option to force cold Lambda containers per run (warm-container RSS ratchet) #171. No deploy was made — espg's next stack update activates the defaults. (template.yaml edits are named in-scope by the issue Dispatch option to force cold Lambda containers per run (warm-container RSS ratchet) #171 plan.)
  • Exit seam: module-level _exit = os._exit, monkeypatched in tests so nothing ever really exits pytest; os._exit (not sys.exit) because the sandbox is being discarded and the exit must not be catchable en route.
  • Observability: one structured line before exiting — ZAGG_SELF_RECYCLE rss_mb=<x> generation=<n> threshold=<t> — and a docs note (docs/deployment/lambda.md) that self-exits appear as runtime failures in CloudWatch Errors (cosmetic; result object is the source of truth per async Lambda dispatch with S3-polled worker results (issue #151) #153) with the metric-filter pattern to split them from real crashes.
  • force_cold and self-recycle are independent (both can be on); the agg() docstring says so.

Testing

tests/test_runner.py::TestForceCold (10 tests) against a stateful fake client mirroring the real API shape (every get_function_configuration response carries both Environment and LastUpdateStatus, with configurable eventual-consistency lag): env-merge preservation, stale-Successful rejection, conflict-retry, denied-update / unreadable-config / server-side-Failed / never-lands failures, superseded-success, exactly-once pre-dispatch invocation, opt-out no-touch, and the public default pinned False. Phase C adds TestSelfRecycle (mirror-before-exit ordering, RSS trigger + structured log line, below-threshold no-op, generation cap on the Nth invocation, disabled-by-default/0, sync-path never, failed-mirror skip, non-numeric knob disabled) and TestTemplateEnvironment::test_self_recycle_env_defaults (template parameter defaults + env wiring; the ExtractFn twin stays in lockstep via the existing mirror test). Phase B adds TestContainerTelemetry (handler: warm-repeat generation ratchet, start-RSS at entry, off-Linux None fallback, sandbox id, 400-envelope coverage, setup-counts-but-stays-clean, process_event coverage, mirrored-result parity) and TestContainerTelemetrySummary (runner: rollup counts + ratchet dict, None-not-zero on no telemetry, mid-rollout mixed fleet, spatial + temporal summary integration). Full suite at phase D: 1435 passed, 27 skipped (plus test_lambda_build.py: 28 passed with only the docker-dependent test_function_build_succeeds failing — pre-existing, reproduces on the unmodified tree). ruff check + ruff format --check clean on every touched file; pre-commit (mypy/codespell/check-yaml) shows only pre-existing baseline failures, byte-identical with the branch tip before this work (36 mypy errors; check-yaml has never parsed CloudFormation short-form tags). Each adversarial self-review pass produced findings that were folded before the next phase: phase A → dead test stubs removed (0972e5d); phase B → entry-time RSS sampling pinned with a first-call sentinel (038b8d0); phase C → module-wide recycle-env scrub + loud exit seam so a dev shell exporting ZAGG_RECYCLE_* can no longer kill pytest with a success-looking exit 0 (77033fc).

Questions for review

  • With the default on, every agg(backend="lambda") caller needs lambda:UpdateFunctionConfiguration … the weekly benchmark CI role likely lacks it Resolved by the phase-A default flip: with force_cold=False the benchmark role needs no new permission and no pin — it keeps measuring the warm regime, now stratified by the phase-B telemetry.
  • Should the runner unset ZAGG_COLD_EPOCH after the run? Leaving it costs nothing (workers ignore it); current behavior leaves it set.

Notes

Comment thread src/zagg/runner.py
lambda_client.update_function_configuration(
FunctionName=function_name, Environment={"Variables": env}
)
except Exception as exc:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

ResourceConflictException is misdiagnosed as a permissions failure. Lambda rejects UpdateFunctionConfiguration with ResourceConflictException whenever another configuration update is still in progress — e.g. two overlapping force_cold runs against the same function, or a run launched while a deploy's config update is mid-flight. The blanket except Exception wraps that into "The caller needs lambda:GetFunctionConfiguration and lambda:UpdateFunctionConfiguration; pass force_cold=False…", which is wrong on both counts: permissions are fine, and disabling force_cold defeats the memory isolation the caller asked for. Since the feature's use case is exactly back-to-back fleet runs (#169/#171), overlap is plausible.

Suggested fix: catch botocore.exceptions.ClientError and branch on response["Error"]["Code"] — on ResourceConflictException, retry the update with backoff until the same deadline the poll uses (env-only updates land in seconds); reserve the permissions wording for AccessDeniedException. Two smaller points in the same clause: (1) the initial get_function_configuration (L1925) sits outside the try, so a caller missing lambda:GetFunctionConfiguration gets a raw botocore error without the actionable message the docstring promises; (2) passing the RevisionId from that get into update_function_configuration closes the read-modify-write window in which a concurrent env edit would be silently clobbered (Lambda then raises PreconditionFailedException, handled by the same retry).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

Folded in ccd35fb: the initial get_function_configuration now has its own actionable error; ResourceConflictException on the update retries until the deadline (matched via the exception's response Error Code, so a concurrent force_cold run or in-flight deploy no longer masquerades as a permissions failure); other update errors keep the permission-oriented message. RevisionId CAS noted but left out — the retry loop plus marker-verified poll covers the practical race without adding a compare-and-swap dance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Verified resolved in ccd35fb: ResourceConflictException is now matched via the exception's response["Error"]["Code"] and retried with backoff until the shared deadline (bounded — no livelock), the initial get_function_configuration has its own actionable error naming the Get permission, and test_resource_conflict_retries_until_free / test_raises_when_update_denied / test_raises_when_configuration_unreadable cover the three paths (verified passing at ccd35fb). Two residuals, neither blocking: the RevisionId conditional-update hardening was not adopted (the read-modify-write clobber window between L1936 and L1948 remains, though it's benign between force_cold runs since both write the marker), and the conflict-timeout raise still uses the permissions wording — folded into the new comment at L1978.

Comment thread src/zagg/runner.py Outdated
status = lambda_client.get_function_configuration(FunctionName=function_name).get(
"LastUpdateStatus"
)
if status == "Successful":

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

The poll accepts any Successful, not this update's — it can return early while every warm sandbox is still live. Nothing ties the polled LastUpdateStatus to the update this run just issued, and two real scenarios break that:

  • Stale read. Lambda's control-plane reads are eventually consistent: a GetFunctionConfiguration issued immediately after UpdateFunctionConfiguration can serve the pre-update record, whose LastUpdateStatus is still Successful from the previous update. Env-only updates normally apply within a couple seconds, so the first poll iteration is usually the deciding one — a stale read there returns instantly, no error raised, and the run dispatches onto warm containers. That silently defeats exactly the "never silently degrade to warm" guarantee this helper exists to provide (PR body, docstring L1918-1920).
  • Concurrent update. If another run (or a deploy) updates the function during the poll window, the observed Failed/Successful may belong to the other update — a spurious hard failure, or a false success.

Cheap fix using data already in the response — gate success on the marker actually landing:

resp = lambda_client.get_function_configuration(FunctionName=function_name)
applied = (resp.get("Environment") or {}).get("Variables") or {}
if resp.get("LastUpdateStatus") == "Successful" and applied.get("ZAGG_COLD_EPOCH") == token:
    ...

(or compare resp["RevisionId"] against the one returned by update_function_configuration, which the code currently discards). Relatedly, the test double (TestForceCold._client in tests/test_runner.py) returns an Environment-only dict followed by status-only dicts — no real GetFunctionConfiguration response looks like that; each carries both keys. Reshaping the mock to full responses both mirrors the API and makes the token check testable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

Folded in ccd35fb: the poll now accepts Successful/Failed only when the polled Environment carries this run's ZAGG_COLD_EPOCH token — a stale read of the prior update's terminal state keeps polling. The test double was rebuilt as a stateful fake mirroring the real API shape (every response carries both Environment and LastUpdateStatus, with configurable eventual-consistency lag); test_stale_successful_from_prior_update_is_not_accepted pins the exact scenario you described.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Verified resolved in ccd35fb: the poll now accepts Successful/Failed only when the polled environment carries this run's ZAGG_COLD_EPOCH token, and the _FakeLambdaClient double mirrors the real API shape (both keys in every response) with an eventual-consistency lag that reproduces the stale-Successful race — test_stale_successful_from_prior_update_is_not_accepted fails against the pre-fold implementation and passes now. Ran the full suite at ccd35fb in an isolated worktree: 142 passed in the two runner test files, 581 passed elsewhere; the one failure (test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds) reproduces identically on main 904704c — pre-existing/environmental, not this PR. One follow-on case the marker gate opens (concurrent run clobbering the marker → spurious deadline failure) is posted as a new comment at L1978.

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

force_cold: bool = False

the default here should be true

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Default flipped to True in ccd35fb per the comment above, with both review findings folded (marker-verified poll, conflict retry) and the test double rebuilt to mirror the real API shape. One consequence now under Questions for review in the PR body: with the default on, every agg(backend="lambda") caller needs lambda:UpdateFunctionConfiguration or gets an actionable hard failure — that includes the weekly benchmark CI role. Either its policy grows the permission (template.yaml, yours per §1), or the benchmark invocation pins force_cold=False — which may be the right call anyway if the benchmark's warm-container behavior is part of what it should measure.

Comment thread src/zagg/runner.py
f"force_cold: {function_name} configuration update failed server-side "
"(LastUpdateStatus=Failed); check the function state in the console"
)
if time.time() >= deadline:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Concurrent force_cold runs: the loser's poll spins the full 120 s and then hard-fails with a self-contradictory message — even though its cold-start goal was already achieved. The marker gate (L1967) is right for stale reads, but it leaves the deadline raise as the only exit when another run's update supersedes ours. Concrete sequence with runs A and B on the same function:

  1. A's update is accepted; B's conflicts and retries (L1955) — Lambda serializes config updates, so B's is accepted only after A's reaches a terminal state.
  2. B's env payload was snapshotted at its initial read (L1936), before A's token existed — so B's accepted update removes A's ZAGG_COLD_EPOCH.
  3. A's poll now can never observe marker == token again: every read shows B's token with LastUpdateStatus=Successful. A burns the full wait_s, then raises: "configuration update still 'Successful' after 120s; workers would reuse warm containers" — self-contradictory ("still Successful"?), and both clauses are false: A's update was accepted (its update_function_configuration returned before the poll began — that acceptance is exactly why B conflicted), so every sandbox warm at that moment was already invalidated, and B's superseding update invalidates again. A's run fails spuriously after a 120 s stall, precisely in the multi-run scenario this feature targets (End-to-end sponsor example: catalog → shardmap → dual aggregation → cached reads #169 back-to-back fleets).

Suggested fix: capture the pre-update epoch ((current.get("Environment") or {}).get("Variables", {}).get("ZAGG_COLD_EPOCH")) and, when the poll observes a third epoch value (≠ token, ≠ pre-update) with LastUpdateStatus == "Successful", treat it as success — a subsequent accepted update proves ours landed first (serialization) and cold containers are guaranteed either way; log the supersession instead of raising. At minimum, branch the deadline message on marker mismatch ("a concurrent configuration update replaced this run's marker after it was accepted") rather than claiming warm reuse. Same wording nit one loop up (L1958-1961): when the conflict-retry loop exhausts the deadline on repeated ResourceConflictException, the raise still says "The caller needs lambda:UpdateFunctionConfiguration" — misattributed for a conflict timeout (permissions are fine); branch that message on code too. The retry loop itself is fine — bounded by deadline, no livelock.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

Folded in the follow-up commit: a third epoch value + Successful now returns as superseded-success (log line, no error) — Lambda's update serialization means the concurrent acceptance proves our invalidation happened; and the conflict-past-deadline raise got its own wording (in-flight update, retry or force_cold=False) instead of the permissions message. Two new tests pin both paths (test_superseded_by_concurrent_update_counts_as_success, test_conflict_past_deadline_reports_conflict_not_permissions).

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Option analysis for the forced-cold goal (per espg, in-session), the resulting decisions, and the implementation plan for this PR. The goal is legitimate and freshly observed — the #169 SERC runs ratcheted container RSS 959 → 1650 → 2029 → OOM at 2047 MB across four back-to-back fleet runs on the same 9 sandboxes, with the deployed #143/#144 mitigations measurably insufficient — but the config-epoch mechanism is one of several ways to get there:

  1. Config-epoch bump (this PR as written) — contractual cold, works everywhere. Costs: lambda:UpdateFunctionConfiguration (a broad write on the production function) for every caller; chills warm pools for ALL users of the function including concurrent runs; update-state races (folded, but inherent to the trick).
  2. Publish-a-version per run, invoke by qualifier — warm pools are per {function, version}, so a fresh version is all-cold by construction and $LATEST callers are untouched. Costs: PublishVersion is still a write perm; version housekeeping; and a landmine — EventInvokeConfig is per-qualifier, so async-invoking a fresh version silently reverts to Lambda's default 2 retries (violating the Don't retry deterministic Lambda failures (OOM); expose max_retries and run the benchmark with max_retries=1 #119/async Lambda dispatch with S3-polled worker results (issue #151) #153 one-clean-invocation contract) unless every publish also sets the invoke config.
  3. Self-recycling workers — the handler tracks its own RSS watermark / invocations-served (module-global); when bloated it finishes the shard, mirrors its result to S3 (result_url, which async Lambda dispatch with S3-polled worker results (issue #151) #153 already does), then os._exit(0), destroying its own sandbox. The runner polls the S3 result object, not the Lambda response, so the "crashed" invocation is cosmetically failed but operationally complete, and MaximumRetryAttempts: 0 means no zombie retry. Zero new IAM for any caller; surgical (only bloated sandboxes die — healthy warm starts keep their latency win); bounds the ratchet within a run too, which no dispatch-side option does. Drawbacks: CloudWatch error-metric noise (self-exits count as runtime failures — dashboards need a filter), async-path-only, needs a well-commented seam.
  4. Detect-and-report — the worker emits container telemetry (cold/warm flag, invocations-served generation, RSS baseline/peak — the Track per-invocation worker peak RSS, not container-lifetime ru_maxrss #141 direction) in its result envelope; the runner surfaces it in the summary. Explains rather than prevents, preserves benchmark series continuity, and is the right companion to everything above.
  5. Idle-wait / twin functions — free but slow and non-contractual; twins isolate callers but don't cold anything within one caller's back-to-back runs, which is exactly the observed ratchet. Not pursued.

Decisions (espg): implement (3) + (4) in this PR as the durable mechanism; flip force_cold back to default False — it stays in the API as the explicit big-hammer for certification runs, where its hard-fail-on-missing-permission is a feature of an explicit request rather than a tax on every caller (and the weekly benchmark then needs no role change and keeps measuring the warm regime, stratified by the new telemetry). The eventual retirement of this whole question is #171's named deeper fix — streaming aggregation for dense shards (bounded-buffer machinery already landed in PR #152 phase 4).

Plan / phases for this PR:

  • Phase A — default flip + repositioning. force_cold default TrueFalse in agg() (both Lambda paths); docstring reframed (explicit certification-run tool; permission requirement documented); tests updated for the new default; PR body's Questions-for-review item about the benchmark role resolved (no role change needed).
  • Phase B — container telemetry (option 4). Worker: cold/warm flag via module-global sentinel, invocations-served generation counter, RSS at handler start + peak delta, sandbox identity (log-stream name), init timestamp — added to the existing result envelope (coordinate with what Track per-invocation worker peak RSS, not container-lifetime ru_maxrss #141/Temporal aggregation #12 already pass through; extend, don't duplicate). Runner: aggregate into the run summary (cold/warm counts, per-shard generation, ratchet visibility — max start-RSS by generation). Tests both sides.
  • Phase C — self-recycle (option 3). Handler end-of-invocation check AFTER the result_url mirror succeeds: if RSS watermark or generation cap exceeded (knobs via function env with template defaults, e.g. ZAGG_RECYCLE_RSS_MB / ZAGG_RECYCLE_MAX_INVOCATIONS, both disable-able), emit one structured log line (ZAGG_SELF_RECYCLE rss=… gen=… — CloudWatch-searchable) then os._exit(0) through an injectable seam (testable without killing pytest). Strictly conditional on the async/result-mirror path — never on sync (the response would be lost). Document the cosmetic failed-invocation metric + a dashboard filter note in deployment docs. Tests: exit-seam ordering (result write completes before exit), threshold logic, sync-path never recycles, disabled-by-default-or-sane-default decision recorded in the PR body.
  • Phase D — polish. Docstrings/README notes, telemetry-stratification example in the benchmark docs if cheap, full local gates, final self-review pass.

Normal procedure throughout: one commit per phase, fresh-context adversarial self-review after each push, findings folded before the next phase.

@espg espg marked this pull request as ready for review July 6, 2026 21:29
@espg espg added the waiting label Jul 6, 2026
@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

All phases complete, all four review findings folded and verified closed by a fresh-context re-review pass, CI green (ruff, builds, 3.12/3.13 matrix) — marking ready for review per §2. Label waiting: the one open decision is yours (PR-body Questions): benchmark CI role permission (lambda:UpdateFunctionConfiguration in template.yaml) vs pinning force_cold=False in the benchmark script. Merge is yours.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread tests/test_runner.py Outdated
monkeypatch.setattr(runner, "_invoke_lambda_setup", lambda *a, **k: None)
monkeypatch.setattr(runner, "_invoke_lambda_finalize", lambda *a, **k: None)
monkeypatch.setattr(runner, "_get_function_timeout_s", lambda *a, **k: 720)
monkeypatch.setattr(runner, "_force_cold_containers", lambda *a, **k: None)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Minor, revert-completeness: this no-op stub (and its five twins at lines 893, 1296, 1381, 1457, and 1811) was added in ccd35fb precisely because the default flipped to True — unrelated _run_lambda tests would otherwise have hit the real config-update path. Commit 504761f reverts the default to False but leaves all six stubs behind. With force_cold=False the default dispatch path never reaches _force_cold_containers (if force_cold: guards both call sites in src/zagg/runner.py), so these are now dead scaffolding:

monkeypatch.setattr(runner, "_force_cold_containers", lambda *a, **k: None)

Why it matters: a reader of e.g. test_lambda_summary_keys_and_cost will infer from the stub that default Lambda dispatch performs a function-configuration update — exactly the assumption this commit removes. Suggested fix: drop all six lines (TestForceCold pins the default via inspect.signature and covers opt-in/opt-out explicitly, so nothing regresses); alternatively, if you want them kept as a guard against a future default flip, a one-line comment saying so would stop them from reading as a live dependency.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

Folded in 0972e5d: all six no-op _force_cold_containers stubs removed — with the phase-A default back to False, default-dispatch tests no longer reach the helper, so the scaffolding (added in ccd35fb precisely because the default was True) was dead. tests/test_runner.py passes without them (136 passed).

espg and others added 2 commits July 6, 2026 14:37
…#171)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread tests/test_lambda_handler.py Outdated
assert first["container_init_ts"] == handler_mod._CONTAINER_INIT_TS

def test_rss_start_sampled_at_entry(self, handler_mod, monkeypatch):
monkeypatch.setattr(handler_mod, "_read_vmrss_kib", lambda: 512 * 1024)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

test_rss_start_sampled_at_entry does not actually pin the "at entry" part of its name — which is the load-bearing property of rss_start_mb. The stub returns a constant:

monkeypatch.setattr(handler_mod, "_read_vmrss_kib", lambda: 512 * 1024)

so the test passes identically whether the implementation snapshots RSS at dispatcher entry (the intended ratchet signal — "Snapshot start RSS here, before any per-unit work inflates it") or anywhere later in the invocation. A refactor that moved the _read_vmrss_kib() call into _attach_container_telemetry (post-work, i.e. end-RSS) would corrupt the #169 ratchet signal while every test in this class stays green.

A deterministic fix is available because the telemetry snapshot is provably the first _read_vmrss_kib call of the invocation: _container_telemetry() runs at lambda_handler entry, before _handle_process constructs the #141 sampler (whose __init__ availability probe and daemon-thread reads come after). So a first-call sentinel pins entry-time sampling:

calls = iter([512 * 1024])
monkeypatch.setattr(handler_mod, "_read_vmrss_kib", lambda: next(calls, 4096 * 1024))
...
assert body["rss_start_mb"] == pytest.approx(512.0)  # a later sample would read 4096.0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

Folded in 038b8d0: the stub is now a first-call sentinel (512 MB to the first _read_vmrss_kib call only, 2000 MB to every later one) with an added calls > 1 guard proving post-entry reads happened — so the test fails if the snapshot ever moves after the per-unit work, pinning the entry-time semantics of the ratchet signal exactly as described.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
def _fresh_container(self, handler_mod, monkeypatch):
monkeypatch.setattr(handler_mod, "_INVOCATIONS_SERVED", 0)
monkeypatch.delenv("ZAGG_RECYCLE_RSS_MB", raising=False)
monkeypatch.delenv("ZAGG_RECYCLE_MAX_INVOCATIONS", raising=False)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Env-var leak into pytest: a dev shell with ZAGG_RECYCLE_* exported silently kills the suite with exit code 0.

The monkeypatch.delenv("ZAGG_RECYCLE_*") guard here protects only TestSelfRecycle. But after this commit, several pre-existing tests traverse _maybe_self_recycle() with the real os._exit seam and the inherited process environment — every test that drives lambda_handler through a successful result_url mirror without stubbing _exit: TestAsyncResult.test_result_url_writes_response_envelope / test_gate_failure_envelope_also_written, test_result_url_mirrors_process_event_envelope / test_result_url_mirrors_500_envelope, and the TestContainerTelemetry mirror test. Since handler_mod is module-scoped, _INVOCATIONS_SERVED climbs past the template default of 8 long before those tests run, so exporting the documented production defaults (the natural thing to do while hand-testing this feature) is sufficient to trigger — no exotic value needed. On Linux, ZAGG_RECYCLE_RSS_MB can trigger too via the real /proc read.

Reproduced at 038b8d0:

$ ZAGG_RECYCLE_MAX_INVOCATIONS=8 pytest tests/test_lambda_handler.py -q
............................................s..........................
$ echo $?
0        # died mid-file via os._exit(0): no summary, ~5 tests never ran

(clean baseline: 76 passed, 1 skipped with a summary line)

The failure mode is the worst one available: os._exit(0) makes pytest report success to any wrapper (CI step, pre-commit, && chain) while silently truncating the run — a test regression after the truncation point becomes invisible.

Suggested fix: hoist the guard out of TestSelfRecycle into a module-level autouse fixture in this file (or tests/conftest.py), e.g.

@pytest.fixture(autouse=True)
def _no_ambient_recycle_env(monkeypatch):
    monkeypatch.delenv("ZAGG_RECYCLE_RSS_MB", raising=False)
    monkeypatch.delenv("ZAGG_RECYCLE_MAX_INVOCATIONS", raising=False)

Belt-and-suspenders alternative (also covers future tests that set the env but forget the spy): have an autouse fixture point handler_mod._exit at a stub that raises AssertionError("unexpected self-recycle in tests"), and let _spy_exit override it where the exit is expected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

Folded in 77033fc: a module-wide autouse fixture now (1) scrubs ZAGG_RECYCLE_RSS_MB / ZAGG_RECYCLE_MAX_INVOCATIONS before every test in the file and (2) replaces the _exit seam with a raising stub, so any unexpected recycle fails LOUDLY instead of killing pytest with a success-looking exit 0. TestSelfRecycle re-enables the knobs and re-replaces the seam per test on top of it. Verified against your exact repro: ZAGG_RECYCLE_MAX_INVOCATIONS=2 pytest tests/test_lambda_handler.py and ZAGG_RECYCLE_RSS_MB=1 … both now run to completion — 76 passed, 1 skipped, full summary.

espg and others added 2 commits July 6, 2026 15:13
…r tests (issue #171)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…171)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Decision from espg (in-session): no grant. The benchmark CI role does not gain lambda:UpdateFunctionConfiguration; the weekly benchmark's permanent posture is warm containers, stratified by this PR's telemetry (cold/warm counts + per-generation RSS in the summary). With force_cold defaulting to False, that means zero changes anywhere — no role edit, no pin. If a cold-certification benchmark column ever earns its place, the grant gets made then, deliberately, as its own decision.

That resolves the last open question — this PR is now purely awaiting review/merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dispatch option to force cold Lambda containers per run (warm-container RSS ratchet)

1 participant