fix(core): serialize writers on the runtime snapshot

[terylt]

Closes: #23

Description

PolicyEngine publishes its runtime state as an ArcSwap<RuntimeSnapshot>, and every mutation is a read-modify-write: load the current snapshot, clone it, apply the change, store the clone. Nothing serialised the writers. Two threads that loaded the same snapshot each cloned it, each applied only their own change, and whichever stored last silently discarded the other's work. The call that lost still returned Ok, and the generation counter still bumped for both, so no downstream cache saw any sign that a registration had gone missing.

The affected paths are mutate_runtime, try_mutate_runtime, and the inline snapshot swap in load_config, which cannot use either helper because it has to swap the registry, the executor, and the cache cap together as one snapshot.

The issue was filed from reading the code and noted that it had not been reproduced. It reproduces. Sixteen threads calling register_handler at the same instant left one plugin registered out of sixteen, with all sixteen calls returning Ok:

assertion `left == right` failed: every registration that returned Ok must be present in the snapshot
  left: 1
 right: 16

The existing test_manager_arc_shareable_with_concurrent_dispatch_and_registration did not catch this because it runs on current_thread, so it can never interleave a load with a store.

Fix

A runtime_write: Mutex<()> on PolicyEngine, held across the copy-on-write in all three paths. Readers never take it, so the invoke path is byte-for-byte what it was and stays lock-free. It guards () rather than data because the data lives in the ArcSwap; the lock exists only to make the read-modify-write atomic with respect to other writers.

Poisoning is ignored via the PoisonError::into_inner convention already used for the other locks in this file. A panic inside a mutation closure leaves the ArcSwap holding whatever was last published, which is a complete snapshot either way, so there is no half-applied state for the next writer to inherit.

Why a mutex rather than arc_swap::rcu

The issue suggested rcu() as the smaller change. Two things pushed the other way.

rcu re-runs its closure on contention, so FnOnce would have to become FnMut, and every call site currently moves its inputs into the closure. Each would need per-attempt cloning of the plugin, config, and handler.

More decisive: load_config cannot use rcu at all in practice. Its critical section calls instantiate_plugins_into, which runs the registered plugin factories. Retrying means constructing plugins two or three times, which is side-effecting and potentially expensive. That path would need a mutex regardless, leaving two different mechanisms in one file when the issue explicitly asks for load_config to be covered by the same one.

The cost of the mutex is that config loads and registrations serialize against each other. These are cold paths, and the issue lists that serialisation as a requirement rather than a regression.

Acceptance criteria

• Concurrent mutate_runtime calls cannot lose a mutation
• try_mutate_runtime gets the same treatment, including that a failed mutation publishes nothing. The ? drops the guard on the way out having stored nothing, so a rejected mutation publishes nothing and blocks no one
• load_config's inline snapshot swap is covered by the same mechanism. The guard is taken before the factories lock so every mutation path acquires in the same order, and dropped before the routing cache is cleared, matching the other paths
• A multi threaded test that registers N handlers from N threads and asserts all N are present, failing against current main
• Read path stays lock free. The lock appears at three call sites, all of them writers, and nowhere in the invoke bodies. A hook invocation still does one ArcSwap::load_full and owns that snapshot for the rest of the call, so it runs the same instructions it did before. Not measured, see below
• The generation counter still bumps exactly once per published mutation

Tests

concurrent_registration_loses_no_plugins puts sixteen real OS threads through register_handler, lined up on a std::sync::Barrier so they all load the same snapshot. Without the barrier the loss is occasional; with it, it is reliable. It asserts both that all sixteen plugins are present and that the generation advanced exactly sixteen times. Real threads rather than tokio::spawn, for the reason the existing test misses the bug.

failed_registration_publishes_nothing registers a duplicate name, asserts the error, and asserts the generation did not move, so a cache keyed on the generation does not evict and rebuild over a registration that never happened.

Verified red against the unfixed code before the fix went in, then green.

Deadlock analysis

Nothing under the lock re-enters a mutating method. The closures are confined to registry.register, HashMap insert and remove, and registry.unregister.

The one path worth checking is config visitors, which can install handlers via annotate_route. They run at the end of load_config_yaml, after load_config has returned and released the guard, not during it. The praxis-policy-apl-runtime visitor tests exercise that path and pass.

Not covered

annotate_route, remove_route_annotation, and unregister share the same lock through mutate_runtime, so they are correct by construction, but there is no test driving them from many threads at once. Only the registration path is exercised concurrently. A mixed-operation concurrency test would be a reasonable follow-up.

The claim that invoke latency is unaffected is reasoning, not a measurement. The workspace has no benchmarks, so it rests on the lock being absent from the read path rather than on a before and after number. An uncontended lock that is never taken costs nothing, but nobody timed it.

What does change is that writers now serialise against each other. Two simultaneous load_config calls, or a registration arriving during a config load, will queue. Both are cold paths, and that serialisation is what the issue asked for.

No live host was involved. The measurements above come from the unit test, not from a proxy under load.

Compatibility

No public API change and no config change. Nothing in the workspace registers or loads config concurrently today, so no shipped deployment was hitting this. It matters because registration takes &self on an engine designed to sit behind an Arc, register_raw is documented for out-of-process bridge hosts, and hot reload would walk straight into it.

Verification

• make ci (fmt, clippy with -D warnings, and both test passes) exits 0
• Full workspace suite passes with --all-features
• make lint-extra fails, but on main as well and in files this branch does not touch. It is a spellcheck hitting unparseable and overrideable across nineteen files, and it is not part of make ci

[araujof]

Nice work! Please address the following findings, and we should be good to go.

• load_config holds runtime_write while calling host-supplied PluginFactory::create() code. A factory that re-enters the engine through register_handler, annotate_route, or unregister will try to acquire the same mutex and deadlock. Please create the plugin instances before taking this lock, then lock only while updating and publishing the latest snapshot.