fix(knowledge): take ingested item ids from the commit callback

[CrysisDeu]

Problem

A folder source large enough to matter crash-loops the gateway. On a 2709-file
source this reproduced as four loop-stall watchdog exits inside 17 minutes,
with the surviving process still reporting 7-9s of event-loop lag between them:

13:39:05 folder_watcher: Source 80b88169-…: capped at 2000 files (709 skipped)
13:44:22 event-loop heartbeat: lag 9.1s (loop was blocked)
13:45:55 event-loop heartbeat: lag 9.2s (loop was blocked)

Dumps land in ~/.kiro/crew/logs/crash-dumps/loopstall-*.txt; the last one in a
loop is usually truncated because the watchdog loses the race to the supervisor's
kill.

Why it matters

Every exit takes the whole gateway with it — live chat sessions, crons and
channels all die — and the respawned process re-enters the identical boot scan,
so the loop is self-sustaining rather than self-healing. Anyone whose Knowledge
Library holds one large folder source is affected, and the auto-registered
project-docs source made that the default rather than the exception.

Fix (symptoms → root cause → change)

_ingest_file learned which items a file produced by reading the source's
entire item-id set before and after handing the file to the pipeline, and
diffing the two. Both reads ran synchronously on the event loop.

idx_items_source_id keeps each read an index scan, so this is not a
missing-index bug. The cost is result size: one row per item in the source
— ~20k rows on a large source, measured at >1s per call — twice for every file in
the scan. That is well past the watchdog's budget.

Neither read is necessary. The pipeline already reports the ids it created
through its on_committed callback, which it invokes inside its own
run_to_completion finalize hop (ingestion.py) — the same uncancellable unit
that commits them. Passing a recorder there removes both reads outright, and
removes the sources.sync_status read as well: on_committed fires only on the
branch that actually commits a group, so an unset recorder is the
silent-rollback signal — evaluated per call, rather than reading a column a
concurrent ingest on the same source can flip.

Why eliminate rather than offload

The first revision of this PR moved the reads to a worker thread with
asyncio.to_thread. The GPT reviewer correctly blocked that on SHA 307263fac,
and the finding was legitimate: offloading adds await points after the
pipeline commits, and the caller (_do_scan) writes the folder_file_state row
that NAMES the new items only once _ingest_file returns. A shutdown cancelling
at such an await leaves committed items that no state row names — the next scan
re-ingests the file and duplicates them, while the first group stays untracked
and undeletable.

Sourcing the ids from the callback means there is no post-commit await to cancel.
The cost problem and the atomicity problem are closed by the same change instead
of being traded against each other.

The callback also persists, fail-safe

Capturing the ids in memory alone left one window open: the pipeline awaits
again after its finalize hop (generate_source_summary), and the caller's
state write only runs once _ingest_file returns. A shutdown cancelling in that
gap strands a committed group nothing names. So the recorder also writes the
folder_file_state row from inside the finalize hop itself — a targeted UPDATE
onto the scanning marker the scan wrote before the call (committed ids,
terminal done, derived text_hash, cleared retry budget). The caller's own
done write still lands with the same values on the uncancelled path, so the
two writers are order-independent; the caller's _write_state_row remains the
authoritative spelling and the callback's UPDATE must stay field-consistent
with it (attempts=0, error_message=NULL, text_hash derivation).

The write is fail-safe, never fail-closed: it runs after the group has
committed and the superseded items are deleted, so an escaping exception there
(writer-lock contention past busy_timeout, e.g. a large concurrent
import_bundle) would poison the finalize hop and convert a successful ingest
into a terminal failed row — causing the exact duplication it exists to
prevent. Errors are swallowed and logged; the memory path and the caller's
write still stand, and the exposure shrinks back to the cancellation window,
never past pre-fix behavior.

Why this wasn't already fixed

This is the site that survived #2175, #2336 and #2507. Each of those moved a
different call off the loop — dedup_document, delete_items_batch, the
duplicate-skip gate — and none touched this one, which is why crash loops kept
being reported on builds carrying all three.

Tests

test/test_knowledge_ingest_scan_off_loop.py, following the two-kinds shape of
the existing test_knowledge_delete_off_loop.py. Both invariants are ratcheted,
because the fix for the first defect is what created the second:

Test	Locks in
test_ingest_file_issues_no_sqlite_call_on_the_event_loop	AST ratchet: no execute/fetchall/fetchone called directly from the coroutine body (nested scopes excluded — those are thread targets)
test_ingest_file_never_awaits_after_the_pipeline_commits	AST ratchet: exactly one await, the pipeline call. This is what makes the orphan window unreachable rather than merely absent — it fails on any future to_thread hop added in good faith to get a query off the loop
test_ingest_file_takes_its_item_ids_from_the_commit_callback	Guards both ratchets against passing because the bookkeeping was deleted rather than rewired
test_ingest_file_reports_the_committed_ids_without_touching_sqlite	Behavioural, not lexical: traces the loop thread's own sqlite connection via set_trace_callback (store.db is per-thread), and asserts the reported ids equal what the pipeline created
test_ingest_file_reports_failure_when_the_pipeline_never_commits	A silent rollback is still detected without reading sync_status; losing this would record a partial failure as done and never retry it
test_ingest_file_still_reports_a_refused_duplicate_as_deduped	The dedupe branch must keep winning over the rollback branch — both leave on_committed unfired
test_ingest_file_lets_cancellation_through_without_a_failed_state_row	CancelledError is a BaseException, so a shutdown keeps the retryable scanning marker instead of a terminal failed row
test_commit_callback_persists_the_state_row_before_returning	The committed group reaches the state row inside the callback, observed from the stand-in pipeline's worker hop — before _ingest_file can run any post-await code
test_a_failed_callback_persistence_does_not_poison_the_ingest	A locked-DB UPDATE inside the callback is swallowed: the ingest still returns (ids, 'done') through the memory path instead of rolling a committed group up as a partial failure
test_scan_records_the_committed_group_on_the_state_row	End to end through scan_source: a callback wired up but dropped on the way out would satisfy every unit assertion above and still strand every file
4 negative controls	Each ratchet fires on the pre-fix shape and ignores nested-frame awaits/queries — so neither can pass vacuously, and neither can push the next author back onto the loop

Manual verification

Static gates run locally on the rebased tree: flake8 and isort --check-only
clean; mypy reports nothing in folder_watcher.py (the 3 hooks.py xattr
errors are pre-existing on main and Linux-only). AST invariants confirmed
directly against the file: zero sqlite calls in the coroutine body, one await
(ingest_file).

The suite itself was deliberately not run locally by the original author (see rationale below); the drive-to-green pass later ran the full suite on a separate host: 56623 passed, 0 failed.

Original rationale: A local pytest -n auto
on this host saturates the CPU and starves the very gateway event loop this PR is
about, which is one of the documented ways it gets killed. CI is the gate for the
tests — and the previous revision's run confirms the shape works there: all four
Backend Tests (3.12, N) shards passed, 53 lanes green, with GPT the only real
failure.

Runtime confirmation is left to CI plus observation: the reproducing condition is
a >2000-file source, and the observable is the absence of new loopstall-*.txt
dumps across a boot scan.

Why no screenshot: backend-only change under src/kiro_crew/knowledge/; no
frontend path is touched and nothing renders differently.

no issue closed: the crash loop was diagnosed from local gateway logs across
several sessions and never filed as a tracked issue.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] acba1fd

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've traced the full contract independently: the pipeline's on_committed (ingestion.py:710-711) fires only inside the if processed == total success branch of _finalize, on the worker thread, right after delete_items_batch. The folder-watcher's _record_committed (folder_watcher.py:938) captures the ids, then does a targeted UPDATE on the thread-local (autocommit) connection, swallowing any exception so it can never poison the finalize hop after commit. The dedup check (line 1006) correctly precedes the committed is None rollback signal (line 1013); the except Exception at 1019 lets CancelledError (a BaseException) through; the 'scanning' marker is always written first (line 492) so the targeted UPDATE always has a row; and text_hash derivation matches _write_state_row. Empty-group (committed=[]) matches prior after_ids - before_ids == set() behavior. No dangling before_ids/after_ids references remain.

The candidate list produced no candidates, and my own Step 2 pass finds nothing groundable to the required bar.

No findings.

[OPUS-REVIEWED] acba1fd

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

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of acba1fde4da214361f491156a682561752c61069 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Root-cause fix on a pre-existing pipeline seam (on_committed, already proven by agent_source.py), with the rejected offload alternative correctly reasoned away.

[DESIGN-REVIEWED] acba1fd

[CrysisDeu]

Disposition for GPT 5.6 findings on 307263faca5b8df49f82e74ba466cdea34c56396

1. folder_watcher.py:938 — post-commit awaits can orphan ingested items — FIXED in dcc7faccf.

The finding was correct and I had it wrong. Offloading the reads added await
points after the pipeline's commit, and the caller (_do_scan) writes the
folder_file_state row that names the new items only once _ingest_file
returns — so a shutdown cancelling at one of those awaits leaves committed items
that no state row names, the next scan re-ingests the file and duplicates them,
and the first group stays untracked and undeletable. My reasoning about
CancelledError propagating cleanly applied to the pre-commit read and I
generalized it to the post-commit ones without checking.

I did not take the suggested remedy of reverting the post-commit hops, because
one of them (after_ids) is one of the two expensive reads — reverting it leaves
~1s per file back on the event loop and the loop-stall watchdog still fires, so
the PR would no longer fix what it exists to fix.

Instead the reads are eliminated. IngestionPipeline.ingest_file already
accepts on_committed, and _finalize invokes it inside the same
run_to_completion hop that commits the items. _ingest_file now passes a
recorder there and returns what it captured, which:

• removes both per-source id reads and the sources.sync_status read outright,
  rather than relocating them to a worker thread; and
• leaves exactly one await in the function — the pipeline call itself — so
  there is no post-commit suspension point for a cancellation to land on.

on_committed fires only on the fully-successful branch, so an unset recorder is
now the silent-rollback signal that the sync_status read used to provide, and
it is evaluated per call rather than reading a column a concurrent ingest on the
same source can flip.

Evidence. Both properties are ratcheted rather than asserted in prose:

• test_ingest_file_never_awaits_after_the_pipeline_commits — AST ratchet over
  the coroutine's own frame; fails if any await other than ingest_file appears,
  including a future asyncio.to_thread hop added in good faith to move a query
  off the loop. This is the ratchet that makes the reported hazard unreachable
  rather than merely absent in this revision.
• test_ingest_file_issues_no_sqlite_call_on_the_event_loop — AST ratchet: no
  synchronous sqlite call directly in the body.
• test_ingest_file_reports_the_committed_ids_without_touching_sqlite —
  behavioural, not lexical: traces the loop thread's own sqlite connection
  (store.db is per-thread) and asserts no ingest query reaches it, plus that
  the reported ids equal what the pipeline created.
• Four negative controls: each ratchet is shown to fire on the pre-fix shape and
  to ignore awaits/queries inside nested frames — so neither can pass vacuously,
  and neither can push a later author back onto the loop.
• test_scan_records_the_committed_group_on_the_state_row drives the real
  scan_source, because a callback wired up but dropped on the way out would
  satisfy every unit assertion above and still strand every ingested file.

Verified on the rebased tree: flake8 and isort --check-only clean, mypy
reports nothing in folder_watcher.py (the 3 hooks.py xattr errors are
pre-existing on main and Linux-only). AST invariants read directly off the
file: zero sqlite calls in the body, one await.

Span note for later rounds: this is round 1 of blocking findings in
folder_watcher._ingest_file. The change is a structural narrowing of that
function (three queries and two awaits removed, one await left) rather than a
point patch, so a further finding in the same span should be read as evidence the
invariant is wrong, not as the next item to patch.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]: 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: GPT 5.6 BLOCKING on folder_watcher.py:920 — recorder does not persist the committed group inside the callback (shutdown-race can orphan ingested items). Fix: update the existing folder_file_state row with committed IDs and terminal status inside _record_committed, closing the CancelledError window.

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

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Pushed 9a57af94f (rebase onto latest main + 1 fix commit, original commit by CrysisDeu preserved as-is). Changes:

1. GPT 5.6 BLOCKING (folder_watcher.py _record_committed) — fixed
The recorder captured the committed ids in memory only. The pipeline awaits again after its finalize hop (generate_source_summary), so a shutdown cancelling there left a committed group nothing named: the caller's state write never ran, the scanning marker survived, and the next sweep re-ingested the file alongside the untracked first group. _record_committed now also persists the folder_file_state row from inside the same uncancellable finalize hop — a synchronous targeted UPDATE onto the scanning marker (committed ids, done, derived text_hash, cleared retry budget). No new await, so the AST ratchets still hold. The caller's own done write still lands on the uncancelled path with the same values (order-independent).

2. Six CI test failures on the branch — fixed

• 5× test_folder_watcher.py: mocks ingested without invoking on_committed, which this PR's new contract correctly reads as a rollback. The mocks now report the ids they create (contract-honoring, no production change).
• 1× test_scan_records_the_committed_group_on_the_state_row (this PR's own e2e test): it mocked chunker.chunk, but a .md file dispatches to chunk_markdown (see _run_chunker), so the bare MagicMock answered with 0 chunks and the scan committed an empty group. The mock now covers both dispatch paths.

3. New test: test_commit_callback_persists_the_state_row_before_returning observes the state row from inside the stand-in pipeline's worker hop at the moment the callback returns — before _ingest_file can run any post-await code.

Local gates: isort ✅ flake8 ✅ mypy ✅ (42 pre-existing errors identical before/after) black baseline gate ✅ full pytest: 56623 passed / 0 failed.

No design change: the callback-based approach is kept exactly as the author built it; the fix only makes the callback's effect durable.

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Advisory premise-level review of acba1fde4da214361f491156a682561752c61069 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

Now I have everything needed. Verifying the sibling count once more before writing the review: on-loop SELECT id FROM items WHERE source_id reads remaining are ingestion.py:541, 558, 636, 777, 786, 811 and artifact_ingest.py:411, 464 (the reads at ingestion.py:719, 872 run inside off-loop finalize hops). The review follows.

First-Principles-Verdict: CONCERNS

The crash-loop fix is cause-level, but a second folder_file_state writer rides along undeclared, and the same per-file full-source read survives inside the pipeline it calls.

What this change ships

Intent: stop a large folder source from stall-crashing the gateway on every boot scan — a FIX.

1. Scan learns each file's item ids from the pipeline's commit callback, not two full-source reads — justified
2. Silent rollback detected by the callback never firing, not by re-reading sync_status — justified
3. The done state row is also written inside the pipeline's commit hop — undeclared, rides along
4. Errors in that in-callback write are swallowed with a warning — rides along (part of 3)
5. New ratchet/behavioural test file pinning "no loop queries, one await" — justified
6. Existing scan-test fakes must now fire on_committed or read as rollback — justified, mechanical

Watch

• Item 3 is absent from the description's fix narrative and its test table (its two tests, test_commit_callback_persists_the_state_row_before_returning and test_a_failed_callback_persistence_does_not_poison_the_ingest, are not listed). It has a named harm (cancellation during generate_source_summary, ingestion.py:733), but it makes the success row a second spelling: the callback's targeted UPDATE and the caller's _write_state_row (folder_watcher.py:775) must stay field-consistent (attempts=0, error_message, text_hash) forever.
• Point patch vs. the stated cause. The description says the cost is the full-source read "twice for every file" and that this is "the site that survived fix(knowledge): run per-file dedup off the event loop #2175, fix(knowledge): run the ingest-path item deletes off the event loop #2336 and fix(knowledge): run the duplicate-skip gate off the event loop #2507" — yet grep for SELECT id FROM items WHERE source_id leaves 8 on-loop siblings (ingestion.py:541, 558, 636, 777, 786, 811; artifact_ingest.py:411, 464), and ingestion.py:635-636 (_before_ids) still runs once per file on this exact scan path — the same ~20k-row read, so roughly a third of the measured per-file stall survives. created_item_ids in the same function already records what the call wrote; accepted-and-deferred, but the crash loop may only shrink, not vanish.
• artifact_ingest.py:408-467 keeps the before/after diff while passing only on_duplicate to the same pipeline whose on_committed (consumed at agent_source.py:372) already reports the ids — an unfixed second spelling of the mechanism this PR just retired.

[FIRST-PRINCIPLES-REVIEWED] acba1fd

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Pushed acba1fde4 (iteration 2/10). Two changes:

1. GPT round-2 BLOCKING (callback persistence can raise past busy_timeout) — finding adopted, prescription adapted
The finding is correct: the UPDATE I added in round 1 was the only raise-capable statement in _record_committed, and it runs inside the finalize hop AFTER the group commits and the superseded items are deleted. A writer-lock timeout there (e.g. a large concurrent import_bundle) would poison the whole finalize: the ingest reports failed, the caller writes a terminal failed row, and the next sweep re-ingests alongside the committed group — the exact duplication the write exists to prevent.

The prescribed fix ('revert the hunk until it can share the pipeline finalizer's transaction') is not implementable as stated: the finalize hop's worker connection is autocommit (isolation_level=None) by design — _finalize's own comment documents that no enclosing transaction spans it — so there is no finalizer transaction to join. Reverting outright would reopen the round-1 cancellation window (generate_source_summary await).

Fix applied instead: the persistence is now fail-safe — any exception is swallowed and logged, the in-memory path still delivers the ids, and the caller's own done write persists the group on the uncancelled path. A failed write shrinks the exposure back to exactly the pre-round-1 behavior, never worse. New regression test test_a_failed_callback_persistence_does_not_poison_the_ingest proves a locked-DB UPDATE still yields (ids, 'done').

2. PR Hygiene (single commit) — squashed
Squashed to one commit via git reset --soft <parent-sha> (not origin/main, avoiding implicit reverts), original authorship (Zezhen Xu / CrysisDeu) preserved on the commit, contribution split documented in the message body, Co-authored-by: Kiro Crew trailer retained. Verified git diff origin/main HEAD --name-only lists exactly the 3 intended files.

Local gates re-run: flake8/isort/black-gate green, mypy identical to baseline, both affected test files 76/76 pass.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Response to First Principles CONCERNS on acba1fde4 — all three points addressed, none require code change in this PR:

1. Item 3 (in-callback persistence) undeclared in the description — adopted. The PR description now has a dedicated section ('The callback also persists, fail-safe') declaring the second writer, its named harm (cancellation during generate_source_summary), the fail-safe swallow rationale, and the field-consistency obligation with the caller's _write_state_row (attempts=0, error_message=NULL, text_hash). Both of its tests are now in the test table. The 'second spelling' coupling is real and acknowledged as the price of closing the post-finalize cancellation window without an (impossible) shared transaction — the finalize hop's worker connection is autocommit by design.

2. _before_ids (ingestion.py:635-636) still runs per file on this scan path + 7 on-loop siblings — accepted-and-deferred, now tracked: filed #4431 with the review's exact line inventory and the created_item_ids-based fix direction for the partial-failure branch. Kept out of this PR deliberately: it is a point fix for the site that survived #2175/#2336/#2507, with ratchet tests scoped to _ingest_file; converting _finalize's rollback arithmetic mid-review would widen the blast radius of a verified fix. The description's claim is 'the two per-file reads at this site are gone', not 'the crash loop is extinct' — the review is right that it shrinks by ~two-thirds of the measured stall rather than vanishing on the largest sources.

3. artifact_ingest.py before/after diff as an unfixed second spelling — same disposition, covered in #4431 (it already receives on_committed support from the pipeline; only the call site needs rewiring).

CI is fully green on acba1fde4 (57 checks), readiness: passed.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

✅ Review-ready at acba1fde4 — all 57 CI checks green, readiness: passed, MERGEABLE. Awaiting human review + merge (never merged/approved by the bot per policy).

Drive summary (2 fix iterations, single squashed commit, original authorship preserved):

• Rebased onto latest main (shallow-clone merge-base corrected first).
• GPT round-1 BLOCKING: committed group not persisted inside the commit callback → _record_committed now writes the folder_file_state row inside the pipeline's uncancellable finalize hop (targeted UPDATE onto the scanning marker).
• GPT round-2 BLOCKING: that write could raise past busy_timeout and poison the finalize → made fail-safe (swallow + log; memory path and caller's write still stand). GPT round 3: no blocking findings.
• 6 CI test failures repaired: 5 mocks now honor the on_committed contract; the PR's own e2e test now covers the chunk_markdown dispatch path.
• 2 new tests: callback persistence observed inside the worker hop; locked-DB persistence failure does not poison the ingest.
• PR Hygiene: squashed to one commit (reset --soft to literal parent SHA, diff verified = 3 intended files).
• First Principles CONCERNS answered: description now declares the second writer + fail-safe rationale; surviving on-loop reads tracked in follow-up knowledge: surviving on-loop full-source item-id reads after #3397 (ingestion.py _before_ids + artifact_ingest before/after diff) #4431.

Design unchanged throughout: the author's callback-based on_committed approach is exactly as built — the fixes only make its effect durable and its failure modes safe.

[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 (Semgrep + CodeQL), security checklist all-NO, AI reviewers green. Category: fix ≤5 files with clear root cause — ingested item IDs sourced from commit callback instead of wrong location.

[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 (Semgrep+CodeQL), security checklist all-NO, AI reviewers green. Category: fix removes double item-id-set reads from event loop in folder_watcher by using the pipeline on_committed callback, eliminating crash-loop on large sources.

[bolichen97]

Tier 1 auto-approve: fix (3 files, clear root cause). 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: replaces O(n) before/after ID query with on_committed callback to fix event-loop stall in folder watcher.