[Bug] Label seeding and session-state writes race under concurrent updates — lost updates and orphan rows
Summary
Two write paths on ConversationStore decide "what to write" from a
snapshot read before the write, rather than inside the write's own
transaction. Under concurrency, each loses data:
- Label seeding loses a concurrent policy write. Initial-label seeding
reads current labels, computes which declared defaults are missing, then
upserts those. A policy write landing between the read and the upsert is
overwritten back to its initial value.
- Session-state updates lose concurrent increments. State is persisted
by writing the whole blob back from the caller's in-memory snapshot. Two
parallel tool calls on one session each hold their own copy; the second
write overwrites the first wholesale — two policies each incrementing a
counter by one persists a total of one, not two.
A third, related gap: neither write path distinguishes "the row doesn't
exist" from "the row is empty." increment_session_usage treats an absent
conversation row as an empty one, so the UPDATE matches nothing and the
mutated total is returned as if persisted — nothing was written, but the
caller isn't told. Label seeding has the sharper version of the same gap:
labels carry no foreign key, so seeding against a conversation that no
longer exists leaves an orphan row behind.
Mechanism
omnigent/stores/conversation_store/sqlalchemy_store.py — label seeding:
read-then-upsert, no existence check, no locking between the read and the
write.- Same file —
set_session_state: writes the full blob from the caller's
snapshot; two overlapping writers silently clobber each other since
neither txn observes the other's change.
omnigent/runtime/policies/engine.py — PolicyEngine.apply_state_updates:
applies every op straight onto the engine's own in-memory
self._session_state, then persists that whole dict — same
snapshot-then-blind-overwrite shape as set_session_state above, one
layer up (the engine, not the store).
Reproduction
- Label race: seed conversation A with declared initial labels; before
the seed's upsert lands, have a concurrent caller write a real value to
one of those same keys via the normal label-write path; the seed's insert
wins and the concurrent write is lost.
- State race: two threads each read the same session's state, each
increment a counter by one in memory, each call the plain
read-modify-write persist; final persisted value is +1, not +2.
Suggested fix
- Insert-if-absent for label seeding as one statement (
ON CONFLICT DO NOTHING / dialect equivalent), so the database — not a prior Python
snapshot — decides which keys are missing.
- A locked read-merge-write for session-state mutation (
SELECT ... FOR UPDATE where supported, BEGIN IMMEDIATE on SQLite to take the write
lock before the first read), so two overlapping writers serialize instead
of clobbering.
- Gotcha for whoever builds this: once the engine's local
self._session_state cache has to reconcile with a locked round trip
through the store (rather than just writing its own copy straight
through), a blanket {**old_cache, **persisted_result} union is the
naive-looking merge and it's wrong — it cannot tell "missing from the
persisted result because this call just deleted it" apart from "missing
because it was never part of this row to begin with" (the second case is
real: a sub-agent's root-inherited cost-approval key is never part of its
own row at all). The correct merge tracks which keys this batch of
operations deleted and excludes only those from what survives the old
cache — not a blanket union, and not a hardcoded key-name exemption list
either (name-based exemption can't tell a root's own copy of a reserved
key apart from a sub-agent's inherited one).
- State explicitly on the store abstraction that an absent row must raise
rather than being treated as empty, for every write path that reads
before it writes — so an out-of-tree implementation can't phantom-write
while nominally conforming to the interface.
Caveats / scope
- The row-missing contract change is a breaking API addition on
ConversationStore (two new abstract methods with no safe default
implementation) — any out-of-tree subclass needs updating before
upgrading.
- Two existing callers (usage accumulation on the relay path, and the
cost-ask checkpoint mirror to a session's tree root) need to absorb the
new not-found signal rather than propagate it: a session deleted mid-turn
has nothing to bill, and a lost approval checkpoint just re-prompts —
neither should fail an in-flight streaming turn.
- Not a performance issue — both races are pre-existing on
main today and
can produce silently wrong data under ordinary concurrent load (multiple
sub-agents/tool calls against one session), not just under adversarial
conditions.
[Bug] Label seeding and session-state writes race under concurrent updates — lost updates and orphan rows
Summary
Two write paths on
ConversationStoredecide "what to write" from asnapshot read before the write, rather than inside the write's own
transaction. Under concurrency, each loses data:
reads current labels, computes which declared defaults are missing, then
upserts those. A policy write landing between the read and the upsert is
overwritten back to its initial value.
by writing the whole blob back from the caller's in-memory snapshot. Two
parallel tool calls on one session each hold their own copy; the second
write overwrites the first wholesale — two policies each incrementing a
counter by one persists a total of one, not two.
A third, related gap: neither write path distinguishes "the row doesn't
exist" from "the row is empty."
increment_session_usagetreats an absentconversation row as an empty one, so the UPDATE matches nothing and the
mutated total is returned as if persisted — nothing was written, but the
caller isn't told. Label seeding has the sharper version of the same gap:
labels carry no foreign key, so seeding against a conversation that no
longer exists leaves an orphan row behind.
Mechanism
omnigent/stores/conversation_store/sqlalchemy_store.py— label seeding:read-then-upsert, no existence check, no locking between the read and the
write.
set_session_state: writes the full blob from the caller'ssnapshot; two overlapping writers silently clobber each other since
neither txn observes the other's change.
omnigent/runtime/policies/engine.py—PolicyEngine.apply_state_updates:applies every op straight onto the engine's own in-memory
self._session_state, then persists that whole dict — samesnapshot-then-blind-overwrite shape as
set_session_stateabove, onelayer up (the engine, not the store).
Reproduction
the seed's upsert lands, have a concurrent caller write a real value to
one of those same keys via the normal label-write path; the seed's insert
wins and the concurrent write is lost.
increment a counter by one in memory, each call the plain
read-modify-write persist; final persisted value is
+1, not+2.Suggested fix
ON CONFLICT DO NOTHING/ dialect equivalent), so the database — not a prior Pythonsnapshot — decides which keys are missing.
SELECT ... FOR UPDATEwhere supported,BEGIN IMMEDIATEon SQLite to take the writelock before the first read), so two overlapping writers serialize instead
of clobbering.
self._session_statecache has to reconcile with a locked round tripthrough the store (rather than just writing its own copy straight
through), a blanket
{**old_cache, **persisted_result}union is thenaive-looking merge and it's wrong — it cannot tell "missing from the
persisted result because this call just deleted it" apart from "missing
because it was never part of this row to begin with" (the second case is
real: a sub-agent's root-inherited cost-approval key is never part of its
own row at all). The correct merge tracks which keys this batch of
operations deleted and excludes only those from what survives the old
cache — not a blanket union, and not a hardcoded key-name exemption list
either (name-based exemption can't tell a root's own copy of a reserved
key apart from a sub-agent's inherited one).
rather than being treated as empty, for every write path that reads
before it writes — so an out-of-tree implementation can't phantom-write
while nominally conforming to the interface.
Caveats / scope
ConversationStore(two new abstract methods with no safe defaultimplementation) — any out-of-tree subclass needs updating before
upgrading.
cost-ask checkpoint mirror to a session's tree root) need to absorb the
new not-found signal rather than propagate it: a session deleted mid-turn
has nothing to bill, and a lost approval checkpoint just re-prompts —
neither should fail an in-flight streaming turn.
maintoday andcan produce silently wrong data under ordinary concurrent load (multiple
sub-agents/tool calls against one session), not just under adversarial
conditions.