fix(sandbox): remove dead snapshot-throughput calibration machinery

[detail-app]

Detail bug report: View on Detail

Summary

DockerBackend measured an EWMA of snapshot throughput after every docker commit/flatten, persisted it to a state file, and loaded it on worker startup — but no production call site ever passed the measured value into _snapshot_timeout_s. Every caller left throughput_bytes_per_second=None, so the adaptive branch was dead code: save_image/load_image always fell back to the fixed sandbox_snapshot_timeout_ns_per_byte rate, while a whole persisted-calibration subsystem (config fields, load-on-startup, record-on-commit/flatten, the snapshot-throughput.json state file) was maintained for a consumer that never existed. The ns_per_byte description promised "until measured throughput is available" — but measured throughput was never made available.

• Root cause: b315836 (Sandbox salvage: derive commit timeout from measured throughput + escalate deterministic failures instead of looping fail-closed #2032) rewired the snapshot-verb timeout to a size_walk_seconds branch and dropped the sole call site that passed the EWMA into _snapshot_timeout_s, leaving the measurement/persistence intact but with no remaining consumer (and save_image/load_image, added later, inherited the omission).
• Fix (deletion, not re-wiring): removed _load_throughput, _record_throughput, self._throughput_bytes_per_second, the throughput_bytes_per_second parameter and dead branch of _snapshot_timeout_s, the size_rw/timing parameters of _commit/_flatten that existed only to feed it, and the sandbox_snapshot_throughput_ewma_alpha / sandbox_snapshot_throughput_state_path config fields. __init__ no longer reads the state file; it is never written. The snapshot verb's size_walk_seconds budget and the retry escalation are unchanged. The ns_per_byte description now honestly describes the fixed save/load budget.
• Why delete rather than wire: the EWMA is measured from docker commit/flatten (single-layer compression, often CPU-bound); its throughput profile differs from docker image save/load (multi-layer tar export/import of the whole image). A naive wiring could produce tighter budgets than today's fixed rate and regress the exact mis-sized-timeout failure the calibration was meant to absorb. Restoring adaptive save/load budgets would require measuring save/load throughput directly and is left as separate work.

Substrate state changes

• Required env-var contract changed (added, removed, renamed, validation tightened)

Removed two env vars for dead subsystem config: AIOS_SANDBOX_SNAPSHOT_THROUGHPUT_EWMA_ALPHA and AIOS_SANDBOX_SNAPSHOT_THROUGHPUT_STATE_PATH. Because Settings uses extra="ignore", deployments that still set them will silently drop them on startup (no validation failure). The persisted state file at /var/lib/aios/snapshot-throughput.json (default) is no longer read or written.

Post-merge ops checklist:

1. Optional cleanup: remove any AIOS_SANDBOX_SNAPSHOT_THROUGHPUT_* env vars
   from deployment configs (no-op if left — they are ignored).
2. Optional cleanup: delete /var/lib/aios/snapshot-throughput.json on workers
   if present (it is stale and no longer read or written).

Test plan

• Unit tests, typecheck (mypy strict), and lint (ruff check + format) all pass. Rewrote test_snapshot_timeout_calibration.py to pin both live branches (fixed-rate save/load; walk-derived snapshot verb), add contract coverage for the previously-untested save_image/load_image timeout call sites, and guard against re-introducing the dead seam (passing throughput_bytes_per_second= now raises; the config fields and backend calibration attrs/methods are gone). Snapshot-verb, disk-gate, registry-salvage/breaker, snapshot-store, and config suites are unchanged and green; full tests/unit is green.
• E2E on a real Docker daemon (pulled ghcr.io/eumemic/aios-sandbox:latest): test_sandbox_salvage.py (crash-corpse + running-corpse salvage/resume), the full test_sandbox_persistence.py suite (real docker commit, docker export|import flatten, and resume-from-snapshot-tag), and test_sandbox_image_contract.py (43 tests) all pass. Verified after these runs that no snapshot-throughput.json file is written anywhere (including /var/lib/aios/).
• Two E2E failures encountered were both confirmed pre-existing environmental, not regressions: the [flatten] persistence variant failed the flatten disk-gate on a 20 GB loop FS that can't satisfy the default 15 GB sandbox_flatten_disk_floor_bytes (passes with the floor lowered), and two test_browser_network_isolation cases failed on a host cgroupv2 "domain threaded mode" limitation that rejects the browser spec's --cpus config. Both reproduced identically against HEAD before the fix.
• Could not run the gVisor variant (AIOS_SANDBOX_RUNTIME=runsc ... pytest -m docker): runsc was downloaded and verified working, but the Docker daemon config lives on a read-only bootstrap filesystem, so runsc could not be registered as a runtime (and SIGHUP reload doesn't register runtimes). It re-runs the same docker-marked E2E suite under a second runtime; the snapshot-verb paths it would exercise already passed under the default runc runtime above.

Risk / rollback

Low. The removed machinery was dead — no timeout behavior changes (save/load already used the fixed rate; the snapshot verb already used the walk-based budget). Roll back by reverting the commit; the deleted state file, if a worker had one, becomes stale and is simply ignored.

---

Automatic Fixes PRs can be configured here.

[eumemic-bot]

Code review

Verdict: pass

No blocking findings. The removed EWMA state/configuration was not consumed by production timeout call sites, and the fixed-rate save/load plus walk-derived commit/flatten timeout behavior remains intact. Repository-wide references confirm no production users of the deleted API remain.

Validated at aed4f446dff0601ab59ac9118381ce05bbefd2bb:

• 65 targeted unit tests passed
• Ruff lint and formatting checks passed for all changed files
• Mypy passed for the changed source files
• git diff --check passed

[eumemic-bot]

Code review

Verdict: pass

Reviewed 682883d6f45e263f274cdf44fcf3b83bc15820a6 (verified git -C /mnt/review rev-parse HEAD matches). No blocking findings.

Scope. Head is a merge of master into the fix branch. Diffing the PR's own files against each revision's merge-base shows the substantive change is byte-identical to the previously reviewed revision aed4f446 — only hunk offsets in src/aios/config.py shifted (@@ -260 → @@ -261) because of unrelated master churn. Per instructions, expensive checks already reported by the earlier eumemic-bot review were not re-run exhaustively; the focused re-verification below confirms the merge did not disturb the change.

What I verified on this head

• The deletion is complete and nothing dangles. No remaining references anywhere in the tree to sandbox_snapshot_throughput_ewma_alpha, sandbox_snapshot_throughput_state_path, _load_throughput, _record_throughput, _throughput_bytes_per_second, or snapshot-throughput.json (only the new tests' negative assertions and their explanatory docstring). No AIOS_SANDBOX_SNAPSHOT_THROUGHPUT_* in docs/, infra/, or compose.yml, so the env-var removal leaves no stale deployment documentation.
• No behavior change at the live call sites. _snapshot_timeout_s (docker.py:98-115) keeps both surviving branches intact: the snapshot verb still gets max(_SNAPSHOT_TIMEOUT_FLOOR_S, size_walk_seconds * _SNAPSHOT_WALK_SAFETY_FACTOR) (docker.py:678), and save_image/load_image still get max(floor, size * ns_per_byte) * safety_margin (docker.py:1061, 1071). The retry escalation (retry_multiplier ** attempt, capped) is unchanged and still applies to both branches. Since every production caller previously passed throughput_bytes_per_second=None, removing the parameter is provably timeout-neutral.
• Removed parameters were genuinely single-purpose. size_rw on _commit/_flatten and the monotonic() timing pairs existed only to feed _record_throughput; no other statement in either method read them. The rw local in _snapshot_uncounted is still live for the disk gate (docker.py:672, 690, 697), so the disk-gate sizing is untouched. Module-level json and monotonic imports both remain in use elsewhere (docker.py:627, 974), so no dead imports were left behind.
• Tests. uv run pytest tests/unit/sandbox/test_snapshot_timeout_calibration.py test_snapshot_verb.py test_tmp_mount_and_disk_gate.py → 65 passed. The rewritten calibration module is a real improvement over what it replaces: it pins both live branches numerically (including that the walk branch ignores ns_per_byte/safety_margin, verified by setting them to absurd values), adds first-ever contract coverage for the save_image/load_image timeout call sites, and adds regression guards that would fail if the dead seam were reintroduced (TypeError on throughput_bytes_per_second=, not hasattr on the config fields and backend attrs). The two fixture edits (test_snapshot_verb.py, test_tmp_mount_and_disk_gate.py) only drop now-nonexistent settings and correctly drop the then-unused tmp_path/Path along with them.
• Static checks on changed files (cheap, so re-confirmed on the merge head): ruff check clean, ruff format --check clean, mypy clean on docker.py and config.py.

Non-blocking observations (no fix required; recorded so a later reader does not mistake them for oversights):

• sandbox_snapshot_timeout_safety_margin (config.py:268-272) still describes itself as applying to "throughput-derived snapshot budgets." It is now applied only to the fixed per-byte estimate, so the adjective is vestigial while the sentence remains true. The sibling ns_per_byte description was corrected in this PR; this one could follow, but the wording is not misleading enough to block.
• docs/design/durable-session-sandboxes.md:300 still describes the commit budget as "per-byte budget at ~10× measured throughput." That text was already stale before this PR (the size_walk_seconds rewiring in Sandbox salvage: derive commit timeout from measured throughput + escalate deterministic failures instead of looping fail-closed #2032 is what invalidated it, and that path is untouched here), so it is out of scope rather than a regression introduced by this diff.
• The PR body's own risk note is accurate: because Settings uses extra="ignore", deployments still setting the two removed env vars will silently drop them rather than fail startup. That is the intended, documented outcome for removing dead knobs, and the post-merge ops checklist covers the stale state file.

Not evaluated (and not required to be, per verification-proportionality): Docker-marked E2E suites and the gVisor runtime variant, both reported in the PR body. They exercise the unchanged walk-derived and disk-gate paths; the unit-level contract tests above cover the behavior this diff actually touches, and no removed code had a runtime-only effect that unit tests could hide.

[eumemic-bot]

Code review

Verdict: pass

Reviewed ddadb13bd19728b96a65e5f2228d30fc74f41a98 (confirmed git -C /mnt/review rev-parse HEAD matches). No blocking findings.

Scope of this round. Head is a new merge of master (3f84909, "redact ingest bearer token") into the fix branch. I diffed the PR's own files against each revision's merge-base and the substantive change is byte-identical to the previously reviewed revision 682883d6 (diff of the +/- lines is empty; git diff 682883d6..HEAD over src/aios/config.py, src/aios/sandbox/backends/docker.py, tests/unit/sandbox/ is empty). The single merged master commit touches none of the sandbox/config surfaces. Per the verification-proportionality instruction, the expensive E2E/gVisor work reported in the PR body was not re-run; the cheap checks below were re-confirmed on this head.

Standing properties re-checked on this head

Prior rounds asserted no blocking properties (both earlier eumemic-bot reviews passed). I re-verified the implicit acceptance criteria of a deletion PR against the current tree rather than trusting the earlier passes:

• The deleted subsystem leaves no live reference. Tree-wide grep for sandbox_snapshot_throughput_ewma_alpha, sandbox_snapshot_throughput_state_path, _load_throughput, _record_throughput, _throughput_bytes_per_second, snapshot-throughput, SNAPSHOT_THROUGHPUT returns matches only inside the new test module's negative assertions and its explanatory docstring. No AIOS_SANDBOX_SNAPSHOT_THROUGHPUT_* in any .md/.yml/.yaml/.env*/.tf/.sh, so the env-var removal leaves no stale deployment docs to mislead an operator.
• Timeout behavior at every live call site is unchanged. _snapshot_timeout_s (docker.py:98–115) retains both surviving branches exactly: the snapshot verb gets max(_SNAPSHOT_TIMEOUT_FLOOR_S, size_walk_seconds * _SNAPSHOT_WALK_SAFETY_FACTOR) (docker.py:678), and save_image/load_image get max(floor, size * ns_per_byte) * safety_margin (docker.py:1061, 1071); the capped retry escalation still multiplies both. Every production caller previously passed throughput_bytes_per_second=None, so removing the parameter and its branch is provably budget-neutral — the deleted else-of-else was unreachable in production.
• Removed parameters were genuinely single-purpose; nothing else read them. size_rw on _commit/_flatten and the monotonic() timing pairs fed only _record_throughput. The rw local in _snapshot_uncounted remains live for the flatten/commit disk gate (docker.py:671, 681, 690, 697), and size_rw is still passed to _snapshot_timeout_s at docker.py:679, so disk-gate sizing and the empty-writable-layer short-circuit (docker.py:654) are untouched. Module-level json and monotonic imports remain used elsewhere (docker.py:627–629, 974, 1326), so no dead imports were left behind. The only two call sites of _commit/_flatten (docker.py:715, 717) were updated in the same hunk; prewarm_commit is a separate path that never took these arguments.
• Tests are green and actually constrain the change. uv run pytest tests/unit/sandbox/test_snapshot_timeout_calibration.py test_snapshot_verb.py test_tmp_mount_and_disk_gate.py → 65 passed. The rewritten calibration module pins both live branches numerically (including that the walk branch ignores ns_per_byte/safety_margin, proven by setting them to absurd values), adds first-time contract coverage of the save_image/load_image timeout call sites, and adds guards that would go red if the dead seam were reintroduced (TypeError on throughput_bytes_per_second=, not hasattr for the config fields and backend attrs). The two fixture edits only drop now-nonexistent settings and the parameters that existed solely for them.
• Static checks on changed files (cheap, so re-run on this merge head): ruff check clean, ruff format --check clean (45 files already formatted), mypy clean on docker.py and config.py.

Non-blocking observations (carried forward from the prior round; still true, still not worth a fix cycle):

• sandbox_snapshot_timeout_safety_margin (config.py:268–272) still reads "Multiplier applied to throughput-derived snapshot budgets." It now applies only to the fixed per-byte estimate, so "throughput-derived" is vestigial — the sentence stays true (a per-byte rate is a throughput), just less precise than the sibling ns_per_byte description this PR corrected. Optional follow-up.
• docs/design/durable-session-sandboxes.md:300 still describes the commit budget as a "per-byte budget at ~10× measured throughput." That text was already stale before this PR — Sandbox salvage: derive commit timeout from measured throughput + escalate deterministic failures instead of looping fail-closed #2032's size_walk_seconds rewiring invalidated it, and that path is untouched here — so it is pre-existing drift, not a regression this diff introduces.
• Because Settings uses extra="ignore", deployments still setting the two removed env vars drop them silently instead of failing startup. That is the intended outcome for retiring dead knobs and is disclosed in the PR body along with the stale-state-file cleanup step.

Not evaluated (and not required, per verification proportionality): the Docker-marked E2E suites and the gVisor runtime variant reported in the PR body. They exercise the walk-derived snapshot verb and disk-gate paths, which this diff does not modify; no removed code had a runtime-only effect that unit-level tests could conceal, since the deleted branch was unreachable from every production call site.

[eumemic-bot]

Code review

Verdict: pass

Reviewed be0f5ba262b9df17d765ca9ea7316b2932e88afa (confirmed git -C /mnt/review rev-parse HEAD matches). No blocking findings.

Scope of this round. Head is a new merge of master (8947e04b, "fix(lanes): drop archive_when_idle from session update PUT body", #2381) into the fix branch. git diff ddadb13b..HEAD -- src/aios/config.py src/aios/sandbox/backends/docker.py tests/unit/sandbox/ is empty — the substantive PR change is byte-identical to the previously reviewed revision. The whole delta since the last reviewed head is confined to src/aios/lanes/activate_script.py and two lanes test files, which belong to the merged master commit and touch no sandbox/config surface. Per the verification-proportionality instruction, the E2E/gVisor work reported in the PR body was not re-run; the cheap checks below were re-confirmed on this head.

Standing properties re-checked on this head

Prior rounds asserted no blocking properties (all three earlier reviews passed). I re-verified the acceptance criteria implicit in a deletion PR against the current tree rather than inheriting the earlier passes:

• The deleted branch was provably unreachable, so its removal cannot change any timeout. Checked at the merge-base (8947e04b), not just on head: git grep throughput_bytes_per_second -- src/ at the base returns only the parameter declaration, its own if/else, and the EWMA machinery — no call site anywhere in src/ ever passed it. All three production call sites of _snapshot_timeout_s at the base (docker.py:714, 1107, 1117) omitted the argument. Removing the parameter and its else arm is therefore budget-neutral by construction, not by assertion.
• Both surviving branches are intact and unchanged. _snapshot_timeout_s (docker.py:98–115) still yields max(_SNAPSHOT_TIMEOUT_FLOOR_S, size_walk_seconds * _SNAPSHOT_WALK_SAFETY_FACTOR) for the snapshot verb (docker.py:678) and max(floor, size * ns_per_byte) * safety_margin for save_image/load_image (docker.py:1061, 1071); the capped retry escalation still multiplies both.
• The deleted subsystem leaves no live reference. Tree-wide grep for sandbox_snapshot_throughput_ewma_alpha, sandbox_snapshot_throughput_state_path, _load_throughput, _record_throughput, _throughput_bytes_per_second, snapshot-throughput, SNAPSHOT_THROUGHPUT, throughput_bytes_per_second returns matches only inside the new test module's negative assertions and its explanatory docstring — 10 hits, all in test_snapshot_timeout_calibration.py. Nothing in docs/, infra/, compose, or env templates, so the env-var removal strands no deployment documentation.
• Removed parameters were genuinely single-purpose; nothing else read them. size_rw on _commit/_flatten and the monotonic() timing pairs fed only _record_throughput. The rw local in _snapshot_uncounted is still live for the disk gate (docker.py:671, 672, 690, 697) and size_rw is still passed to _snapshot_timeout_s at docker.py:678, so disk-gate sizing and the empty-writable-layer short-circuit are untouched. Module-level json (6 uses) and monotonic (docker.py:627, 629 — the size-walk measurement that feeds the live branch) remain in use, so no dead imports were left behind.
• Tests are green and actually constrain the change. uv run pytest tests/unit/sandbox/test_snapshot_timeout_calibration.py test_snapshot_verb.py test_tmp_mount_and_disk_gate.py → 65 passed. The rewritten calibration module pins both live branches numerically (including that the walk branch ignores ns_per_byte/safety_margin, proven by setting them to absurd values), adds first-time contract coverage of the save_image/load_image timeout call sites, and adds guards that go red if the dead seam is reintroduced (TypeError on throughput_bytes_per_second=, not hasattr for the config fields and backend attrs). The two fixture edits only drop now-nonexistent settings and the parameters that existed solely to supply them.
• Static checks on changed files (cheap, so re-run on this merge head): ruff check clean, ruff format --check clean (45 files already formatted), mypy clean on docker.py and config.py.

Non-blocking observations (carried forward; still true, still not worth a fix cycle):

• sandbox_snapshot_timeout_safety_margin (config.py:268–272) still reads "Multiplier applied to throughput-derived snapshot budgets." It now applies only to the fixed per-byte estimate, so "throughput-derived" is vestigial — the sentence stays true (a per-byte rate is a throughput), just less precise than the sibling ns_per_byte description this PR corrected. Optional follow-up.
• docs/design/durable-session-sandboxes.md:300 still describes the commit budget as a "per-byte budget at ~10× measured throughput." That text was already stale before this PR — Sandbox salvage: derive commit timeout from measured throughput + escalate deterministic failures instead of looping fail-closed #2032's size_walk_seconds rewiring invalidated it, and that path is untouched here — so it is pre-existing drift, not a regression from this diff.
• Because Settings uses extra="ignore", deployments still setting the two removed env vars drop them silently rather than failing startup. That is the intended outcome for retiring dead knobs and is disclosed in the PR body alongside the stale-state-file cleanup step.

Not evaluated (and not required, per verification proportionality): the Docker-marked E2E suites and the gVisor runtime variant reported in the PR body. They exercise the walk-derived snapshot verb and disk-gate paths, which this diff does not modify; no removed code had a runtime-only effect that unit-level tests could conceal, since the deleted branch was unreachable from every production call site. The lanes changes carried in by the master merge are outside this PR's diff and were reviewed under #2381.

[eumemic-bot]

Code review

Verdict: pass

Reviewed 96ad9485ef0ce9781d961635afb6ef2c828e2f00 (confirmed git -C /mnt/review rev-parse HEAD matches). No blocking findings.

Scope of this round. Head is a new merge of master (8be49b3a) into the fix branch. git diff be0f5ba2..HEAD -- src/aios/config.py src/aios/sandbox/backends/docker.py tests/unit/sandbox/ is empty — the substantive PR change is byte-identical to the previously reviewed revision. The entire delta since the last reviewed head is scripts/pooled_connection_lint.py, src/aios/services/wake.py, tests/integration/test_wake_session.py, tests/unit/test_wake_rate_limit_lock.py, all carried in by the master merge and touching no sandbox/config surface. Per the verification-proportionality instruction, the E2E/gVisor work reported in the PR body was not re-run; the cheap checks below were re-confirmed on this head.

Standing properties re-checked on this head

Prior rounds asserted no blocking properties (all four earlier reviews passed). I re-verified the acceptance criteria implicit in a deletion PR against the current tree rather than inheriting the earlier passes:

• The deleted branch was unreachable, so removing it cannot change any timeout. At the merge-base (8be49b3a), git grep throughput_bytes_per_second -- src/ returns only the parameter declaration, its own if/else, and the EWMA machinery — no call site in src/ ever passed it. Removing the parameter and its else arm is budget-neutral by construction.
• Both surviving branches are intact. _snapshot_timeout_s (docker.py:98–115) still yields max(_SNAPSHOT_TIMEOUT_FLOOR_S, size_walk_seconds * _SNAPSHOT_WALK_SAFETY_FACTOR) for the snapshot verb (docker.py:678) and max(floor, size * ns_per_byte) * safety_margin for save_image/load_image (docker.py:1061, 1071); the capped retry escalation still multiplies both.
• The deleted subsystem leaves no live reference. Tree-wide grep for sandbox_snapshot_throughput_ewma_alpha, sandbox_snapshot_throughput_state_path, _load_throughput, _record_throughput, _throughput_bytes_per_second, snapshot-throughput, SNAPSHOT_THROUGHPUT, throughput_bytes_per_second returns 11 hits, all inside tests/unit/sandbox/test_snapshot_timeout_calibration.py (negative assertions and its explanatory docstring). No SNAPSHOT_THROUGHPUT in any .md/.yml/.yaml/.env*/.tf/.sh, so the env-var removal strands no deployment documentation.
• Removed parameters were genuinely single-purpose. size_rw on _commit/_flatten and the monotonic() timing pairs fed only _record_throughput. The rw local in _snapshot_uncounted is still live for the disk gate (commit and flatten required sizing, docker.py:671–697), size_rw is still passed to _snapshot_timeout_s (docker.py:678–680), and the empty-writable-layer short-circuit (docker.py:654) is untouched. monotonic remains in use at docker.py:627/629 — the size-walk measurement that feeds the live branch — and json has 6 remaining uses, so no dead imports were left behind. Both _commit/_flatten call sites were updated in the same hunk.
• Tests are green and constrain the change. uv run pytest tests/unit/sandbox/test_snapshot_timeout_calibration.py test_snapshot_verb.py test_tmp_mount_and_disk_gate.py → 65 passed. The rewritten calibration module pins both live branches numerically (including that the walk branch ignores ns_per_byte/safety_margin, proven by setting them to absurd values), adds first-time contract coverage of the save_image/load_image timeout call sites, and adds guards that go red if the dead seam is reintroduced (TypeError on throughput_bytes_per_second=, not hasattr for the config fields and backend attrs). The two fixture edits only drop now-nonexistent settings and the parameters that existed solely to supply them.
• Static checks on changed files (cheap, so re-run on this merge head): ruff check clean, ruff format --check clean (45 files already formatted), mypy clean on docker.py and config.py.

Non-blocking observations (carried forward; still true, still not worth a fix cycle):

• sandbox_snapshot_timeout_safety_margin (config.py:268–272) still reads "Multiplier applied to throughput-derived snapshot budgets." It now applies only to the fixed per-byte estimate, so "throughput-derived" is vestigial — the sentence stays true (a per-byte rate is a throughput), just less precise than the sibling ns_per_byte description this PR corrected. Optional follow-up.
• docs/design/durable-session-sandboxes.md:300 still describes the commit budget as a "per-byte budget at ~10× measured throughput." That text was already stale before this PR — Sandbox salvage: derive commit timeout from measured throughput + escalate deterministic failures instead of looping fail-closed #2032's size_walk_seconds rewiring invalidated it, and that path is untouched here — so it is pre-existing drift, not a regression from this diff.
• Because Settings uses extra="ignore", deployments still setting the two removed env vars drop them silently rather than failing startup. That is the intended outcome for retiring dead knobs and is disclosed in the PR body alongside the stale-state-file cleanup step.

Not evaluated (and not required, per verification proportionality): the Docker-marked E2E suites and the gVisor runtime variant reported in the PR body. They exercise the walk-derived snapshot verb and disk-gate paths, which this diff does not modify; no removed code had a runtime-only effect that unit-level tests could conceal, since the deleted branch was unreachable from every production call site. The wake/lint changes carried in by the master merge are outside this PR's diff.

[eumemic-bot]

Code review

Verdict: pass

Reviewed 91a6217f025c7f75102b309af608d6808ca851e1 (confirmed git -C /mnt/review rev-parse HEAD matches). No blocking findings.

Scope of this round. Head is a new merge of master into the fix branch. git diff 96ad9485..HEAD -- src/aios/config.py src/aios/sandbox/backends/docker.py is empty, and the only delta under tests/unit/sandbox/ is the addition of test_secret_egress_proxy.py (193 lines), which arrives with src/aios/sandbox/secret_egress_proxy.py from the merged master commit and is not part of this PR's diff (the GitHub files list for #2367 contains exactly the same five files as the previously reviewed revision). The substantive PR change is therefore byte-identical to the revision reviewed at 96ad9485. Per verification proportionality, the E2E/gVisor work reported in the PR body was not re-run; the cheap checks below were re-confirmed on this head.

Standing properties re-checked on this head

Prior rounds asserted no blocking properties (all five earlier reviews passed). I re-verified the acceptance criteria implicit in a deletion PR against the current tree rather than inheriting the earlier passes:

• The deleted branch was unreachable, so removing it cannot change any timeout. At the merge-base (b3cdc118), git grep throughput_bytes_per_second -- src/ returns only the parameter declaration, its own if/else, and the EWMA machinery — no call site in src/ ever passed it. Removing the parameter and its else arm is budget-neutral by construction, not by assertion.
• Both surviving branches are intact. _snapshot_timeout_s (docker.py:98–115) still yields max(_SNAPSHOT_TIMEOUT_FLOOR_S, size_walk_seconds * _SNAPSHOT_WALK_SAFETY_FACTOR) for the snapshot verb (docker.py:678) and max(floor, size * ns_per_byte) * safety_margin for save_image/load_image (docker.py:1061, 1071); the capped retry escalation (retry_multiplier ** attempt, clamped to retry_cap) still multiplies both.
• The deleted subsystem leaves no live reference. Tree-wide grep for sandbox_snapshot_throughput_ewma_alpha, sandbox_snapshot_throughput_state_path, _load_throughput, _record_throughput, _throughput_bytes_per_second, snapshot-throughput, SNAPSHOT_THROUGHPUT, throughput_bytes_per_second returns 11 hits, all inside tests/unit/sandbox/test_snapshot_timeout_calibration.py (negative assertions and its explanatory docstring). Nothing in docs/, infra/, compose, or env templates, so the env-var removal strands no deployment documentation.
• Removed parameters were genuinely single-purpose. size_rw on _commit/_flatten and the monotonic() timing pairs fed only _record_throughput. The rw local in _snapshot_uncounted is still live for both disk-gate required computations (flatten docker.py:690, commit docker.py:697) and for projected_unique/the flatten trigger (docker.py:672); size_rw is still passed to _snapshot_timeout_s (docker.py:678) and still drives the empty-writable-layer short-circuit (docker.py:654). monotonic remains used at docker.py:627/629 — the size-walk measurement that feeds the live branch — and json has 6 remaining uses, so no dead imports were left behind. Both _commit/_flatten call sites were updated in the same hunk.
• Tests are green and constrain the change. uv run pytest tests/unit/sandbox/test_snapshot_timeout_calibration.py test_snapshot_verb.py test_tmp_mount_and_disk_gate.py → 65 passed. The rewritten calibration module pins both live branches numerically (including that the walk branch ignores ns_per_byte/safety_margin, proven by setting them to absurd values), adds first-time contract coverage of the save_image/load_image timeout call sites, and adds guards that go red if the dead seam is reintroduced (TypeError on throughput_bytes_per_second=, not hasattr for the config fields and backend attrs). The two fixture edits only drop now-nonexistent settings and the parameters that existed solely to supply them.
• Static checks on changed files (cheap, so re-run on this merge head): ruff check clean, ruff format --check clean (45 files already formatted), mypy clean on docker.py and config.py.

Non-blocking observations (carried forward; still true, still not worth a fix cycle):

• sandbox_snapshot_timeout_safety_margin (config.py:268–272) still reads "Multiplier applied to throughput-derived snapshot budgets." It now applies only to the fixed per-byte estimate, so "throughput-derived" is vestigial — the sentence stays true (a per-byte rate is a throughput), just less precise than the sibling ns_per_byte description this PR corrected. Optional follow-up.
• docs/design/durable-session-sandboxes.md:300 still describes the commit budget as a "per-byte budget at ~10× measured throughput." That text was already stale before this PR — Sandbox salvage: derive commit timeout from measured throughput + escalate deterministic failures instead of looping fail-closed #2032's size_walk_seconds rewiring invalidated it, and that path is untouched here — so it is pre-existing drift, not a regression from this diff.
• Because Settings uses extra="ignore", deployments still setting the two removed env vars drop them silently rather than failing startup. That is the intended outcome for retiring dead knobs and is disclosed in the PR body alongside the stale-state-file cleanup step.

Not evaluated (and not required, per verification proportionality): the Docker-marked E2E suites and the gVisor runtime variant reported in the PR body. They exercise the walk-derived snapshot verb and disk-gate paths, which this diff does not modify; no removed code had a runtime-only effect that unit-level tests could conceal, since the deleted branch was unreachable from every production call site. The secret_egress_proxy change carried in by the master merge is outside this PR's diff.