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
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.
The counters probe reported "ordinal" and "requests" as the same number: the ordinal is read at callback entry and nothing increments the callback count before the body is built, so the two fields were always identical. Report the ordinal alone and document that it is also the lifetime callback count. The probe reads counters from response headers, so no consumer depends on the JSON field. Restore the explicit guard deref in the Next.js script accumulator take.
…hLab/trusted-server into 856-fastly-reusable-sandbox
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 2, against 147655e3. All six findings from the previous pass are resolved, and each was re-derived from the code and the Fastly SDK rather than taken from the replies. Two new findings, both introduced by the round-1 fixes themselves.
Verified at this head in a reviewer worktree: cargo test-fastly-reuse 215 passed / 0 failed (up from 211, the new regressions), cargo clippy-fastly (--all-features --all-targets -D warnings) clean, ./scripts/test-cli.sh 499 passed / 0 failed. All 20 CI checks pass.
Round-1 findings, all confirmed fixed
- Cacheable counter headers - fixed with the right mechanism. Requiring both
privateandno-storethrough the existing quote-awarecache_control_headers_have_directive, evaluated after terminal effects. The two regressions cover the cases a naivecontains()would fail: field-qualifiedprivate="set-cookie"and a quoted extension containingprivate, no-store. - Discarded
init_loggererror - fixed, narrowly scopedeprintln!with an explicit clippy allowance and reason; diagnostics stay pending until installation succeeds. with_max_memory- implemented beyond what was suggested. Now a required fourth bound, and excluding theu32::MAXsentinel is a correct catch I had missed: the SDK doesheap_memory_snapshot_mib().unwrap_or(u32::MAX)(fastly-0.12.1/src/http/serve.rs:304-310), so a bound ofu32::MAXwould let an unsupported reading fail to retire the guest. Excluding it makes unavailability retire conservatively.AppStatedoc comment - fixed, now describes opt-in retention accurately.IP_CIDR_SOURCE_CACHEtest - added, and it is a real test: 32 requests varying method, path, query, IP and ASN with the TTL forced to zero, asserting the key set stays at exactly the two configured sources. That is the config-vs-traffic assertion the design doc promised.- Stale pin SHA in the body - fixed, now matches
Cargo.toml.
Also worth noting: the results doc states plainly that the earlier measurements were not rerun under the new private/no-store restriction, rather than leaving stale numbers implying continued validity.
This round
Both findings follow from the finding-1 fix. The first is the blocking one: that fix added a second reason counters can be absent, and the diagnostic whose whole job is to explain their absence still names only the first.
One inline comment below carries a one-click
suggestion. It was scratch-verified in an isolated worktree:cargo fmt --all -- --checkclean,cargo clippy-fastlyclean, all 499 CLI tests pass (includingabsent_counters_report_unverified_not_negative, which asserts on this exact string), and the post-verification drift check found no difference.
Blocking
wrench
- Probe's "no counters" hint is now incomplete and misdirects operators - see inline at
crates/trusted-server-cli/src/commands/dev/sandbox_probe.rs:261
Cross-cutting / body-level findings
- 📝 One forward-looking doc reference still names the superseded revision -
docs/superpowers/specs/2026-09-17-fastly-reusable-sandbox-results.md:510reads "Move to a release tag once one contains76c59b44." The tree now pinsc4841b60, two revisions later, so a tag containing76c59b44would not necessarily contain what ships. The other76c59b44mentions in these docs are correctly historical ("Runtime observations at76c59b44", "service_scoped_runtime_env_keyis private at76c59b44") and should stay as they are - this one is guidance about a future action, so it should point at the current pin. Not suggestion-eligible: the docs are outside this diff's RIGHT-side hunks.
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
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 3, against 037fd5a8. Both round-2 findings are fixed. The substantive event this round is the merge of main's Next.js streaming refactor (#1135), which rewrote html_processor.rs, deleted html_post_process.rs, added rsc_stream.rs, and required hand-resolving seven conflicts, including all three files that carry this PR's cross-request disclosure fix. That reconciliation was the focus.
The merge resolution is correct. Main replaced the Next.js accumulation path with its own capture_fragment / rsc_stream::document_state, dropping this PR's ScriptTextAccumulator for that one file. The disclosure fix still holds, because main's document_state() keys through the same per-document IntegrationDocumentState::get_or_insert_with. The property is now upheld by two mechanisms: GTM keeps ScriptTextAccumulator, Next.js uses main's state machine. I confirmed this by running the regressions rather than reading the diff:
test integrations::google_tag_manager::tests::an_interrupted_document_leaves_no_residue_for_the_next_document ... ok
test integrations::nextjs::script_rewriter::tests::an_interrupted_document_leaves_no_residue_for_the_next_document ... ok
Also verified intact after the merge: the round-1 cache gate on attach_sandbox_counters, the four-bound resolve_mode, with_max_memory wiring, the logger eprintln! fallback, and the IP_CIDR_SOURCE_CACHE key-space test.
Verified at this head: cargo test-fastly-reuse 215 passed / 0 failed, cargo clippy-fastly (--all-features --all-targets -D warnings) clean, all 20 CI checks pass.
Round-2 findings, both fixed
- Probe hint - the suggested bytes were applied verbatim; the diagnostic now names both causes.
- Forward-looking revision reference - now points at
c4841b60. The five remaining76c59b44mentions are the historical ones ("Runtime observations at76c59b44", "service_scoped_runtime_env_keyis private at76c59b44"), which are correct as they stand.
This round
Two findings. Neither is a defect this PR introduced; the first is inherited from main and the second is fallout from the merge. Both are recorded here because they touch the mechanism this PR exists to protect.
Neither finding carries a one-click
suggestion. The first needs a state machine ported across files; the second needs a judgment call about which references stay historical. Scratch verification therefore had nothing to apply, and the results quoted above are of the PR head as-is.
Blocking
wrench
- Design doc cites a buffer that no longer exists after the merge - see inline at
docs/superpowers/specs/2026-09-17-fastly-reusable-sandbox-design.md:256
thinking
- GTM script accumulation is unbounded while Next.js is now capped - see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:1023
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
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Round 4 record, against 75d218aa. Approved separately; this comment carries the findings and the verification behind them, and its threads are resolved.
Round-3 findings, both closed
- Design doc citation - rewritten in past tense and anchored to the revision the audit was performed at (
c56ff745e), noting the Next.js buffer was subsequently replaced by per-documentrsc_stream::FragmentStatein #1135. Line 566 now names both mechanisms explicitly rather than "two movedaccumulated_textbuffers". - GTM unbounded accumulation - documented at design doc lines 291-294 and tracked as #1224, which is open.
The pin move, checked at the contract level
c4841b60 to 35a72835 could have invalidated the framework contract earlier rounds rested on. It did not:
lifecycle.rsis semantically unchanged across the move. The only delta is an added#[must_use]onserve_custom. Lazy successful-only retention, counter ordering, andsetup_onceretry semantics all still hold.request.rschanges are purely additive: one new public function (into_core_request_with_registries), no signature change to anything this adapter calls.RUNTIME_ENV_STORE_NAMEis intact at the same value, soread_raw_limitsstill resolves.- The body's
Cargo.lockclaim holds exactly. Diffing the lockfile with edgezerosourcelines excluded yields zero other changes: no version drift, no transitive churn.
All seven fixes from earlier rounds survived this round's main merge: four-bound resolve_mode, with_max_memory, the counters cache gate, the logger stderr fallback, the probe hint, the IP_CIDR key-space test, and ScriptTextAccumulator. A scan of the core and adapter diff for newly introduced module-scope mutable state found none.
Verification at this head
- Both cross-request leak regressions pass, run explicitly rather than inferred:
integrations::google_tag_manager::tests::an_interrupted_document_leaves_no_residue_for_the_next_document ... ok
integrations::nextjs::script_rewriter::tests::an_interrupted_document_leaves_no_residue_for_the_next_document ... ok cargo test-fastly-reuse: 219 passed, 0 failed, matching the body's figure.cargo clippy-fastly(--all-features --all-targets -D warnings): clean.- All 20 CI checks pass.
Finding
- ⛏ Pin commit message understates its scope and carries no body - see inline at
Cargo.toml:60. Recorded rather than blocking; the information exists in the PR body, just not wheregit logwill keep it.
This branch has not been deployed
4 participants
Summary
reusable-sandboxCargo feature (whichfastly.tomldoes not pass) and four explicit bounds (requests, lifetime, wait timeout, memory) 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.52942ba8bdd2e9316385853bb317f80ae01c4841b60.ea7c450aad4e4d2c6A 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
35a72835322fe0127beb2ce998e4988e95974373onfeat/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.EdgeZero dependency update
Advance all six workspace pins and eight lockfile sources to repaired EdgeZero PR #379 head
35a72835322fe0127beb2ce998e4988e95974373. No transitive dependencies change. Update the current dependency guidance while preserving historical runtime evidence.The new revision adds registry-aware Fastly conversion and typed Cloudflare/Spin dispatch internally. The adapter entry points used here remain compatible. Trusted Server keeps its custom raw Fastly conversion and application-managed stores, preserving response extensions and progressive streaming without introducing additional required KV bindings.
Current update validation
Validated commit
75d218aadlocally:cargo metadata --lockedconfirms all eight EdgeZero packages resolve to the new revision; no other lockfile entries changed.Runtime benchmarks and deployed eviction/memory behavior were not remeasured for this pin update.
Closes
Closes #856
Review fixes
Cache-Control: private, no-store; cacheable responses preserve their policy and omit counters. Probes of these routes report unverified observations. Both feature configurations use this rule.TS__SANDBOX__MAX_MEMORY_MIBin1..u32::MAX. Existing three-key configurations fall back to single-request operation until updated. The SDK checks memory between callbacks; this is retirement, not a per-request allocation ceiling. Unsupported snapshots cause retirement.AppStatedocuments opt-in sandbox-lifetime retention. A regression checks that varied traffic and expired-entry refreshes leave the CIDR cache keyed only by configured store/key pairs.Earlier review-fix validation
AGENTS.mdCI gates pass: Rust formatting, all six target-matched clippy aliases, Fastly feature-off and feature-on tests, Axum/Cloudflare/Spin tests, cross-adapter parity, JS build/tests/format and docs format.Test plan (original branch validation)
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
These measurements predate the review fixes. They have not been rerun with the fourth runtime bound and restricted counter channel.
The middle row is the control: the serving loop alone buys almost nothing. The gain is retention.
Historical validation 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, with an approved narrow stderr fallback when logger installation fails