fix(artifacts): keep the folder-store reads off the event loop

[leonlaiyc]

Problem / Motivation

ArtifactFolderStore guards its entire API with a single threading.Lock, and
every mutating call holds that lock across _save():

def create(self, name, parent_id="", color=""):
    with self._lock:
        ...
        self._save()          # mkdir + mkstemp + write + os.fsync + os.replace

create, rename, reparent, set_icon, set_color, reorder and delete
all have that shape. The artifact handlers correctly push those mutations into
the shared executor — and that is exactly what makes the reads dangerous to
leave inline. Ten folder-store calls run on the gateway event loop today:

site	call
_resolve_folder_ref_off_loop (create_missing=False branch)	resolve_path
api_artifact_folders	breadcrumb, once per folder
api_artifact_folder_create	resolve_path (parent_id), breadcrumb
api_artifact_folder_update	exists, get, breadcrumb
api_artifact_folder_delete	exists
api_artifact_set_folder	resolve_path (folder_id), exists
_spawn_artifact_folder_icon_task	exists

None of them writes anything, but every one of them takes self._lock. When a
concurrent request is mid-create/rename/delete on a worker thread, the
inline read cannot acquire the lock until that worker has finished its
mkdir + mkstemp + fsync + os.replace — so the whole gateway loop
(chat streaming, heartbeat, every other request) is parked on another user's
folder write. Lock.acquire() from a coroutine is a hard block, not a
suspension: nothing else on the loop runs.

_resolve_folder_ref_off_loop states the premise that made this look safe:

create_missing=False is a pure in-memory walk, so it runs inline.

The walk is in memory. It is not lock-free, and the lock is the part that
blocks.

Why it matters

The folder store is small, so the pathological case is not a slow parse — it is
the write itself. _save() is an fsync plus a rename; on a busy disk, a
network-backed home directory, or a Windows box where a scanner holds the
target open, that is tens to hundreds of milliseconds, and the store is a single
shared file so every folder mutation in the process serializes on it. Any
concurrent artifact-folder read then converts one slow write into a stall of the
entire dashboard gateway, for every session it serves — not just the two
requests involved.

This is the rule AUTOSDE.yaml states as no-blocking-call-on-event-loop
(blocking: true, file-patterns: src/kiro_crew/**/*.py): "a single blocking
call there freezes every task — the user's chat turn AND the liveness heartbeat
— until the watchdog kills the process and the supervisor respawns into the same
condition (a crash loop). This has caused multiple production wedges." It closes
with "When in doubt, offload."

The same module already treats this as the standard it holds itself to
(_run_off_loop: "so its os.fsync/os.replace never blocks the event
loop"; api_artifact_folders: "Offload it so the dashboard event loop stays
responsive"). This closes the gap between that stated standard and the reads.

What changed (motivation → approach → change)

Root cause: the offload decision was made per call ("does this one write?")
when the blocking property belongs to the lock ("can this one wait on someone
who writes?"). Correcting the premise makes every folder-store call a handler
issues an executor call.

• _resolve_folder_ref_off_loop offloads both modes instead of short-circuiting
  create_missing=False inline, and its docstring now records why the in-memory
  walk still cannot run on the loop. The two direct _resolve_folder_ref(...)
  callers (api_artifact_folder_create's parent_id branch and
  api_artifact_set_folder's folder_id branch) go through it.
• exists(), get() and breadcrumb() in the create / update / delete /
  set-folder handlers move into _run_off_loop.
• api_artifact_folders folds breadcrumb() and _serialize_folder() into the
  executor call it already makes for list_with_counts. Wrapping the
  comprehension per folder would have put O(folders) lock acquisitions back on
  the loop and cost a round trip each; one call does the whole list.
• _spawn_artifact_folder_icon_task moves its exists() guard inside the
  closure that already runs set_icon(), so the check and the write share one
  executor hop and one lock acquisition — which also removes the check-then-act
  gap the two-step version had.

No behaviour changes: same status codes, same payloads, same ordering of the
existence check against body parsing. _apply_updates in
api_artifact_folder_update already re-checks fstore.get(fid) inside the
executor, so the concurrent-delete guard it documents is untouched.

Tests

New test/test_artifact_folder_store_off_loop.py. It does not assert on handler
shape — it wraps the real ArtifactFolderStore methods, records
threading.get_ident() for every call, drives each handler, and asserts no call
landed on the event loop's own thread. That keeps the property true regardless
of how a handler reaches the store. The mutators are wrapped as well, so a
regression that moves an already-offloaded call back inline fails the same
assertion.

Six tests, one per entry point: folder list, create by parent_id, update,
delete, set_folder by folder_id, and the background icon task.

Red-before, measured against pristine origin/main production code
(ad0825392) with the new test file in place — 6 failed, each naming the
methods that ran on the loop thread, which is the defect itself and not a
fixture or environment artifact:

folder-store calls ran on the event loop thread: ['breadcrumb']
folder-store calls ran on the event loop thread: ['exists']
folder-store calls ran on the event loop thread: ['exists']
folder-store calls ran on the event loop thread: ['breadcrumb', 'resolve_path']
folder-store calls ran on the event loop thread: ['breadcrumb', 'exists', 'get']
folder-store calls ran on the event loop thread: ['exists', 'resolve_path']

Green-after: 6 passed. Blast radius: 686 passed / 16 skipped across
test_artifact_folder_handlers.py, test_artifact_folders.py,
test_artifacts_handlers.py, test_artifacts_handlers_coverage.py,
test_handlers_artifacts_coverage.py, test_artifacts.py,
test_mcp_artifact_folders.py and the new file.

flake8, isort and mypy are clean on both files; the new test file is
black-clean. handlers/artifacts.py is on .github/black-baseline.txt and
reports the same eight pre-existing hunks before and after this diff — none of
them in the changed regions.

Manual verification

N/A — unit coverage sufficient: the property under test is which thread a call
executes on, which the tests observe directly at the store; a manual click-through
of the folders UI could not distinguish the two implementations.

Related Issues

Self-reported while auditing the module's own offload comments against its call
sites. No separate issue was filed.

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]

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

Premise-level review of c518ca1d29d4e2a4bcc9667c7442540b19f2b8ac 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 verification is complete. The fix's premise checks out against the base (every ArtifactFolderStore read takes self._lock, mutators hold it across _save(), and the AUTOSDE rule is real), but I found two things worth flagging: the same file leaves seven inline ArtifactStore blocking calls on the loop (same root cause, unfixed siblings), and the description claims an icon-task change the diff doesn't contain.

First-Principles-Verdict: CONCERNS

Real fix, but point-scoped: the same file keeps 7 inline blocking ArtifactStore calls, and the description claims an icon-task change the diff doesn't ship.

Not justified as shipped

• Item 7 — rides along, undeclared: the success-audit reorder is never mentioned in the description.
• Item 8 — rides along, undeclared: the icon-task spawn moving after the last await is justified only in a diff comment, not the description.

What this change ships

Intent: stop artifact-folder reads from stalling the whole gateway loop behind another request's folder write — a FIX.

1. Folder resolution by parent_id/folder_id now runs on a worker thread — justified
2. Folder-list breadcrumbs computed inside the existing executor call — justified
3. Create handler's response breadcrumb moved off the loop — justified
4. Update handler's exists/get/breadcrumb moved off the loop — justified
5. Delete handler's existence check moved off the loop — justified
6. Set-folder handler's existence check moved off the loop — justified
7. Create's success audit now logs before the icon task and breadcrumb — rides along, undeclared
8. Icon task now spawns after the last await — rides along, undeclared
9. New test pinning every handler folder-store call off-loop — justified
10. Icon-race test hook re-keyed to the rename landing — justified

Watch

• Point patch with counted siblings: the stated root cause ("the blocking property belongs to the lock") holds equally for ArtifactStore (self._lock, handlers/artifacts.py:1121), yet grep get_default_store(). finds 7 calls inline in async handlers in this same file — create:1473, get:2002/2196/2217, delete:2111, list_versions:2180, record_impression:2292 — some of them writes, i.e. worse than the reads this PR fixes. A general fix is larger; deferring is fine, silence is not.
  Clears when: a linked follow-up/issue covers the counted ArtifactStore inline calls, or the PR states the folder-store-only scope and what is left.
• Description contradicted by diff on one item: "_spawn_artifact_folder_icon_task moves its exists() guard inside the closure" and the table row "_spawn_artifact_folder_icon_task | exists" — but the base has no such guard (handlers/artifacts.py:3077: "No exists() pre-check: it was a TOCTOU gap") and the diff contains no hunk in that function; consequently the "6 failed" red-before (which includes the icon-task test) cannot reproduce on the current base, where set_icon_if_epoch is already off-loop (line 3080).
  Clears when: the phantom item is dropped and red-before is re-measured against the actual merge base.

[FIRST-PRINCIPLES-REVIEWED] c518ca1

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

All claims verified. The core premise holds (single coarse lock, reads contend with _save() writes), the fix follows the module's established offload pattern, remaining call sites in the module are already covered, and the test pins thread placement at the store rather than handler shape. The one description discrepancy — the claimed _spawn_artifact_folder_icon_task change — has no hunk in the authoritative patch because the trusted base already contains that end state (base drift, not a phantom fix).

Design-Verdict: PASS

Correct root-cause framing — the blocking property belongs to the lock, not the call — fixed at every reachable site with a thread-placement test that pins the property itself.

Suggestions

• The description's _spawn_artifact_folder_icon_task bullet (moving the exists() guard into the closure) has no hunk in the recomputed diff — the base already landed that shape via set_icon_if_epoch; refresh the description after rebase so reviewers don't audit a change that isn't there.
• The invariant is still enforced per-call-site (a future handler with an inline fstore. read escapes the new test's six entry points); a follow-up making reads lock-free by construction (mutate-then-swap an immutable snapshot in ArtifactFolderStore) would retire the whole class instead of enumerating it.

[DESIGN-REVIEWED] c518ca1

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] c518ca1

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- test/test_artifact_folder_store_off_loop.py:206 -- Background icon task can outlive its event loop
_spawn_artifact_folder_icon_task(req, folder["id"], "F", expected_epoch=0)
Slow save -> polling exits on method entry -> pending task remains globally referenced after loop closure, causing later drains to fail.
Anchor: no-test-side-effects
Fix: await current-loop icon tasks before returning.
[BLOCK-MERGE] c518ca1
[GPT-REVIEWED] c518ca1

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

The adjudicable block is empty. There is one fenced finding, F1, under the no-test-side-effects anchor.

I confirmed the mechanism from code opened this run:

• The new test test/test_artifact_folder_store_off_loop.py:206 spawns _spawn_artifact_folder_icon_task(...), which registers the task in the module-global _ARTIFACT_FOLDER_ICON_TASKS set (src/kiro_crew/dashboard/handlers/artifacts.py:3084-3090) and only discards it via a done-callback.
• The test's poll (test_artifact_folder_store_off_loop.py:389-392) breaks the instant "set_icon_if_epoch" appears in the record. That record is written on method ENTRY by the threads wrapper (:257-259), before set_icon_if_epoch finishes. After the break the test does only a synchronous fstore.get and asserts — it never awaits again and never drains, so the loop never resumes _run() to fire the done-callback. The task is therefore reliably still in the global set, bound to this test's loop, when the loop closes.
• The sibling suite test/test_artifact_folder_handlers.py:592-593, 618-619, 627-628, 647-648 iterates that same global set and await t UNFILTERED, so a leaked task from a closed loop raises "future belongs to a different loop" far from its cause. The already-present docstrings at test/test_artifact_folder_icons.py:104-108 and :131-133 document exactly this hazard, and every other test path drains it (_drain_icon_tasks, _held_generation); only this new test omits the drain.

This is a deterministic leak, not a rare race, so no rarity argument for FLAG can be completed. Verdict: UPHOLD-FENCED.

Harm rung: unbounded per the fence (cross-test poisoning, surfaces far from cause). Conditions confirmed: leak at test_artifact_folder_store_off_loop.py:206+:389-392 (no drain, poll breaks on entry), unfiltered await at test_artifact_folder_handlers.py:592. Recovery: none in the test itself. Real fix: await/drain current-loop icon tasks before return (small, matches existing _drain_icon_tasks) — cannot argue conditions are extreme.

[ADJUDICATION] c518ca1d29d4e2a4bcc9667c7442540b19f2b8ac total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] c518ca1d29d4e2a4bcc9667c7442540b19f2b8ac

[ADJUDICATION-FENCED] c518ca1d29d4e2a4bcc9667c7442540b19f2b8ac fenced=1 flagged=0
UPHOLD-FENCED F1 test/test_artifact_folder_store_off_loop.py:206 -- The test polls on set_icon_if_epoch's entry and never drains, so it deterministically leaves a task in the module-global set bound to a closed loop, which the unfiltered awaits in test_artifact_folder_handlers.py then poison; the leak is common, not extreme, so no FLAG rarity argument holds.
[GPT-ADJUDICATED-FENCED] c518ca1d29d4e2a4bcc9667c7442540b19f2b8ac

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#66809557]: This PR has been inactive for 7+ 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: A behavior-preserving offload of the artifact folder-store reads onto the executor (both create_missing modes take the store lock that mutations hold across _save()), plus consolidating breadcrumb/list_with_counts into single executor hops. The only failing gate is the Opus fork review lane (which "could not complete" — an infra timeout, rerunnable) rolled up by PR Readiness; the base is stale. Plan: rebase onto main, rerun the incomplete lane, drive to green.

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

[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 PARTIALLY_COVERED with PR #8097. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5979: REBASE. The merged fix supersedes exactly one of the primary's seven change sites and edits the identical lines, which is the source of the 'merge conflict' label; the rebase drops that hunk and the icon-task test (or retargets it at set_icon_if_epoch) and keeps the remaining nine offloads. Files: src/kiro_crew/dashboard/handlers/artifacts.py.

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

[bolichen97]

Rebased onto main 6fa5519c by a maintainer as part of the 2026-09-08 open-PR audit. Old head 5bcd4b8a -> new head c518ca1d.

Conflicts and resolution:

• src/kiro_crew/dashboard/handlers/artifacts.py, icon task: dropped this PR's exists() + set_icon hunk and took main's set_icon_if_epoch (merged fix(artifacts): guard artifact-folder icon write-back with a per-folder epoch #8097 already removed that inline read). All 9 other offload sites kept.
• src/kiro_crew/dashboard/handlers/artifacts.py, folder update: kept main's armed_icon_epoch arming plus this PR's off-loop breadcrumb.
• test/test_artifact_folder_store_off_loop.py: icon-task test now passes expected_epoch and watches set_icon_if_epoch.
• test/test_artifact_folder_icons.py: the create handler now spawns the icon task after its last await, and the arming-window hook keys off the rename having landed instead of off call order, since this PR offloads reads before the commit.

Gates run locally: isort, flake8, black (changed files; artifacts.py has the same 11 pre-existing black hunks as main and is baselined), and pytest on the artifact folder, icon, handler and MCP folder suites (121 passed). Two failures in test_artifacts_handlers.py (test_copy_outside_allowed_roots_is_refused, test_request_cannot_nominate_its_own_root) also fail on unmodified main.

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