Skip to content

feat(test): concurrency testing for the engine #22

Description

@terylt

Description

The engine is built to be shared across threads behind an Arc and mutated while requests are in flight, but almost nothing in the test suite runs on more than one thread.

What we have today:

  • 484 #[tokio::test] functions. 9 of them specify flavor = "multi_thread", 8 in ppe-orchestration and 1 in the JWKS e2e. The other 475 run on current_thread, where tasks interleave at await points but never run in parallel.
  • No loom, no shuttle, no proptest, no stress or soak test anywhere in the workspace.
  • await_holding_lock and await_holding_refcell_ref are denied workspace wide. This is real coverage and it closes the deadlock class statically.

What is untested:

crates/ppe-core/src/engine.rs has 64 lock and atomic sites, an arc_swap::ArcSwap runtime snapshot, a hand written Release/Acquire pairing on the config generation counter, and a shared route cache. None of it has ever been exercised under real parallelism. Lost updates, stale cache windows, and memory ordering bugs cannot manifest on a single threaded executor.

The gap is specific: static lints cover the deadlock class, nothing covers the interleaving class.

Motivating evidence

engine.rs:390, mutate_runtime is a read modify write on the ArcSwap with no lock and no compare and swap:

let current = self.runtime.load_full();
let mut next = (*current).clone();
let result = f(&mut next);
self.runtime.store(Arc::new(next));

Two threads calling it concurrently both load the same snapshot, both clone, both mutate their own copy, both store. Last writer wins and the first mutation is silently lost. Callers are all public &self methods: register_handler, register_handler_for_names, register_raw, annotate_route, remove_route_annotation, unregister. test_manager_arc_shareable_with_concurrent_dispatch_and_registration at engine.rs:2467 shows concurrent registration is intended to work, and it runs on current_thread so it cannot interleave the load and the store.

Tracked separately, but it is the example of what this testing is for. A multi threaded stress test settles it in minutes.

Scope

Priority order. The first two are most of the value.

  1. Move the engine state tests to flavor = "multi_thread". Not all 484, just the ones touching registration, hot reload, and the route cache.
  2. Add a stress test: N threads invoking while M threads reload config and register handlers. Assert no registration is lost and no invoke observes a torn or stale config.
  3. Run the stress test under ThreadSanitizer in a nightly CI job. Finds data races directly rather than by symptom.
  4. Use loom on a small extracted model of the generation counter and snapshot pair. Loom is exhaustive and does not scale to the whole engine, so model just the ordering claim that is written in the comments.
  5. Consider shuttle if we want randomized interleaving exploration over larger code than loom can take.

Acceptance Criteria

  • Engine tests covering registration, hot reload, unregister, and route cache run on a multi threaded runtime
  • A stress test exercising concurrent invoke against concurrent config mutation, with a lost update assertion
  • The stress test is repeatable and seeded so a failure can be reproduced
  • A nightly CI job runs the stress test under ThreadSanitizer
  • The Release/Acquire pairing between generation and the ArcSwap snapshot is verified by a loom model, or the comment claiming the guarantee is downgraded to what we can actually show
  • Any races found are filed as their own issues rather than fixed silently inside this one
  • Guidance in CONTRIBUTING on when a new test needs the multi threaded flavor

Metadata

Metadata

Labels

Type

Projects

Relationships

None yet

Development

No branches or pull requests

Issue actions