[bug] Whole-blob writes to agent_instances.config lose concurrent updates (16 call sites)

[serge-ivo]

Why

agent_instances.config is one JSON blob, and 16 call sites read it whole, mutate one key,
and write it whole:

routes/instances-apply.ts · routes/instances-translation.ts · routes/tools.ts
routes/instances-behaviour.ts · routes/coding.ts · routes/instances.ts
lib/behaviour-store.ts · lib/board.ts

Two concurrent requests touching different keys therefore lose one of them: both read the
same pre-state, and the second write clobbers the first.

This is not hypothetical — it already shipped a user-visible bug. The Behaviour tab's "Reset
group" fired one PUT per field; all five read the same config and the last write won, so it
cleared exactly one field, apparently at random. Fixed for that one caller (5365774) by batching
into a single request, but the underlying pattern is untouched and every other caller has it.

Realistic collisions today, none needing an unusual race:

• The agent calls set_behaviour (or configure_board, or a settings write) while the owner
  saves something in the console.
• A trigger-driven pipeline writes config while the owner edits Settings.
• Two browser tabs open on the same instance.

The loss is silent — no error, no conflict, the setting just doesn't stick.

Options

1. Optimistic concurrency — add a config_version column, UPDATE … WHERE config_version = ?, retry on 0 rows. Correct, one migration, every write site changes.
2. Single-key writes in SQL — json_set(config, '$.behaviour', ?) so a write only touches
   its own subtree. No migration; each site becomes a targeted patch. Doesn't help two writers of
   the same key, which is the rarer and less harmful case.
3. One helper all sites must go through (patchInstanceConfig(env, id, uid, key, value)),
   implementing whichever of the above is chosen, plus a lint/test forbidding raw
   UPDATE agent_instances SET config elsewhere.

(3) + (2) is probably the cheapest correct answer.

Verification

• A test that fires two concurrent patches to different keys and asserts BOTH survive. It must
  fail against today's code — otherwise it isn't reproducing the bug.

[serge-ivo]

Partial fix in 41471dc — the helper exists and two of the eight files are converted. Leaving this OPEN.

patchInstanceConfig / removeInstanceConfigKey (lib/instance-config.ts) issue a targeted json_set / json_remove, i.e. option (3)+(2) as you proposed. It does not help two writers of the same key — agreed that is the rarer and less harmful case.

Two things I verified against real SQLite before writing it, both of which would have made this fix silently useless:

• json('…') must be bound directly. Through a scalar subquery the JSON subtype is lost and an object lands as an escaped string — the trap migration 0071 documents at length. Bound directly it survives.
• '' (the column default) and malformed JSON must be coerced to {} first, or json_set returns NULL and the patch no-ops on a fresh instance. That would have been the same silent-failure shape this fix exists to remove.

The key is concatenated into a JSON path, so it is validated rather than trusted, even though every current caller passes a literal.

Converted: lib/board.ts (the agent's own configure_board can fire while the owner saves Settings) and the disabledTools toggle in routes/tools.ts — which I added earlier today, and which is exactly the kind of thing done with other settings open.

Not converted, and why: the remaining six files (routes/instances.ts ×5, instances-translation.ts ×2, coding.ts ×2, instances-behaviour.ts, instances-apply.ts, lib/behaviour-store.ts) are being edited by other sessions right now. Converting them concurrently would risk losing work to the very race this fixes. They are mechanical once the tree is quiet.

Also not done: the lint/test forbidding raw UPDATE agent_instances SET config. It would fail immediately on the six remaining sites, so it belongs with the last conversion — otherwise it is a broken build rather than a guard.

[serge-ivo]

Reassessed at 41471dc — partially fixed. The count in the title is now wrong (13 remaining, not 16).

lib/instance-config.ts landed with patchInstanceConfig / removeInstanceConfigKey doing targeted json_set / json_remove, which is option 2 + 3 from the description and the right shape. The commit body is careful about the parts that bite: '' and malformed JSON coerced to {} first, the key validated rather than concatenated blind, and json('…') bound directly so the JSON subtype survives (the trap migration 0071 documents).

Adopted by 2 files: lib/board.ts, routes/tools.ts.

13 raw whole-blob writes remain, across 7 files:

routes/tools.ts ← :209, despite this file already using the helper elsewhere
routes/instances.ts
routes/instances-apply.ts
routes/instances-translation.ts
routes/instances-behaviour.ts
routes/coding.ts
lib/behaviour-store.ts

routes/tools.ts having both patterns is the thing to watch: a half-migrated file is how the remaining sites stop looking like a to-do list. Two of the others (instances-behaviour.ts, behaviour-store.ts) are mine from #223/#224 and should move too.

Still missing from the description's asks:

• No lint or test forbidding a raw UPDATE agent_instances SET config outside the helper, so nothing stops the count going back up.
• No concurrency test — the verification section asks for one that fails against the old code. Worth adding with the migration, since it is the only thing that proves the helper is actually doing what it claims under two writers.

[serge-ivo]

Complete in 82d1f6c. All sixteen call sites are now targeted json_set/json_remove, and there is a guard so there is no seventeenth.

Your option (3)+(2) was the right call. patchInstanceConfig/removeInstanceConfigKey (lib/instance-config.ts) + a test that walks workers/api/src and fails on any UPDATE agent_instances SET config = ?, naming the offending files. Proven non-vacuous: reintroducing one whole-blob write fails it with Offenders: routes/coding.ts; removing it passes again. A reviewer cannot be expected to catch this by eye when the failure mode is silent.

Three things found on the way that are worth recording:

1. Verified against real SQLite first, and two details would have made the fix silently useless. json('…') must be bound DIRECTLY — through a scalar subquery the JSON subtype is lost and objects land as escaped strings (the trap migration 0071 documents). And '' (the column default) must be coerced to {} or json_set returns NULL and the patch no-ops on a fresh instance — the same silent-failure shape being removed.

2. The conversion introduced a regression the tests caught. These routes used to mutate the local cfg and then write it, so the RESPONSE could be built from that object. A per-key patch mutates nothing locally, so every response derived from the pre-write read became stale by one save: saving a voice speed echoed back the previous one, "use my defaults" reported the override it had just deleted, and attachSettingRepo got the settings from before the save — so a repo would not attach on the turn you first named it. Each now resolves from the value as written.

3. Two sites shed a read-parse-try/catch commented "overwrite a corrupt config". With a per-key patch there is nothing to overwrite: json_set coerces a malformed blob in SQL and touches only the named key, so a corrupt config can no longer cost the owner their other settings on the way past.

Assertions moved with the behaviour rather than being loosened — tests now assert the statement issued (json_set '$.displayName'), and several additionally assert that no whole-blob write was issued at all.