Skip to content

perf(server): lock each counter partition separately (S-C32) #477

Description

@justin13888

What

capsule_server::counter::InMemoryCounters holds all twelve CounterKey partitions behind a single Mutex<Partitions>. Give each partition its own lock, or replace the map with a sharded/concurrent structure, so a flood against one key kind does not serialise hit/peek/reset for the others.

Why this is filed rather than fixed

Raised during review round 4 of #459 and deliberately deferred there.

Decision 22 on that PR partitioned the ceiling per CounterKey variant, so admission is independent: one partition's occupancy is invisible to another's ceiling, and filling DropLink cannot deny a first key to ShareLink or OidcAuthorize. Latency is not independent, because every partition shares one lock.

That is a real residual and not a practical denial at the current sizes:

  • the critical section holds no .await — it is a purge, a BTreeMap lookup and at most one insert;
  • the work is O(log n) over at most 20 000 entries (ceilings::DROP_LINK);
  • the flood that would contend for it is itself bounded by the ceiling it is filling.

So it is contention, not denial. It is worth recording because the module doc claims "the windows one surface holds are not the windows another surface is denied", and that sentence is about admission — it should not be read as a latency guarantee. The doc now says so explicitly.

When this matters

  • A durable adapter (Valkey, server: Valkey adapter for OidcAuthorizationStore and Postgres adapter for FederatedAccounts #460) changes the shape entirely and may make this moot for deployments that use one; the in-memory adapter is the development and single-node profile.
  • Raising any ceilings::* constant materially raises the O(log n) work under the shared lock.
  • A future CounterKey whose partition is both hot and caller-influenced would make the contention easier to reach than it is today.

Suggested shape

Partitions becomes BTreeMap<&'static str, Mutex<BTreeMap<CounterKey, Window>>> built once from the known variant names, so the outer map is read-only after construction and needs no lock at all. That keeps CounterKey::as_str as the partition key and leaves CounterStore's contract untouched.

Whatever the shape, the atomicity CounterStore::hit promises — charge and decide as one operation — must survive: it is the property the whole port exists for, and a per-partition lock preserves it because a key never spans two partitions.

Acceptance

  • hit on one CounterKey variant does not block hit on another.
  • The existing counter::tests suite passes unchanged, including no_sequence_of_calls_admits_more_than_the_budget and flooding_one_surface_does_not_deny_a_fresh_key_to_another.
  • The module docs' "one lock" section is updated or removed.

Refs #459.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions