feat: add Lambda SnapStart support - #831
Conversation
Switch from the git-branch dependency to the published lambda_http/lambda_runtime 1.3.0 from crates.io, removing the release blocker. Also fix the integration test body-reader helpers to accept the BoxBody response type, and resolve a clippy ok().expect() lint.
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
…er%0A) canonicalize_hook_path treated a control byte as undecidable and returned None, which matches_hook_path turned into pass-through — but a router like Starlette still resolves /snapstart/after%0A (decoded /snapstart/after\n) to the hook route (Python $ matches before a trailing newline), leaving the state-mutating hook externally reachable. Strip control bytes during canonicalization so the path collapses onto the hook and is blocked; malformed percent-escapes still pass through (no /reports/100% false 403). Adds %0A/%0a/%0d%0a/%00 regression cases.
Remove Bug Fixes entries for issues introduced and fixed within this PR (missing TracingLayer on the new concurrent-runtime path, the control-byte hook-guard bypass, and the configured-hook-path set_path normalization / matrix-param gaps — all in code this PR adds), and the pool_max_idle_per_host(0) 'restore' which nets to no change versus the last release. Keep only the genuinely pre-existing AWS_LWA_REMOVE_BASE_PATH fix, plus the SnapStart features.
…e guard The hook-path guard had two fail-open holes, both reachable on the FastAPI examples this PR ships. 1. A configured path that could not be canonicalized (a malformed % escape) fell back to HookTarget::Raw, which compared raw strings on both sides. With AWS_LWA_SNAPSTART_AFTER_RESTORE_PATH=/snapstart/after%, a request for /snapstart/after%25 did not match the raw configured string and passed through, while uvicorn/Starlette unquoted it onto the very same route — so the one case the Raw branch existed to protect is the case it failed to protect. 2. Configuring that route the correct way (/snapstart/after%25, canonical `after%`) left the bare-% spelling reachable: an undecidable request path passes through the guard, but Starlette still resolves it onto the route. Verified end-to-end: POST /snapstart/after% -> 200, handler ran. hook_target now returns Result and rejects both classes, so Adapter::new fails initialization with an actionable error rather than starting with a state-mutating route reachable. HookTarget goes away entirely, and with it matches_hook_path's raw-compare arm. Rejecting a literal % is what makes the request-side pass-through provably safe rather than incidentally safe, on any framework and without modelling per-framework decoding: an undecidable request path is either rejected by the router outright (Node throws URIError, so Express answers 400; Go and Spring likewise) or decoded leniently into a path containing a literal % or U+FFFD (Python's unquote) — and neither can equal a %-free hook route. The pass-through itself is unchanged, so /reports/100% still takes no false 403.
fd885e7 to
ca56fd9
Compare
hook_target returned Ok(None) for a configured path that canonicalizes to the
root ("/", "//", "/..", "/.", "/foo/..", "/%2f"), silently disabling the guard.
But after_restore POSTs the RAW configured path — it reads after_restore_path,
not the guard target — so the hook still fired at "/". The two diverged with no
diagnostic: with AWS_LWA_SNAPSTART_AFTER_RESTORE_PATH=/.. the adapter POSTs to
"/" on every restore, which is a 405 on both FastAPI examples (they declare only
`@app.get("/")`), and post_hook treats any non-2xx as fatal — so every restore
failed and nothing explained why.
Reject it instead. The guard cannot cover the root without returning 403 for all
normal traffic, and the docs require a hook path "your normal application traffic
does not use", which the root never is. Same rule as the % cases: if the adapter
cannot guard the route, it refuses to run with it rather than starting up with a
state-mutating route reachable or a hook that fails every restore.
Unset and empty still mean "no hook" and are unaffected.
…r SnapStart build_client applied pool_max_idle_per_host(0) whenever AWS_LAMBDA_INITIALIZATION_TYPE=snap-start, and that variable stays set for the whole lifetime of a restored environment. Both call sites went through it, so the client rebuilt in after_restore -- the one Adapter::client() returns for every invocation after a restore -- also never retained a connection. The configured idle keep-alive was therefore a no-op on exactly the functions this feature targets, and every invocation opened a fresh TCP connection to the inner app for the life of the environment, consuming a file descriptor each time against Lambda's limit. The snapshot hazard only applies to the client built BEFORE the snapshot. A client built inside after_restore starts with an empty pool and cannot hold a snapshotted connection, so it is safe for it to pool normally. build_client no longer reads the environment; the caller decides, so the post-restore rebuild cannot silently inherit the pre-snapshot restriction. Adapter::new passes Duration::ZERO under SnapStart via the new base_client_idle_timeout, which disables idle keep-alive for the pre-snapshot client -- measured equivalent to pool_max_idle_per_host(0), including for back-to-back requests. The configured value is retained on Adapter::pool_idle_timeout and used for the after-restore rebuild. This keeps the pre-snapshot client safe by construction, so a consumer driving the Service impl directly (who never triggers the after-restore hook) is still protected against hyper#3810, and it removes the post-restore path's dependence on AWS_LAMBDA_INITIALIZATION_TYPE. Also makes the SnapStartHooks::pool_idle_timeout field comment true: the post-restore client now really does honor the configured value.
…ness branch
Findings from a final systematic pass over the branch.
1. hook_target short-circuits on configured.is_empty() and returns Ok(None) ("no
hook"), but Adapter::new stored the raw Some(""), which run() hands to
SnapStartHooks. before_snapshot/after_restore then took their `if let
Some(path)` branch and called post_hook(.., ""), and Url::set_path("") yields
"/" -- so the adapter POSTed to the unguarded application root on every
lifecycle event (405 on both FastAPI examples, which post_hook treats as
fatal). This is the same guard-versus-POST divergence the root-collapse
rejection closed; "" slipped past by returning before canonicalization.
Adapter::new now normalizes an empty hook path to None before anything reads
it, so both sides agree by construction and the documented "empty means unset"
semantics are preserved. Only reachable via a directly constructed
AdapterOptions -- env-derived options already drop empties.
2. readiness::wait_until_ready drives Retry::spawn over an unbounded
FixedInterval, so it can only return ready or never return. Its bool, and the
`if !ready` branches plus "readiness check failed" errors in
check_readiness_with_timeout / check_readiness_unbounded, were unreachable.
Removed; wait_until_ready now returns (). check_init_health's ready_at_init
comes from whether the wait COMPLETED within its bound, which is what the
value always meant. Documented that an unbounded post-restore wait holds the
restore open until Lambda's own timeout, with the escalating "app is not ready
after {}ms" log as the adapter-side signal, and that
AWS_LWA_READINESS_CHECK_TIMEOUT_SECONDS converts that into /restore/error.
Doc corrections, no behavior change:
- canonicalize_hook_path's rustdoc listed a control/null byte as a None case, but
control bytes are stripped and canonicalization continues -- they WIDEN the
blocked class. Stale in the fail-open direction. The same paragraph still
described the HookTarget::Raw fallback deleted in ca56fd9, and the guard
comment in fetch_response repeated the control-byte claim.
- build_client / base_client_idle_timeout claimed Duration::ZERO means no
connection is "retained". It does not: hyper's pool stays enabled
(Config::is_enabled is max_idle_per_host > 0), so the socket is still parked and
captured in the snapshot; ZERO guarantees it is evicted on checkout rather than
reused. Documented the real guarantee, and the init-time reconnect cost it
carries (27 connections per 300ms of readiness polling versus 1) -- confined to
init, and unchanged from the pool_max_idle_per_host(0) behavior it replaced.
- SnapStartHooks::client is used for the before-checkpoint hook only;
after_restore deliberately uses the fresh client.
- The guide's rejection rule said "a path containing a percent sign", stricter
than the code, which rejects only when the DECODED form contains one --
/snapstart/%61fter is accepted and guarded as /snapstart/after.
- Two public-docs-link-to-private-item rustdoc warnings; cargo doc --no-deps is
now clean.
- Unused `import os` in the zip example.
…g it be50614 replaced pool_max_idle_per_host(0) with pool_idle_timeout(Duration::ZERO) for the pre-snapshot client, and claimed the two were "identical on reuse". They are not, in exactly the scenario the original workaround was written for. A zero idle timeout leaves hyper's pool ENABLED (Config::is_enabled() is max_idle_per_host > 0), so the connection is parked in the idle map and reuse is decided at checkout by `now.saturating_duration_since(idle_at) > timeout`. That saturates to ZERO when the recorded instant is ahead of `now`, and ZERO > ZERO is false -- so the entry counts as fresh and is handed out. A monotonic clock that has not advanced across a restore is precisely the condition hyper#3810 / rust-lang/rust#79462 describe, so the guarantee rested on the very clock the workaround exists to distrust. is_closed() does not catch it either: the app process was restored from the same snapshot and never sent a FIN. Under run() this is masked, because after_restore publishes a fresh client before any invocation. The only exposed path is a consumer driving the Service impl directly -- which is the sole reason the pre-snapshot restriction exists, so the protection was vacuous for its only beneficiary. build_client now takes an explicit Pooling parameter: Disabled sets pool_max_idle_per_host(0) (pool off, no clock consulted) and Adapter::new uses it under SnapStart via base_client_pooling; the after-restore rebuild passes Enabled and keeps the configured AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS, so everything gained in be50614 on the invocation path is retained. test_adapter_new_client_never_pools_under_snapstart could not catch this: it sleeps 40ms between requests, so elapsed() is non-zero and it passes either way. The new test_pre_snapshot_client_pool_is_disabled_not_merely_expiring observes the connection's lifetime instead -- whether the socket is dropped or parked after one request -- which no clock reading can satisfy.
2033c84 to
81fadbd
Compare
There was a problem hiding this comment.
Code Review Results
Reviewed: 986113f..81fadbd
Files: 27
Comments: 3
Most items from the prior review rounds are now resolved in the code I read: check_init_health returns Err on sync-init timeout (src/lib.rs, and src/main.rs propagates it), Duration::try_from_secs_f64 replaces the panicking constructor, hook_target rejects root-collapsing and %-bearing configured paths at init, both sides of the guard now normalize through Url::set_path, control bytes are stripped-and-blocked rather than passed through, pool_max_idle_per_host(0) is restored for the pre-snapshot client via base_client_pooling(), and the duplicated guard comment block is gone. The remaining findings are narrow.
Comments on lines outside the diff:
[src/lib.rs:1446] [GENERAL] hook_target fails initialization for hook paths it cannot guard (root-collapsing, literal %), but it does not detect a hook path that collides with pass_through_path. Because the pass-through rewrite runs before the guard:
if matches!(request_context, RequestContext::PassThrough) && parts.method == Method::POST {
path = self.pass_through_path.as_str();
}
// ... guard runs on this rewritten pathsetting AWS_LWA_SNAPSTART_AFTER_RESTORE_PATH=/events (the default pass-through path) makes every non-HTTP trigger event get rewritten onto the guarded route and answered with 403 instead of being delivered to the app — silently, with only a per-invocation warn!. Given the init-time validation already exists for the other unguardable cases, rejecting a hook path equal to pass_through_path there would be consistent and cheap.
…h; warn on bad env values Three findings from the latest bot pass. 1. A hook path equal to AWS_LWA_PASS_THROUGH_PATH was accepted, but the pass-through rewrite in fetch_response replaces `path` with pass_through_path for a PassThrough POST BEFORE the guard runs. So configuring the hook at /events -- the default pass-through path -- made every non-HTTP trigger event canonicalize onto the guarded route and get a 403 instead of reaching the app, silently, with only a per-invocation warn!. Adapter::new now rejects a hook path that resolves to the same route as the pass-through path, alongside the existing unguardable cases, and compares canonical routes so /Events, /events/, /./events and /%65vents are caught too. 2. duration_secs_from_env silently fell back to the default on any unparseable value -- the exact failure mode readiness_check_timeout_from_env was written to avoid. The two sibling variables also accepted different numeric formats: AWS_LWA_READINESS_CHECK_TIMEOUT_SECONDS took fractional seconds while AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS took only whole ones, so `0.5` and `4.5` parse-failed into 4s with no diagnostic. It now accepts fractional seconds like its sibling and warns on any set-but-unusable value. 3. The request path was canonicalized twice per invocation whenever both hook paths are configured -- the configuration both examples and the guide use -- because each matches_hook_path call canonicalized independently. New matches_any_hook_path canonicalizes once and compares against both targets; semantics are identical, and the no-hooks case still short-circuits before canonicalizing. matches_hook_path becomes the #[cfg(test)] single-target form. The guide's rejection list gains the pass-through case, and Adapter::new's # Errors section now names all four unguardable hook-path conditions.
Regression from de0ea31. The pass-through collision check ran let pass_through_target = hook_target(&domain, &Some(pass_through_path))?; unconditionally, so a pass_through_path that hook_target rejects aborted Adapter::new -- with a SnapStart-flavored error -- even when no hook was configured and there was therefore no guard and nothing to collide with. AWS_LWA_PASS_THROUGH_PATH is read straight from the environment with no prior validation, and "/" is a plausible setting for an app that handles non-HTTP event payloads at its root; it collapses to the root and so was rejected. The same applied to a pass-through path whose decoded form contains a literal % or a malformed escape. Existing tests missed it because they all use the default /events, which canonicalizes cleanly. The check now runs only when a hook target exists, and treats an unguardable pass-through path as "no collision" rather than an error. That is sound, not just lenient: hook targets are canonicalizable and non-empty by construction, so a request rewritten onto a root-collapsing, %-bearing, or non-canonicalizable pass-through path can never canonicalize onto one -- the guard would pass it through anyway.
There was a problem hiding this comment.
Code Review Results
Reviewed: 986113f..f6679db
Files: 27 (see note on src/lib.rs)
Comments: 4
│ Note: .lambda-tooling-pr-reviewer/diff/file_src_lib.rs.json contains "(no patch available; status=modified)", and the workspace copy of src/lib.rs is the pre-PR state (no snapstart, build_client, canonicalize_hook_path, or restored_client symbols in it). The largest change in this PR (+1689/-94) therefore could not be read. Findings 1–3 below are prior-round issues on that file that the author never explicitly dismissed; they are re-raised unverified, and each notes what would resolve it.
`let _ = self.restored_client.set(fresh.clone())` discarded the "already set" case and then used `fresh` for steps 2 and 3. If the cell were already populated, the hook POST and the readiness check would run over a client no request can reach, so the restore would report healthy on the basis of something the request path never touches -- with no signal anywhere. New publish_or_adopt returns whichever client invocations will actually use: `fresh` when it wins the race, otherwise the already-published one. That removes the divergence rather than merely reporting it, and warns so an unexpected second lifecycle run is visible in logs. Latent today, since lambda_runtime drives the restore lifecycle once. The test pins the property directly via Arc::ptr_eq -- a second call must return the first client, not its own -- so it cannot become real.
There was a problem hiding this comment.
Code Review Results
Reviewed: 986113f..928bc8c
Files: 26 of 27 (see note)
Comments: 4
Note on coverage: .lambda-tooling-pr-reviewer/diff/file_src_lib.rs.json contains "patch": "(no patch available; status=modified)", so the largest change in this PR (src/lib.rs, +1689/-94 — the hook guard, env parsing, build_client, register_and_run, fetch_response) could not be read. The workspace copy of src/lib.rs is the pre-PR state (no snapstart, Pooling, build_client, or canonicalize_hook_path symbols present), so it is not a substitute. Comments 1–3 are therefore re-raises of prior findings on that file that carry no author response; they are based on the code quoted in the earlier threads, not on the current head, and should be re-verified against it. Everything else (src/snapstart.rs, src/readiness.rs, src/main.rs, tests/integ_tests/main.rs, both examples, docs, Cargo.toml/Cargo.lock, CHANGELOG.md) was reviewed in full and I found no further issues.
register_extension_internal built its RAPID client with default pooling, so a connection to the Runtime API could be parked in the idle pool and captured in a SnapStart snapshot. That is the same hazard lambda_runtime handles by calling reset_pool() on its own RAPID client during the restore lifecycle -- but nothing resets or re-establishes this one, and register_default_extension terminates the process with exit(1) when its request fails, so handing out a dead connection after restore would kill the restored environment before it serves anything. Pooling costs nothing to give up here: the client issues exactly two requests, `register` and then the long poll for the first extension event, and the long poll's in-flight connection is unaffected by the idle-pool setting. Disabled unconditionally rather than gated on AWS_LAMBDA_INITIALIZATION_TYPE, since there is no case where reuse helps. Extracted as runtime_api_client so the property is testable; the test observes connection lifetime (dropped vs parked), and I confirmed it fails against a default-pooled client before the change.
There was a problem hiding this comment.
Code Review Results
Reviewed: 986113f..db37e17
Files: 27 (16 reviewed in detail; lock file, .gitignores, and event fixtures skipped)
Comments: 1
A caveat before the findings: .lambda-tooling-pr-reviewer/diff/file_src_lib.rs.json contains "patch": "(no patch available; status=modified)", so the diff for src/lib.rs (+1729/−95) was not available, and the working-tree copy is the pre-PR state. That file holds the hook guard, canonicalize_hook_path, the env-var parsing for the two new timeouts, and the reworked Adapter::new/fetch_response. I could not verify or refute the unresolved prior findings scoped to it (the Duration::from_secs_f64 overflow path, the configured-vs-request canonicalization asymmetry, the root/pass-through collapse guards, AdapterOptions not being #[non_exhaustive]). Several of those look addressed judging by src/main.rs, the integ-test changes, and the new docs — the cold-start timeout now propagates (check_init_health().await?) and the guide documents startup rejection of root-collapsing, percent-ambiguous, and pass-through-colliding hook paths — but that is inference from adjacent files, not verification. Re-run the review with the src/lib.rs patch present before treating it as reviewed.
The pre-PR comment explaining why pooling was disabled under SnapStart was the only record of the reason, and it was deleted. Restore it with the measurement that settles it, taken from a SnapStart container function deployed from this branch: across the restore: monotonic +0.54s while wall +161s after the restore: monotonic +6.079s / +6.059s vs wall +6.1s / +6.0s CLOCK_MONOTONIC does not advance across the snapshot gap, but never goes backwards, and after the restore it tracks wall time exactly. So the anomaly is confined to the boundary, which is what makes the two sites correct in opposite directions: - Adapter::new (pre-snapshot) must have the pool OFF. hyper decides reuse with `now.saturating_duration_since(idle_at) > idle_timeout`, so an entry pooled before the snapshot reads as ~0.5s idle after restore however long the snapshot sat -- fresh, and dead. No idle timeout fixes that, including Duration::ZERO, since ZERO > ZERO is false. This is hyper#3810 / rust-lang/rust#79462. - after_restore may have the pool ON. Every entry it holds is post-boundary, where accounting is reliable; verified live with idle gaps longer than the configured 4s keep-alive all succeeding. This is also the only way AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS affects the invocations that serve traffic. Comment-only; no behavior change.
The guide and both example READMEs warn that the 403 hook guard only exists while the adapter is in the request path; the top-level README presented the guard without it, and it is the most widely read of the four. Its own opening advertises that the same image runs on EC2, Fargate and local machines -- exactly the deployments where the hook routes are reachable and unauthenticated. All four docs now carry the caveat.
Findings from the final systematic pass. 1. before_snapshot was the only path into the application that was not readiness-gated. With AWS_LWA_ASYNC_INIT=true, check_init_health gives up at 9.8s and returns Ok(()) with ready_at_init=false so the app can keep booting; run() then drives snapstart_lifecycle straight into before_snapshot, which POSTed immediately. For an app that has not bound its port the POST fails at once with ECONNREFUSED -- the 60s HOOK_TIMEOUT never applies to a refusal -- and lambda_runtime reports it to /init/error, so publishing the SnapStart version fails with what looks like an application bug. That is exactly the slow-booting app async_init exists for. Both hooks now go through ensure_ready, bounded by AWS_LWA_READINESS_CHECK_TIMEOUT_SECONDS when set. The timeout error names the phase, so an initialization failure is distinguishable from a restore failure. check_readiness_with_timeout / check_readiness_unbounded collapse into ensure_ready / ensure_ready_with_timeout. 2. The five pooling tests mutated the process-global AWS_LAMBDA_INITIALIZATION_TYPE with directly conflicting expectations, so under plain `cargo test` -- the runner a contributor following the standard Rust workflow uses, where nextest's per-test process isolation does not apply -- they could interleave and invert each other's assertions. Those are the assertions guarding the snapshot-connection hazard, so a silent inversion is the worst case. Four of them now pass Pooling explicitly to build_client and touch no environment at all. The env-var side collapses into one test, test_base_client_pooling_from_env, per the convention the neighbouring env-mutating tests already follow; verified it fails when the decision is inverted. 3. AdapterOptions::pool_idle_timeout's rustdoc and the CHANGELOG said AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS takes "whole seconds", left stale by the commit that made it accept fractional seconds and pinned 0.5 -> 500ms in a test. 4. CHANGELOG now records the lambda_http 1.1.1 -> 1.3.0 bump and that it changes the Cookie header on every deployment: a multi-entry API Gateway v2 cookies array is joined with "; " instead of ";", so apps see `a=1; b=2`. RFC 6265 form and universally accepted, but it is a plain-request-path change that was undocumented.
There was a problem hiding this comment.
Code Review Results
Reviewed: 986113f..85ad8cb
Files: 27
Comments: 3
Comments on lines outside the diff:
[src/lib.rs:0] [SECURITY] Re-raising (previously flagged, no author response): the hook-path guard's equivalence class does not include path parameters (matrix variables). The guard canonicalizes percent-encoding, collapses // and ./.., and compares case-insensitively — the guide enumerates exactly these — but a ;-suffixed segment is kept verbatim by the url crate, so:
request: POST /snapstart/after;x=1
set_path: /snapstart/after;x=1 (';' left in the path)
guard: != /snapstart/after -> forwarded
Jakarta Servlet containers and Spring strip ;-parameters from each segment before routing, so a request the guard forwards can still land on the hook handler. That is a reachable, unauthenticated, state-mutating route on the frameworks this repo supports (examples/springboot, examples/springboot-zip, examples/javalin-zip). Suggest truncating each segment at the first ; inside canonicalize_hook_path so both sides land in the same equivalence class.
[src/lib.rs:0] [SECURITY] Re-raising (previously flagged, no author response): the guard fails open on the request side, which contradicts its own documented contract. canonicalize_hook_path's rustdoc states:
/// Returns None only for genuinely undecidable inputs (a malformed % escape,
/// non-UTF-8 after decoding, or a control/null byte). The caller treats None
/// as "reject" (fail closed).
but on the request side None is turned into "not the hook" and the request is forwarded. Configured-side None is rejected at startup (that part is fail-closed and correct); the request side is the half that matters for the guard, and there the undecidable branch admits the request instead of returning 403. Even if no concrete bypass spelling exists for a given framework today, a security guard whose comment says "fail closed" while the code fails open is very likely to be broken by a future edit. Either return 403 on None for request paths, or correct the doc comment to state that the request side deliberately fails open and why.
[src/lib.rs:0] [BUG] Re-raising (previously flagged, no author response): AWS_LWA_READINESS_CHECK_TIMEOUT_SECONDS (and the same pattern for AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS) can panic at startup on an out-of-range value. As quoted in the earlier thread:
.filter(|secs| secs.is_finite() && secs >= 0.0)
.map(Duration::from_secs_f64)Duration::from_secs_f64 panics on overflow, not just on NaN/infinity/negatives. AWS_LWA_READINESS_CHECK_TIMEOUT_SECONDS=1e20 parses as a finite positive f64 above Duration's max (~1.8e19 s), so it passes the filter and panics — the adapter aborts during init on a mistyped env var rather than warning and falling back, which is the behavior the README and guide both promise ("a set-but-<= 0 or malformed value is ignored with a warn!"). Use Duration::try_from_secs_f64 (or bound the value) and route the error into the existing fallback-with-warn! path. Note: I could not read the current src/lib.rs to confirm this line survives at HEAD — if the parser was already switched to try_from_secs_f64, this is resolved and can be dismissed.
…cation The guard's summary line and a test-section banner called it "strict, fail-closed" while the detailed doc a dozen lines below correctly explains that the REQUEST side forwards an undecidable path rather than 403-ing it. Only the configured side fails closed, and the review bot has now twice read the summary, concluded the request side returns 403 on doubt, and re-raised it as a contradiction -- quoting doc text that was already replaced. The risk is not the confusion itself but the obvious "fix" it invites: turning the request-side pass-through into a 403 would reinstate the /reports/100%25 false-403 regression that pass-through exists to prevent. Both labels now state which half does which, the summary points at the reasoning rather than summarizing it away, and the term is gone from the codebase entirely (the remaining historical use in a test comment is reworded to "undecidable branch"). Comment-only; no behavior change.
The extension registers with `{"events": []}` and then parks on
GET /2020-01-01/extension/event/next forever. Nothing said why, so the standing
question "the long poll is not re-established after a SnapStart restore -- is that
a leak or a crash risk?" kept coming back.
It is neither, and the empty events list is the reason. Being registered at all is
the entire point: Lambda only delivers SIGTERM at environment shutdown when an
extension is registered, which is what the graceful-shutdown feature relies on.
Subscribing to nothing means RAPID has no event to deliver, so the long poll is
never going to resolve -- parking on it IS what keeps the extension alive.
Under SnapStart that request is captured mid-flight in the snapshot and never
re-established, which is harmless for the same reason, and RAPID's registration
state is part of the snapshotted microVM so the effect survives even though the
connection does not.
Verified on a deployed SnapStart container function: the restored environment
received SIGTERM 457s after restore, in the same log stream that logged
after_restore, and the failure path that would exit(1) never fired across ~8
minutes and five restores. A non-SnapStart control on $LATEST behaved identically
apart from the clocks (monotonic_delta == wall_delta on a normal cold start, versus
0.38s against 184.3s across a restore).
Re-registering after restore would be robustness, not a fix.
Comment-only; no behavior change.
There was a problem hiding this comment.
Code Review Results
Reviewed: 986113f..db2eaba
Files: 27
Comments: 3
Comments on lines outside the diff:
[.lambda-tooling-pr-reviewer/diff/file_src_lib.rs.json:1] [GENERAL] The diff payload for src/lib.rs — by far the largest change in this PR (+1787/-95, including the SnapStart hook guard, the env-var parsing for the two new timeout variables, build_client/register_and_run, and the AWS_LWA_REMOVE_BASE_PATH semantics change) — is empty:
{"filename": "src/lib.rs", "status": "modified", "additions": 1787, "deletions": 95,
"patch": "(no patch available; status=modified)"}The workspace checkout is the pre-PR state (no src/snapstart.rs, check_init_health still returns ()), so the post-change content of that file is not available anywhere in the workspace. The two findings below are therefore based on code quoted verbatim in the earlier review threads rather than on the current file, and coverage of src/lib.rs in this pass is incomplete — please re-run the diff generation for it before treating this review as complete.
2. [BUG] src/lib.rs — readiness_check_timeout_from_env
Re-raising (previously flagged, no author response): the parser rejects <= 0 and unparseable values but not finite values outside Duration's range, and Duration::from_secs_f64 panics on overflow, not just on NaN/infinity/negatives.
match trimmed.parse::<f64>() {
Ok(secs) if secs <= 0.0 => None, // intentional unbounded, no warning
...AWS_LWA_READINESS_CHECK_TIMEOUT_SECONDS=1e20 parses as a finite positive f64 above Duration::MAX (~1.8e19 s), so the conversion panics during AdapterOptions construction and the function fails to initialize with a panic rather than the documented warn!-and-ignore path. The new docs in this PR only promise handling for <= 0 and malformed input:
a set-but-<= 0 or malformed value is ignored with a warn!
Use Duration::try_from_secs_f64 (or bound the value before converting) and route the out-of-range case into the same warn!-and-fall-back branch as a malformed value. The same applies to AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS if it shares the conversion.
3. [SECURITY] src/lib.rs — canonicalize_hook_path / matches_hook_path
Re-raising (previously flagged, no author response on this specific point — the author's replies addressed the separate normalization-divergence thread, not the fail-open direction): the guard's documented contract and its behavior on the request side point in opposite directions.
/// Returns None only for genuinely undecidable inputs (a malformed % escape,
/// non-UTF-8 after decoding, or a control/null byte). The caller treats None
/// as "reject" (fail closed).
but on the request side None is turned into "this is not the hook path" and the request is forwarded to the app. For configured values the PR does fail closed (startup rejects them, per docs/guide/src/features/snapstart.md), which makes the asymmetry easy to miss in future edits: a request path whose decoded form contains a control byte, or that carries a spelling the canonicalizer deliberately does not model (e.g. a matrix/path-parameter suffix such as /snapstart/after;x=1, which Url::set_path leaves intact but several frameworks strip before routing), skips the 403 and reaches a state-mutating route.
Two options: either make the request side match the doc — treat None as "reject" and return 403 — or, if forwarding undecidable paths is deliberate, correct the rustdoc so it states that only the configured side fails closed, and say why forwarding is safe. As written, the comment asserts a property the code does not have on the path that faces untrusted input.
Not re-raised, as the author explicitly dismissed them: the unconditional post-restore readiness check, the hook-path normalization-divergence thread, publish = false, PackageType: Image + SnapStart in the container example, and the unreleased 1.1.0 image / layer 30 pins in both examples. Several other earlier findings (the 0-means-unbounded and cold-start-failure documentation, the AWS_LWA_REMOVE_BASE_PATH doc update, the guide's missing "adapter not in the request path" warning, the contradictory example README wording, and the Adapter::new SnapStart pooling branch) appear addressed in the current diffs.
1 participant
Summary
Adds Lambda SnapStart support to the Lambda Web Adapter.
What it does
src/snapstart.rs: registers a SnapStart resource with the Lambda runtime andbridges the before-checkpoint / after-restore lifecycle to the inner web app over HTTP.
AWS_LWA_SNAPSTART_BEFORE_CHECKPOINT_PATHandAWS_LWA_SNAPSTART_AFTER_RESTORE_PATH.not reachable by external callers.
restored_client(write-onceOnceLock) so no pre-snapshot connections are reused.build_client()extraction,register_and_run()dedup of therun()arms,fetch_responsereturnsBoxBody<Bytes, Error>.Docs & examples
examples/fastapi-snapstart(container image) andexamples/fastapi-snapstart-zip(zip),both wiring the hook env vars through the SAM template.
Testing
cargo build— cleancargo test— 78 tests pass (4 e2e tests ignored, as they require deployed infrastructure)cargo clippy --all-targets— cleanNote:
nextestwas unavailable in this environment, so tests ran viacargo test -- --test-threads=1to preserve the env-var isolation the SnapStart config tests rely on.