Skip to content

Commit 81fadbd

Browse files
committed
fix(snapstart): disable the pre-snapshot pool instead of zero-expiring 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.
1 parent 02ddf85 commit 81fadbd

2 files changed

Lines changed: 147 additions & 47 deletions

File tree

‎src/lib.rs‎

Lines changed: 142 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -832,52 +832,65 @@ pub struct Adapter<C, B> {
832832
/// idle-connection keep-alive, resolved from [`AdapterOptions::pool_idle_timeout`]
833833
/// (env `AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS`, default 4 seconds).
834834
///
835-
/// `Duration::ZERO` means a pooled connection is never *reused*: hyper's pool stays
836-
/// enabled (`Config::is_enabled()` is `max_idle_per_host > 0`), so a finished
837-
/// connection is still parked in the idle map, but `Expiration::expires` compares
838-
/// `elapsed > timeout` and so evicts it at the next checkout instead of handing it
839-
/// out. No idle-eviction task is spawned either (`spawn_idle_interval` returns early
840-
/// on `Duration::ZERO`), so nothing closes it proactively. [`Adapter::new`] uses
841-
/// `ZERO` for the pre-snapshot client under SnapStart — see
842-
/// [`base_client_idle_timeout`]. This function itself reads no environment: the
843-
/// caller decides, so the post-restore rebuild cannot silently inherit the
844-
/// pre-snapshot restriction.
845-
fn build_client(idle_timeout: Duration) -> Client<HttpConnector, Body> {
835+
/// When `pooling` is [`Pooling::Disabled`] the client sets
836+
/// `pool_max_idle_per_host(0)`, which turns hyper's pool off outright
837+
/// (`Config::is_enabled()` is `max_idle_per_host > 0`): a finished connection is
838+
/// dropped rather than parked, so reuse is impossible *by construction*.
839+
///
840+
/// That distinction is load-bearing, and a zero `idle_timeout` is NOT a substitute.
841+
/// With the pool enabled, reuse is decided at checkout by
842+
/// `now.saturating_duration_since(idle_at) > timeout`; that saturates to `ZERO` when
843+
/// the recorded instant is ahead of `now`, and `ZERO > ZERO` is false, so the entry
844+
/// is treated as fresh and handed out. A monotonic clock that has not advanced
845+
/// across a restore is exactly the condition hyper#3810 / rust-lang/rust#79462
846+
/// describe — so an expiry-based scheme would depend on the very clock the
847+
/// workaround exists to distrust.
848+
///
849+
/// This function reads no environment: the caller decides, so the post-restore
850+
/// rebuild cannot silently inherit the pre-snapshot restriction.
851+
fn build_client(idle_timeout: Duration, pooling: Pooling) -> Client<HttpConnector, Body> {
846852
let mut builder = Client::builder(hyper_util::rt::TokioExecutor::new());
847853
builder.pool_idle_timeout(idle_timeout);
854+
if pooling == Pooling::Disabled {
855+
builder.pool_max_idle_per_host(0);
856+
}
848857
builder.build(HttpConnector::new())
849858
}
850859

851-
/// Idle keep-alive for the client [`Adapter::new`] builds — the one used before a
852-
/// SnapStart snapshot is taken.
860+
/// Whether a client may keep idle connections alive for reuse. See [`build_client`].
861+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
862+
enum Pooling {
863+
/// Normal keep-alive, bounded by the configured idle timeout.
864+
Enabled,
865+
/// No connection is retained at all — required before a SnapStart snapshot.
866+
Disabled,
867+
}
868+
869+
/// Connection-pool policy for the client [`Adapter::new`] builds — the one used
870+
/// before a SnapStart snapshot is taken.
853871
///
854-
/// Under SnapStart this is [`Duration::ZERO`], so a connection captured in the
855-
/// snapshot can never be handed out — dead — after a restore (hyper#3810). Note the
856-
/// precise guarantee: the socket from the last pre-snapshot request *is* still in
857-
/// the pool, and therefore in the snapshot; what `ZERO` ensures is that it is
858-
/// evicted on checkout rather than reused, so no request ever travels over it. (The
859-
/// former `pool_max_idle_per_host(0)` disabled the pool outright and dropped the
860-
/// connection immediately — stronger on retention, identical on reuse.) `run()`
861-
/// additionally rebuilds a fresh client in the after-restore hook, but keeping this
862-
/// one safe by construction also protects a consumer driving the `Service` impl
863-
/// directly, who never triggers that hook.
872+
/// Disabled under SnapStart, so no connection can be captured in the snapshot and
873+
/// handed out — dead — after a restore (hyper#3810). `run()` additionally rebuilds a
874+
/// fresh client in the after-restore hook, but keeping this one safe by construction
875+
/// also protects a consumer driving the `Service` impl directly, who never triggers
876+
/// that hook — and that consumer is the only one for whom this is the sole
877+
/// protection, so it must not depend on the post-restore clock.
864878
///
865879
/// The cost is that a pre-snapshot readiness poll reconnects on every 10ms attempt
866-
/// (measured: 27 connections per 300ms of polling, versus 1 with keep-alive). That
867-
/// is confined to init, which under SnapStart runs once per published version rather
868-
/// than per restore, and it is unchanged from the `pool_max_idle_per_host(0)`
869-
/// behavior this replaced.
880+
/// (measured: 27 connections per 300ms of polling, versus 1 with keep-alive). That is
881+
/// confined to init, which under SnapStart runs once per published version rather
882+
/// than per restore.
870883
///
871-
/// The configured value is NOT lost: it is kept on [`Adapter::pool_idle_timeout`]
872-
/// and used for the after-restore rebuild, whose pool starts empty and therefore
873-
/// cannot hold a snapshotted connection. That is what makes
874-
/// `AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS` effective for the invocations that actually
875-
/// serve traffic, instead of being a no-op for the whole life of the environment.
876-
fn base_client_idle_timeout(configured: Duration) -> Duration {
884+
/// The configured idle timeout is NOT lost: it is kept on
885+
/// [`Adapter::pool_idle_timeout`] and applied to the after-restore rebuild, whose
886+
/// pool starts empty and therefore cannot hold a snapshotted connection. That is what
887+
/// makes `AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS` effective for the invocations that
888+
/// actually serve traffic, instead of a no-op for the life of the environment.
889+
fn base_client_pooling() -> Pooling {
877890
if env::var("AWS_LAMBDA_INITIALIZATION_TYPE").as_deref() == Ok("snap-start") {
878-
Duration::ZERO
891+
Pooling::Disabled
879892
} else {
880-
configured
893+
Pooling::Enabled
881894
}
882895
}
883896

@@ -970,7 +983,7 @@ impl Adapter<HttpConnector, Body> {
970983
/// keep-alive disabled instead, so no connection can be captured in the snapshot
971984
/// and handed out dead after a restore; the configured value is retained and
972985
/// applied to the client rebuilt in the after-restore hook, which is what serves
973-
/// invocations. See the private `base_client_idle_timeout` for the rationale.
986+
/// invocations. See the private `base_client_pooling` for the rationale.
974987
///
975988
/// # Arguments
976989
///
@@ -995,7 +1008,7 @@ impl Adapter<HttpConnector, Body> {
9951008
/// let adapter = Adapter::new(&options).expect("Failed to create adapter");
9961009
/// ```
9971010
pub fn new(options: &AdapterOptions) -> Result<Adapter<HttpConnector, Body>, Error> {
998-
let client = build_client(base_client_idle_timeout(options.pool_idle_timeout));
1011+
let client = build_client(options.pool_idle_timeout, base_client_pooling());
9991012

10001013
let schema = "http";
10011014

@@ -1706,7 +1719,7 @@ mod tests {
17061719
async fn test_pool_idle_timeout_applies_under_snapstart() {
17071720
std::env::set_var("AWS_LAMBDA_INITIALIZATION_TYPE", "snap-start");
17081721
// This is the call `SnapStartHooks::after_restore` makes.
1709-
let restored = build_client(Duration::from_secs(4));
1722+
let restored = build_client(Duration::from_secs(4), Pooling::Enabled);
17101723
let used = connections_used(&restored, 3).await;
17111724
std::env::remove_var("AWS_LAMBDA_INITIALIZATION_TYPE");
17121725
assert_eq!(
@@ -1721,9 +1734,8 @@ mod tests {
17211734
/// (hyper#3810). This also covers a consumer driving the `Service` impl directly,
17221735
/// who never triggers the after-restore rebuild.
17231736
///
1724-
/// A zero idle timeout is the mechanism: a pooled connection is evicted on
1725-
/// checkout rather than reused, so it is never served over. See
1726-
/// [`build_client`] for exactly what that does and does not guarantee.
1737+
/// This counts reuse; `test_pre_snapshot_client_pool_is_disabled_not_merely_expiring`
1738+
/// pins the stronger, clock-independent property that the pool is off entirely.
17271739
#[tokio::test]
17281740
async fn test_adapter_new_client_never_pools_under_snapstart() {
17291741
std::env::set_var("AWS_LAMBDA_INITIALIZATION_TYPE", "snap-start");
@@ -1743,6 +1755,94 @@ mod tests {
17431755
assert_eq!(adapter.pool_idle_timeout, Duration::from_secs(4));
17441756
}
17451757

1758+
/// Issues one request over `client`, then reports whether the connection is still
1759+
/// open afterwards (i.e. parked in hyper's idle pool).
1760+
///
1761+
/// This distinguishes a *disabled* pool from a pool whose entries merely expire:
1762+
/// with `pool_max_idle_per_host(0)` the connection is dropped as soon as the
1763+
/// response completes, so the server side finishes; with
1764+
/// `pool_idle_timeout(Duration::ZERO)` the socket stays parked and is only
1765+
/// evicted at the next checkout. It observes the connection's lifetime rather
1766+
/// than elapsed time, so unlike a reuse count it cannot be satisfied by a clock
1767+
/// that happens to have advanced.
1768+
async fn connection_retained_after_request(client: &Client<HttpConnector, Body>) -> bool {
1769+
use std::sync::atomic::{AtomicBool, Ordering};
1770+
1771+
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
1772+
let addr = listener.local_addr().unwrap();
1773+
let closed = Arc::new(AtomicBool::new(false));
1774+
let flag = closed.clone();
1775+
tokio::spawn(async move {
1776+
let (stream, _) = listener.accept().await.unwrap();
1777+
let svc = hyper::service::service_fn(|_req: hyper::Request<hyper::body::Incoming>| async {
1778+
Ok::<_, std::convert::Infallible>(hyper::Response::new(String::from("ok")))
1779+
});
1780+
// Returns once the peer closes the connection.
1781+
let _ = hyper::server::conn::http1::Builder::new()
1782+
.serve_connection(hyper_util::rt::TokioIo::new(stream), svc)
1783+
.await;
1784+
flag.store(true, Ordering::SeqCst);
1785+
});
1786+
1787+
let req = hyper::Request::builder()
1788+
.uri(format!("http://{addr}/"))
1789+
.body(Body::Empty)
1790+
.unwrap();
1791+
let resp = client.request(req).await.unwrap();
1792+
let _ = resp.into_body().collect().await.unwrap();
1793+
// Give the client a moment to either drop or park the connection.
1794+
tokio::time::sleep(Duration::from_millis(100)).await;
1795+
!closed.load(Ordering::SeqCst)
1796+
}
1797+
1798+
/// The pre-snapshot client must make reuse impossible *by construction*, not by
1799+
/// relying on the monotonic clock.
1800+
///
1801+
/// Regression for the bot `[BUG]` finding: `pool_idle_timeout(Duration::ZERO)`
1802+
/// leaves hyper's pool enabled and parks the connection, deciding reuse at
1803+
/// checkout via `now.saturating_duration_since(idle_at) > timeout`. That
1804+
/// saturates to `ZERO` when the recorded instant is ahead of `now`, and
1805+
/// `ZERO > ZERO` is false — so the entry counts as fresh and is handed out. A
1806+
/// monotonic clock that did not advance across a restore is exactly the
1807+
/// condition hyper#3810 / rust-lang/rust#79462 describe, and exactly what the
1808+
/// original `pool_max_idle_per_host(0)` was written to distrust. Under `run()`
1809+
/// it is masked by the after-restore rebuild, but the direct-`Service` consumer
1810+
/// this restriction exists for is the one path where it can fail.
1811+
///
1812+
/// `pool_max_idle_per_host(0)` disables the pool, so no clock is consulted.
1813+
#[tokio::test]
1814+
async fn test_pre_snapshot_client_pool_is_disabled_not_merely_expiring() {
1815+
std::env::set_var("AWS_LAMBDA_INITIALIZATION_TYPE", "snap-start");
1816+
let options = AdapterOptions {
1817+
pool_idle_timeout: Duration::from_secs(4),
1818+
..Default::default()
1819+
};
1820+
let adapter = Adapter::new(&options).unwrap();
1821+
let retained = connection_retained_after_request(&adapter.client).await;
1822+
std::env::remove_var("AWS_LAMBDA_INITIALIZATION_TYPE");
1823+
assert!(
1824+
!retained,
1825+
"the pre-snapshot client must DROP its connection, not park it in the idle \
1826+
pool where a non-advancing clock could see it as fresh and reuse it"
1827+
);
1828+
}
1829+
1830+
/// The post-restore client, by contrast, must keep pooling: its pool starts empty
1831+
/// so it cannot hold a snapshotted connection, and retaining one is the whole
1832+
/// point of `AWS_LWA_POOL_IDLE_TIMEOUT_SECONDS`.
1833+
#[tokio::test]
1834+
async fn test_post_restore_client_retains_its_connection() {
1835+
std::env::set_var("AWS_LAMBDA_INITIALIZATION_TYPE", "snap-start");
1836+
// The call `SnapStartHooks::after_restore` makes.
1837+
let restored = build_client(Duration::from_secs(4), Pooling::Enabled);
1838+
let retained = connection_retained_after_request(&restored).await;
1839+
std::env::remove_var("AWS_LAMBDA_INITIALIZATION_TYPE");
1840+
assert!(
1841+
retained,
1842+
"the post-restore client must keep its connection alive for reuse"
1843+
);
1844+
}
1845+
17461846
/// Without SnapStart nothing changes: keep-alive to the inner app is honored, so
17471847
/// the safety mechanism above must not cost every other deployment its pooling.
17481848
#[tokio::test]
@@ -2390,7 +2490,7 @@ mod tests {
23902490
let base_ptr = Arc::as_ptr(adapter.client()) as *const ();
23912491

23922492
// Publish a fresh client.
2393-
let fresh = Arc::new(build_client(Duration::from_secs(4)));
2493+
let fresh = Arc::new(build_client(Duration::from_secs(4), Pooling::Enabled));
23942494
let fresh_ptr = Arc::as_ptr(&fresh) as *const ();
23952495
assert!(adapter.restored_client.set(fresh).is_ok(), "set should succeed once");
23962496

‎src/snapstart.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use lambda_http::{Body, BoxFuture, Error, SnapStartResource};
1010
use tokio::time::timeout;
1111
use url::Url;
1212

13-
use crate::{build_client, readiness, Protocol};
13+
use crate::{build_client, readiness, Pooling, Protocol};
1414

1515
/// Maximum time the adapter waits for an inner-app hook to respond before
1616
/// failing the SnapStart phase. Bounds a hung or unresponsive hook so the
@@ -121,7 +121,7 @@ impl SnapStartResource for SnapStartHooks {
121121
// 1. Publish a fresh client FIRST so the hook POST below (and all
122122
// subsequent invocations) use post-restore connections rather
123123
// than stale pre-snapshot ones. Ignore "already set".
124-
let fresh = Arc::new(build_client(self.pool_idle_timeout));
124+
let fresh = Arc::new(build_client(self.pool_idle_timeout, Pooling::Enabled));
125125
let _ = self.restored_client.set(fresh.clone());
126126

127127
// 2. Notify the app over the fresh client. Failure fails the restore;
@@ -205,7 +205,7 @@ mod tests {
205205
.unwrap();
206206
SnapStartHooks::new(
207207
Arc::new(OnceLock::new()),
208-
Arc::new(build_client(Duration::from_secs(4))),
208+
Arc::new(build_client(Duration::from_secs(4), Pooling::Enabled)),
209209
domain,
210210
before.map(str::to_string),
211211
after.map(str::to_string),
@@ -296,7 +296,7 @@ mod tests {
296296
then.status(200).delay(Duration::from_secs(2));
297297
});
298298
let domain: Url = format!("http://{}:{}", server.host(), server.port()).parse().unwrap();
299-
let client = build_client(Duration::from_secs(4));
299+
let client = build_client(Duration::from_secs(4), Pooling::Enabled);
300300

301301
let result =
302302
SnapStartHooks::post_hook_with_timeout(&client, &domain, "/slow", Duration::from_millis(100)).await;
@@ -336,7 +336,7 @@ mod tests {
336336
then.status(503);
337337
});
338338
let h = hooks_with_health(&server, None, None, "/never");
339-
let client = build_client(Duration::from_secs(4));
339+
let client = build_client(Duration::from_secs(4), Pooling::Enabled);
340340

341341
let result = h
342342
.check_readiness_with_timeout(&client, Duration::from_millis(100))

0 commit comments

Comments
 (0)