Skip to content

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

Open
1 of 1 issue completed

Description

@mrveiss

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

  • idempotency declared in store_authority.py with its system of record, and the TTL-expiry behaviour stated where a reader of the middleware will see it.
  • pre_auth_throttle declared, 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_authority entry, with the existing undeclared set baselined and shrink-only per RATCHET_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

  1. mrveiss commented on Sep 25, 2026

    @mrveiss
    OwnerAuthor

    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.

    pgrep -af "autobot-backend/venv/bin/uvicorn main:app"  ->  --workers 1
    process count                                          ->  2  (uvicorn master + 1 worker)
    

    So the "effective threshold is N× the configured one" half is latent in this deployment, not live. With one worker there is one PreAuthThrottle and the configured threshold is the actual threshold.

    But the second half is live regardless of worker count. _throttle is a module-level singleton holding OrderedDict[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:

    48 × --workers 1
    11 × --workers 2
     4 × --workers 4
     4 × --workers 8
     7 × --workers=1
    

    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:

    mcp_server/autobot_server.py:477  blocked, reason = get_pre_auth_throttle().check(client_ip)
    mcp_server/autobot_server.py:505  get_pre_auth_throttle().record_success(client_ip)
    mcp_server/autobot_server.py:523  get_pre_auth_throttle().record_failure(client_ip)
    

    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:

    When uvicorn runs with --workers N, each worker is a separate OS process with its own memory space. An in-process rate limiter is invisible to sibling workers — so the effective aggregate rate is N× the configured limit.

    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_rate semantics — 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 guard counting only the buckets it knows about keeps reporting clean about the one it does not look in.

    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

    Stated limits

    • I measured the backend's worker count and the process count. I did not assess exploitability, attacker economics, or whether the current threshold is appropriate — those are judgements for whoever owns the threat model.
    • The worker-count tallies above are file occurrences across the deployment tree, not a count of running services. They establish that non-1 values are in use somewhere, not which services use them.
  2. mrveiss commented on Sep 25, 2026

    @mrveiss
    OwnerAuthor

    My own acceptance criterion is not implementable, and the measurement says why

    I wrote:

    store_authority.py declares every persisted concept, and a guard fails when a concept persists state without a declaration.

    Measured before building it. Approximate, by grep rather than the real detector, and stated as approximate:

    modules with a durable Redis write (setex/hset/lpush/rpush/sadd/zadd) : ~173
    of those, NOT declared as a write site in store_authority.py          : ~164
    

    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:370 is test_no_undeclared_dual_store_module_exists, over dual_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:

    Concept Why the guard cannot see it
    idempotency (middleware/idempotency_middleware.py) writes one store, so 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.
    pre-auth lockout (mcp_server/auth_throttle.py) not expressible. Store has POSTGRES, REDIS, CHROMADB, DISK and no in-process member. The datum lives in an OrderedDict on a module-level singleton, which is not a Store at all — so stores_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:

    • Store gains 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.
    • A guard flags a module whose system of record is Redis and which has no declaration — the case the registry already says needs justifying. Population to be measured before committing to it; if that is also in the hundreds, it is the same trap and I will say so.
    • The lockout survives a restart and is shared across workers, following perf(llm): add centralized LLM rate limiter shared across all 4 uvicorn workers via Redis token bucket #8170's pattern, verified at a worker count above one (at --workers 1 a per-process and a shared implementation are indistinguishable).
    • idempotency declared 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.

  3. mrveiss commented on Sep 25, 2026

    @mrveiss
    OwnerAuthor

    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":

    writes Redis                                       173
    Redis-ONLY  (approx. Redis is the system of record) 164
    of those, undeclared                               161
    

    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:

    • idempotency — replay protection. Losing the datum changes observable behaviour, so which store owns it is a correctness question, not a caching one.
    • pre-auth lockout — a security control whose storage makes it weaker than configured.

    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

    • Store gains 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.
    • The two concepts are declared, by judgement, with the reasoning in the note field — the registry already requires justification for a Redis system of record, which is the right place for exactly this argument.
    • The pre-auth lockout survives a restart and is shared across workers, following perf(llm): add centralized LLM rate limiter shared across all 4 uvicorn workers via Redis token bucket #8170, verified above one worker — at --workers 1 a per-process and a shared implementation are indistinguishable.
    • No new population guard. Explicitly out of scope, with the 161 figure recorded so the next person does not re-derive it and reach the same dead end. The existing dual-store guard stays as it is; its narrowness is correct.

    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.

  4. added
    needs-decisionBlocked on an owner decision; options and a recommendation are on the issue
    on Oct 3, 2026
  5. mrveiss commented on Oct 3, 2026

    @mrveiss
    OwnerAuthor

    Closure check against origin/main (24a9a1d46f): 2 of 5. Two criteria were withdrawn inside a PR, not by a ruling

    Landed in #17519 (Refs).

    Criterion State Evidence
    idempotency declared, TTL behaviour stated where a middleware reader sees it Met autobot_shared/store_authority.py:278 request_idempotency; autobot-backend/middleware/idempotency_middleware.py:26 and :123-196 state the claim TTL and its lapse behaviour
    pre_auth_throttle declared, per-worker behaviour fixed or accepted, worker count measured first Met store_authority.py:292 pre_auth_lockout, with Store.PROCESS added (: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.
    Budget reservations measured, then declared or shown not to be persisted Not met No reservation concept in store_authority.py, and PR #17519 does not address it.
    A guard fails on a persisted concept with no store_authority entry (baselined, shrink-only) Withdrawn, not delivered PR #17519 records withdrawing it "after measuring": 164 of ~173 concepts undeclared, and the narrower "Redis-as-authority" form selects 161, "so the property does not discriminate".
    The guard has positive and negative fixtures, red before the declarations Withdrawn, as above

    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:

    1. Accept the withdrawal and amend the criteria.
    2. Keep the guard with the 164-entry shrink-only baseline as written, which makes every new undeclared concept fail even if the existing ones do not.
    3. Find a narrower discriminating property first.

    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. Labelled needs-decision. Criterion 3 is plain remaining work.

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

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions