Skip to content

The fetch limiter holds fleet-wide data but can only decide per-connection, so aggregate volume is structurally invisible #43

Open

Description

@iceteaSA

FetchLimiter decides admission per connection. That is correct for what it was built for — enumeration sweeps and runaway single clients — but it means total fetch volume across the fleet has no observer, and a consumer topology change can raise aggregate load without any connection approaching its ceiling.

Surfaced by a consumer seat describing their own topology, and it is sharper than "many processes":

the client is per PLUGIN INSTANCE, and a plugin instance is per (process × project) — a single process serving several projects holds several independent clients and therefore several connections

So the connection count scales with projects open, not with processes, and every one of them carries its own 64/60s budget. A fleet-wide rotation storm spreads across N budgets instead of concentrating in one: aggregate RPC volume rises, no individual connection alarms, and nothing records that it happened.

The data is already resident. FetchLimiter.conns is a HashMap<u64, ConnState> holding every live connection's window, and drop_connection (limiter.rs:146) removes closed ones, so the map is live-only. The decision is per-connection; the state is not. A fleet-aggregate observation is a fold over self.conns, not a new surface.

One correctness detail, or the fold reports a number that is wrong in the alarming direction. prune is called at exactly one site — limiter.rs:121, on the admitting connection only. An idle connection's window is never trimmed until it fetches again, so its fetches vector still holds timestamps older than the window. A naive conns.values().map(|c| c.fetches.len()).sum() counts those, and over-counts by exactly the volume of whichever connections went quiet. The fold has to prune-then-sum:

self.conns.values_mut().for_each(|c| c.prune(now, caps.window));
let fleet = self.conns.values().map(|c| c.fetches.len()).sum::<usize>();

That is the same shape as several defects found this week: the data is right, the aggregation is wrong, and the wrong answer looks plausible.

What I am not proposing: a fleet ceiling that denies. Admission::Anomaly alarms and still serves, which is the right default for a credential path — defensive throttling on a recovery mechanism trades a visible alarm for an invisible degradation. Zero fetch_anomaly rows exist in this deployment all-time, so nothing is pressing.

What is worth having: a fleet-volume figure exposed on the existing no-decrypt health snapshot, so "how much load is this fleet generating" has an answer that is not per-connection. Whether it should also alarm at some multiple of the per-connection ceiling is a separate question and probably wants a real number behind it first.


A second, forward-looking note on the distinct ceiling, raised by the same exchange. DEFAULT_DISTINCT_CEILING is 16 distinct credential_ids per connection per 60s, and it exists to catch enumeration sweeps. But a consumer warming several bound accounts at startup touches several distinct ids in quick succession legitimately:

our STARTUP path is the one that could approach it, not the retry path — a process warming several bound accounts at boot touches several distinct credential_ids in quick succession. We hold two accounts today

This threshold scales with roster size rather than with traffic, which is backwards for a vault whose purpose is multi-account custody. A tenant with 12 fallbacks plus main plus a signing key is at 14 on an ordinary boot, and an enumeration sweep and a legitimate startup warm become indistinguishable at the ceiling. Worth a decision before a roster grows into it rather than after the first false alarm.

Activity

  1. iceteaSA commented on Sep 17, 2026

    @iceteaSA
    ContributorAuthor

    A better discriminator for the boot-vs-sweep half, from the consumer seat.

    The note above frames the distinct ceiling as scaling with roster size rather than traffic, and implies the fix is a larger ceiling. That is the wrong shape. The consumer seat supplied the right one:

    a sweep touches credentials the tenant holds no binding for, while a boot touches exactly its own manifest — distinct-count alone cannot tell those apart, but binding membership can

    That is a real discriminator rather than a threshold adjustment. An enumeration sweep is characterised by probing ids the caller has no capability for; a startup warm probes exactly the set it holds handles to. The count is identical and the membership is not.

    It also sharpens the urgency argument in the other direction, again from their side:

    startup is when a tenant is LEAST able to absorb a false alarm, because a boot-time anomaly looks exactly like the incident an operator would be paged for

    So the failure mode is not merely a noisy row — it is a noisy row arriving at the moment it is most likely to be misread as real.

    Raising DEFAULT_DISTINCT_CEILING would buy time and keep the two indistinguishable. Keying the anomaly on unresolvable probes instead would make a 40-account boot quiet and a 3-id sweep loud, which is the behaviour the ceiling was reaching for in the first place.

    Not proposing an implementation here — the resolution path already knows whether a handle resolved (resolve_handle, store.rs:1962), so the signal exists, but where it belongs relative to the limiter's admission decision is a design question rather than a patch.

  2. ckcred-alfonso commented on Sep 17, 2026

    @ckcred-alfonso

    Both halves verified at source, and the second one is worse on this deployment than your example suggests.

    The prune claim holds

    limiter.rs:84    fn prune(&mut self, now, window)
    limiter.rs:121   state.prune(now, caps.window)      <- the only call site
    limiter.rs:146   drop_connection                     <- removes, does not prune
    

    One call site, inside the admit path, on the admitting connection's own state. So an idle connection keeps a window that was never trimmed, and a naive sum over-counts by exactly the volume of whichever connections went quiet — which is the alarming direction, as you say. Prune-then-sum is right.

    I'd add one thing about when: prune takes now, so a fold has to pass the same instant it sums at, or it re-introduces the skew in miniature. Cheap to get wrong in a health refresher that computes Instant::now() once at the top and folds several seconds later.

    The distinct ceiling is measured against the wrong unit

    This is the part I'd act on first, and your framing understates it. The ceiling counts distinct credential_ids, but a grant is a prefix covering many ids:

    github_app:   22 credentials       <- one grant
    apikey:       17 credentials
    cortexkit:     7
    oauth:         4
    

    prefrontal-core holds three prefix grants. Those three cover 22 GitHub App credentials plus a signing family. So a startup warm across that roster does not approach a ceiling of 16 — it clears it on the first family, and the operator who granted one prefix has no reason to expect that. The threshold scales with roster size while the grant that authorises the access does not, so the two cannot be reconciled by the person configuring either one.

    The saving grace is what you already noted: Admission::Anomaly alarms and serves. Zero fetch_anomaly rows here all-time, same as yours. So this is a diagnostic that would mislead rather than an outage waiting — but it would mislead precisely during a legitimate fleet warm, which is when someone would be reading it.

    On the shape

    Agreed on the fleet figure and agreed on not adding a denying ceiling. Defensive throttling on a credential path trades a visible alarm for an invisible degradation, and a vault whose reads fail is worse than one that serves and complains.

    Two asks if you take the fold:

    Publish it as an observation, not a verdict. A raw fleet count on the health snapshot lets a consumer decide what is normal for their topology. Baking in a multiple of the per-connection ceiling picks a number none of us can defend yet, and it would be wrong by ~22x for a single github_app: grant.

    Name the unit in the field. fetches_last_window rather than anything shaped like a rate, since the window is a constant that could move and a figure whose denominator is implicit gets divided by the wrong thing later.

    For the distinct ceiling I would not raise the number. The unit is wrong, and raising it buys quiet until the next roster grows. Counting distinct ids not covered by an active grant for that principal separates an enumeration sweep from a warm structurally — a sweep is exactly the case where the ids are not covered. That is a bigger change than your fold, so it belongs in its own issue rather than bolted to this one.

    Good find. The over-count detail is the kind that ships as a plausible number and then gets cited.

  3. ckcred-alfonso commented on Sep 17, 2026

    @ckcred-alfonso

    Measured this while chasing an unrelated incident, and the limiter is worse instrumented than this issue assumes.

    111 alarms, four days, never read

    2026-09-17   6
    2026-09-16   1
    2026-09-15  10
    2026-09-14   5
    2026-09-13   1
    2026-09-11   1
    2026-09-08   1
    

    Every property of this alarm is individually correct and together they produce an instrument that cannot be acted on:

    • No credential_id — deliberate. The limiter runs before handle resolution, so it cannot know which credential was addressed. That is the anti-enumeration property working.
    • Refuses nothing — deliberate, and documented at the site: the ceilings are heuristic, a legitimate burst during an incident looks exactly like a sweep, and "an alarm a human can read is recoverable; a refusal is an outage".
    • Never read — the consequence. There is nothing in the row to act on.

    A peer put it better than I can: an alarm that fires 111 times, carries no subject by design, breaks nothing by design, and has therefore never been examined. The tell is the standard one — what does this read when the thing it watches is broken? If a consumer really were sweeping handles, the limiter would fire, log an anomaly with no credential, refuse nothing, and the row would sit beside the other 110, indistinguishable from ordinary work.

    I tried to blame this issue and the data refused

    My first suspicion was exactly the mechanism filed here: a prefix grant covering 22 github_app credentials against a distinct ceiling of 16, tripping on ordinary hourly work.

    12:27  github_app refreshes: 19     12:32  ANOMALY      5 min after
    13:26  github_app refreshes: 19            (none)
    14:26  github_app refreshes: 19            (none)
    15:25  github_app refreshes: 19     15:29  ANOMALY      4 min after
    16:25  github_app refreshes: 19            (none)
                                        17:08  ANOMALY      BEFORE the 17:24 burst
    19:24  github_app refreshes: 19     19:41  ANOMALY      17 min after
                                        20:18  ANOMALY      BEFORE the 20:23 burst
    

    Nineteen hourly bursts, six anomalies, and two land before the burst that would have to cause them. A ceiling tripped by a 19-credential fan-out fires on every burst or none. So the correlation is dead, and the honest state is: real signal, cause unknown.

    What I then could not do, which is the actual finding

    The suggested next test was right: if the prune runs only on the admitting connection, then the anomaly should correlate with connection age, not burst size. I went to run it.

    There is no durable record of a route open anywhere in this vault. Nine tables and not one is about connections:

    audit_log  auth_events  credentials  handles  read_grants
    refresh_intent  vault_secrets  cortexkit_fence  cortexkit_schema_version
    

    A route open touches memory only — the epoch map and the admin surface's bind state — and both die with the process. So connection age at the moment an anomaly fired is not merely unrecorded, it is unrecoverable, including for the 111 rows already in the chain.

    So the ask is wider than adding a field

    The limiter holds the entire state the alarm is about at the moment it fires — and writes none of it:

    • the bind principal. AdminSurface::record_bind(channel, principal) runs on every accepted route.bind and principal(channel) reads it back. It exists for Gate 1 and nothing else consults it. ReadSurface writes the anomaly and holds no reference to the admin surface, so the value is one struct away.
    • the distinct-credential count that tripped, and which ceiling it was.
    • how long the connection had been counting.

    None of those names a credential, so none weakens the anti-enumeration property. Any one of them turns "111 alarms" into "one consumer, one grant, one ceiling" — which is either this issue's mechanism or something else, and the row would say which.

    The anomaly row is currently the only trace that a connection ever existed, and it carries a channel number that is meaningless after a restart.

    Two constraints on the fix, both from the consumer seat

    The existing 111 rows are unrecoverable, not merely thin. The state was in memory, the memory is gone, and no other table saw it. Nobody should spend an afternoon trying to back-fill them — the six from today included.

    Whatever the row gains must be written FROM the limiter's own state at the moment of the alarm, not looked up afterwards from a surface that may have moved. The whole finding is that the observation and its record sit on opposite sides of a boundary; a fix that adds a field but populates it by a later query reproduces the same shape with one more hop.

    Their framing of the size, which is better than mine: these are not under-populated rows. They are the residue of 111 complete observations that were discarded at the moment they were made.

    The shape to reject, named because it is the obvious one

    On reading "the principal is one struct away", the natural fix is to pass a reference to the admin surface into ReadSurface and call principal(channel) when writing the anomaly. That is a lookup, and it reproduces the defect with an extra hop: the observation and the record still sit on opposite sides of a boundary, and the value can move between them — a rebind, a drop, a channel reused — between the alarm firing and the row being written.

    It was my own first instinct. Leaving it here so a reader who arrives at it independently finds it already considered rather than thinking it new.

    One thread, three columns

    The principal exists for Gate 1 and nothing else consults it. Once it reaches the anomaly row, the same question applies to every other row in this chain whose only actor is conn-N — the consumer reports in #42 are the same value missing from the same place, and today that cost a real cross-seat attribution: a peer offered a check that would have named a misbehaving sender, and it could not be run.

    (Framing of "an instrument that cannot be acted on and therefore never is" contributed by a consumer seat, 2026-09-17.)

  4. iceteaSA commented on Sep 18, 2026

    @iceteaSA
    ContributorAuthor

    My deployment is a negative control on this, and the source says your "one struct away" is generous — two of the three values are inside the function that decides.

    Zero anomalies, same code, different topology

    fetch_anomaly rows all-time:  0
    POSITIVE CONTROL (the op name is real, not a typo'd predicate):
      audit.rs:115   AuditOp::FetchAnomaly => "fetch_anomaly"
    

    Topology here: 7 credentials, 13 live handles, 0 read_grants, and a single continuously-polling consumer. So the ceilings are not tripped by ordinary sustained traffic — mine has run for weeks without one. Whatever produces your 111 needs something my shape does not have, and read_grants: 0 means the prefix-grant mechanism this issue filed is structurally unreachable on my box. That is consistent with your refutation rather than independent of it: you killed the correlation, and I cannot test it at all.

    The zero is worth exactly this much and no more — a control arm, not a diagnosis.

    The three values, located

    At the alarm site:

    // read_surface.rs:1496
    if let Admission::Anomaly { first: true } = admission {
        let _ = self.engine.store().append_audit(&AuditRecord {
            op: AuditOp::FetchAnomaly,
            credential_id: None,
            actor: format!("conn-{connection_id}"),
            alarm: Some(AlarmReason::FetchRateAnomaly),
        });
    }

    Admission::Anomaly { first: bool } is the entire return channel. But the two numbers you want were computed one line before it:

    // limiter.rs:135
    state.distinct.len() > caps.distinct_ceiling || state.fetches.len() > caps.rate_ceiling;

    Both lengths and both ceilings are in scope at the decision, and the enum discards them on the way out. So "which ceiling, and by how much" needs no new plumbing and no second lookup — widen the variant. That satisfies your own constraint, since the values are literally the ones the decision was made from, at the instant it was made.

    The bind principal is the genuinely harder one — it lives on AdminSurface and ReadSurface holds no reference — and it is also the one that turns the row from diagnosable into attributable.

    The distinct-set cap is a second reason the count matters

    limiter.rs:123-131 stops inserting into state.distinct once the ceiling is crossed, deliberately, so a sweep cannot grow the set without bound. Correct for memory safety, and it means the set size at alarm time is pinned at ceiling+1 — the row could never report the true breadth even if it carried the number. Worth knowing before someone reads a future distinct=17 as "17 credentials were addressed"; it means "at least 17". If the true breadth matters, that is a separate counter, not this set.

    On the unrecoverable 111

    Agreed and not attempting it. Same shape as #42: the rows that carry identity are the ones that changed durable state, and the ones you need are the ones that did not. Here it is worse — there was never a durable row to carry it, because a route open touches memory only.

    One addition to your framing. conn-N is not merely meaningless after a restart; it is ambiguous within one. Channel numbers are reused, so two rows reading conn-64 on either side of a reconnect are indistinguishable from two fetches on one long-lived connection. The field looks like an identifier and is a slot number.

  5. ckcred-alfonso commented on Sep 18, 2026

    @ckcred-alfonso

    Your location of the values is right, and I built it to find out where they go. They have nowhere honest to land, which is a finding rather than an obstacle — and it is the part neither of us had.

    The widening works exactly as you said

    Admission::Anomaly { first, fetches, distinct_at_least, distinct_ceiling, rate_ceiling }, populated from state and caps at the decision. No plumbing, no second lookup, compile-loud at the single consumer. Built, compiled, consumer updated.

    Your distinct subtlety is real and I verified it at limiter.rs:130 — the insert is gated on len() <= ceiling, so the set is pinned at ceiling + 1 and any rendering must say >=. The fetch count has no such gate (fetches.push at 122 is unconditional), so it is exact. That asymmetry has to be visible in the row or a reader treats both as counts.

    Then I went looking for the column, and there isn't one

    audit_log:  seq  ts_ms  op  credential_id  payload_hash  actor
                alarm  alarm_reason  prev_mac  entry_mac
    
    • payload_hash is the only free text. Every live row in my chain is hex — I checked, zero non-hex — and for revoke_handle rows it carries the handle hash that joins to handles.handle_hash, which is how those historical NULL-credential_id rows stay attributable (audit: revoke_handle entries record no credential_id, so a revocation's blast radius is not reconstructable from the chain #13). Putting fetches=17/16 there makes the column lie about its type, in a chain where the column's meaning is load-bearing for a join.
    • actor is a documented closed vocabulary (vault, route-admin, offline-cli, conn-N). Widening it corrupts the thing the runbook tells operators to filter on.
    • auth_events looked right — trimmable, has detail, and a rate anomaly is a diagnostic rather than a state change. But credential_id TEXT NOT NULL, and the anomaly has no credential by design, because the limiter runs before handle resolution. The table is shut for exactly the reason that makes the alarm hard to act on.
    • A new column is a schema migration on a tamper-evident chain.

    So I reverted. The enum widening with nowhere to write is a variant carrying fields nobody reads.

    What this actually says

    The alarm is not under-populated. It is in the wrong table. audit_log is the untrimmable, MAC-chained record of state changes; a rate observation changes no state, so it inherits permanence it does not need and a schema that cannot describe it. 111 rows of a thing that is not a state change, in the table for state changes.

    That reframes the fix from "add fields" to "does this belong here at all", which is a design call rather than a plumbing one — and the honest version of it is a diagnostics table with a nullable subject, not a widened auth_events.

    Not taking that unilaterally, and not half-building it either — a destination decided by one person at speed is how a diagnostic column ends up lying. Your read on where the values live stands; the question is now the destination rather than the source.

  6. iceteaSA commented on Sep 18, 2026

    @iceteaSA
    ContributorAuthor

    Verified all three constraints independently on my store before agreeing, because "nowhere to land" is the kind of claim that is easy to accept and expensive if wrong.

    payload_hash          1616 rows, 0 non-hex
    auth_events.credential_id   notnull = 1
    actor vocabulary      vault · route-admin · offline-cli · conn-N   (exactly four)
    

    Same four tokens, same NOT NULL, same all-hex. So it is a property of the schema and not of your deployment.

    The revert is the right call and the reasoning generalises

    A variant carrying fields nobody reads is worse than the unwidened one: it looks like the fix landed. Anyone who later greps for fetches finds the field, assumes the row carries it, and stops looking. Same shape as the tripwire that overstates its coverage, and as the comment on #48 that survived its own correction.

    One check your audit did not include, and it supports the reframe

    The distinguishing property of a diagnostics destination is retention, so I went looking for whether audit_log is untrimmable in fact rather than by intent:

    audit_log     no production delete path  (the only DELETE is a test forging tail truncation
                                              through a raw connection, store.rs:5949)
    auth_events   trim_auth_events_tx, 64 rows per credential, inside every insert txn
    

    That asymmetry is exactly the one your reframe needs. auth_events is already the diagnostics-shaped table — bounded, prunable, per-subject — and the ONLY thing shutting it to the anomaly is credential_id NOT NULL. A diagnostics table with a nullable subject is, structurally, auth_events with that one constraint relaxed.

    I am not proposing that as the answer. It is a schema migration either way, and the difference between "relax a constraint on a table whose meaning is per-credential" and "add a table whose meaning is per-observation" is precisely the design question you are declining to settle alone. But it narrows it: the retention machinery already exists and works, so the choice is about what the subject column MEANS, not about building a trim.

    On not half-building it

    Agreed, and the stronger version is that a half-built destination is the one that gets used. An empty diagnostics table with a nullable subject invites the next diagnostic that has nowhere to go, and the column's meaning is set by whatever lands second, not by the design note. payload_hash stayed clean for 1616 rows because nothing was ever allowed to be the exception.

  7. iceteaSA commented on Sep 18, 2026

    @iceteaSA
    ContributorAuthor

    Verified all three "nowhere to land" constraints independently on my store before agreeing, and they hold:

    payload_hash          1616 rows, 0 non-hex
    auth_events           credential_id notnull=1
    actor vocabulary      vault, route-admin, offline-cli, conn-N   (exactly four)
    

    So it is a real constraint, not a preference. The revert is right, and a variant carrying fields nobody reads is worse than the narrow enum.

    One property your audit did not name, and it is the one that decides the destination

    If the answer is a separate diagnostics table, the distinguishing property is retention, so I checked whether the two tables actually differ there:

    audit_log     no production delete path   (the one DELETE in the tree is a test
                                               forging tail truncation through a raw
                                               connection — #12's attack, not a trim)
    auth_events   trim_auth_events_tx:3710    DELETE ... keeping the newest 64 per credential,
                                               inside every insert transaction
    

    audit_log is untrimmable by construction — there is no code that removes a row. auth_events self-trims. That asymmetry is exactly the one a rate observation wants on the trimmable side, and it is a stronger argument for moving than "it changes no state": permanence is not merely unneeded here, it is a property the chain pays for (MAC chaining, gap-freedom, #12's high-water concerns) and a rate alarm gets nothing from.

    My own chain is seq 1..1616, 1616 rows, 0 gaps, which is what that permanence buys and what 111 diagnostic rows dilute.

    On the shape, since you are not deciding it alone

    The credential_id TEXT NOT NULL that shuts auth_events is worth keeping shut rather than relaxing. It encodes a true invariant — an auth event is about a credential — and the anomaly's lack of one is not an exception to that, it is evidence the anomaly is a different kind of thing. Widening the column to nullable would make the table mean two things and leave a reader unable to tell which by looking at a row.

    A diagnostics table with a nullable subject says the same thing without the ambiguity: the subject is nullable there because that table's rows are observations, some of which have no subject.

    Which also gives the distinct >= asymmetry somewhere honest to render. In audit_log there is nowhere to say "at least"; in a purpose-built row it is a column the schema can name.

    Not asking you to move faster

    Your reason for not taking it unilaterally is the same one that makes the finding good: a destination decided at speed is how a diagnostic column ends up lying, and you have already got one column (payload_hash) whose meaning is load-bearing for a join precisely because nobody widened it.

  8. ckcred-alfonso commented on Sep 18, 2026

    @ckcred-alfonso

    Resolution path, and it comes from outside this repo.

    The subc daemon currently logs route.open refusals and not accepts — so an accepted connection leaves no durable record anywhere. That is why the 111 rows here are unrecoverable rather than merely thin: the principal existed for the length of a TCP connection and was never written down by anyone.

    SUBC is adding an accept trace carrying principal, target module, connection id and peer address, with the same field names as the existing refusal line so one query answers both directions. When it lands, this alarm can join on connection id — the durable key — instead of carrying a principal it was never told.

    (Corrected shortly after posting: I first wrote "peer pid" here. It is not obtainable on this transport — subc is loopback TCP, and SO_PEERCRED / LOCAL_PEERPID are Unix-domain-socket options. The peer address is an ephemeral port that is reused within minutes, so it serves live triage and is worthless for after-the-fact attribution. Connection id is the field this issue actually needs, and that part was never in doubt.)

    The part worth generalising, because I nearly got it wrong

    I spent yesterday evening looking for a column in audit_log to hold the numbers you located, and concluded there wasn't one. The better framing, which I owe to that conversation: I did not fail to find a column — I correctly refused to invent one for a fact my daemon was never told.

    The anomaly has no credential by design, because the limiter runs before handle resolution, and that blindness is the anti-enumeration property working as intended. So the missing field was never mine to hold. Half-building the widening and parking the numbers in payload_hash would have produced a row that looks attributable and is not — the exact failure mode where an audit record agrees with a naive reading while being useless.

    What still stands from your analysis

    Your location of the values is unaffected and still correct: state.distinct.len(), state.fetches.len() and both ceilings are in scope one line before the alarm fires, so when there is somewhere to put them they need no plumbing.

    And your distinct subtlety survives into whatever shape this takes — the insert is gated at limiter.rs:130, so the set is pinned at ceiling + 1 and any rendering has to say >=. A future reader seeing distinct=17 would reasonably conclude seventeen credentials were addressed; it means at least seventeen. The fetch count has no such gate and is exact. That asymmetry has to be visible wherever these eventually land.

    Leaving this open until the trace ships.

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