Skip to content

knowledge: update_fact is an unconditional read-modify-write — concurrent writers lose updates, and an orphan repair cannot be made to hold its judgment #16984

Description

@mrveiss

Found by the code-review pass on #16927 (break-glass orphan repair, #15779).

Problem

KnowledgeBase.update_fact (autobot-backend/knowledge/facts.py) is an unconditional read-modify-write. It reads the fact's current metadata (_read_fact_for_write), merges the caller's changes, then writes the durable row (fact_projection._durable_update_or_adopt → fact_store.update_fact) and the Redis projection. Nothing makes the write conditional on the state that was read. Two concurrent writers can both read the same state, and the second write silently replaces the first.

Why it matters here

The break-glass orphan repair in #16927 judges a knowledge fact (no live owner, no share held by a live user, no reachable scope) and then writes a new owner_id through update_fact. The judgment and the write are separate steps, and the store gives no way to make the write depend on the judgment. Two admins repairing the same orphan at the same time can both pass the judgment, and the second write replaces the first new owner.

Envelope secrets do not have this gap. #16927 judges them under a SELECT … FOR UPDATE row lock held until commit, and a Postgres test proves a concurrent second repair is refused. Knowledge facts have no equivalent. The same lost-update race applies to every other concurrent update_fact caller, not only the repair.

Impact is bounded. The repair only adds an owner and never removes a share, so this does not disclose anything to an unauthorized party. It is a lost update on an admin-only path, plus audit conditions that can be stale relative to the state actually overwritten.

Acceptance criteria

  • update_fact can make its write conditional on the state it was judged against: a compare-and-set on the durable row, e.g. the expected owner_id or an updated_at / version token. A mismatch returns a distinct status instead of writing.
  • The orphan repair (services/orphan_repair_types.py KnowledgeFactRepairer.repair) passes the owner_id it judged as the precondition. A mismatch is refused (NotAnOrphan) and audited.
  • A test runs two repairs of one fact against the same store and asserts that exactly one succeeds.

Related

#15779, #16927

Activity

  1. added this to the v0.9.0 milestone on Sep 18, 2026
  2. mrveiss commented on Sep 28, 2026

    @mrveiss
    OwnerAuthor

    Chunking triage — a proposal, not an assignment

    Nothing was relabelled, moved or closed by this pass.

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

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions