fix(dashboard): serialize the auto-update toggle against both config writers

[leonlaiyc]

Problem / Motivation

POST /api/update/auto — the auto-update toggle in dashboard settings — persists the flag with a bare offload:

# handlers/updates.py:1204
await asyncio.to_thread(update_config_locked, config_path(), mutate=_set_auto_update)

That is correct about the event loop and wrong about exclusion. config.json has two writer generations that do not exclude each other:

• update_config_locked takes the sidecar advisory flock — covering the CLI, the boot refresh, and a second gateway process.
• The legacy dashboard handlers — core.py's theme/settings PUT, the agents endpoint, security.py, messaging.py, mcp.py, computer_use.py — do their own read-modify-write of the same file while holding only the loop-side _get_config_lock asyncio lock.

Holding just the flock excludes nothing that second family respects. core.py:424 is the plainest counterpart: async with _get_config_lock(): → KiroCrewConfig.load → mutate → save, with no flock anywhere. A theme save landing between this endpoint's read and its write commits from a snapshot taken before it, and silently reverts the auto-update flag the user just toggled — or this write reverts their theme.

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, and it is reachable by ordinary use rather than by a race a user has to provoke — toggling auto-update and saving a setting are both routine dashboard actions. Nothing errors and nothing is logged; the endpoint reports success in both directions because the response is built from enabled rather than from a re-read of what landed on disk. The write window is not narrow either: it 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.

Auto-update is also the wrong setting to lose silently. A user who turned it off and finds it back on has an install that will update itself against their explicit decision.

What changed (motivation → approach → change)

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

src/kiro_crew/dashboard/handlers/updates.py:

-    await asyncio.to_thread(update_config_locked, config_path(), mutate=_set_auto_update)
+    await run_config_write(update_config_locked, config_path(), mutate=_set_auto_update)

run_config_write (dashboard/chat_utils.py) acquires _get_config_lock on the event loop, then runs the blocking writer in a worker — so the flock wait still never stalls the loop, which is the property the bare to_thread was there for, unchanged. It also shields and drains that worker across cancellation, so a client disconnecting mid-toggle cannot unwind the lock while the write is still in flight.

Three properties checked before making the swap rather than assumed, because a canonical-helper swap on a persistence path is not mechanical:

• The file is the same one. config_path() here is the main config.json the legacy family mutates, not a sidecar.
• No outer config lock is already held. api_update_auto is a flat coroutine with no async with of its own, so run_config_write is its only lock acquisition and this introduces no nesting. (This is what distinguishes the site from apps/routes.py, where the same swap in fix(apps): serialize the builtin config sync against both config writers #6984 sat inside app_lifecycle_lock and needed an explicit lock-ordering argument. There is no second lock here to order against.)
• The fail-closed contract survives. run_config_write awaits the worker through asyncio.shield and re-raises, so ConfigReadError still reaches the handler's 500 arm and an unreadable config is still never overwritten with a one-key file. Pinned by a test rather than asserted.

The import is module-level, unlike #6984's call-time one: updates.py already lives inside kiro_crew.dashboard, so there is no layering inversion to avoid and no import cycle (verified by importing both modules in either order).

The comment block above the call is rewritten. The old one explained why the flock replaced _get_config_lock and read as though that were the end of the story; it now says why both are needed.

Scope. This is the one update_config_locked site in updates.py. It is the sibling the First Principles review on #6984 counted as the remaining same-class one-lock config writer, and #6984 (merged) names updates.py in its own problem statement. No other handler is touched.

Tests

New in test/test_config_rmw_preserves_settings.py, class TestAutoUpdateToggleHoldsBothConfigLocks — the file that already owns this endpoint's config-write contract:

• test_the_loop_side_lock_is_held_across_the_write — the defect, pinned behaviourally. Spies update_config_locked and, from inside the worker, records _get_config_lock().locked() and the thread identity. Probing from inside is what makes this a test of the property rather than of the spelling of the dispatch. It also asserts the write is still off the loop, so the fix cannot pass by moving the blocking call back onto it.
• test_the_loop_side_lock_is_released_afterwards — holding it is only correct if the handler gives it back; also asserts the toggle actually landed on disk.
• test_an_unreadable_config_still_fails_closed_and_releases — the 500 arm survives the new dispatch, the torn file is byte-identical afterwards, and the lock is not stranded on the exception path.
• test_the_dispatch_cannot_regress_to_a_one_lock_offload — an AST ratchet (not a substring search, so a reformat cannot defeat it) that names the offending updates.py:<line> if update_config_locked is ever dispatched with a bare asyncio.to_thread again.
• test_the_ratchet_can_actually_fail — a scan that matches nothing passes vacuously; this plants a violating source and asserts the scan finds exactly one.

Red-before, production change reverted with the tests in place: 2 failed, 3 passed.

AssertionError: config.json was rewritten without the loop-side lock, so a legacy
                dashboard writer could interleave and revert the toggle
AssertionError: update_config_locked dispatched with a bare asyncio.to_thread at
                updates.py:[1204] -- that holds only the sidecar flock; use
                run_config_write, which holds both config locks

Green after: test_config_rmw_preserves_settings.py + test_dashboard_updates_coverage.py — 99 passed, 3 skipped, 0 failed (Python 3.10.6, Windows).

Gates green: flake8, isort, scripts/check_black_formatting.py, mypy on the changed handler (no issues in it), and scripts/check_loop_bound_locks.py including its own --test self-check (21/21 probes).

Manual verification

N/A — unit coverage sufficient: the defect is which lock is held around a write, and the test observes that lock's state from inside the worker doing the write, which is the exact moment a manual toggle could not show. Reproducing it by hand would require winning a millisecond-scale interleaving between two dashboard requests.

Related Issues

Sibling of #6984 (merged), which made the same correction at the two apps/routes.py call sites and whose First Principles review counted this one as the remaining member of the class.

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

[leonlaiyc]

Backend Tests (3.10, 1) is red on this PR and is not attributable to the diff.

The failure is test/test_agent_spec_hardened_reads.py::TestResolveMcpServer::test_valid_agent_spec_still_resolves_under_the_same_cap — AssertionError: assert (('c', 'a'), {}) == ('c', 'a'). #2602 (merged as 5fe1d64e4) changed _resolve_mcp_server to return (argv, env) and updated two of the three test files that assert it; this one still asserts the old bare tuple. It reproduces on origin/main with no patch applied (1 failed, 52 passed, 2 skipped), and this PR touches only dashboard/handlers/updates.py and test_config_rmw_preserves_settings.py — no path to cron_script or agent-spec reads.

Fixed separately in #7176 rather than folded in here. No SHA churn on this branch for it.

[leonlaiyc]

Full CI triage on this head — three distinct failures, none attributable to the diff. This PR touches only dashboard/handlers/updates.py and test_config_rmw_preserves_settings.py.

1. test_agent_spec_hardened_reads.py::TestResolveMcpServer::test_valid_agent_spec_still_resolves_under_the_same_cap — assert (('c','a'), {}) == ('c','a'). On 3.10 shard 1, 3.12 shard 1, and Windows shard 1. Main-side: fix(cron): forward per-server env, sanitized, and pin wrappers at their source #2602 changed _resolve_mcp_server to return (argv, env) and updated two of the three asserting test files. Reproduces on origin/main unpatched. Fixed in test(cron): pin the env half of the post-#2602 resolver contract #7176. It also fails identically on fix(kiro-cli): derive the POSIX search dirs from the home that was asked for #7172, whose diff is unrelated to both.
2. test_denied_commands_security.py::TestSelfModuleIndexIsLinear::test_the_scan_is_linear_not_quadratic — 0.0402s -> 0.1354s looks super-linear. A wall-clock complexity guard on a shared runner; no path from a config-lock change to a denied-commands module index scan.

No commit, no rebase, and no empty push to reroll — none of these is a signal about this branch.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#66809557]: This PR has been inactive for 3+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: The failing Backend Tests are not in this PR's diff. Shard-1 (3.10 & 3.12) fails on test_agent_spec_hardened_reads.py:359 (a mock call-args shape (('c','a'), {}) == ('c','a') that drifted on main after this PR's base), and the Windows shards fail on test_denied_commands_security.py:3910, a known super-linear timing-ratio flake unrelated to the change. This PR's own tests (test_config_rmw_preserves_settings.py) pass. Fix plan: rebase onto current origin/main to pick up the corrected mock assertion and re-mint the merge ref, then re-run to clear the timing flake. The change itself (routing the updates.py auto-update write through run_config_write to hold both config locks, sibling of merged #6984) is untouched.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#66809557]: Rebased this branch onto current main and force-pushed (bcbaecb→586e55d, lease-pinned). Changes made:

1. Rebase onto 711544f (current main). One conflict in test/test_config_rmw_preserves_settings.py: main had appended TestEveryConfigWriterIsLocked (Legacy config writers bypass the advisory lock, so a locked read-modify-write can still lose their change #8032) at the same end-of-file position where this PR appends TestAutoUpdateToggleHoldsBothConfigLocks. Resolved by keeping both classes in sequence (main's first, this PR's after) — no line of either class was altered.
2. No production-code change. handlers/updates.py is byte-identical in intent to the original commit: the single asyncio.to_thread → run_config_write swap and its comment block are unchanged. The design is preserved as-authored.
3. The rebase picks up test(cron): pin the env half of the post-#2602 resolver contract #7176's corrected (argv, env) assertion in test_agent_spec_hardened_reads.py, which should clear the shard-1 failures (3.10/3.12/Windows) that were inherited through the stale merge ref. The Windows test_denied_commands_security.py timing flake was not touched, per assessment — a re-run should clear it.
4. Commit trailer credits the rebase as Co-authored-by: Kiro Crew; original work remains authored by Leon (leonlaiyc).

Local gates before push, all green: check_black_formatting.py, isort, flake8, mypy src/kiro_crew (1282 files, no issues), pytest test_config_rmw_preserves_settings.py test_dashboard_updates_coverage.py (107 passed, 1 skipped) and test_agent_spec_hardened_reads.py (65 passed).

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

I verified the claims against the base tree: run_config_write already exists and is imported in updates.py (base line 30, used at line 1780), the other two bare to_thread(update_config_locked, …) sites (core.py:2386, tailnet_mobile.py:670) already hold _get_config_lock() around the offload manually, so this endpoint genuinely was the last one-lock writer of the class. The fix routes through the documented canonical helper, preserves the off-loop and fail-closed properties, and the new tests probe the lock state from inside the worker rather than asserting the spelling. The change is a one-liner, fully reversible, with no contract or schema impact.

Design-Verdict: PASS

Last one-lock config writer moved onto the canonical two-lock helper the repo already established; behaviorally pinned, trivially reversible.

[DESIGN-REVIEWED] 3fbbd88

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The change is a sound one-line dispatch swap. run_config_write acquires the loop-side lock via async with _get_config_lock(), runs the blocking writer in a worker (keeping the flock off the loop), propagates writer exceptions unchanged through await asyncio.shield(fut) (so ConfigReadError still reaches the handler's 500 arm), and releases the lock on every path via the async with. The call signature matches the original semantics. No grounded defect in the changed lines, and nothing to add in Step 2.

No findings.

[OPUS-REVIEWED] 3fbbd88

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 3fbbd8871dd43d8877de8e341f7f245e44bd8f77 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 checks done. Every potential sibling site (core.py:1650 and 2386, tailnet_mobile.py:670, whatsapp_setup.py:132, files.py:4285, mcp.py:3531, messaging.py:6671 per its caller-holds-lock comment) already wraps the flock write in _get_config_lock; the base updates.py:1214 was the only one-lock dispatch. The defect has in-repo provenance — run_config_write's own docstring (chat_utils.py:80) states that a one-lock writer "can interleave with the other family and silently revert its settings" — and the fix routes through that existing helper (60+ consumers) rather than adding anything. The existing TestEveryConfigWriterIsLocked scan covers only write_config_atomically callers, so the new ratchet does not duplicate it. The run_config_write import already exists at updates.py:30, so the patch is coherent without an import hunk.

First-Principles-Verdict: PASS

Nothing to check.

What this change ships

Intent: stop a routine dashboard settings save from silently reverting the auto-update toggle (and vice versa) — a FIX.

Inventory (5 items)

1. Toggling auto-update can no longer be silently undone by a concurrent settings/theme save, nor undo one — justified
2. A client disconnecting mid-toggle no longer releases the lock while the write is still in flight — justified
3. The comment above the write now explains why both locks are needed — rides along
4. Four behavioural tests pin the lock held, released, and the fail-closed 500 — justified
5. A self-checked static scan names the site if the one-lock offload is respelled — justified

Verified against base: the one-lock defect is real (updates.py:1214 is a flat coroutine holding only the flock; chat_utils.py:80 documents that shape as a silent-revert bug), the swap targets the existing canonical helper rather than adding one, and a grep of every update_config_locked dispatch found zero unfixed siblings — all eight other offload sites already hold _get_config_lock.

[FIRST-PRINCIPLES-REVIEWED] 3fbbd88

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/dashboard/handlers/updates.py:1204 -- "agents endpoint" and security.py already acquire the sidecar lock, contradicting this one-lock claim -> Fix: list only actual loop-lock-only writers. (origin: validation)
[GPT-REVIEWED] 3fbbd88

[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

• This PR is OVERLAPPING with PR #4550. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7167: REBASE. Independent call site; landing either one does nothing for the other. Files: src/kiro_crew/apps/routes.py.
• This PR is OVERLAPPING with PR #6984. 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.
• This PR is OVERLAPPING with PR #8095. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7167: REBASE. PR #8095 rewrote the tail of the same test file on main, so the head no longer merges cleanly there; git merge-tree against origin/main auto-merges updates.py but conflicts on test/test_config_rmw_preserves_settings.py. Files: test/test_config_rmw_preserves_settings.py, src/kiro_crew/dashboard/handlers/updates.py.

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

[bolichen97]

Rebased onto main 41dcadf2c by a maintainer as part of the 2026-09-08 open-PR audit. Old head 586e55df3 -> new head 3fbbd8871.

Clean rebase: no conflicts. Your api_update_auto change sits on top of main's already-converted api_log_level, and both now share the module-level run_config_write import.

Gates run locally on the changed files only: isort clean, flake8 clean, pytest test/test_config_rmw_preserves_settings.py 29 passed / 1 skipped. black --check reports the test file as reformattable, but that is pre-existing on main and the file is listed in .github/black-baseline.txt, so nothing was reformatted.

Please review the rebased result. A maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed. Reply here if anything looks wrong.