fix: move theme-create mkdir and collision check off the event loop (#6198)

[CrysisDeu]

Problem / Motivation

api_themes_create still runs two blocking filesystem calls directly on the asyncio event loop: themes_path.mkdir(parents=True, exist_ok=True) and a target.exists() pre-check. On a UNC/SMB-backed data home either call can block for as long as the network takes, stalling every other request the gateway serves — the identical defect class #5963 fixed for the theme detail route.

Closes #6198

Why it matters

One on-loop stat freezes the user's chat turn AND the liveness heartbeat until the watchdog kills the process — the crash-loop wedge the no-blocking-call-on-event-loop rule exists to prevent. The create route was the last on-loop filesystem site in the theme handlers after #5943 and #5963.

What changed (motivation → approach → change)

Fix by subtraction, per the First Principles review that raised this on PR #6190:

• Deleted the on-loop target.exists() pre-check outright. The in-lock check inside _create_locked already returns the same 409 authoritatively, so the pre-check was a redundant second spelling, not protection.
• Moved the mkdir into _create_locked, which already runs on a worker thread (run_in_executor(discovery_executor(), ...)) under the per-slug lock — no extra executor hop in the handler. The mkdir runs before the collision checks in the same critical section, so _atomic_write_theme_json still finds its parent directory present.
• The handler now does only path construction on the loop (_themes_dir() is the config_dir() memo resolved at boot).
• Re-snapshotted error-code-baseline.json: deleting the uncoded pre-check 409 improved the ratchet count for themes.py (22 → 21), and the stale-baseline test requires improved counts to be recorded.
• Verified the other mkdir/exists sites in themes.py (~331, ~372, ~462): all run inside _do_install/_copy_installed_theme, which are executor-dispatched — already off-loop, left untouched.

Recorded trade-off: an exact-duplicate-slug POST now always takes a discovery_executor worker and the per-slug lock where it previously answered 409 from the (on-loop) fast path. Accepted deliberately — a single authoritative in-lock check is worth more than a fast path, and re-adding a cheap pre-check is what created the on-loop stat in the first place. Bounded cost on an authenticated, local-only endpoint.

Tests

• New TestApiThemesCreateOffLoop (parametrized: fresh create + 409 collision): spies Path.mkdir/exists/is_dir/is_file on the handler's paths and the data home, asserting every filesystem call ran on a worker thread — mirrors TestApiThemeDetailStatsOffLoop from fix(themes): move the theme-detail target stats off the event loop (#5963) #6190. Mutation-verified: reds on the pre-fix code with "2 filesystem call(s) ran on the event loop".
• Updated a stale comment in test_installed_pack_with_the_same_slug_is_409 (there is no pre-lock check anymore).
• Existing create-route tests pass unchanged, including test_creates_the_themes_directory_when_absent (the mkdir-inside-lock still creates an absent themes dir) and both 409 paths.

Local gates: isort / flake8 / mypy / black gate / subprocess-encoding gate / brand gate / harness gate all green; full backend suite 69,972 passed with 87 failures proven pre-existing host-env on the unmodified base (identical failure set, zero theme-related).

Manual verification

N/A — unit coverage sufficient: the off-loop property is asserted directly by the spy test, and the route's observable contract (200/409 bodies) is pinned by the existing tests.

no linked issue: N/A — linked, closes #6198.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 392187d512ea012bedad4dcea7dfeccc87e80d1e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Fix-by-subtraction on an already-locked path, mirroring the #5963 discipline, with a spy test pinning the invariant — sound and minimal.

[DESIGN-REVIEWED] 392187d

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 392187d512ea012bedad4dcea7dfeccc87e80d1e and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 392187d

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 392187d512ea012bedad4dcea7dfeccc87e80d1e: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 392187d512ea012bedad4dcea7dfeccc87e80d1e — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 392187d

Verdict parsed from the review's SHA-scoped output markers for commit 392187d512ea012bedad4dcea7dfeccc87e80d1e.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 392187d512ea012bedad4dcea7dfeccc87e80d1e: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 392187d512ea012bedad4dcea7dfeccc87e80d1e — 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 verification checks pass. I have everything needed for the review.

Verified against the repo:

• The fix matches the description: mkdir moved into _create_locked (already executor-dispatched at themes.py:199-200), the on-loop exists() pre-check deleted, and the in-lock check returns the identical 409 body.
• The "last on-loop filesystem site in the theme handlers" claim holds — every other mkdir/exists/is_dir site in themes.py sits inside _list_themes_sync, _do_install, _copy_installed_theme, _stat_targets, or _locked_remove, all run_in_executor-dispatched.
• The baseline change is mandated: test_baseline_is_not_stale in test/test_error_code_contract.py:310 fails when a recorded count drops without regeneration.
• Sibling on-loop sites do exist in other handler files (confirmed files.py:1247 in an async handler, mcp.py:117 in an async __aenter__), but the description honestly scopes its claim to the theme handlers and the series (fix(themes): enable theme pack routes on Windows #5943 → Move the remaining six api_theme_detail stats off the event loop #5963 → this) is draining sites per-file.

First-Principles-Verdict: PASS

A declared, subtraction-shaped fix: it deletes a redundant on-loop check and moves the rest into an existing off-loop critical section, adding zero surface.

What this change ships

Intent: stop a theme-create POST on a slow (UNC/SMB) data home from stalling the whole gateway — a FIX.

1. Theme create's mkdir + collision check now run on a worker thread — justified (the reported defect class, and no-blocking-call-on-event-loop is a documented rule).
2. The duplicate-slug 409 fast path is gone; same status and body, now always in-lock — justified deletion, trade-off declared.
3. Error-code baseline for themes.py drops 22→21 — mandated by test_baseline_is_not_stale.
4. New regression test pins every filesystem call off-loop — justified, mutation-verified per description.
5. Stale test comment reworded to match the single-check reality — rides along, trivial, declared.

No new config key, flag, parameter, or public symbol; nothing to consumer-count. The fix reuses the existing executor path rather than adding a hop, and the deleted pre-check was a second spelling of the in-lock check (themes.py:192) — the subtraction this lane asked for on #6190, delivered.

Watch

Same defect class survives outside the theme handlers: grep for mkdir(parents=True across dashboard/handlers/ returns 22 sites; at least 2 confirmed on-loop (files.py:1247 in api_upload_file, mcp.py:117 in _McpFileLock.__aenter__). Accepted-and-deferred — the per-file series (#5943, #5963, this) is the general fix in progress, not a demand on this PR.

[FIRST-PRINCIPLES-REVIEWED] 392187d

[chenmingwei23]

Tier 1 auto-approve: fix (3 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 - move theme-create mkdir and collision check off the event loop, clear root cause, no auth/parsing/trust/sandbox/gate/secret surface.

[bolichen97]

Tier 1 auto-approve: fix (3 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 moves theme-create mkdir and collision check off the event loop into the worker-thread install lock (#6198), a clear root cause with no behaviour change to the theme record.

[bolichen97]

Tier 1 auto-approve: fix (3 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 with clear root cause (#6198 — on-loop mkdir/exists blocks the gateway on UNC/SMB-backed data homes; moved inside _create_locked worker thread, collision semantics preserved).

[iamwhatever]

Tier 1 auto-approve: fix (3 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: moves the theme-create mkdir and slug-collision stat off the event loop into the existing _create_locked worker thread, preserving the 409 via the in-lock check; third file is the error-code-baseline ratchet decrement.

[bolichen97]

Tier 1 auto-approve: fix (3 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 with clear root cause — moves theme-create mkdir and collision check off the event loop into the locked worker-thread section (#6198), preventing SMB/UNC-backed filesystem calls from stalling the gateway; behavior covered by new thread-affinity tests.

[dwu96]

Tier 1 auto-approve: fix (3 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: moves the themes-create mkdir and the pre-lock target.exists() collision check off the event loop into the existing _create_locked worker-thread body, so a UNC/SMB-backed data home cannot stall the gateway on an on-loop stat; the 409 for a taken slug now comes from the in-lock check that already covered the installed-pack case, error-code-baseline.json is ratcheted down 1358 -> 1357, and a parametrized spy test asserts every watched filesystem call ran off the loop.