fix(knowledge): run the duplicate-skip gate off the event loop - #2507
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound root-cause fix following the codebase's own derive-under-lock idiom, but the new callback-inside-an-open-transaction seam is a contract enforced only by docstring. Watch
Suggestions
[DESIGN-REVIEWED] 794bf1e |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe candidate file contains no findings — the discovery pass concluded "No candidates." I may not extend it with findings of my own (Step 2). There is nothing to validate. No findings. [OPUS-REVIEWED] 794bf1e Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
1c4a84c to
d4fe1a4
Compare
github-actions
Bot
added
readiness: checking
Disposition — GPT 5.6 finding on
|
github-actions
Bot
added
readiness: action required
Parking this PR — the approach is wrong for this siteConverting to draft. Not because GPT's findings are wrong (they are both correct), but The decisive fact. #2336 is merged and shipped, this PR's site is the last one it Why site-by-site off-loading cannot win here. On a 20k-item source, every
Round 2 is not a defect in the round-1 fix. It is the next step that was also implicitly What actually fixes it. Two things, neither of them this PR:
What is worth keeping from this branch. No override requested. GPT is flagging real data loss, and "accept stranding items to |
d4fe1a4 to
36e31d4
Compare
github-actions
Bot
added
readiness: checking
|
Round 2 disposition, and why this is out of draft again. BLOCKING — The finding is correct, and the mechanism is worth stating precisely. The gate commits its own What was NOT done, and why. The suggested remedy — revert the off-loop gate hunk — was not taken, for the same reason as round 1: reverting restores the loop-stall crash loop this PR exists to fix, trading data loss for a gateway that dies every 6–8 minutes. Nor was the alternative I had written off in my own parking note taken: making the transaction span The fix.
There is no interleaving left in which the terminal write can contradict the database. Two short transactions do the job; one long one is not needed. Mechanically the change also splits Regression test, proven red pre-fix. Verified locally after rebasing onto On the parking note above. I wrote that this strategy could not win because round 2 would not be the last implicitly-atomic step. I still think the two things named there are the real fixes — make |
github-actions
Bot
added
readiness: action required
36e31d4 to
d8f6e9c
Compare
github-actions
Bot
added
readiness: checking
|
Round 3 disposition. BLOCKING — Correct, and it is a regression I introduced in the round-2 fix rather than a pre-existing property. The fix is the one suggested: the call now goes through Two notes on why this is safe rather than merely quieter:
Regression guard. Rather than add a second near-duplicate scan test, the round-2 test now also records which thread the terminal write lands on, and is renamed Verified: 1174 tests pass across the knowledge, folder, dedup and ingest suites; The other reds on this PR, for the record — neither is from this diff
|
github-actions
Bot
added
readiness: action required
|
Round 8 disposition — accepted, and it simplifies the change. Pushed as BLOCKING — Correct. Round 7 put the caller's record inside the gate's hop but still after its The fix is the suggested one, and it makes the code smaller. Two properties come free from that ordering rather than needing their own guards:
Test. The The remaining red on this PR is a main-side breakage, not this diff
Those symbols now live in Nothing in this diff touches Verified after rebasing onto |
github-actions
Bot
added
readiness: action required
c27ded5 to
c29101e
Compare
github-actions
Bot
added
readiness: checking
`delete_items_batch` rebuilds the whole entity graph inside its own transaction, so reaching it from a coroutine body blocks the gateway loop past the stall watchdog: the watchdog exits, the supervisor respawns, and the fresh process runs the same scan. The duplicate gate was the last site still calling it on the loop. Moving the gate to a worker thread spends a guarantee it never declared. While `_skip_as_duplicate` ran on the loop it contained no `await`, so it was atomic against every other coroutine by construction -- a user's `delete_source_cascade` could only land before or after it, never inside. The dashboard already runs that cascade off-loop, so after the offload two threads on two connections genuinely interleave, and the gate's read-then-write became a real time-of-check window at two boundaries. Both are closed here by taking the write lock and DERIVING state rather than predicting it -- the idiom this codebase already uses for the same hazard in `delete_source_cascade` and `start_rebuild_job` -- and both of the resulting locked sections run OFF the loop, because acquiring a write lock is itself a blocking wait. 1. The gate itself is now one `BEGIN IMMEDIATE` unit that re-reads the holder under the lock and falls through to a normal ingest when it is gone. A cheap unlocked probe runs first, so the overwhelmingly common not-a-duplicate answer does not serialize every ingest behind the lock. 2. The terminal `deduped` write takes the same lock and READS the document's item group instead of assuming it empty. The gate commits before returning, so a cascade can land in between; that cascade is benign by design -- it sees this source's location row, reassigns the surviving item here, and adopts it into this very state row. Writing `[]` afterwards erased the adoption and left the only remaining copy owned by the source but named by no row: unreachable by the delete path and undeletable. Deriving the group closes the window in both orders instead of one. It travels through `run_to_completion` for the same reason the gate does -- `BEGIN IMMEDIATE` waits on any concurrent writer up to the connection's busy timeout (10s: `PRAGMA busy_timeout=10000` overrides the 30s connect timeout), and that wait on the loop thread would be the original stall reintroduced one line further along. **All three doc-state tables have that window, and all three are fixed.** `folder_file_state` was the first one found; `artifact_item_state` and `agent_item_state` reached the same terminal write through `artifact_ingest.ingest_artifact` and `agent_source._add_agent_document`, both `async def`, both writing a hardcoded empty group after the gate committed. The aggregate case is the more reachable one: `_OWNERSHIP_HASH_COL` maps those tables to `content_hash`, already the text-hash domain, so `_adopt_reassigned_item` matches and adopts rather than silently missing the way it does for a transformed file. **And the record is written INSIDE the gate's transaction, not after its commit.** Leaving it to the caller is not merely riskier, it is unsound in two ways. `run_to_completion` guarantees the gate FINISHES and then re-raises a cancellation, so a shutdown lands with the delete, the location claim and the terminal job row all durable and the caller's write never reached; the row then keeps its pre-ingest marker, and because a `scanning` row has no `text_hash`, `detach_source_location_by_hash` short-circuits and nothing can release the claim. And even without a cancellation, a FIRST-TIME aggregate document has no state row yet at commit time, so a `delete_source_cascade` landing in the gap reassigns the surviving item here and has nothing to adopt it into -- `_adopt_reassigned_item` matches on `(source_id, hash)`, finds no row, and returns without logging. So each caller passes its record in as `on_duplicate`, the duplicate-branch sibling of the `on_committed` finalizer this pipeline already accepts for the success branch and for the identical stated reason, and the gate invokes it before its own `COMMIT`. The delete, the claim, the job row and the record are one atomic unit: nothing can observe a claim without the row that names it, and a failure in the record rolls the whole refusal back. The transaction does not span `ingest_file`'s return -- it stays inside the gate; only the callback is injected. The derivation lives once, in `KnowledgeStore.surviving_group_in_txn(table, source_id, key)`, with `_DOC_STATE_KEY_COL` naming the column that identifies one document per table -- an allowlist, because those identifiers are interpolated into SQL. It is row-scoped, never `(source_id, content_hash)`: two documents in one source may legitimately hold identical text, so a hash-scoped read names one physical item into both rows and destroys it on the first delete of either -- the cross-wire `_adopt_reassigned_item` already refuses with its own ambiguity guard. An aggregate row that ends up owning items is written `active`, not `deduped`, because `find_document_by_hash` only matches `active` and a row owning content while reporting `deduped` would let the same text in again under a second slug. Two seams were needed to make the gate atomic without touching existing callers. `delete_items_batch_in_txn` carries the body so a caller already holding a write transaction can include the delete in it, while `delete_items_batch` keeps its own transaction and graph reload. And `add_source_location` grows an `_in_txn` variant -- this one is load-bearing and easy to miss: the connection is in autocommit mode, so its trailing `commit()` would have ENDED the caller's `BEGIN IMMEDIATE` early and reopened the very window the lock closes. `run_to_completion` also now forwards its return value, which is what lets the whole gate travel through the hop as one unit. Both races have a regression test proven red before the fix, each with its own non-vacuity guard: stripping the in-transaction revalidation fails with "the gate consulted the holder only once", predicting an empty group fails with "the terminal 'deduped' write overwrote the adoption" for folders and "the terminal write erased the adoption" for both aggregates, and calling any of the three terminal writes synchronously fails the `_record_deduped_state`-on-the-loop ratchet, which names the offending file and line. The AST ratchet that pins every `delete_items_batch` call site was taught the indirection so it still sees the hop. And `test_duplicate_gate_and_terminal_state_are_one_transaction` makes the finalizer raise and requires the gate's delete, its claim on the holder's items and its terminal job row to be absent afterwards -- it fails if the finalizer runs after `COMMIT`. Not addressed here, and deliberately: off-loading makes the gateway survive the scan, it does not make the work cheap. `delete_items_batch` still runs a full graph rebuild plus the orphan sweep per call. Incremental graph maintenance touches every mutation path and is its own change. Also not addressed, and pre-existing: for a TRANSFORMED file (PDF, DOCX, HTML) the cascade's adoption matches nothing at all, because `_adopt_reassigned_item` keys a folder row on `COALESCE(text_hash, content_hash)` while `items.content_hash` is over extracted text, and a refused row derives its `text_hash` from a byte-identical sibling row that a lone PDF does not have. `main` writes an unconditional empty group at this site, so the row never named the item there either -- this change is equivalent for that shape and strictly better for plaintext. Closing it needs the incoming document's text hash carried out of the gate instead of guessed from a sibling, which changes what the gate reports. `test_deduped_state_write_recovers_a_transformed_files_reassigned_item` pins it as a strict xfail with a live repro, so it fails the moment someone lands that and the exemption goes stale.
c29101e to
794bf1e
Compare
github-actions
Bot
added
readiness: checking
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (10 files). Criteria: no conflict, no requested changes, no security surface, AI reviewers green. Category: knowledge subsystem — move duplicate-skip gate off the event loop.
iamwhatever
left a comment
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: small-fix (10 files). Criteria: no conflict, no requested changes, no security surface, AI reviewers green. Category: moves a blocking duplicate-skip check off the event loop to prevent knowledge ingest stalls — pure async correctness fix, no behavioral change.
github-actions
Bot
removed
the
readiness: passed
`_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 rather than a table scan, but it still materializes one row per item in the SOURCE: about 20k rows on a large folder source, measured at over a second per call, twice for every file in the scan. That is far past the loop-stall watchdog's budget, so the watchdog exits the process, the supervisor respawns it, the boot scan re-enters the same reads, and the gateway crash-loops -- four exits inside 17 minutes on a 2709-file source. 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 -- the same uncancellable unit that commits them. Passing a recorder there removes both reads outright instead of moving them to a worker thread, 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, per call rather than per source. Eliminating the reads matters beyond cost. Offloading them would have added await points AFTER the pipeline commits, and the caller writes the `folder_file_state` row naming the new items only once `_ingest_file` returns. A shutdown cancelling at such an await would leave 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. Both properties are ratcheted: the coroutine body may contain no synchronous sqlite call, and no await other than the pipeline call itself. Each ratchet ships with negative controls in both directions, so neither can pass vacuously. This is the site that survived #2175, #2336 and #2507 -- each 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 continued on builds carrying all three.
`_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 rather than a table scan, but it still materializes one row per item in the SOURCE: about 20k rows on a large folder source, measured at over a second per call, twice for every file in the scan. That is far past the loop-stall watchdog's budget, so the watchdog exits the process, the supervisor respawns it, the boot scan re-enters the same reads, and the gateway crash-loops -- four exits inside 17 minutes on a 2709-file source. 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 -- the same uncancellable unit that commits them. Passing a recorder there removes both reads outright instead of moving them to a worker thread, 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, per call rather than per source. Eliminating the reads matters beyond cost. Offloading them would have added await points AFTER the pipeline commits, and the caller writes the `folder_file_state` row naming the new items only once `_ingest_file` returns. A shutdown cancelling at such an await would leave 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. Both properties are ratcheted: the coroutine body may contain no synchronous sqlite call, and no await other than the pipeline call itself. Each ratchet ships with negative controls in both directions, so neither can pass vacuously. This is the site that survived #2175, #2336 and #2507 -- each 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 continued on builds carrying all three.
`_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 rather than a table scan, but it still materializes one row per item in the SOURCE: about 20k rows on a large folder source, measured at over a second per call, twice for every file in the scan. That is far past the loop-stall watchdog's budget, so the watchdog exits the process, the supervisor respawns it, the boot scan re-enters the same reads, and the gateway crash-loops -- four exits inside 17 minutes on a 2709-file source. 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 -- the same uncancellable unit that commits them. Passing a recorder there removes both reads outright instead of moving them to a worker thread, 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, per call rather than per source. Eliminating the reads matters beyond cost. Offloading them would have added await points AFTER the pipeline commits, and the caller writes the `folder_file_state` row naming the new items only once `_ingest_file` returns. A shutdown cancelling at such an await would leave 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 recorder also PERSISTS the committed group from inside the finalize hop: the pipeline awaits again after that hop (`generate_source_summary`), so a shutdown cancelling there would otherwise leave a committed group that only the closure remembered -- the caller's state write never runs, the 'scanning' marker survives, and the next sweep re-ingests the file alongside the untracked first group. The write is a targeted UPDATE onto the 'scanning' marker the scan wrote before the call, and it is fail-safe: a writer-lock timeout is swallowed (the memory path and the caller's own 'done' write still stand) because a raise inside the finalize hop after the commit would poison the whole ingest and cause the very duplication it prevents. Both properties are ratcheted: the coroutine body may contain no synchronous sqlite call, and no await other than the pipeline call itself. Each ratchet ships with negative controls in both directions, so neither can pass vacuously. This is the site that survived #2175, #2336 and #2507 -- each 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 continued on builds carrying all three. Original approach and branch by CrysisDeu (Zezhen Xu); callback persistence, test-contract repairs and cancellation-window tests by Kiro Crew. Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
`_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 rather than a table scan, but it still materializes one row per item in the SOURCE: about 20k rows on a large folder source, measured at over a second per call, twice for every file in the scan. That is far past the loop-stall watchdog's budget, so the watchdog exits the process, the supervisor respawns it, the boot scan re-enters the same reads, and the gateway crash-loops -- four exits inside 17 minutes on a 2709-file source. 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 -- the same uncancellable unit that commits them. Passing a recorder there removes both reads outright instead of moving them to a worker thread, 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, per call rather than per source. Eliminating the reads matters beyond cost. Offloading them would have added await points AFTER the pipeline commits, and the caller writes the `folder_file_state` row naming the new items only once `_ingest_file` returns. A shutdown cancelling at such an await would leave 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 recorder also PERSISTS the committed group from inside the finalize hop: the pipeline awaits again after that hop (`generate_source_summary`), so a shutdown cancelling there would otherwise leave a committed group that only the closure remembered -- the caller's state write never runs, the 'scanning' marker survives, and the next sweep re-ingests the file alongside the untracked first group. The write is a targeted UPDATE onto the 'scanning' marker the scan wrote before the call, and it is fail-safe: a writer-lock timeout is swallowed (the memory path and the caller's own 'done' write still stand) because a raise inside the finalize hop after the commit would poison the whole ingest and cause the very duplication it prevents. Both properties are ratcheted: the coroutine body may contain no synchronous sqlite call, and no await other than the pipeline call itself. Each ratchet ships with negative controls in both directions, so neither can pass vacuously. This is the site that survived #2175, #2336 and #2507 -- each 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 continued on builds carrying all three. Original approach and branch by CrysisDeu (Zezhen Xu); callback persistence, test-contract repairs and cancellation-window tests by Kiro Crew. Co-authored-by: Zezhen Xu <zezhexu@amazon.com> Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
…irodotdev#2507) `delete_items_batch` rebuilds the whole entity graph inside its own transaction, so reaching it from a coroutine body blocks the gateway loop past the stall watchdog: the watchdog exits, the supervisor respawns, and the fresh process runs the same scan. The duplicate gate was the last site still calling it on the loop. Moving the gate to a worker thread spends a guarantee it never declared. While `_skip_as_duplicate` ran on the loop it contained no `await`, so it was atomic against every other coroutine by construction -- a user's `delete_source_cascade` could only land before or after it, never inside. The dashboard already runs that cascade off-loop, so after the offload two threads on two connections genuinely interleave, and the gate's read-then-write became a real time-of-check window at two boundaries. Both are closed here by taking the write lock and DERIVING state rather than predicting it -- the idiom this codebase already uses for the same hazard in `delete_source_cascade` and `start_rebuild_job` -- and both of the resulting locked sections run OFF the loop, because acquiring a write lock is itself a blocking wait. 1. The gate itself is now one `BEGIN IMMEDIATE` unit that re-reads the holder under the lock and falls through to a normal ingest when it is gone. A cheap unlocked probe runs first, so the overwhelmingly common not-a-duplicate answer does not serialize every ingest behind the lock. 2. The terminal `deduped` write takes the same lock and READS the document's item group instead of assuming it empty. The gate commits before returning, so a cascade can land in between; that cascade is benign by design -- it sees this source's location row, reassigns the surviving item here, and adopts it into this very state row. Writing `[]` afterwards erased the adoption and left the only remaining copy owned by the source but named by no row: unreachable by the delete path and undeletable. Deriving the group closes the window in both orders instead of one. It travels through `run_to_completion` for the same reason the gate does -- `BEGIN IMMEDIATE` waits on any concurrent writer up to the connection's busy timeout (10s: `PRAGMA busy_timeout=10000` overrides the 30s connect timeout), and that wait on the loop thread would be the original stall reintroduced one line further along. **All three doc-state tables have that window, and all three are fixed.** `folder_file_state` was the first one found; `artifact_item_state` and `agent_item_state` reached the same terminal write through `artifact_ingest.ingest_artifact` and `agent_source._add_agent_document`, both `async def`, both writing a hardcoded empty group after the gate committed. The aggregate case is the more reachable one: `_OWNERSHIP_HASH_COL` maps those tables to `content_hash`, already the text-hash domain, so `_adopt_reassigned_item` matches and adopts rather than silently missing the way it does for a transformed file. **And the record is written INSIDE the gate's transaction, not after its commit.** Leaving it to the caller is not merely riskier, it is unsound in two ways. `run_to_completion` guarantees the gate FINISHES and then re-raises a cancellation, so a shutdown lands with the delete, the location claim and the terminal job row all durable and the caller's write never reached; the row then keeps its pre-ingest marker, and because a `scanning` row has no `text_hash`, `detach_source_location_by_hash` short-circuits and nothing can release the claim. And even without a cancellation, a FIRST-TIME aggregate document has no state row yet at commit time, so a `delete_source_cascade` landing in the gap reassigns the surviving item here and has nothing to adopt it into -- `_adopt_reassigned_item` matches on `(source_id, hash)`, finds no row, and returns without logging. So each caller passes its record in as `on_duplicate`, the duplicate-branch sibling of the `on_committed` finalizer this pipeline already accepts for the success branch and for the identical stated reason, and the gate invokes it before its own `COMMIT`. The delete, the claim, the job row and the record are one atomic unit: nothing can observe a claim without the row that names it, and a failure in the record rolls the whole refusal back. The transaction does not span `ingest_file`'s return -- it stays inside the gate; only the callback is injected. The derivation lives once, in `KnowledgeStore.surviving_group_in_txn(table, source_id, key)`, with `_DOC_STATE_KEY_COL` naming the column that identifies one document per table -- an allowlist, because those identifiers are interpolated into SQL. It is row-scoped, never `(source_id, content_hash)`: two documents in one source may legitimately hold identical text, so a hash-scoped read names one physical item into both rows and destroys it on the first delete of either -- the cross-wire `_adopt_reassigned_item` already refuses with its own ambiguity guard. An aggregate row that ends up owning items is written `active`, not `deduped`, because `find_document_by_hash` only matches `active` and a row owning content while reporting `deduped` would let the same text in again under a second slug. Two seams were needed to make the gate atomic without touching existing callers. `delete_items_batch_in_txn` carries the body so a caller already holding a write transaction can include the delete in it, while `delete_items_batch` keeps its own transaction and graph reload. And `add_source_location` grows an `_in_txn` variant -- this one is load-bearing and easy to miss: the connection is in autocommit mode, so its trailing `commit()` would have ENDED the caller's `BEGIN IMMEDIATE` early and reopened the very window the lock closes. `run_to_completion` also now forwards its return value, which is what lets the whole gate travel through the hop as one unit. Both races have a regression test proven red before the fix, each with its own non-vacuity guard: stripping the in-transaction revalidation fails with "the gate consulted the holder only once", predicting an empty group fails with "the terminal 'deduped' write overwrote the adoption" for folders and "the terminal write erased the adoption" for both aggregates, and calling any of the three terminal writes synchronously fails the `_record_deduped_state`-on-the-loop ratchet, which names the offending file and line. The AST ratchet that pins every `delete_items_batch` call site was taught the indirection so it still sees the hop. And `test_duplicate_gate_and_terminal_state_are_one_transaction` makes the finalizer raise and requires the gate's delete, its claim on the holder's items and its terminal job row to be absent afterwards -- it fails if the finalizer runs after `COMMIT`. Not addressed here, and deliberately: off-loading makes the gateway survive the scan, it does not make the work cheap. `delete_items_batch` still runs a full graph rebuild plus the orphan sweep per call. Incremental graph maintenance touches every mutation path and is its own change. Also not addressed, and pre-existing: for a TRANSFORMED file (PDF, DOCX, HTML) the cascade's adoption matches nothing at all, because `_adopt_reassigned_item` keys a folder row on `COALESCE(text_hash, content_hash)` while `items.content_hash` is over extracted text, and a refused row derives its `text_hash` from a byte-identical sibling row that a lone PDF does not have. `main` writes an unconditional empty group at this site, so the row never named the item there either -- this change is equivalent for that shape and strictly better for plaintext. Closing it needs the incoming document's text hash carried out of the gate instead of guessed from a sibling, which changes what the gate reports. `test_deduped_state_write_recovers_a_transformed_files_reassigned_item` pins it as a strict xfail with a live repro, so it fails the moment someone lands that and the exemption goes stale. Co-authored-by: t <t@t>
…dotdev#3397) `_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 rather than a table scan, but it still materializes one row per item in the SOURCE: about 20k rows on a large folder source, measured at over a second per call, twice for every file in the scan. That is far past the loop-stall watchdog's budget, so the watchdog exits the process, the supervisor respawns it, the boot scan re-enters the same reads, and the gateway crash-loops -- four exits inside 17 minutes on a 2709-file source. 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 -- the same uncancellable unit that commits them. Passing a recorder there removes both reads outright instead of moving them to a worker thread, 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, per call rather than per source. Eliminating the reads matters beyond cost. Offloading them would have added await points AFTER the pipeline commits, and the caller writes the `folder_file_state` row naming the new items only once `_ingest_file` returns. A shutdown cancelling at such an await would leave 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 recorder also PERSISTS the committed group from inside the finalize hop: the pipeline awaits again after that hop (`generate_source_summary`), so a shutdown cancelling there would otherwise leave a committed group that only the closure remembered -- the caller's state write never runs, the 'scanning' marker survives, and the next sweep re-ingests the file alongside the untracked first group. The write is a targeted UPDATE onto the 'scanning' marker the scan wrote before the call, and it is fail-safe: a writer-lock timeout is swallowed (the memory path and the caller's own 'done' write still stand) because a raise inside the finalize hop after the commit would poison the whole ingest and cause the very duplication it prevents. Both properties are ratcheted: the coroutine body may contain no synchronous sqlite call, and no await other than the pipeline call itself. Each ratchet ships with negative controls in both directions, so neither can pass vacuously. This is the site that survived kirodotdev#2175, kirodotdev#2336 and kirodotdev#2507 -- each 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 continued on builds carrying all three. Original approach and branch by CrysisDeu (Zezhen Xu); callback persistence, test-contract repairs and cancellation-window tests by Kiro Crew. Co-authored-by: Zezhen Xu <zezhexu@amazon.com> Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
3 participants
Problem
The gateway crash-loops every 6–8 minutes while the knowledge folder watcher
scans. Ten loop-stall dumps landed in one hour on a build that already carries
the earlier off-loop fixes, every one with
delete_items_batchon the mainthread. The user-visible symptom is the app dying and relaunching repeatedly,
each relaunch re-paying full startup.
Why it matters
A stalled event loop freezes every task on it — the user's chat turn and the
liveness heartbeat alike — so the watchdog kills the process and the supervisor
respawns it into the same scan. That is a self-sustaining loop, not a one-off
hitch: the app is never reliably usable while a source with duplicate content
is being scanned.
Fix (symptom → root cause → change)
The earlier pass wrapped the five
delete_items_batchsites reachable from theingest body, but
_skip_as_duplicatewas missed. It is a sync method calledbare from two
async defs (ingest_file,ingest_text), and it deletes thesuperseded items at
ingestion.py:217directly on the loop.Why that delete is expensive:
delete_items_batchdoes not just delete. Afterthe per-item cascade it re-runs the orphan-entity sweep — three
NOT INsubqueries over the mention and relation tables — and then calls
_load_graph(),a full rebuild of the entity graph. On a large library that is seconds of
blocking SQLite, paid per duplicate file the scan meets.
The change routes the gate through the existing
run_to_completionhelper, sothe delete, the source-location attach and the terminal job row travel as one
hop. That grouping is deliberate and load-bearing: splitting the committed delete
from the row that records it strands data, because the next scan then re-ingests
alongside the orphaned items — the same hazard the helper was introduced for.
run_to_completionis widened to forward its callable's return value(
Callable[[], _T] -> _T). The gate reports a job id its callers branch on;with the old
-> Nonesignature that value would have to be read after theawait, splitting the very unit the hop exists to keep whole. Cancellation
semantics are unchanged — the work is still drained and the
CancelledErrorstill wins; only the value is dropped. Backward-compatible for the two existing
None-returning finalizers.Scope note, stated plainly: this makes the gateway survive the scan. It does
not make the work cheap — a full graph rebuild after deleting a handful of items
is a separate defect, and each duplicate file still burns that cost on a worker
thread. Incremental graph maintenance touches every mutation path and belongs in
its own change, not smuggled into a crash-loop fix.
Tests
Two added to
test/test_knowledge_delete_off_loop.py, alongside the existingAST ratchet and off-thread assertions:
test_duplicate_skip_runs_the_delete_off_the_loop_thread— records the threadident inside a wrapped
delete_items_batchand asserts it is not the loopthread. It also asserts the gate was actually reached, so the test cannot pass
vacuously if the duplicate path is skipped.
test_run_to_completion_forwards_the_return_value— pins the widenedsignature, which is what lets the mutation and its reported value stay in one
hop.
Manual verification
N/A — unit coverage is sufficient. The defect is "which thread runs this call",
which the off-thread assertion checks directly and the AST ratchet prevents
regressing. Reproducing the crash loop itself needs a multi-thousand-file corpus
with duplicate content and a live watchdog, which no unit test should carry.
Screenshots
N/A — no user-visible UI change.