[P1 data integrity] Concurrent agent record saves can orphan records from the secondary index #139

New issue

Open

opened 2026-07-28 23:08:09 +00:00 by lost-rob0t · 0 comments

lost-rob0t commented

2026-07-28 23:08:09 +00:00

(Migrated from github.com)

Finding

Agent records are written as two independent PouchDB operations:

const stored = await putState(record._id, stripPouchFields(record));
await updateIndex(stored);

updateIndex() performs an unguarded read-modify-write of one shared index document:

const index = await getState(INDEX_ID, { records: {} });
const records = { ...(index.records || {}) };
records[record._id] = metadata;
await putState(INDEX_ID, { schemaVersion: AGENT_SCHEMA_VERSION, records });

putState() itself reads the current revision once and performs one put(), with no conflict retry.

If two agent-system records are saved concurrently:

both saves can successfully write their individual record documents;
both index updates read the same index revision;
one index update succeeds and the other receives a 409 conflict;
the losing record remains stored but is absent from the index;
listAgentRecords() cannot discover it, because listing trusts only the index.

Deletes have the same race. Concurrent runs, tool calls, imports, UI edits, and recovery writes make this realistic. The caller may see an error even though the primary record was already committed.

Required fix

Prefer eliminating the fragile secondary index. Record IDs already share a sortable prefix, so list directly from the database:

const PREFIX = "agent-system:";

export async function listAgentRecords(type) {
  const result = await stateDb.allDocs({
    startkey: PREFIX,
    endkey: `${PREFIX}\ufff0`,
    include_docs: true
  });

  return result.rows
    .map((row) => row.doc)
    .filter(Boolean)
    .filter((record) => !type || record.recordType === type)
    .map(stripPouchFields)
    .sort((left, right) =>
      String(right.updatedAt).localeCompare(String(left.updatedAt))
    );
}

If an index is still required for scale, treat it as rebuildable derived state and update it with a bounded compare-and-swap retry:

async function mutateState(id, mutate, { retries = 8 } = {}) {
  for (let attempt = 0; attempt < retries; attempt += 1) {
    const current = await getState(id, null);
    const next = mutate(current);
    try {
      return await stateDb.put({
        ...next,
        _id: id,
        ...(current?._rev ? { _rev: current._rev } : {})
      });
    } catch (error) {
      if (error?.status !== 409 || attempt === retries - 1) throw error;
      await new Promise((resolve) => setTimeout(resolve, 2 ** attempt));
    }
  }
}

A repair/rebuild command should scan prefix records and recreate any derived index.

Primary-record mutation also needs explicit optimistic concurrency semantics. Do not silently overwrite a newer agent configuration after reading an old revision.

Acceptance criteria

Two or more concurrent saves cannot make a stored record disappear from listAgentRecords().
Concurrent save/delete operations do not lose unrelated index entries.
Listing remains correct after a crash between primary-record and index writes.
A deterministic repair operation rebuilds derived indexes from primary records.
Callers can distinguish “primary committed, derived update failed” from a complete failure until the index is removed.
Tests use barriers to force two writers to read the same revision and verify no orphan/lost entry occurs.

## Finding Agent records are written as two independent PouchDB operations: ```js const stored = await putState(record._id, stripPouchFields(record)); await updateIndex(stored); ``` `updateIndex()` performs an unguarded read-modify-write of one shared index document: ```js const index = await getState(INDEX_ID, { records: {} }); const records = { ...(index.records || {}) }; records[record._id] = metadata; await putState(INDEX_ID, { schemaVersion: AGENT_SCHEMA_VERSION, records }); ``` `putState()` itself reads the current revision once and performs one `put()`, with no conflict retry. If two agent-system records are saved concurrently: 1. both saves can successfully write their individual record documents; 2. both index updates read the same index revision; 3. one index update succeeds and the other receives a 409 conflict; 4. the losing record remains stored but is absent from the index; 5. `listAgentRecords()` cannot discover it, because listing trusts only the index. Deletes have the same race. Concurrent runs, tool calls, imports, UI edits, and recovery writes make this realistic. The caller may see an error even though the primary record was already committed. ## Required fix Prefer eliminating the fragile secondary index. Record IDs already share a sortable prefix, so list directly from the database: ```js const PREFIX = "agent-system:"; export async function listAgentRecords(type) { const result = await stateDb.allDocs({ startkey: PREFIX, endkey: `${PREFIX}\ufff0`, include_docs: true }); return result.rows .map((row) => row.doc) .filter(Boolean) .filter((record) => !type || record.recordType === type) .map(stripPouchFields) .sort((left, right) => String(right.updatedAt).localeCompare(String(left.updatedAt)) ); } ``` If an index is still required for scale, treat it as rebuildable derived state and update it with a bounded compare-and-swap retry: ```js async function mutateState(id, mutate, { retries = 8 } = {}) { for (let attempt = 0; attempt < retries; attempt += 1) { const current = await getState(id, null); const next = mutate(current); try { return await stateDb.put({ ...next, _id: id, ...(current?._rev ? { _rev: current._rev } : {}) }); } catch (error) { if (error?.status !== 409 || attempt === retries - 1) throw error; await new Promise((resolve) => setTimeout(resolve, 2 ** attempt)); } } } ``` A repair/rebuild command should scan prefix records and recreate any derived index. Primary-record mutation also needs explicit optimistic concurrency semantics. Do not silently overwrite a newer agent configuration after reading an old revision. ## Acceptance criteria - Two or more concurrent saves cannot make a stored record disappear from `listAgentRecords()`. - Concurrent save/delete operations do not lose unrelated index entries. - Listing remains correct after a crash between primary-record and index writes. - A deterministic repair operation rebuilds derived indexes from primary records. - Callers can distinguish “primary committed, derived update failed” from a complete failure until the index is removed. - Tests use barriers to force two writers to read the same revision and verify no orphan/lost entry occurs.