Skip to content

Prevent transient table lookup errors from marking all partitions pending#9750

Open
begelundmuller wants to merge 2 commits into
mainfrom
begelundmuller/fix-model-exists-error-handling
Open

Prevent transient table lookup errors from marking all partitions pending#9750
begelundmuller wants to merge 2 commits into
mainfrom
begelundmuller/fix-model-exists-error-handling

Conversation

@begelundmuller

@begelundmuller begelundmuller commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
  • Exists() in the DuckDB and ClickHouse model managers swallowed all lookup errors, so a transient connectivity failure (e.g. an unreachable database ATTACHed via init_sql) read as a missing table and escalated a scheduled refresh into a full reset.
  • The model reconciler now fails the reconcile on an errored existence check (retrying at the next scheduled refresh) instead of assuming the table is missing; a manually triggered full refresh still proceeds as an escape hatch.
  • The metrics view validation error could not find table now says failed to look up table, since it wraps any lookup error; only genuine absence reports "does not exist".

@begelundmuller begelundmuller self-assigned this Jul 24, 2026
@begelundmuller
begelundmuller requested a review from k-anshul July 24, 2026 11:11
@begelundmuller begelundmuller changed the title Prevent transient table lookup errors from triggering full model resets Prevent transient table lookup errors from marking all partitions pending Jul 24, 2026
@nishantmonu51

Copy link
Copy Markdown
Collaborator

@codex: review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d56a69bbc9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if !model.Spec.TriggerFull {
return runtime.ReconcileResult{
Err: fmt.Errorf("failed to check if model output exists (trigger a full refresh to rebuild anyway): %w", err),
Retrigger: refreshOn,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Recompute a future retry time after lookup failures

When a scheduled model reaches its refresh time and Exists returns a transient error, refreshOn was computed from the last successful RefreshedOn, so it is already in the past. Returning that same value makes processCompletedInvocation treat the retrigger as immediate (inv.reschedule = true when the time is not after time.Now()), causing a tight reconcile loop that repeatedly hits the failing connector until it recovers. For this error path, use a future retry time such as nextRefreshTime(time.Now(), ...) instead of the stale scheduled time.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants