Conversation
Fastly SDK 0.12.1 exposes `fastly::http::serve::Serve`, which lets one Wasm
sandbox serve several requests. Adopt it behind a `reusable-sandbox` feature
that is off by default, so the shipped entry point is unchanged.
Split `main` into a loop owner and `handle_request`. The handler keeps sending
its own response and returns `()`, which the SDK's `HandlerResult` impl treats
as already sent, so progressive streaming, duplicate `Set-Cookie` headers,
response extensions, and post-send pull sync are untouched. No application
state is retained yet; that follows in a later change.
Three execution modes, only the first identical to today:
feature off -> original entry path
feature on, effective max_requests <= 1 -> single-request handler path
feature on, validated reuse limits -> Serve loop
Bounds come from the `edgezero_runtime_env` config store. They cannot be read
through `runtime_env_config`, whose `runtime_env_keys` allowlist drops every
key outside the adapter, logging, and per-store selectors; routing them there
would read as absent and silently disable reuse while passing every test. The
adapter opens the store itself and builds the service-scoped key from
`service_id()`, since edgezero's own key helper is private. Reads use
`try_get`, never `get`: `get` panics on a lookup error, and this runs before
the health probe. Any failure resolves to single-request operation.
Reuse needs all three bounds. A bare request limit is refused because the SDK
reads an omitted lifetime and wait timeout as `Duration::MAX`, and an
application-level `0` normalizes to `1` because the SDK reads
`with_max_requests(0)` as unlimited.
Guard the global logger. `fern`'s `apply()` panics when a logger is already
installed, which a reused sandbox would hit on its second request. Startup
diagnostics are deferred and flushed once the logger exists, since limit
resolution runs before there is anywhere to log.
Add measurement so the lifecycle can be evaluated rather than assumed. A
`sandbox_metrics_enabled` debug flag attaches instance id, request ordinal,
build count, and correlation id to workload responses before headers commit;
post-commitment stream failures carry the same context in logs. The flag is
skipped while false because `DebugConfig` denies unknown fields and a default
blob must stay readable by an older binary. The counters endpoint is feature
gated and short-circuits ahead of application construction so polling cannot
perturb the build count it reports. Response counters stay feature independent
so the feature-off baseline is measurable on the same channel.
Add `ts dev sandbox-probe`, which issues a keep-alive sequence and reports
reuse only from strictly increasing ordinals under one instance id. Repeated
or decreasing ordinals, incomplete attribution, a missing instance identity,
and transport errors all report unverified rather than a negative result.
`test-fastly` does not pass `--all-features` and CI invokes it directly, so
add a `test-fastly-reuse` alias and CI step; clippy compiling the feature is
not the same as running its tests.
`GoogleTagManagerIntegration` and `NextJsNextDataRewriter` accumulate inline script fragments across `lol_html` chunk boundaries, draining only when `is_last_in_text_node` arrives. Both held that buffer as a `Mutex<String>` field on the rewriter itself. The registry stores rewriters as `Arc<dyn IntegrationScriptRewriter>`, registered once at build time, so the buffer lives as long as the registry. When a document's stream ends before the final fragment — client disconnect, origin error, truncated body — its partial script stays in the buffer, and the next document through that registry prepends the residue to its own accumulation. That corrupts the response and can disclose the previous document's content. Today each Fastly sandbox serves one request, so the buffer is destroyed before anything else can observe it. Retaining the registry across requests is what makes it reachable, so this has to land before retention does. Add `ScriptTextAccumulator` and store it in `IntegrationDocumentState`, which is already constructed per document and already threaded through `IntegrationScriptContext`. Keyed per integration id, so each integration gets its own buffer and it is dropped with the document. The rewriter objects become stateless. `NextJsRscPlaceholderRewriter` deliberately does not accumulate and is unaffected. Both regression tests drive one rewriter through two documents, interrupting the first mid-accumulation, and assert the second carries no trace of it. Against the previous code they fail with the first document's content prepended to the second document's response.
Build the application lazily, once per sandbox, and keep it in `Sandbox`. This is what actually amortizes the initialization the earlier commits made safe to share: settings load and parse, secret resolution, auction plan compilation, orchestrator and integration registry construction, and the telemetry sink. The `OnceLock` regex caches in `settings.rs` now survive past the request that populated them. Construction sits behind the health, counters, and JA4 short-circuits, so none of those pays for it. A failed build is never retained. `build_app_with_state` returns an error router with no state; that serves the current request and is dropped, so a transient config-store failure cannot pin the sandbox into permanent error mode, and the next request retries. The attempt is still counted, so a retry loop shows up in the build counter rather than hiding. Nothing request-scoped is retained. The config store handle, client info, device signals, TLS metadata, `RuntimeServices`, correlation id, EC finalize state and request filter effects are all still built per request. Everything reachable from the retained `AppState` was audited as config-derived, and the script accumulation buffers that were not are fixed in the preceding commit. With no reuse configured this changes nothing observable: a single-request sandbox builds once, serves once, and exits. Retention does not remove the per-request Ed25519 signing key parse, which runs from `RequestSigner::from_services` against the per-request `RuntimeServices` inside the retained orchestrator. That needs its own measurement and rotation decision.
Issue #856 asked for CPU and memory observations alongside build counts and latency. Add them to the sandbox counters: cumulative guest vCPU milliseconds from `elapsed_vcpu_ms` and the heap snapshot from `heap_memory_snapshot_mib`, each reported as `unsupported` rather than a zero when the host lacks the call, so an unsupported counter is never mistaken for an idle one. Take the instance id from `compute_runtime::sandbox_id()` instead of reading `FASTLY_TRACE_ID` directly. The SDK documents that function as the per-sandbox identifier and resolves it to `FASTLY_TRACE_ID` on wasm32-wasip1, so this is the same value by the supported API, and it keeps working on the component path where the environment variable does not exist. Record the local A/B/C measurement in the results document: commands, commit ids, configuration, raw observations, and limitations. Headline: retention takes builds per sandbox from 6 to 1. Reuse without retention does not (arm B rebuilds on all 6). A reused request is 11.7x faster than the feature-off baseline at p50 on an edge-only route, and about 3.9x averaged over a full sandbox once the one build is amortized. Progressive delivery, duplicate Set-Cookie, and request isolation are each verified on observed reused instances. Failed builds are confirmed not retained; recovery after a failure in the same sandbox is covered only at unit level, because Viceroy's stores are fixed for a guest's lifetime. Long-lived memory behaviour and deployed eviction remain unverified, and the six-requests-per-sandbox figure is a repeated local observation under Viceroy 0.17.0 rather than a demonstrated universal ceiling.
The recovery test drove the `Sandbox` API by hand: it incremented the build counter and inserted an application itself, never invoking the production build/reuse/retry decision. It would have passed with that decision broken, so it was not evidence for the recovery claim made in the results. Extract the decision into `Sandbox::resolve_app`, which takes the builder as a closure. `edgezero_main` now calls it instead of open-coding the same logic, and the tests drive it with an injected builder. Two tests replace the hand-driven one. Reuse: build once, then three requests whose builder panics if called. Recovery: a failing build, then a succeeding one, then a third request whose builder panics if called — so recovery is shown to be both reachable and durable. Both were checked against deliberate mutations of `resolve_app` (never reusing the retained app, and swallowing the failed build) and fail as intended. Results document corrections: - Recovery is described as unit-level only, naming the new test and why an end-to-end same-sandbox recovery is not locally reproducible. - The vCPU and heap numbers are labelled pre-send cumulative samples. They are read before headers commit, so the first excludes that request's remaining work, consecutive differences straddle two requests, and the heap figure is linear memory including host buffering rounded to MiB, not Rust heap usage. - The aggregate comparison uses like-for-like sample means (3.872 / 0.934 ≈ 4.15x) instead of a modelled six-request average against a median. The modelled amortization is kept but labelled as a model. - Resource-measurement provenance is recorded: those headers landed in f51ba6f, not in the arm C commit, they were read with curl rather than the probe, and the latency experiment was not rerun on that revision. Also drop the stale "no application state" comment on `Sandbox`, which stopped being true when retention landed.
`ts dev sandbox-probe` builds on all non-wasm hosts and writes through `crate::output`, but that module was gated to macOS because the macOS-only `ts dev proxy` was its first consumer. On Linux the probe therefore referenced a module that did not exist, and CI failed with `cannot find output in crate` in three jobs. Widen the gate to `not(target_arch = "wasm32")`, matching `commands`, `run`, and the crate's other host-only modules. Nothing in `output` is platform specific: it is a `println!` / `eprintln!` wrapper and the only place in the crate permitted to write to the console. This was not caught locally because the host-target CLI clippy was run with the macOS triple; CI hardcodes `x86_64-unknown-linux-gnu`.
`output::warn` had no caller outside the macOS-gated proxy, so widening `mod output` to every host target left it dead on Linux and CI failed with `function warn is never used`. The probe should have been warning anyway. A transport error invalidates the run, and the report goes to stdout, so a caller piping stdout to a file would otherwise lose that signal entirely. Emit it on stderr as well. An earlier draft of the probe did call `warn`; the rewrite onto hyper replaced that path with the `Ending` enum and dropped the call without noticing.
Move all six EdgeZero dependencies from `tag = "v0.0.8"` to `rev = "277544c431c1ab9bafa14a45d5f35975b5587e97"` on `feat/reusable-app-lifecycle`. The lockfile changes 16 lines, all of them the `source` field of the eight edgezero packages; no other dependency moves, one distinct edgezero source resolves, and no `v0.0.8` reference survives. The repin is deliberately not for the `Serve` re-export. That type comes from the already-pinned `fastly 0.12.1` SDK, and EdgeZero's own custom-lifecycle contract says not to repin merely to swap the import; the entry point still calls `fastly::http::serve::Serve` directly. The pin is for three fixes at that revision: push/diff validation scoped to the selected adapter, secret references redacted from Spin diagnostics, and duplicate response headers preserved on Cloudflare. The first two are the CLI defects found while measuring this branch, where a Spin naming rule blocked a Fastly push and the error printed the rejected reference. No local code was removed. The contract allows removal only where a public EdgeZero API now provides equivalent behaviour; at this revision `service_scoped_runtime_env_key` is still private and `runtime_env_keys` is still a closed allowlist, so the local key construction remains necessary. Every `edgezero-core` change across the range is test-only and the Fastly adapter change is additive, so nothing we depend on moved. Re-verified fresh against this revision rather than carrying prior results: default stays single-request with limits configured, health and debug probes still bypass construction, a reused sandbox holds one successful build, failed initialization is retried every request and never retained, request state stays isolated, delayed chunks reach the client about two seconds before the origin finishes, duplicate Set-Cookie and finalization survive, and three consecutive post-commitment stream failures on one sandbox each produced exactly one response before a normal request succeeded on that same sandbox. EdgeZero's own Fastly compatibility suite passes at this checkout, including the custom-lifecycle arms. Documentation records the dependency diff, the CLI checks performed with synthetic values, the fresh observations, and the remaining gaps: in-guest initialization recovery is unit-level only for this application, Linux verification stays CI-only, comparative latency was not rerun, and the pin is an unmerged branch revision that should move to a release tag once available.
Repin EdgeZero to `76c59b440fb35d1317dcb3fa8c1172161e3f5309` on `feat/reusable-app-lifecycle`, which adds `edgezero_adapter_fastly::lifecycle`, and use it instead of this adapter's own equivalents. The lockfile moves only the eight edgezero packages' `source` fields; one distinct source resolves and no other dependency changes. Deleted here, now owned by the framework: local `struct Sandbox` -> `lifecycle::Sandbox<RetainedApp>` `resolve_app`/`retain_app`/`retained_app` -> `Sandbox::initialize` `ensure_logger` + `logger_installed` -> `Sandbox::setup_once` `begin_request`/`record_build`/... -> `requests()`/`initialization_attempts()` `Serve::new()…run_with_context(…)` -> `serve_custom` / `run_custom` `initialize` retains only success. A failed build hands its error router back as the error payload, which serves the current request and is dropped, so the next callback retries. Request ordinals now come from the framework, which counts a callback before invoking it, so probe short-circuits advance the ordinal without attempting construction — and no local counter shadows it. `logging::init_logger` returns `Result` rather than panicking on the install path, so `setup_once` marks setup complete only after a successful install. That allows a retry; it does not by itself make retrying safe, since `setup_once` rolls nothing back. It is safe here because neither failure mode leaves the process partially configured: a failed builder installs nothing, and a failed `apply()` means a global logger already exists so the retry fails identically. The function's docs record that reasoning. `serve_custom` owns the `Sandbox` and drops it when serving ends, so the retirement line reports snapshots the callback captured rather than reading the sandbox afterwards. `serve_app` is deliberately not adopted: its response conversion buffers streams, which would end progressive delivery and leave no place for response-extension finalization. Application-owned and unchanged: feature gate, kill switch, limit parsing and fallback, settings retention and refresh, health/JA4/metrics routing, fresh per-request metadata, handles, services, extensions, bodies and correlation ids, raw request conversion, router dispatch, response-extension finalization, progressive streaming, duplicate Set-Cookie, post-send work, and per-document rewrite-buffer isolation. One new local type, `StartupDiagnostics`, holds messages produced by limit resolution before any logger exists; it cannot live in the framework `Sandbox`, which exposes no slot for application state other than the retained payload. Verified fresh at this revision on macOS: feature-off stays single-request with limits configured; lazy start with six requests and one build; probes counted but not constructing; failed initialization retried five times and never retained; request isolation; duplicate Set-Cookie and finalization; progressive delivery about two seconds ahead of origin completion; and two post-commitment failures each producing exactly one response before a normal request succeeded on the same sandbox. EdgeZero's own Fastly compatibility suite exits 0 at this checkout. Limitations recorded in the results document: same-sandbox initialization recovery stays unit-level because Viceroy's stores are fixed for a guest's lifetime; Linux remains CI-only and is not claimed to pass from these macOS runs; long-lived memory and deployed eviction stay unverified; no comparative latency was rerun at this revision. The pin is an unmerged branch revision and should move to a release tag once one contains it.
`serve_loop` snapshotted `initialization_attempts()` before invoking the callback. Initialization happens during the callback, so a build performed by the final callback was missing from the retirement line. The count itself was never lost — it lives in EdgeZero's `Sandbox` until serving ends — but the snapshot was stale by the time that callback finished. `requests()` on entry was already correct: `lifecycle::Sandbox::handle` increments its callback count before invoking the handler, so the value read on entry is the current callback's 1-based ordinal. Extract `RetirementCounters::observe`, which reads `requests()` on entry and `initialization_attempts()` on exit, and wrap every callback in it. Extracting rather than reordering one line means the regression tests drive the same method production uses, instead of restating the ordering in a test where it could drift. Both values are still read from EdgeZero's counters; nothing here increments anything. Three regressions cover it: a successful build on the final callback, a failed build on the final callback, and that the request snapshot mirrors the framework counter rather than deriving one. The failed case matters because the application state is not retained, so the attempt count is the only record that the build happened. Reintroducing the entry-read ordering fails the first two. `RetirementCounters` is gated to the reuse feature, matching `scoped_key` and `sandbox_metrics_response`, so the default build stays warning-free. Also record a follow-up on `sandbox::scoped_key`, which reproduces EdgeZero's private runtime-store key format. A public key-construction or lookup helper would remove the duplication; the working implementation stays until one exists, and the `TS__SANDBOX__*` suffixes and limit-validation policy remain application-owned either way.
prk-Jr
changed the title
`SANDBOX_METRICS_PATH` and `sandbox_metrics_response` were gated `#[cfg(any(feature = "reusable-sandbox", test))]`, but their only caller is the short-circuit gated on the feature alone. A feature-off test build therefore compiled both without a caller, and CI failed on `dead_code`. Gate them on `feature = "reusable-sandbox"` to match their call site. The other `any(feature, test)` gates in this module stay: `resolve_mode`, `collect_raw_limits`, `scoped_key`, the key constants and `RetirementCounters` are all exercised by tests that run in both feature configurations. This did not reproduce locally because CI's `cargo test` job uses `actions-rust-lang/setup-rust-toolchain`, which defaults `RUSTFLAGS` to `-D warnings`. The `test-*` aliases carry no such flag, so warnings that fail CI are merely printed locally. Verified this fix under `RUSTFLAGS="-D warnings"` across every test alias, the CLI suite, the parity suite, and all clippy invocations.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
An opt-in reusable-sandbox lifecycle for the Fastly adapter, gated twice over (Cargo feature plus three runtime bounds) so the shipped build is unchanged. The commit sequence is readable in order, the design and results docs are candid about what was not established, and the cross-request disclosure fix correctly lands in its own commit ahead of the retention that makes it reachable.
Verified independently in a reviewer worktree at 17f80ae0: cargo check -p trusted-server-adapter-fastly --target wasm32-wasip1 --features reusable-sandbox clean, cargo test-fastly-reuse 211 passed / 0 failed, cargo clippy-fastly (--all-features --all-targets -D warnings) clean. The framework contract in the pinned EdgeZero lifecycle.rs and the SDK's run_with_context were read directly; the ordinal and attempt-count semantics the PR relies on hold as documented.
Two hypotheses were chased and did not hold, so they are not reported as findings: dynamic-backend registration growth under reuse is already mitigated by canonicalize_transport_timeout_ms quantization (with a test asserting at most 16 distinct values, explicitly "well under the dynamic backend limit"), and StartupDiagnostics is bounded at three messages pushed only in main.
One blocking finding, on the counters channel rather than the lifecycle itself.
No inline comment below carries a one-click
suggestion. Each fix needs a design decision or targets a file with no RIGHT-side hunk, so all are prose. Scratch verification (applying suggestion bytes in a worktree and re-running the gates) therefore had nothing to verify and was not run; the verification quoted above is of the PR head as-is, not of any proposed patch.
Blocking
wrench
- Per-request counter headers can be cached and replayed to other users - see inline at
crates/trusted-server-adapter-fastly/src/main.rs:222
Non-blocking
init_loggerfailure is silently discarded - see inline atcrates/trusted-server-adapter-fastly/src/main.rs:176with_max_memoryis the one available bound left unused - see inline atcrates/trusted-server-adapter-fastly/src/main.rs:127
Cross-cutting / body-level findings
-
♻️
AppStatedoc comment now asserts the opposite of what this PR makes true -crates/trusted-server-adapter-fastly/src/app.rs:174-177still reads: "In Fastly Compute each request spawns a new Wasm instance, so this struct is effectively per-request." That is precisely the invariant this PR removes, andAppStateis the struct now retained across requests byRetainedApp. A reader who trusts this comment will assume per-request isolation that no longer holds under the reuse path. The file has no RIGHT-side hunk in this diff, so this cannot be a suggestion; please update the comment to describe the sandbox-lifetime reality and note that retention is opt-in. -
📝 A test the design doc commits to does not exist -
docs/superpowers/specs/2026-09-17-fastly-reusable-sandbox-design.md:527-531states the test plan assertsIP_CIDR_SOURCE_CACHE's "key space is config-derived and not traffic-derived." GreppingIP_CIDRacrosscrates/returns onlyprotection_scope.rsitself, so no such test exists. The cache does appear genuinely config-bounded (keyed by{config_store, key}fromDataDomeConfig), so this reads as a commitment gap rather than a defect. Worth resolving either way, because it is a module-scopeHashMapwith no eviction sweep whose caching becomes real for the first time under reuse: either add the test the plan names, or amend the plan to say why it was dropped. -
📝 PR body names a stale EdgeZero pin - the body says the pin is
76c59b440fb35d1317dcb3fa8c1172161e3f5309, but the tree pinsc4841b609ec366ebabd3489416e2cb8c1359f61d(Cargo.toml:58-63). Commits385853bb3and17f80ae01moved it after the body was written, and the body's commit table omits both. The body is the reviewer's map for a branch meant to be read commit by commit, so please refresh it. The body's own note that an unmerged branch revision should move to a release tag before merge still stands and is the right call.
CI Status
All 20 reported checks PASS. No failures, cancellations, or pending checks.
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo test (axum native): PASS
- cargo test (cloudflare native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CLAUDE.md symlink guard: PASS
| /// before `stream_to_client`. The counters therefore describe the request up | ||
| /// to commitment and cannot report its eventual outcome; a failure after | ||
| /// commitment is recorded in logs instead and reconciled during analysis. | ||
| fn attach_sandbox_counters(response: &mut HttpResponse, counters: &SandboxCounters) { |
There was a problem hiding this comment.
🔧 wrench - Per-request counter headers can be cached and replayed to other users.
attach_sandbox_counters runs after apply_terminal_response_effects (see send_edgezero_response at main.rs:614 then main.rs:628-630) and inserts x-ts-sandbox-request-id, x-ts-sandbox-instance, and x-ts-sandbox-ordinal onto every response. main.rs:531 is the universal terminal send path, so publicly cacheable asset-proxy and publisher HTML responses go through here too. There is no Vary, no cache-privacy downgrade, and no exclusion for shared-cacheable responses — grepping x-ts-sandbox across the tree returns no cache handling for these names anywhere.
This is not confined to the reuse build. SandboxCounters::capture (sandbox.rs:163-176) is not feature-gated; only debug.sandbox_metrics_enabled controls it, and settings.rs:2560-2564 documents that independence as deliberate so the feature-off baseline can be measured on the same channel. So the shipped, feature-off binary emits these headers whenever the config flag is on — which is exactly the configuration the results doc uses.
Failure scenario. With debug.sandbox_metrics_enabled = true, a response carrying public, s-maxage=3600 (a proxied asset, or publisher HTML on a cacheable route) is stored in the Fastly cache with request A's unique x-ts-sandbox-request-id and x-ts-sandbox-instance. Every subsequent client served from that cache entry receives request A's correlation id for the next hour. That is a per-request identifier crossing users, and it silently corrupts the measurement channel these headers exist to provide: the probe in sandbox_probe.rs reads ordinals from exactly these headers, and a cached response replays a stale ordinal that verdict() would score as a repeated-ordinal anomaly.
Proposed fix (apply manually — needs a design decision, so it is not expressed as a suggestion). Pick one:
// Option A - attach only to responses that are already private/no-store.
// Cheapest and matches the "debug channel" intent.
if let Some(counters) = counters.as_ref()
&& trusted_server_core::response_privacy::is_private_no_store(&response)
{
attach_sandbox_counters(&mut response, counters);
}
// Option B - attach unconditionally, but force the response private first,
// so a measurement run never populates a shared cache entry.
if let Some(counters) = counters.as_ref() {
trusted_server_core::response_privacy::enforce_private_no_store(&mut response);
attach_sandbox_counters(&mut response, counters);
}Option B changes cache behaviour while measuring, which is itself a measurement confound worth stating in the results doc. Option A keeps caching intact and simply yields no counters on cacheable routes — the probe already handles absent counters correctly (sandbox_probe.rs:260-281 reports unverified rather than a negative), so A degrades cleanly. Either way, please add a regression test alongside the existing cache-privacy tests in main.rs (late_filter_effects_cannot_make_an_assembled_response_public and siblings) asserting a cacheable response never carries x-ts-sandbox-request-id.
| // Marked complete only once installation succeeds, so a failed install is | ||
| // retried on a later callback. `setup_once` rolls nothing back, so that is | ||
| // only correct because `init_logger` is harmless to repeat — see its docs. | ||
| if sandbox.setup_once(crate::logging::init_logger).is_ok() { |
There was a problem hiding this comment.
🤔 thinking - The init_logger failure is discarded without a trace.
setup_once returns Result<(), String> and this only tests .is_ok(), so the error string is dropped. The PR deliberately converted these two panics into errors (logging.rs:91-117, with a careful doc comment on why retry is safe), and then the one caller throws the value away.
Both failure modes are diagnostically interesting. A failed Logger::builder().build() means the tslog named endpoint is misconfigured; a failed fern::Dispatch::apply() means something else already installed a global logger. In either case every subsequent callback in the sandbox retries and fails the same way, invisibly, and startup.flush() never runs — so the diagnostics explaining why reuse was declined (sandbox.rs:333-356, the config-store and service-id messages) are silently stranded for the sandbox's whole life. That is precisely the situation where an operator most needs them.
Logging is unavailable by definition at this point, which is the obvious objection to logging the error. But init_logger already sets echo_stdout(true), and the surrounding code path can still reach stdout, so a bare eprintln! is a genuine escape hatch here even though println!/eprintln! are otherwise banned by AGENTS.md — this is the one place where the logger provably does not exist.
Proposed fix (apply manually — an eprintln! in production code warrants an explicit #[allow] with a reason, so this is better as a deliberate edit than a one-click suggestion):
// The logger is unavailable by definition here, so this is the one place
// stderr is the only channel. A failed install repeats every callback, and
// the startup diagnostics stay stranded until it succeeds.
match sandbox.setup_once(crate::logging::init_logger) {
Ok(()) => startup.flush(),
Err(e) => {
#[allow(clippy::print_stderr, reason = "no logger exists to report this")]
{
eprintln!("logger installation failed, retrying next callback: {e}");
}
}
}| // performed by the final callback is included. | ||
| let mut counters = RetirementCounters::default(); | ||
|
|
||
| let summary = edgezero_adapter_fastly::lifecycle::serve_custom( |
There was a problem hiding this comment.
🌱 seedling - with_max_memory is the one available bound this loop leaves unset.
serve_loop sets with_max_requests, with_max_lifetime, and with_timeout, but not with_max_memory. In fastly-0.12.1/src/http/serve.rs:225-232 that field defaults to 0, and 0 is documented as unlimited — the loop's memory check at serve.rs:306-314 is skipped entirely when it is zero. So a reused sandbox has no heap ceiling at all.
This lines up directly with the PR's own leading limitation: "Long-lived memory behaviour and deployed eviction: unverified. Viceroy admits ~6 requests per guest, so this cannot be established locally." The SDK ships a purpose-built rung for exactly that risk, and it is the one bound not being used. It also fits the existing policy shape rather than fighting it: resolve_mode (sandbox.rs:223-244) already refuses reuse unless all bounds are present, precisely so an omitted bound cannot silently become Duration::MAX. A memory bound omitted here becomes unlimited by the same mechanism the function was written to prevent.
Not blocking, and reasonable to defer — the feature is off by default and cannot engage without explicit configuration. But since the memory question is the headline unknown before any deployment, a fourth TS__SANDBOX__MAX_MEMORY_MIB key threaded through RawLimits / SandboxLimits / resolve_mode would let the first deployed experiment start bounded rather than open-ended. Worth deciding before this is switched on in a real service, not after.
Note the heap value is already being read on this path — heap_mib() at main.rs:293-298 calls heap_memory_snapshot_mib(), the same accessor the SDK's own limit check uses — so the reporting side is in place and only the bound is missing.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Approved on the condition that the outstanding requested changes in the existing review are addressed before merge. This approval does not waive those requests or resolve their threads.
Reviewed 17f80ae01cb98e5a590a9381d8944f8036b7b7f9 against 6cae7f5da8911c746cf873581885f90c3820dd96, including the pinned EdgeZero lifecycle implementation and affected callers and consumers. No additional actionable findings beyond the existing feedback.
Validation
cargo test-fastly-reuse --locked: 211 tests passed.cargo test-fastly --locked an_interrupted_document_leaves_no_residue_for_the_next_document: both regressions passed.cargo test-fastly --locked sandbox_metrics: both serialization tests passed.cargo test -p trusted-server-cli --target x86_64-unknown-linux-gnu --locked: 98 tests passed, including the 11 sandbox-probe tests.cargo build-fastly --locked --features reusable-sandbox,cargo fmt --all -- --check, andcargo clippy-fastly: passed.- Independent Viceroy 0.17.0 runtime check: health and counters probes performed no application build; four subsequent workload requests shared one build, preserved separate cookies and request markers, and delivered initial bytes approximately 0.36 seconds before origin completion.
- All 20 reported CI checks passed.
Deployed eviction and long-lived memory remain unverified. Same-sandbox initialization recovery is unit-tested, not established end to end. Post-commit stream-failure recovery was traced but not independently exercised in this review.
3 participants
Summary
reusable-sandboxCargo feature (whichfastly.tomldoes not pass) and explicit bounds in the runtime config store. Absent or partial configuration stays single-request, so the shipped build is unchanged.What changed, in order
The branch is meant to be read commit by commit.
c56ff745emainsplit, logger guard, measurement counters,ts dev sandbox-probe.00c4e4278ca7e7fa7af51ba6f510b447f804dd74fa202,66529f8f1outputmodule on Linux).b6df0b3e9277544c4(CLI + Cloudflare fixes).281c89a98edgezero_adapter_fastly::lifecycle, deleting our equivalents. Pin76c59b44.52942ba8bA cross-request disclosure fix, worth reading on its own
GoogleTagManagerIntegrationandNextJsNextDataRewriteraccumulated inline script fragments in aMutex<String>on the rewriter, which the registry holds for its whole lifetime. If a document's stream ended before its final fragment — client disconnect, origin error, truncated body — the partial script stayed in that buffer and the next document prepended it, corrupting the response and potentially disclosing the previous document's content.This is latent today only because each sandbox serves one request and then dies. Retention is what makes it reachable, so it is fixed in its own commit (
00c4e4278) ahead of retention. Both regression tests fail against the previous code with the first document's content prepended to the second's response.What EdgeZero owns vs what we own
After
281c89a98the framework owns lazy successful-only retention, the callback count, the initialization-attempt count, the one-time setup guard, and the serving wrappers. Deleted here:struct Sandboxlifecycle::Sandbox<RetainedApp>resolve_app/retain_app/retained_appSandbox::initializeensure_logger+logger_installedSandbox::setup_oncebegin_request/record_build/ …requests()/initialization_attempts()Serve::new()…run_with_context(…)serve_custom/run_customStill ours, deliberately: feature gate, kill switch, limit parsing and fallback; settings retention and refresh policy; health/JA4/metrics routing; fresh per-request metadata, handles, services, extensions, bodies and correlation ids; raw request conversion and router dispatch; response-extension finalization; progressive streaming; duplicate
Set-Cookie; post-send work; per-document rewrite-buffer isolation.serve_appis not adopted: its response conversion buffers streams, which would end progressive delivery and leave nowhere for response-extension finalization.Dependency
EdgeZero pinned by SHA to
76c59b440fb35d1317dcb3fa8c1172161e3f5309onfeat/reusable-app-lifecycle.Cargo.lockmoves only the edgezero packages'sourcefields; no other dependency changes. The pin is an unmerged branch revision and should move to a release tag once one contains it.Closes
Closes #856
Test plan
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test-fastly-reuse— new alias;test-fastlydoes not pass--all-features, so feature-on tests would otherwise never runcargo fmt --all -- --checkLocal evidence
The middle row is the control: the serving loop alone buys almost nothing. The gain is retention.
Verified on observed reused sandboxes: progressive delivery (first byte ~2 s ahead of origin completion), duplicate
Set-Cookie, request isolation with distinct per-request metadata, probes not constructing the app, failed initialization retried and never retained, and post-commitment stream failures producing exactly one response before a normal request succeeds on the same sandbox.Full method, raw observations and provenance:
docs/superpowers/specs/2026-09-17-fastly-reusable-sandbox-results.md.Limitations — deployed behaviour is not established
Rollback
No rung is immediate — limits are read at sandbox startup, so a running sandbox retires on the limits it started with.
Checklist
unwrap()in production codelogmacros (notprintln!)