test(e2e): provider_key rotation with zero in-flight disruption (#196 L3, #271) - #523
Conversation
…196 L3, #271) Closes the open #271 gap. provider_key_rotation_test.go (CP) pins rotation correctness + post-swap liveness but rotates with no traffic in flight. This pins the distinguishing L3 property: under sustained concurrency (8 workers, 80 requests), an in-place secret rotation (PUT /admin/v1/provider_keys/:id — same id + api_base, new secret) fired mid-stream must cause ZERO failed requests, and the resource revision must bump. A DP that rebuilt its per-provider_key upstream client non-atomically (briefly losing the key or dropping in-flight requests on the snapshot swap) would surface ≥1 failure. Out of scope (separate gap #220): which secret the DP dialed upstream — the mock ignores the credential, so only the disruption-free property is observable here, which is exactly L3's claim. Refs #196 L3, #127 L3, closes #271.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds an end-to-end test that verifies provider key secret rotation does not cause in-flight request failures. The test initializes the app and mock OpenAI upstream, runs sustained concurrent chat requests across multiple workers, triggers a provider key rotation mid-flight at a fixed request index, and asserts zero disruption with a confirmed revision increment. ChangesProvider Key Rotation E2E Test
Sequence DiagramsequenceDiagram
participant TestRunner
participant Etcd
participant MockOpenAIUpstream
participant AppProxy
participant AdminAPI
TestRunner->>Etcd: ping for readiness
TestRunner->>MockOpenAIUpstream: start mock upstream
TestRunner->>AppProxy: spawn app under test
TestRunner->>AdminAPI: create provider key, model, caller API key
TestRunner->>AppProxy: create OpenAI client -> probe chat completions until "ok"
TestRunner->>AppProxy: spawn concurrent workers issuing chat completions
AppProxy->>MockOpenAIUpstream: forward chat completion requests
loop during workload
TestRunner->>AdminAPI: (one worker at ROTATE_AT) admin PUT to rotate provider key secret
AdminAPI->>AppProxy: provider key secret updated (same id/api_base)
end
AppProxy->>TestRunner: responses for each chat request (expect "ok")
TestRunner->>AdminAPI: fetch provider key revision after run
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
) The audit (HIGH) showed "zero in-flight disruption" is an ARCHITECTURAL guarantee, not a falsifiable property: the DP holds one shared upstream client and reads pk.secret/api_base per-request from an atomic ArcSwap snapshot, with in-flight requests holding their own snapshot Arc — there is no per-provider_key client/pool to tear down on a hot-swap. So the old "≥1 failure on swap" framing certified a hollow green. Re-scope honestly: this is a liveness/smoke pin that an in-place secret rotation under sustained load keeps dispatch serving + bumps revision (catches a future regression that wedged dispatch or broke watch-apply on a PK PUT), with the architecture documented in the header. The real remaining facet — the rotated secret actually reaching upstream — needs a credential-sensitive mock and stays as #220 (not closed here). Also bump maxRetries 0→2 to immunize the strict all-succeed gate against a loopback transient (a real wedge fails all retries too). Refs #196 L3, #127 L3, #271, #220.
Independent audit (CLAUDE.md §8) — resolvedVerdict BLOCK on HIGH-1; addressed by re-scoping (latest commit). The auditor verified against DP source ( Fixed: header + PR + test title re-scoped to a rotate-under-load liveness/smoke pin + revision bump, with the architecture documented; the real "rotated secret reaches upstream" facet stays as #220 (credential-sensitive mock needed), not closed here. Also bumped Auditor confirmed CLEAR on the mechanics: valid PUT body, correct revision bump (1→2), real in-flight overlap during the PUT, correct isolation/cleanup, |
CI: my test passed; failure is an unrelated cache flake — re-runningThe The failure is in an unrelated file — Re-enqueued the failed job. If |
1 participant
What
A liveness/smoke pin over the provider_key in-place rotation under load path (#196 L3). Under sustained concurrency (8 workers, 80 requests), a mid-stream
PUT /admin/v1/provider_keys/:id(same id + api_base, new secret) keeps every request serving and bumps the resource revision; the caller's api_key + model alias are untouched.Scope honesty (per the #523 audit)
The audit (HIGH) established that "zero in-flight disruption" is an architectural guarantee, not a falsifiable property in this DP: there is no per-provider_key upstream client/pool — the DP holds one shared client and reads
pk.secret/api_baseper-request from an atomic ArcSwap snapshot, with each in-flight request holding its own snapshotArcto completion. So the original "≥1 failure on the swap" framing would have certified a hollow green.Re-scoped accordingly: this is a smoke/liveness pin (catches a future regression that wedged dispatch or broke watch-apply on a PK PUT) + a revision-bump check, with the architecture documented in the header. It does not claim to probe a teardown race (there isn't one).
The real remaining facet → #220 (NOT closed here)
Asserting the rotated secret actually reaches upstream (old rejected / new accepted) needs a credential-sensitive mock — that's the pre-existing #220. This PR refs #271/#220 but does not close them; see the #271 comment for the architectural finding.
Verification
tsc --noEmitclean for this file. Isolated DPe2eCI is authoritative.maxRetries:2immunizes the strict all-succeed gate against a loopback transient (a real wedge fails all retries too).Refs #196 L3, #127 L3, #271, #220.
Summary by CodeRabbit