fix(AAIDomains): separate Supabase fetch retry from onPage callback

[maxtechera]

Problem

In _executeLoad, the retry loop was catching ALL errors — including errors from the onPage callback (DB save). A failed DB insert would cause _executeLoad to re-fetch the same Supabase page and retry the save, potentially creating duplicate chunks in the document store.

Fix

Restructure _executeLoad so the retry loop covers only the Supabase fetch. onPage is called after a successful fetch, outside the retry block. If onPage throws, the error propagates up to the caller (_saveChunksToStorage catches it and sets saveFailed = true).

Also fixes: Production server is running 3.0.11 (PR #1024 image) without loadStream — typeof docNodeInstance.loadStream === 'function' returns false, falls back to init() which loads all pages. This PR + staging deploy will activate the streaming path.

[linear]

KUMELLO-68 fix(AAIDomains): streaming loader — never hold all documents in memory

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
answerai-docs	Building	Preview	Mar 16, 2026 10:31pm
the-answerai	Building	Preview	Mar 16, 2026 10:31pm

[claude]

PR Review posted below as separate comments due to formatting constraints.

[claude]

PR Review - fix(AAIDomains): separate Supabase fetch retry from onPage callback. Full review below.

[claude]

SUMMARY: This PR fixes a correctness bug in _executeLoad where the retry loop wrapped both the Supabase fetch and the onPage callback. A failed DB save caused the same page to be re-fetched and re-processed, potentially producing duplicate document chunks. The fix narrows the retry scope to the Supabase fetch only so onPage errors propagate to the caller without triggering retries.

[claude]

CRITICAL ISSUES: None identified.

[claude]

MAJOR CONCERN 1 - Silent iteration when all pages are filtered out by tag logic (AAIDomains.ts lines 569-596): After a successful fetch, pageData may be non-empty but filteredDomains can still be empty after tag filtering. fetchedDomainCount is not incremented in that case. The outer while loop condition (fetchedDomainCount < this.limit) means if every page is fully filtered out, the loop only terminates when Supabase returns a short page or no data. The lastId cursor is correctly advanced from the unfiltered pageData regardless, so pagination is safe. This is not a regression - the same behaviour existed before - but the refactor makes this path more prominent and worth noting.

[claude]

MAJOR CONCERN 2 - Dead-code guard at line 567: The check [if (retryCount >= maxRetries) break] is effectively unreachable. When all retries are exhausted the inner while loop unconditionally throws lastError (line 562), so execution never arrives at line 567. It would only activate if maxRetries were 0, but it is hardcoded to 3. Consider removing it or adding a comment clarifying it is a purely defensive guard, to avoid misleading future readers.

[claude]

MINOR SUGGESTIONS: (1) shouldStopPagination flag - In the new structure this flag is set and checked within the same outer iteration. Direct break calls at each assignment site would be equally clear with less cognitive overhead. Style only, no correctness impact. (2) pageData typed as any[] | null - Pre-existing pattern throughout the file, discards Supabase SDK type info. No action required in this PR but worth addressing in a future cleanup pass.

[claude]

THEANSWER-SPECIFIC CHECKLIST: tags:[AAI] present on AAIDomains_DocumentLoaders (line 30) confirmed. Document loader component only - no routes/controllers/services modified so multi-tenancy and enforceAbility checks are not applicable. No new database queries introduced. No credentials or secrets touched. No unit or integration tests added (see Test Coverage note).

[claude]

TEST COVERAGE: No tests added or updated. Given that the bug (retry loop re-invoking onPage on transient DB failures) is non-trivial to reproduce manually, a focused unit test for _executeLoad would significantly increase confidence and guard against regressions. Suggested approach: mock the Supabase client to return a successful page while the onPage callback throws on the first call, then verify that onPage is called exactly once per page and the error propagates without triggering a re-fetch of the same page.

[claude]

POSITIVE OBSERVATIONS: The refactor correctly separates concerns at the right boundary: network-level retries vs. application-level callback processing - this is the right architectural decision. Commit message follows conventional commit format with a thorough body explaining both root cause and fix. Renaming error to fetchError in the catch block makes the error origin unambiguous at a glance. The inline comment at line 573 clearly documents the intended error-propagation behaviour for future maintainers. Cursor advancement (lastId) is only updated after onPage succeeds, correctly preserved from the original. PR targets staging as required by project conventions.

[claude]

NEXT STEPS: (1) Consider adding a unit test for _executeLoad covering the separated retry/callback boundary to protect this fix from future regressions. (2) Optionally remove or annotate the dead-code guard at line 567. (3) The logic of the fix is sound. The PR is otherwise ready to merge to staging.

[claude]

PR Review: fix(AAIDomains): separate Supabase fetch retry from onPage callback

Summary: This PR fixes a genuine production bug where a failed DB save inside onPage could trigger a Supabase re-fetch and produce duplicate document chunks. The fix correctly separates the fetch-with-retry loop from the callback invocation, and adds a targeted select: { id: true } projection optimization to two chunk-ID queries. The changes are well-scoped, the logic is sound, and the PR description accurately captures the problem and the solution.

---

Critical Issues

None.

---

Major Concerns

None. The core bug fix is correct.

---

Minor Issues and Suggestions

1. Dead code guard on line 567 of AAIDomains.ts

Location: packages/components/nodes/documentloaders/AAIDomains/AAIDomains.ts — the if (retryCount >= maxRetries) break guard after the inner while loop.

The inner while (retryCount < maxRetries) loop has only two exit paths:

• A successful fetch hits break, leaving retryCount < maxRetries.
• The final failure enters the else branch and executes throw lastError, which propagates immediately out of the outer loop as well.

There is no code path through which the inner loop exits normally with retryCount >= maxRetries. The guard is therefore unreachable dead code. It does not cause a runtime problem, but it suggests the author may have been uncertain about the exit conditions, and it adds noise for future readers. Consider removing it and relying on the thrown error to surface fetch failures, or add a comment explaining why it is kept as a defensive measure.

2. shouldStopPagination variable is now used only locally

Location: packages/components/nodes/documentloaders/AAIDomains/AAIDomains.ts around lines 443, 571, 594, 599.

In the original code, shouldStopPagination was checked in the loop condition if (shouldStopPagination || retryCount >= maxRetries). In the refactored version, both the true assignment and the subsequent if (shouldStopPagination) break happen within the same outer-loop iteration. The variable now only spans a few lines and adds a level of indirection without much benefit. This is a minor style point — an immediate break at the assignment sites would be equally readable and slightly more direct.

3. Silent swallow of streamError in _saveChunksToStorage

Location: packages/server/src/services/documentstore/index.ts, the catch (streamError) block around line 1400.

This is pre-existing behavior and not introduced by this PR, but the PR description explicitly calls out that _saveChunksToStorage catches the error and sets saveFailed = true. It is worth confirming that the caller of _saveChunksToStorage correctly surfaces this failure to the user or retries appropriately, since swallowing streamError without logging or re-throwing means the original exception detail is permanently lost. If it is not already logged before being caught, adding at minimum console.error('[documentstore] Stream save failed:', streamError) before setting saveFailed = true would improve observability.

---

Positive Observations

• The structural separation of fetch-with-retry from callback invocation is clean and solves the stated problem directly. The intent is immediately clear from the code and comment // Process page and invoke callback — errors propagate up, not retried.
• Renaming error to fetchError in the catch block improves clarity by signaling that this is a network/Supabase error, not a processing error.
• The select: { id: true } projection in both chunk-ID queries is a meaningful performance improvement. Fetching only the id column instead of all chunk columns avoids deserializing potentially large pageContent and metadata fields for what is just an existence/ID check.
• The existing multi-tenancy filters (userId, organizationId) are preserved correctly in both modified queries.
• The tags: ['AAI'] requirement is already satisfied — the component class carries this.tags = ['AAI'] at construction time.
• lastId cursor advancement remains correctly tied to pageData (the raw Supabase response) rather than the filtered filteredDomains array, which is the right behavior — pagination must advance based on the full page, not just the subset that passes tag filtering.

---

TheAnswer-Specific Checklist

• Multi-tenancy: both modified chunkRepository.find() calls retain userId and organizationId filters
• Authentication: no route changes in this PR
• Tags: tags: ['AAI'] present on AAIDomains_DocumentLoaders
• Error handling: fetch errors are thrown and not silently swallowed in the new code path
• Testing: no unit or integration tests are included for the refactored _executeLoad logic — given this is a subtle concurrency/ordering fix, a test that verifies onPage errors do not trigger a Supabase re-fetch would be a valuable regression guard

---

Next Steps

The fix is safe to merge. The suggestions above are all minor; none block the PR. If bandwidth allows, adding a test for the onPage error isolation behavior would prevent this class of regression from reappearing silently in future refactors.