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
Related
#15779, #16927
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_idthroughupdate_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 UPDATErow 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 concurrentupdate_factcaller, 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
conditionsthat can be stale relative to the state actually overwritten.Acceptance criteria
update_factcan make its write conditional on the state it was judged against: a compare-and-set on the durable row, e.g. the expectedowner_idor anupdated_at/ version token. A mismatch returns a distinct status instead of writing.services/orphan_repair_types.pyKnowledgeFactRepairer.repair) passes theowner_idit judged as the precondition. A mismatch is refused (NotAnOrphan) and audited.Related
#15779, #16927