fix(apps): serialize the builtin config sync against both config writers

[leonlaiyc]

Problem / Motivation

Enabling or disabling a builtin gateway app writes config.json through _sync_builtin_config, and both call sites offloaded it with a bare asyncio.to_thread:

await asyncio.to_thread(_sync_builtin_config, name, enabled=True)   # routes.py:1603
await asyncio.to_thread(_sync_builtin_config, name, enabled=False)  # routes.py:1699

That offload is correct about the event loop and wrong about exclusion. _sync_builtin_config performs a read-modify-write of the main config.json under update_config_locked, which takes only the sidecar advisory flock. config.json has two writer generations that do not exclude each other:

• update_config_locked takes the sidecar flock (CLI, boot refresh, other processes).
• The legacy dashboard writers — agents endpoint, updates.py, security.py, messaging.py, mcp.py, core.py STT — take the loop-side _get_config_lock asyncio lock alone.

Holding only the flock excludes nothing that second family respects. A settings PUT that lands between this handler's read and its write commits from a snapshot taken before it — silently reverting the enabled flag the user just toggled, or losing whatever settings they changed. config/loader.py states the rule directly: a caller running while the dashboard serves requests must also hold the in-process asyncio lock.

Why it matters

The lost update is silent and bidirectional. Either the app comes back enabled after the user disabled it, or an unrelated settings write is reverted by an app toggle. Nothing errors, nothing is logged, and the UI reports success in both directions — the response is built from the handler's own result, not from a re-read of what actually landed on disk.

It is also reachable by ordinary use rather than by a race a user has to provoke: enabling an app and saving a setting are both routine dashboard actions, and the write window includes a filesystem lock wait plus, on Windows, an owner-only DACL application that can cost an unbounded SMB round-trip on a network-homed data home.

What changed (motivation → approach → change)

Symptom → a config write that can be silently reverted. Root cause → the dispatch holds one of the two locks that guard config.json. Change → route both sites through the entry point that holds both.

src/kiro_crew/apps/routes.py:

await run_config_write(_sync_builtin_config, name, enabled=True)

dashboard.chat_utils.run_config_write is the repository's existing helper for exactly this — no new lock and no new abstraction is introduced here. It acquires the loop-side _get_config_lock, then runs the sync in a worker thread, so the flock wait never blocks the loop. The off-loop property the to_thread existed for is unchanged; what is added is exclusion against the legacy family.

Lock ordering was proven before the change, not assumed. The new nesting is app_lifecycle_lock → config lock, which is already what handle_app_uninstall does in this same file (routes.py:1348) for the same reason. The reverse order was checked across the whole tree by walking every with/async with block: 14 functions acquire app_lifecycle_lock, and none of them is reachable from inside a _get_config_lock / run_config_write block. There is no inversion to deadlock against, and the only existing nesting anywhere in src/kiro_crew is the forward one.

The import is call-time, matching the _get_config_lock import a few hundred lines above and for the same documented reason: apps sits below dashboard in the package tree, and no module in src/kiro_crew/apps/ imports kiro_crew.dashboard at load time. This is layering, not a circular-import claim.

Deliberately unchanged: the error contract (OSError still degrades to a warning on the response, because the config write is not the point of the request), the restart notification, and _sync_builtin_config itself.

Tests

test/test_builtin_app_lifecycle.py:

• test_disable_holds_the_loop_side_lock_across_the_write and test_enable_holds_the_loop_side_lock_across_the_write — behavioural, not shape. Each patches _sync_builtin_config with a probe that records _get_config_lock().locked() and its own thread identity from inside the worker, then drives the real handler. Both assert the lock is held and that the work is off the event loop, so a fix that took the lock by moving the write back onto the loop would fail too.
• test_the_loop_side_lock_is_released_afterwards — holding it is only correct if the handler gives it back; also asserts the flag actually landed on disk.
• TestBuiltinConfigDispatchRatchet — a static AST guard that fires only when the callable being offloaded is _sync_builtin_config itself, so ordinary asyncio.to_thread use elsewhere in the module stays legal. Its failure names the exact line numbers.
• test_the_ratchet_can_actually_fail — feeds the ratchet's own predicate the shape it exists to reject, so a scan that silently stopped matching cannot pass vacuously.

Red-before, with only the two production lines reverted and the tests in place:

• behavioural: assert False is True on both enable and disable — the loop-side lock is not held;
• ratchet: assert not [1603, 1699] — the two offending sites named;
• shape test: assert 0 == 2.

test_async_call_sites_offload_off_the_event_loop already existed and pinned the weaker asyncio.to_thread spelling. It is updated rather than left passing by accident: it now pins the stronger dispatcher, and its docstring says why the property it guards is unchanged.

Green on this head: test_builtin_app_lifecycle.py, test_apps_routes_coverage.py, test_lifecycle_hooks.py, test_enable_deps_resolution.py, test_apps_instances_loop_offload.py — 263 passed, 2 skipped, 0 failed.

Gates green: mypy --platform linux on the changed module, the baselined black gate (2 files in scope, no new offenders), isort, flake8. The full backend suite was not run locally; CI runs it authoritatively.

Manual verification

N/A — unit coverage sufficient: the defect is a lock-holding property, and the tests observe that property directly from inside the worker thread that performs the write, on both the enable and the disable path. A manual reproduction would require winning a race that the probe asserts deterministically.

Related Issues

No filed issue. The residual was counted by the First Principles review on #4550, which fixed the same one-lock-only defect for the registry PUT in this module and explicitly left these two sites out of scope. This closes them as their own change rather than widening that PR.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of afe5aa960112a7f4bbb26ad5c14f25a011d50d5c via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims in the description verified against the base tree: run_config_write (src/kiro_crew/dashboard/chat_utils.py:67) is the documented both-locks entry point, _sync_builtin_config (src/kiro_crew/apps/routes.py:195) does a flock-only read-modify-write, and the lifecycle-lock → config-lock nesting already exists at routes.py:1348. The change is two dispatch-line swaps through an existing helper, with behavioural tests that probe the lock from inside the worker.

Design-Verdict: PASS

Real silent-lost-update defect, closed by routing through the repo's existing both-locks dispatcher — minimal, precedent-matching, trivially reversible.

[DESIGN-REVIEWED] afe5aa9

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed afe5aa960112a7f4bbb26ad5c14f25a011d50d5c via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/routes.py:1627 -- function-local from ... import run_config_write here and at line 1744 violates top-level-imports -> Fix: import it at module scope and remove both local imports.
[GPT-REVIEWED] afe5aa9

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of afe5aa960112a7f4bbb26ad5c14f25a011d50d5c via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base. I have everything needed for the verdict: the fix is real and routes through the existing 27-consumer helper; one sibling one-lock writer remains (updates.py:1203); and the new AST ratchet is a third guard on the same two lines.

First-Principles-Verdict: CONCERNS

The lock fix earns its place, but it ships three guards for one property, and one counted sibling of the same defect stays unfixed.

What this change ships

Intent: stop a concurrent dashboard settings save from silently reverting a builtin app's enable/disable toggle — a FIX.

1. Enabling a builtin app can no longer be silently reverted by a concurrent settings save — justified
2. Same for disabling — justified
3. Behavioural tests probing the lock from inside the worker — justified
4. Updated regex shape test pinning the new dispatcher — justified
5. New AST ratchet class (2 tests) pinning the same dispatch — duplicate of items 3–4
6. 22-line rationale comment duplicated verbatim at both sites — oversized

Watch

• One counted sibling of the same one-lock defect remains: of the 7 off-loop update_config_locked dispatch sites in dashboard handlers I checked (grep update_config_locked + read each), 6 hold both locks; dashboard/handlers/updates.py:1203 holds the flock only — its own comment shows it swapped _get_config_lock for the flock instead of holding both. The general fix is the same one-line run_config_write routing; deferring it leaves the auto-update toggle exposed to the identical lost-update.
• The fix sits at mechanism level; the nameable cause — two writer generations exist at all — is out of scope, and the description says so. Fine, but the sibling above is the cost of leaving it.

Subtractions

• Drop TestBuiltinConfigDispatchRatchet (both tests, ~50 lines): the PR's own red-before evidence shows the behavioural probes ("assert False is True") and the updated src.count(...) == 2 assertion each already fail on a regression to bare to_thread. Three spellings of one guard must all be maintained.
• Shrink the duplicated 22-line comment at both routes.py sites to two lines pointing at run_config_write's docstring (chat_utils.py:68–86), which already carries the two-generations rationale; the embedded "14 functions take app_lifecycle_lock" snapshot goes stale silently, exactly the prose-count trap AGENTS.md bans elsewhere.

[FIRST-PRINCIPLES-REVIEWED] afe5aa9

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed afe5aa960112a7f4bbb26ad5c14f25a011d50d5c via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] afe5aa9

[bolichen97]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix — routes the builtin-config enable/disable sync through run_config_write (holds both the loop-side config lock and the sidecar flock) instead of a bare asyncio.to_thread, so a concurrent settings PUT can no longer commit from a stale snapshot and silently revert the app's enabled flag; keeps the blocking work off the event loop. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[iamwhatever]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: routes the builtin enable/disable config.json sync through run_config_write instead of a bare asyncio.to_thread, so it holds the loop-side config lock as well as the sidecar flock and no longer loses updates against the legacy dashboard writers. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[bolichen97]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: routes the builtin config sync through run_config_write instead of a bare asyncio.to_thread, so the read-modify-write of config.json holds BOTH the loop-side config lock and the sidecar flock -- a concurrent settings PUT can no longer commit from a pre-write snapshot and silently revert the app's enabled flag; lock order app_lifecycle_lock -> config lock matches the existing handle_app_uninstall nesting, and the blocking work still runs off the loop. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7167 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7167: REBASE. Merged sibling that established the pattern and explicitly excluded this call site; it provides precedent, not coverage. Files: src/kiro_crew/apps/routes.py.
• PR #4550 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #4550: KEEP. PR #6984 landed the sibling half in the same file; the registry PUT remains the uncovered half and this PR was already narrowed to exactly it, so the two are complementary rather than redundant. Files: src/kiro_crew/apps/routes.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.