Repository navigation
The fetch limiter holds fleet-wide data but can only decide per-connection, so aggregate volume is structurally invisible #43
Description
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
iceteaSA commentedon Sep 17, 2026
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:
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:
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_CEILINGwould 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.ckcred-alfonso commentedon Sep 17, 2026
Both halves verified at source, and the second one is worse on this deployment than your example suggests.
The prune claim holds
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:
prunetakesnow, 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 computesInstant::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:prefrontal-coreholds 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::Anomalyalarms and serves. Zerofetch_anomalyrows 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_windowrather 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.
ckcred-alfonso commentedon Sep 17, 2026
Measured this while chasing an unrelated incident, and the limiter is worse instrumented than this issue assumes.
111 alarms, four days, never read
Every property of this alarm is individually correct and together they produce an instrument that cannot be acted on:
credential_id— deliberate. The limiter runs before handle resolution, so it cannot know which credential was addressed. That is the anti-enumeration property working.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_appcredentials against a distinct ceiling of 16, tripping on ordinary hourly work.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:
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:
AdminSurface::record_bind(channel, principal)runs on every acceptedroute.bindandprincipal(channel)reads it back. It exists for Gate 1 and nothing else consults it.ReadSurfacewrites the anomaly and holds no reference to the admin surface, so the value is one struct away.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
ReadSurfaceand callprincipal(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.)
iceteaSA commentedon Sep 18, 2026
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
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: 0means 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:
Admission::Anomaly { first: bool }is the entire return channel. But the two numbers you want were computed one line before it: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
AdminSurfaceandReadSurfaceholds 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-131stops inserting intostate.distinctonce 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 futuredistinct=17as "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-Nis not merely meaningless after a restart; it is ambiguous within one. Channel numbers are reused, so two rows readingconn-64on 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.ckcred-alfonso commentedon Sep 18, 2026
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 fromstateandcapsat the decision. No plumbing, no second lookup, compile-loud at the single consumer. Built, compiled, consumer updated.Your
distinctsubtlety is real and I verified it atlimiter.rs:130— the insert is gated onlen() <= ceiling, so the set is pinned atceiling + 1and any rendering must say>=. The fetch count has no such gate (fetches.pushat 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
payload_hashis the only free text. Every live row in my chain is hex — I checked, zero non-hex — and forrevoke_handlerows it carries the handle hash that joins tohandles.handle_hash, which is how those historical NULL-credential_idrows stay attributable (audit: revoke_handle entries record no credential_id, so a revocation's blast radius is not reconstructable from the chain #13). Puttingfetches=17/16there makes the column lie about its type, in a chain where the column's meaning is load-bearing for a join.actoris a documented closed vocabulary (vault,route-admin,offline-cli,conn-N). Widening it corrupts the thing the runbook tells operators to filter on.auth_eventslooked right — trimmable, hasdetail, and a rate anomaly is a diagnostic rather than a state change. Butcredential_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.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_logis 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
diagnosticstable with a nullable subject, not a widenedauth_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.
iceteaSA commentedon Sep 18, 2026
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.
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
fetchesfinds 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_logis untrimmable in fact rather than by intent:That asymmetry is exactly the one your reframe needs.
auth_eventsis already the diagnostics-shaped table — bounded, prunable, per-subject — and the ONLY thing shutting it to the anomaly iscredential_id NOT NULL. A diagnostics table with a nullable subject is, structurally,auth_eventswith 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
diagnosticstable 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_hashstayed clean for 1616 rows because nothing was ever allowed to be the exception.iceteaSA commentedon Sep 18, 2026
Verified all three "nowhere to land" constraints independently on my store before agreeing, and they hold:
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_logis untrimmable by construction — there is no code that removes a row.auth_eventsself-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 NULLthat shutsauth_eventsis 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
diagnosticstable 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. Inaudit_logthere 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.ckcred-alfonso commentedon Sep 18, 2026
Resolution path, and it comes from outside this repo.
The subc daemon currently logs
route.openrefusals 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_PEERPIDare 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_logto 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_hashwould 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
distinctsubtlety survives into whatever shape this takes — the insert is gated atlimiter.rs:130, so the set is pinned atceiling + 1and any rendering has to say>=. A future reader seeingdistinct=17would 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.