test(sdk): close test gaps — registry concurrency, Rust pagination + hooks#17
Merged
Conversation
…hooks Three SDKs (Rust / Go / Python) had a TOCTOU window in their token / pubkey caches where N concurrent first-fetch callers would each fire a duplicate HTTP request. The Staff-engineer review flagged it; this PR fixes the bug AND adds regression tests so it stays fixed. REGISTRY TOCTOU FIX - Rust: `tokio::sync::Mutex<()>` on `AggregatorRegistry::fetch_lock`. Slow path acquires the mutex, double-checks the read-lock cache, then issues the HTTP fetch. - Go: `sync.Mutex` on `aggregatorRegistry.fetchMu`. Same pattern — double-check after acquiring. - Python: `threading.Lock` on `AggregatorRegistry._fetch_lock`. Same pattern; previously had no thread-safety at all. - TypeScript / PHP unaffected (single-threaded by default). REGRESSION TESTS Each language stands up an in-process counting HTTP fixture, fires 16 concurrent first-fetches, and asserts exactly 1 HTTP hit reaches the server. Without the serializer the tests observe 16 hits. - sdks/rust/src/registry.rs::tests::concurrent_first_fetches_share_one_request Uses `tokio::net::TcpListener` for a real TCP-level fixture so the reqwest transport is exercised end-to-end. - sdks/go/sdk/registry_concurrency_test.go (token fetch + pubkey fetch). Runs clean under `go test -race`. - sdks/python/tests/test_registry_concurrency.py Uses stdlib `http.server` + `concurrent.futures.ThreadPoolExecutor`. RUST TEST GAPS FROM BUCKET 3 The previous review noted Rust shipped pagination + hooks features without per-feature tests: - list_all_orders_walks_pages — three-page counting fixture verifies the collector terminates after `total` is reached and yields the full ordered set. - request_hooks_fire_per_attempt — runs a 200 then a 401, verifies on_request / on_response / on_error fire in the right sequence with the real status code in the hook context. Rust contract tests: 12 → 17; total Rust tests: 17 → 20. WIDENED VISIBILITY - HttpContext::new: `fn new` → `pub(crate) fn new` so registry-level concurrency tests inside the crate can construct a minimal context without going through the public PaycrestClient. Crate-private visibility, no external surface change. - tokio features expanded: added `net`, `io-util`, `sync` so the TcpListener-based fixture compiles. VERIFICATION - ./scripts/tests/run_all_smoke.sh — all 5 SDKs green - go test ./sdk/... -race — clean, no data race - ./scripts/tests/parity/run_parity.sh — all 5 SDKs replay identical HTTP envelopes through the fixture server Test counts after this PR: - TypeScript: 35 (unchanged) - Python: 29 (was 27, +2 concurrency) - Go: 34 (was 32, +2 concurrency) - Rust: 20 (was 17, +1 concurrency, +1 pagination, +1 hooks) - Laravel/PHP: 29 (unchanged) Total: 147 tests, +6 over the previous tally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test gaps closed
Follow-up to the Staff-engineer review (PR #16): closes the registry TOCTOU + the bucket-3 Rust test gaps with proper regression tests.
🐛 Registry first-fetch TOCTOU — fixed in 3 SDKs
Rust, Go, and Python all had the same
RLock → drop → Lockpattern in their token / pubkey caches. Under contention, N concurrent first-fetch callers would each see a cache miss before the first writer populated it — and each would issue a duplicate HTTP request. Now serialized:tokio::sync::Mutex<()>onAggregatorRegistry::fetch_locksync.MutexonaggregatorRegistry.fetchMuthreading.LockonAggregatorRegistry._fetch_lock(was unsynchronized at all)TypeScript and PHP are unaffected — both single-threaded by default.
🧪 Regression tests
Each language stands up an in-process counting HTTP fixture, fires 16 concurrent first-fetches, and asserts exactly 1 HTTP hit reaches the server. Without the serializer these tests would observe 16 hits.
sdks/rust/src/registry.rs::tests::concurrent_first_fetches_share_one_request— usestokio::net::TcpListenerfor a real TCP fixture so the reqwest transport is exercised end-to-end.sdks/go/sdk/registry_concurrency_test.go— covers token and pubkey paths; runs clean undergo test -race.sdks/python/tests/test_registry_concurrency.py— uses stdlibhttp.server+concurrent.futures.ThreadPoolExecutor.The fixtures inject a 50ms delay on the slow path so concurrent callers reliably overlap on the cache miss instead of finishing serially. Subsequent calls verify the cache is populated (no further hits).
🦀 Rust bucket-3 test gaps filled
The Staff review noted Rust shipped pagination + hooks in bucket 3 with only constructor/default-value coverage. New tests:
list_all_orders_walks_pages— three-page counting fixture verifies the collector terminates aftertotalis reached and yields the full ordered set across pages.request_hooks_fire_per_attempt— runs a 200 then a 401 against the fixture and verifieson_request/on_response/on_errorfire in the right sequence with the real status code inHookContext.Rust contract tests: 12 → 17. Total Rust tests: 17 → 20.
🔧 Crate-private widening
HttpContext::new:fn new→pub(crate) fn newso the new registry concurrency test (inside the crate) can construct a minimal context without going through the publicPaycrestClient. No external surface change.tokiofeatures expanded to includenet,io-util,syncso the TcpListener fixture compiles.📊 Test counts after this PR
✅ Verified
./scripts/tests/run_all_smoke.sh— all 5 SDKs greengo test ./sdk/... -race— clean, no data race detected./scripts/tests/parity/run_parity.sh— all 5 SDKs replay identical HTTP envelopes through the fixture serverhttps://claude.ai/code/session_01XqN2uu93AQVrD5KuhJvqoY