Repository navigation
canonical(state): two persisted concepts have no declared system of record — idempotency is Redis-only behind a TTL, and the pre-auth lockout is per-process memory #17450
Description
Gap
autobot_shared/store_authority.py declares 17 concepts. At least two persisted concepts are not declared, so nothing records what their durable store is — which is the exact condition the registry exists to prevent, and #15668's stated purpose.
Declared today:
activity_audit_trail · agent_work_claims · auth_sessions · auth_signing_key
autoresearch_proposal · chat_sessions · code_index · codebase_analytics
collaboration_event_history · device_credentials · device_pairing_challenge
identity_and_rbac · knowledge_facts · llc_work · multimodal_vectors
session_collaboration · verbatim_memory
1. Idempotency — Redis-only, behind a TTL
autobot-backend/middleware/idempotency_middleware.py claims and records idempotency keys in Redis and nowhere else: no SQLAlchemy model, no column, no durable row. The module's own comments describe the consequence of losing a claim:
the claim will lapse on its TTL
caller then gets 409 for the whole claim TTL and can neither retry ...
So the record that says "this request already ran" is a TTL'd Redis key. If Redis is lost, or the TTL passes before a retry, a replayed request executes a second time — which is the one thing an idempotency layer is for. Nothing declares which store is authoritative, so nothing forces the question.
2. Pre-auth throttle / lockout — in-process memory, not Redis at all
autobot-backend/mcp_server/auth_throttle.py keeps its state in the process:
50: from collections import OrderedDict, deque
93: self.failures: Deque[float] = deque()
112: self._ips: "OrderedDict[str, _IpRecord]" = OrderedDict()
113: self._global_failures: Deque[float] = deque()
No Redis, no durable store. Two consequences, neither written down anywhere:
- The lockout is per worker. With more than one process serving auth, an attacker's failed attempts are divided across workers, so the effective threshold is the configured one multiplied by the worker count.
- A restart clears every lockout. An in-progress attack resets for free on any deploy or crash-loop.
This is worse than a Redis-only concept, and it is worth stating plainly: Redis-only means one node's loss erases it; in-process means it was never shared in the first place. I have not established the deployed worker count — that is the measurement that turns this from a design gap into a sized one, and it needs host evidence.
Not established
Budget reservations. Reported as a third Redis-only concept. I found candidate call sites (autobot-backend/integrations/cloud_integration.py among them) but did not pin where in-flight reservations persist, so it is listed here as unverified rather than counted. Someone should measure it rather than assume it matches the other two.
Provenance
Raised by an external correspondent mapping their own storage model against this repo's store_authority pattern from PR #15668. They found the same class in their codebase — one concept with two systems of record, and three concepts held only in Redis with the semantics unrecorded. The three names came from them; the measurements above are this repo's own and differ from theirs (their idempotency splits Redis/Postgres, ours is Redis-only; their lockout is Redis, ours is in-process).
Their framing is the useful part and belongs in the fix: an undeclared store silently changes a concept's semantics. Nothing breaks, nothing logs, and the guarantee quietly is not the one the code reads as offering.
Acceptance criteria
-
idempotencydeclared instore_authority.pywith its system of record, and the TTL-expiry behaviour stated where a reader of the middleware will see it. -
pre_auth_throttledeclared, and the per-worker/restart-clearing behaviour either fixed by moving to a shared store or documented as accepted with its reasoning. Measure the deployed worker count first — the trade-off depends on it. - Budget reservations measured, then declared or shown not to be a persisted concept.
- A guard that fails when a module persists a concept — Redis key, table, or process-level store surviving a request — with no
store_authorityentry, with the existing undeclared set baselined and shrink-only perRATCHET_BASELINES.md. Without this the registry records only what someone remembered to add, which is how these two stayed absent. - The guard has a known-positive and known-negative fixture, and goes red against the current tree before the declarations land.
Related: #15668 (the registry and its guard), docs/developer/EVENT_STATE_DOCTRINE.md.
Activity
mrveiss commentedon Sep 25, 2026
Host evidence for the worker count, and it changes the severity
This was flagged as the measurement that sizes the lockout defect, so I took it from the running system rather than from config.
The backend runs one worker.
So the "effective threshold is N× the configured one" half is latent in this deployment, not live. With one worker there is one
PreAuthThrottleand the configured threshold is the actual threshold.But the second half is live regardless of worker count.
_throttleis a module-level singleton holdingOrderedDict[str, _IpRecord], so a restart clears every lockout — and nothing about that depends on how many workers there are. A process restart mid-attack resets the attacker's budget.The configuration that makes the first half live already exists in this tree
Worker counts across the deployment, counted from the host:
The backend is on 1 today. Nothing in the codebase ties the auth throttle's correctness to that value, and nothing fails if it changes. Raising the backend's worker count is an ordinary capacity change that would silently weaken a security control — which is the property worth fixing, more than the current number.
The throttle is wired, for the record
I checked, because an unwired control would be a different and larger finding:
It is reached on the real auth path.
The codebase already articulated this exact hazard, for a different subsystem
autobot-backend/llm_shared/cross_worker_rate_limiter.py(#8170) opens with:Same reasoning, written down, applied to LLM provider rate limits and not to an authentication control.
It is not a drop-in, and I want to be accurate about that. It is a per-provider token bucket keyed by provider name with
capacity/refill_ratesemantics — the right shape (Redis-backed, shared across workers) but not the right interface for an IP lockout. So the fix is "apply the established pattern", not "call the existing function". Worth knowing before someone scopes it as a one-line reuse.Why this belongs with the registry problem rather than beside it
The store-authority registry records what someone remembered to add. Nothing fails when an entry is missing, so a concept that is persisted-but-undeclared looks identical to a concept that does not exist. That is the same structure as #17419's two declaration planes, and the criterion is the one that applies there too:
A registry whose completeness is asserted by nothing will drift every time someone adds persistence without adding a declaration — and the drift is invisible by construction, because the registry is also the thing you would consult to find it.
Suggested criteria
store_authority.pydeclares every persisted concept, and a guard fails when a concept persists state without a declaration. Verified by failing against the current tree — with two known-missing entries it must go red before the fix.--workers 1a per-process implementation and a shared one are indistinguishable — testing at 1 proves nothing, exactly as aMAIN-based test proved nothing in Passing a RedisDatabase enum where a database NAME is expected silently routes to DB 0 — analytics writes land in main and nothing reads them there #17435.Stated limits
mrveiss commentedon Sep 25, 2026
My own acceptance criterion is not implementable, and the measurement says why
I wrote:
Measured before building it. Approximate, by grep rather than the real detector, and stated as approximate:
So "declare every persisted concept" means a 164-entry grandfather baseline — the whole population minus nine. A registry that declares everything distinguishes nothing, and a guard whose baseline is its population is the pattern this repo already calls out. The criterion as I wrote it would produce a worse artifact than the gap it closes. Withdrawing it.
The existing guard is narrower on purpose, and the narrowing is right
repo_tests/store_authority_test.py:370istest_no_undeclared_dual_store_module_exists, overdual_store_modules(). Its docstring is explicit: "Every module here writes the same concept into two durable stores."That is a defensible subject. A single Redis write with no second copy has no authority question — there is nothing to adjudicate, no projection to rebuild, no contradiction to resolve. Flagging all 164 would be measuring writes, not authority.
The guard also already has the things a good guard needs: a behaviour detector (
stores_written), positive controls (test_the_detector_matches_the_shapes_it_claims_to), and a reach floor (_MIN_MODULES_PARSED). It is not a weak guard.So the two missing concepts are missing for two DIFFERENT reasons
That distinction matters more than the count, because it decides the fix:
middleware/idempotency_middleware.py)dual_store_modules()excludes it by design — but Redis is its system of record, and this registry explicitly requires a justification note for that case (test_a_redis_system_of_record_is_justified). It belongs in the registry for a reason the guard does not test for.mcp_server/auth_throttle.py)StorehasPOSTGRES,REDIS,CHROMADB,DISKand no in-process member. The datum lives in anOrderedDicton a module-level singleton, which is not aStoreat all — sostores_written()correctly returns nothing and the concept cannot be declared even by someone who wanted to.The second is the interesting one. The registry has no vocabulary for the storage that is wrong, so the absence is not an oversight anyone could have corrected — it is a gap in the type, not in the table. That is a different defect from "somebody forgot".
Revised scope proposal
Narrower than my withdrawn criterion, and aimed at what the registry is actually for:
Storegains a member for process-local state, so a concept held in process memory can be declared and seen to be inadequate rather than being invisible. Declaring the lockout is then possible, and the declaration is what makes its weakness reviewable.--workers 1a per-process and a shared implementation are indistinguishable).idempotencydeclared with its Redis-as-SoR justification, or the datum moved if Redis is the wrong authority for it.Still not established
Budget reservations. Listed unmeasured on this issue, and I have not looked — saying so rather than letting the omission read as a clean result.
mrveiss commentedon Sep 25, 2026
Withdrawing the revised criterion too — measured, and it is the same trap
I said I would measure the population before committing to "flag a module whose system of record is Redis and which has no declaration", and call it the same trap if it came out in the hundreds. It did.
Approximating "Redis is the authority" as "writes Redis and no other durable store":
161 versus the 164 I withdrew an hour ago. The reframing sounded much narrower and measures almost identically, because in this codebase writing only Redis is the norm, not the exception. "Redis is the system of record" is not a discriminating property here — it describes nearly every Redis writer.
So that criterion is withdrawn on the same grounds as the first, and I am recording why it felt different when it was not: it named a more specific-sounding condition without changing the set it selects. A narrower sentence is not a narrower population.
Which leads to the actual conclusion, and it is not another guard
The dual-store guard works because dual-store is rare — nine declared concepts, and a real authority question in each. Redis-only is the default state of the codebase, so no population rule over it can separate the two concepts that matter from the 160 that do not.
There is no mechanical rule that catches these two and not 160 others. They belong in the registry by judgement:
Both are "this datum's authority matters", and that is a judgement a detector cannot make. Trying to make it mechanical is what produced two bad criteria in a row.
So the revised scope, now smaller and I believe finally right
Storegains a process-local member, so a concept held in process memory is declarable and visibly inadequate rather than unrepresentable. This is the only part that is a type fix rather than a judgement, and it is the part no audit of entries could ever have reached.notefield — the registry already requires justification for a Redis system of record, which is the right place for exactly this argument.--workers 1a per-process and a shared implementation are indistinguishable.What this exercise was worth
Two criteria written, both measured, both withdrawn. The measurements cost minutes; either criterion would have produced a 160-entry baseline that made the registry meaningless while looking like rigour. An acceptance criterion is a claim about the size of the work, and it can be wrong in that dimension alone — which is not visible from reading it.
Budget reservations: still unmeasured, still stated rather than implied.
fix(store-authority): declare two invisible concepts; share the pre-a…
mrveiss commentedon Oct 3, 2026
Closure check against
origin/main(24a9a1d46f): 2 of 5. Two criteria were withdrawn inside a PR, not by a rulingLanded in #17519 (
Refs).idempotencydeclared, TTL behaviour stated where a middleware reader sees itautobot_shared/store_authority.py:278request_idempotency;autobot-backend/middleware/idempotency_middleware.py:26and:123-196state the claim TTL and its lapse behaviourpre_auth_throttledeclared, per-worker behaviour fixed or accepted, worker count measured firststore_authority.py:292pre_auth_lockout, withStore.PROCESSadded (:58-59). Shared store with per-process degradation:mcp_server/auth_throttle_store.py/auth_throttle.py. The worker count was measured on the host in the comment above.store_authority.py, and PR #17519 does not address it.store_authorityentry (baselined, shrink-only)Needs a ruling on criteria 4-5. The withdrawal reasoning is substantive: a guard whose baseline is 164 of 173 records almost nothing. But the criterion's own text anticipated that ("Without this the registry records only what someone happened to notice"), and the decision to drop it was made inside the PR. The options:
Recommendation: 2. A shrink-only baseline is how this repo ratchets large existing populations elsewhere (
RATCHET_BASELINES.md). Its value is in stopping growth, not in the size of the population. Labelledneeds-decision. Criterion 3 is plain remaining work.