Race-Condition & Redundancy Audit

Scope: src/kiss/core, src/kiss/agents/sorcar, src/kiss/server, src/kiss/agents/vscode · Baseline commit c8e9dd2e · September 11, 2026

Development and every fix were done by claude-fable-5; three successive read-only review rounds were done by gpt-5.6-sol (each review sub-agent switched itself with set_model('gpt-5.6-sol') and was instructed to report only demonstrable issues, never invented ones). Each round the reviewer tried to break the previous round's fixes and to find anything missed; each round its confirmed findings were reproduced with a failing end-to-end test before the fix landed. The loop ended when the remaining reviewer items were either fixed or documented residual windows requiring multiple independently-timed thread-kill injections.

The audit ran three full fix→review cycles. 21 confirmed defects were fixed; every one has a regression test that failed before its fix.

Outcome at a glance

Directory	Race conditions fixed	Redundancies fixed	New regression tests
`src/kiss/core`	1	1	2 files
`src/kiss/agents/sorcar`	8	0	3 files (24 tests)
`src/kiss/server`	10	0	6 files (54 tests)
`src/kiss/agents/vscode`	5	1	6 files

Total: 24 production files changed (+3,272/−413 in the four directories), plus 17 new test files. uv run check --full passes; the impacted suites pass: 704 server tests, 526 sorcar persistence/worktree tests, 308 core model tests, and the extension's 297 jsdom suites (tsc clean).

src/kiss/core

Finding	Kind	Fix
Duplicated system-message hoisting logic in `anthropic_model.py` and `gemini_model.py` (two verbatim ~14-line copies that could drift).	Redundancy	Extracted one shared `merge_system_texts()` into `models/model.py`; both adapters call it. Parity pinned by `test_system_hoist_dedup.py`.
Cross-process key resurrection in `vscode_config.py`: the legacy-RC migration decided under only the process-local lock, so a second process's completed `save_api_key(key, "")` deletion could be overwritten with the stale legacy value. Reproduced with two real subprocesses.	Race	The migration now holds the store's sidecar flock across its whole read→scan→source→write transaction, and `save_api_key` holds it across store-edit + RC-scrub (lock order: store flock → RC flock; no same-process double-flock). Test: `test_fixer_migration_resurrection.py` (deterministic two-process schedule).

src/kiss/agents/sorcar

Finding	Fix
Worktree merges/discards on the shared main tree ran under only the process-local lock; a peer process's `git reset --hard` could wipe a staged squash merge, which then reported success on an empty index and deleted the branch — work lost.	`_do_merge()` and `discard()` hold the cross-process `kiss-reclaim.lock` flock inside `repo_lock`, matching every other taker's order.
`persistence._RWLock` could be permanently wedged by the server's own stop mechanism (`PyThreadState_SetAsyncExc`): stranded reader/writer state, a stranded condition mutex, ownership prolonged by a retained exception traceback, and (found by our own stress test) an acquisition orphaned between `__enter__` and the `with` block.	Rebuilt around weak-referenced per-acquisition tokens whose liveness is judged three ways — token alive, acquisition generator not closed (`gi_frame`), owner thread alive — so every stranding heals without the stranded code running. 400-real-injection stress test stays healthy across repeated runs; the only residual window needs two separately-timed injections and is documented in the class docstring.
Abandoned sub-agent usage could momentarily read a torn (regressing) snapshot during executor detach and subtract already-banked cost/tokens/steps from the parent.	Live snapshots are clamped componentwise to the banked floor.
Event-journal replay was not exactly-once: a crash between the SQLite commit and the snapshot unlink duplicated events; snapshots replayed in PID/UUID filename order rather than chronology, and a restored-after-failure snapshot could interleave old and new rows.	A `replayed_journals` marker commits in the same transaction as the rows; claims embed a monotonic `time_ns` key (forced past every existing key under the journal flock) and replay sorts by it; the restore path was removed (claimed snapshots stay discoverable).
`_reclaim_process_lock`'s thread-local re-entry marker could outlive the kernel flock after an injected stop, silently bypassing cross-process locking on that thread forever.	The marker is a weak reference to a token that owns the open lock-file handle: marker-live ⟺ handle-open ⟺ flock-held at every injection boundary (fail-closed). Verified with kernel-truth `LOCK_NB` probes from a second process.
Same-repository task handoffs split the retirement and the branch/dirty-state baseline across two flock holds, trusting a branch cached in the gap — a peer process could move the branch in between.	`_try_setup_worktree` holds the (now re-entrant) flock across retirement and baseline capture as one transaction.

src/kiss/server

Finding	Fix
UDS socket cleanup (old daemon) did not take the startup sidecar flock: it could pass its inode check, let a successor rebind, then unlink the successor's live socket. Two deeper holes surfaced while fixing: asyncio's own unguarded `cleanup_socket` stat+unlink, and inode reuse defeating the witness.	Cleanup takes the same flock non-blocking (fail-closed), disables asyncio's cleanup, and probes liveness under the flock.
Main-tree mutators (Discard, manual Git Commit, and — found in round 2 — the automatic post-task commit and its sibling-repository pass) busy-checked without publishing any claim, so a direct task admitted in the gap had its half-written files reset or swept into a commit.	One per-repo claim protocol under `STATE_LOCK`: mutators publish their claim in the same locked section as the busy check, task admission refuses while a claim is held, and the finishing task's own admission is excluded so it can commit its own results.
The manual-commit dispatcher claimed the repo the raw path resolved to, while the worker remaps a vanished worktree path to the parent repository — the parent was mutated with the wrong (or no) claim.	The dispatcher applies the same stale-worktree fallback before claiming; the worker re-runs the atomic busy-check+claim if its resolved repo differs.
Claims could be stranded until daemon restart when the production task-stop `KeyboardInterrupt` landed between claim publication and the local release assignment (reproduced with the production primitive).	Claims are appended to a caller-armed holder before publication, release is conditional on the claim object's identity, and a claim whose owner thread died heals on the next admission check or claimant.
Run-vs-closeTab races left mixed registry/state combinations matching neither serial order; round-2's own undo could delete a later legitimate `resumeSession` recreation, and the returned generation could belong to a later publisher; explicit-close and chat-displacement cleanup tails were identity-blind.	Every registry publication is stamped with a monotonic generation returned atomically from the locked update; compensating closes go through `close_tab_if_generation`; removals carry clock tokens and the cleanup tails stand down when `republished_since` reports a newer publication (checked under `STATE_LOCK`, where reopens rebind state).

src/kiss/agents/vscode

Finding	Fix
A timed-out Monaco loader flight left live callbacks that cleared a NEWER in-flight promise, allowing concurrent AMD bootstraps.	Per-flight `retire()` only clears the slot its own flight still owns; a `dead` flag retires a timed-out flight's callbacks.
An attachment added after a send began waiting fell outside the one-time promise snapshot: omitted from the message, then silently discarded by the composer reset. The first fix introduced a follow-on bug (caught in re-review): removing the awaited chip left the send blocked on a hung decoder forever.	`attachmentsReady()` is a fixpoint over the owning tab's live attachment list, and every slot carries a removal signal raced against its conversion — late attachments ship, removed ones release the send, failed ones still block it.
Dropped-URI resolution wasn't tab-scoped: a tab switch mid-resolution inserted tab A's file path into tab B's composer.	The request stamps `tabId`, the host echoes it, and the webview drops replies addressed to a tab no longer visible.
A stale `visible:true` continuation focused the Sorcar view after the user had already hidden it (and could throw after `deactivate()`).	A visibility generation counter re-checked after the settle wait, plus an explicit undefined check.
The eight-item API-key inventory was duplicated verbatim across the load loop, save loop, and input wiring — free to drift.	One shared `FIRST_PARTY_KEY_IDS` constant drives all three; a parity test pins the round trip.

Verification

Every confirmed race got a test that reproduced the exact reviewer schedule and failed before the fix: real threads, real subprocesses holding real flocks, real SQLite, real git repositories, real PyThreadState_SetAsyncExc injections — no mocks. Injections at exact bytecode boundaries (a finally's entry, a publication line) are driven by a trace raising at that line; the trace chooses only when the exception arrives.
Impacted suites: 704 server tests, 526 sorcar tests, 308 core tests, 297 extension jsdom suites, plus the 88 new regression tests — all green. uv run check --full (ruff + mypy + compileall + docs) passes.

Documented non-fixes

Out of scope: the reviewer noted that the rsorcar deploy script (repo root, not one of the four audited directories) overwrites the remote machine's canonical api_keys.env via scp without taking the remote store's sidecar lock while the remote daemon is still running — a concurrent remote save_api_key() and a deploy can overwrite each other's whole-file snapshots. Pre-existing, unrelated to this audit's changes; left for a follow-up.

Residual windows (documented in code): stranding the _RWLock condition mutex now requires two separately-timed injections landing on consecutive single-bytecode boundaries (one stop attempt performs one injection); a 400-injection randomized stress test shows no deadlock. A cross-process legacy-key window between two daemons sharing one $KISS_HOME beyond the fixed migration path is likewise documented as unsupported deployment.

Detailed per-round evidence lives in the task's working notes: findings logs (findings-*.md), reviewer reports (review*-*.md) and fix logs (fixes*-*.md).