Merge branch 'wt/review-002-client-policy-wire-tests'

tests/client_config_reload.rs

@@ -0,0 +1,33 @@
+//! COV-12/FWD-12: `SharedHttpClient::config()` reflects the config the
+//! running clients were built from, and a `reload` swaps the config
+//! together with both clients in one atomic store — after a reload,
+//! `config()` shows the new generation and the old one is unreachable
+//! for every late reader (FWD-12's atomicity assertion).
+
+use std::time::Duration;
+
+use alkhttp::client::{HttpClientConfig, SharedHttpClient};
+
+#[test]
+fn config_accessor_tracks_a_reloaded_config() {
+    let initial = HttpClientConfig {
+        retry_after_ceiling: Duration::from_secs(11),
+        ..HttpClientConfig::default()
+    };
+    let http = SharedHttpClient::new(initial).expect("initial client builds");
+    assert_eq!(
+        http.config().retry_after_ceiling,
+        Duration::from_secs(11),
+        "config() starts as the config the client was built from"
+    );
+    let reloaded = HttpClientConfig {
+        retry_after_ceiling: Duration::from_secs(22),
+        ..HttpClientConfig::default()
+    };
+    futures::executor::block_on(http.reload(reloaded)).expect("reload succeeds");
+    assert_eq!(
+        http.config().retry_after_ceiling,
+        Duration::from_secs(22),
+        "config() reflects the reloaded generation, not the initial one (FWD-12)"
+    );
+}

tests/client_tls.rs

@@ -21,7 +21,7 @@ use std::sync::atomic::{AtomicU32, Ordering};
 use std::sync::Arc;
 use std::time::Duration;
 
-use alkhttp::client::{ClientCertConfig, HttpClientConfig, SharedHttpClient};
+use alkhttp::client::{ClientCertConfig, HttpClientBuildError, HttpClientConfig, SharedHttpClient};
 
 /// A throwaway private PKI: CA, server leaf for `127.0.0.1`/`localhost`,
 /// and a client leaf, freshly minted per test.
@@ -373,3 +373,105 @@ async fn reload_to_a_ca_bundle_backed_client_succeeds() {
     cleanup_dir(&dir);
     tokio::time::sleep(Duration::from_millis(1)).await;
 }
+
+/// COV-11/CLI-03 parse-failure arms: a `ca_bundle` file that parses as
+/// PEM framing but carries a corrupt section fails the build with
+/// `CaBundleParse`, carrying the offending path. (Purely non-PEM text
+/// yields zero sections and leaves the trust store empty — reqwest
+/// accepts it — so the failure arm needs a structurally broken PEM.)
+#[test]
+fn corrupt_ca_bundle_fails_ca_bundle_parse_with_path() {
+    let dir = std::env::temp_dir().join(format!(
+        "alkhttp-pem-parse-{}-{}",
+        std::process::id(),
+        uuid::Uuid::new_v4()
+    ));
+    std::fs::create_dir_all(&dir).expect("temp dir");
+    let ca_path = dir.join("ca.pem");
+    std::fs::write(
+        &ca_path,
+        b"-----BEGIN CERTIFICATE-----\n!!not-base64!!\n-----END CERTIFICATE-----\n",
+    )
+    .expect("write corrupt pem");
+    let error = SharedHttpClient::new(client_config(Some(ca_path.clone()), None))
+        .expect_err("a corrupt CA bundle must fail the build");
+    match error {
+        HttpClientBuildError::CaBundleParse { path, .. } => {
+            assert_eq!(path, ca_path, "the error names the unparseable path");
+        }
+        other => panic!("expected CaBundleParse, got {other:?}"),
+    }
+    cleanup_dir(&dir);
+}
+
+/// COV-11/CLI-03 parse-failure arm: client cert files that exist but do
+/// not form a valid reqwest `Identity` fail the build with
+/// `ClientCertParse` carrying the cert path — and the message never
+/// carries key material (the PEM bytes are never echoed).
+#[test]
+fn garbage_client_cert_fails_client_cert_parse_with_path_and_no_key_material() {
+    let dir = std::env::temp_dir().join(format!(
+        "alkhttp-pem-parse-{}-{}",
+        std::process::id(),
+        uuid::Uuid::new_v4()
+    ));
+    std::fs::create_dir_all(&dir).expect("temp dir");
+    let cert_path = dir.join("client-cert.pem");
+    let key_path = dir.join("client-key.pem");
+    std::fs::write(
+        &cert_path,
+        b"-----BEGIN GARBAGE-----\nnope\n-----END GARBAGE-----\n",
+    )
+    .expect("write garbage cert");
+    std::fs::write(
+        &key_path,
+        b"-----BEGIN PRIVATE KEY-----\nnot-a-key\n-----END PRIVATE KEY-----\n",
+    )
+    .expect("write garbage key");
+    let key_marker = "not-a-key";
+    let error = SharedHttpClient::new(client_config(
+        None,
+        Some(ClientCertConfig {
+            cert_pem: cert_path.clone(),
+            key_pem: key_path,
+        }),
+    ))
+    .expect_err("a non-parseable client identity must fail the build");
+    let rendered = format!("{error}");
+    match error {
+        HttpClientBuildError::ClientCertParse { path, .. } => {
+            assert_eq!(path, cert_path, "the error names the identity's cert path");
+        }
+        other => panic!("expected ClientCertParse, got {other:?}"),
+    }
+    assert!(
+        !rendered.contains(key_marker),
+        "the error must never echo key material: {rendered}"
+    );
+    cleanup_dir(&dir);
+}
+
+/// FWD-12, config() half: after a `reload`, `config()` reflects the
+/// reloaded generation (the companion wire test lives in
+/// tests/client_config_reload.rs); here it is pinned on the TLS-config
+/// path where reload rebuilds from PEM files.
+#[tokio::test]
+async fn config_accessor_reflects_a_tls_config_reload() {
+    let pki = TestPki::generate();
+    let (ca, _cert, dir) = pki.write_config_files(false);
+
+    let http = SharedHttpClient::new(HttpClientConfig::default()).expect("initial client");
+    assert!(
+        http.config().ca_bundle.is_none(),
+        "the initial config has no CA bundle"
+    );
+    http.reload(client_config(ca, None))
+        .await
+        .expect("reload with the CA bundle succeeds");
+    let visible = http.config();
+    assert!(
+        visible.ca_bundle.is_some(),
+        "config() reflects the reloaded generation's CA bundle (FWD-12)"
+    );
+    cleanup_dir(&dir);
+}

tests/retry_policy_wire.rs

@@ -0,0 +1,164 @@
+//! Retry-policy wire coverage (review-002 CLI-03, FWD-04): counting
+//! responders against the real `SharedHttpClient` middleware stack.
+//!
+//! Pinned here:
+//!
+//! - the method gate — a POST gets exactly one upstream hit even when
+//!   the upstream answers 500 (non-idempotent requests never re-send);
+//! - the idempotent retry path — a GET that 500s twice then succeeds
+//!   results in exactly three upstream hits;
+//! - the wall-clock budget — with a generous attempt cap but a small
+//!   `max_total_retry_duration`, an always-500 upstream stops the
+//!   retry loop on budget exhaustion, bounding both the wall time and
+//!   the hit count.
+
+use std::sync::atomic::{AtomicU32, Ordering};
+use std::sync::Arc;
+use std::time::Duration;
+
+use alkhttp::client::{HttpClientConfig, RetryConfig, SharedHttpClient};
+use std::sync::Mutex;
+
+/// A raw-TCP HTTP/1.1 responder that counts accepted connections and
+/// answers each request from a per-attempt script: every entry is one
+/// response head; the last entry repeats when the script runs out.
+struct ScriptedResponder {
+    addr: std::net::SocketAddr,
+    hits: Arc<AtomicU32>,
+}
+
+impl ScriptedResponder {
+    async fn spawn(script: Vec<&'static str>) -> Self {
+        let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
+            .await
+            .expect("responder binds");
+        let addr = listener.local_addr().expect("responder address");
+        let hits = Arc::new(AtomicU32::new(0));
+        let script = Arc::new(Mutex::new(script));
+        let hits_loop = Arc::clone(&hits);
+        let script_loop = Arc::clone(&script);
+        tokio::spawn(async move {
+            loop {
+                let Ok((mut sock, _)) = listener.accept().await else {
+                    return;
+                };
+                hits_loop.fetch_add(1, Ordering::SeqCst);
+                let head = {
+                    let mut queue = script_loop.lock().unwrap_or_else(|e| e.into_inner());
+                    if queue.len() <= 1 {
+                        queue
+                            .first()
+                            .copied()
+                            .unwrap_or("HTTP/1.1 500 Internal Server Error")
+                    } else {
+                        queue.remove(0)
+                    }
+                };
+                let body = "";
+                let response = format!(
+                    "{head}\r\ncontent-length: {}\r\nconnection: close\r\n\r\n",
+                    body.len()
+                );
+                let _ = tokio::io::AsyncWriteExt::write_all(&mut sock, response.as_bytes()).await;
+                tokio::io::AsyncWriteExt::shutdown(&mut sock).await.ok();
+            }
+        });
+        Self { addr, hits }
+    }
+
+    fn hits(&self) -> u32 {
+        self.hits.load(Ordering::SeqCst)
+    }
+}
+
+fn retrying_config(max_total_retry_duration: Duration) -> HttpClientConfig {
+    HttpClientConfig {
+        request_timeout: Some(Duration::from_secs(5)),
+        connect_timeout: Some(Duration::from_secs(2)),
+        read_timeout: Some(Duration::from_secs(2)),
+        retry: RetryConfig {
+            max_retries: 50,
+            initial_backoff: Duration::from_millis(10),
+            max_retry_interval: Duration::from_millis(50),
+        },
+        max_total_retry_duration,
+        ..HttpClientConfig::default()
+    }
+}
+
+#[tokio::test]
+async fn post_500_receives_exactly_one_upstream_hit() {
+    let responder = ScriptedResponder::spawn(vec!["HTTP/1.1 500 Internal Server Error"]).await;
+    let http =
+        SharedHttpClient::new(retrying_config(Duration::from_secs(10))).expect("client builds");
+    let response = http
+        .client()
+        .post(format!("http://{}/mutate", responder.addr))
+        .send()
+        .await
+        .expect("500 is a delivered response, not a transport error");
+    assert_eq!(response.status(), 500);
+    assert_eq!(
+        responder.hits(),
+        1,
+        "the method gate must bypass the retry middleware for POST: a non-idempotent request is never re-sent"
+    );
+}
+
+#[tokio::test]
+async fn get_500_twice_then_success_is_exactly_three_hits() {
+    let responder = ScriptedResponder::spawn(vec![
+        "HTTP/1.1 500 Internal Server Error",
+        "HTTP/1.1 503 Service Unavailable",
+        "HTTP/1.1 200 OK",
+    ])
+    .await;
+    let http =
+        SharedHttpClient::new(retrying_config(Duration::from_secs(10))).expect("client builds");
+    let response = http
+        .client()
+        .get(format!("http://{}/flaky", responder.addr))
+        .send()
+        .await
+        .expect("retry-to-success delivers the final response");
+    assert_eq!(response.status(), 200);
+    assert_eq!(
+        responder.hits(),
+        3,
+        "two failed attempts retried, third attempt succeeded"
+    );
+}
+
+#[tokio::test]
+async fn budget_exhaustion_stops_retries_despite_a_generous_attempt_cap() {
+    let responder = ScriptedResponder::spawn(vec!["HTTP/1.1 500 Internal Server Error"]).await;
+    let http =
+        SharedHttpClient::new(retrying_config(Duration::from_millis(400))).expect("client builds");
+    let started = std::time::Instant::now();
+    let response = http
+        .client()
+        .get(format!("http://{}/stuck", responder.addr))
+        .send()
+        .await
+        .expect("500 is a delivered response, not a transport error");
+    assert_eq!(response.status(), 500);
+    let hits = responder.hits();
+    assert_eq!(
+        response.status(),
+        500,
+        "the surfaced status is the final upstream 500"
+    );
+    assert!(
+        hits >= 2,
+        "at least one retry ran before the budget closed, hits: {hits}"
+    );
+    assert!(
+        hits < 50,
+        "budget exhaustion must stop retries long before the 50-attempt cap, hits: {hits}"
+    );
+    assert!(
+        started.elapsed() < Duration::from_secs(3),
+        "wall time is bounded by max_total_retry_duration + one attempt, took {:?} over {hits} hits",
+        started.elapsed()
+    );
+}