Skip to content

feat(data-warehouse): split failing backfills out of the duckgres blocked backlog - #68785

Merged
EDsCODE merged 9 commits into
masterfrom
feat/duckgres-sink-failing-backfill-tracking
Jul 9, 2026
Merged

feat(data-warehouse): split failing backfills out of the duckgres blocked backlog#68785
EDsCODE merged 9 commits into
masterfrom
feat/duckgres-sink-failing-backfill-tracking

Conversation

@EDsCODE

@EDsCODE EDsCODE commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Problem

The duckgres sink's blocked-backlog gauges conflate two very different things: batches waiting behind a schema whose backfill is progressing normally (drains on its own), and batches waiting behind a schema whose backfill fails persistently (never drains without intervention). One wedged schema pins duckgres_sink_blocked_oldest_age_seconds for days, so the gauge can neither carry a page nor distinguish a real throughput problem from a single broken table.

Two aggravating factors:

  • The gauges are batch-derived and bounded by the queue's 14-day retention, so a schema wedged past retention falls off the metric while still broken - the failure becomes invisible exactly when it's oldest.
  • The backfill planner retries a failing schema in a ~30s tight loop forever, hammering a broken Delta table with no backoff and no durable record of how long it's been failing.

Changes

Failure lifecycle on DuckgresSinkSchemaState (migration 1248 adds consecutive_failures, first_failed_at):

  • Every failed backfill attempt bumps the streak and stamps first_failed_at once per streak. Any forward progress (successful plan, chunk applied, primed) resets both plus last_error.
  • A schema is failing when its streak reaches 3, or it's parked in needs_resync. Capacity reverts, supersession by a replace run, and operator replan_backfill are streak-neutral.
  • A terminally failed backfill run observed by reconcile escalates the streak straight to threshold - duckgres failed statuses are terminal, so there's no retry loop to wait out, and without this a mid-backfill wedge would sit in the pageable bucket forever.

Retry backoff: the planner skips a failing schema still inside min(30s * 2^(failures-1), 30min) (jittered) since its last attempt. It never gives up - at the cap that's one attempt per ~30min, so a schema self-heals the moment the underlying issue clears.

Metric split (batch-derived, retention-bounded):

Gauge Now counts Alerting
duckgres_sink_blocked_backlog / _oldest_age_seconds healthy in-progress schemas only pageable - fires only on legitimate pileup
duckgres_sink_failing_blocked_backlog (new) blocked batches behind failing schemas none, dashboard only

Durable stuck gauges (state-derived, survive queue retention): duckgres_sink_stuck_backfill (count of failing schemas) and duckgres_sink_stuck_oldest_age_seconds (age of the oldest first_failed_at). Deliberately unalerted: they're an operator remediation queue and the data source for a future UI surface, not a page.

How did you test this code?

Automated tests only (I'm an agent; no manual prod testing). Full duckgres pipeline_v3 suite passes locally (99 tests). New tests, each catching a regression nothing covered before:

  • TestFailureStreak - streak records once per attempt with a stable first_failed_at anchor; BackfillUnsupportedError parks with a streak; capacity reverts stay neutral (a busy org must not misclassify); backoff skips inside the window and retries after it; mark_primed/replan_backfill reset the streak; reconcile escalates a terminally failed run to failing exactly once (the silent-suppression gap), stays neutral on supersession, and resets on chunk progress.
  • TestFailingSchemaClassification - the failing predicate over state x streak combinations, including that primed never classifies.
  • Stuck-gauge test asserts the gauges derive from the state table with zero rows in the batch queue - the retention-independence this PR exists for.
  • Extended test_backlog_stats - failing schemas' batches leave the pageable blocked bucket (count and oldest-age) and land in the failing bucket; a failing-but-eligible schema is not counted (failing splits the blocked set only).

Migration applies cleanly; makemigrations --check shows no drift.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

Internal consumer/metrics behavior; no user-facing docs affected.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

  • Authored with Claude Code (Fable 5) at Eric's direction, implementing a design brainstormed in-session: track hard-blocked schemas separately and durably so the blocked-backlog gauge only pages on legitimate pileup, with hard-blocked failures surfaced for a later UI (explicitly not alerted).
  • Skills invoked: /django-migrations (additive columns, no external writers of the table so no db_default needed) and /writing-tests (value gate for the new test classes).
  • Key decisions: failure fields merged into each site's existing CAS update so a failure is never recorded over a state another pod advanced; reconcile jumps a terminally failed run's streak straight to threshold rather than dripping one per tick (a terminal status has no retry semantics to ride out); the classification test lives in a transaction=True class because failing_schema_ids does thread-entry close_old_connections(), which severs the transaction-wrapped test connection.

🤖 Generated with Claude Code

…cked backlog

A schema whose backfill fails persistently (planner error or a terminally
failed backfill run) previously kept its blocked batches on the pageable
blocked-backlog gauges forever, pinning the oldest-age panel and hiding
genuine throughput problems - and once its batches aged past queue
retention the failure disappeared entirely. The planner also retried the
broken schema every ~30s with no backoff.

- DuckgresSinkSchemaState gains consecutive_failures + first_failed_at;
  every failed attempt records the streak, any forward progress resets it
- failing := streak >= 3 (or parked in needs_resync); capacity reverts and
  supersession stay neutral
- planner retries failing schemas with capped exponential backoff
  (30s -> 30min, never gives up)
- blocked gauges now count healthy in-progress schemas only (pageable);
  failing schemas' batches move to duckgres_sink_failing_blocked_backlog
  (unalerted)
- new state-derived duckgres_sink_stuck_backfill{,_oldest_age_seconds}
  gauges survive queue retention - the durable backfill-owed signal

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@EDsCODE EDsCODE self-assigned this Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Migration SQL Changes

Hey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:

posthog/migrations/1249_duckgressinkschemastate_consecutive_failures_and_more.py

BEGIN;
--
-- Add field consecutive_failures to duckgressinkschemastate
--
ALTER TABLE "posthog_duckgressinkschemastate" ADD COLUMN "consecutive_failures" integer DEFAULT 0 NOT NULL;
ALTER TABLE "posthog_duckgressinkschemastate" ALTER COLUMN "consecutive_failures" DROP DEFAULT;
--
-- Add field first_failed_at to duckgressinkschemastate
--
ALTER TABLE "posthog_duckgressinkschemastate" ADD COLUMN "first_failed_at" timestamp with time zone NULL;
COMMIT;

Last updated: 2026-07-09 20:03 UTC (befaf94)

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🔍 Migration Risk Analysis

We've analyzed your migrations for potential risks.

Summary: 1 Safe | 0 Needs Review | 0 Blocked

✅ Safe

Brief or no lock, backwards compatible

posthog.1249_duckgressinkschemastate_consecutive_failures_and_more
  └─ #1 ✅ AddField
     Adding NOT NULL field with constant default (safe in PG11+)
     model: duckgressinkschemastate, field: consecutive_failures
  └─ #2 ✅ AddField
     Adding nullable field requires brief lock
     model: duckgressinkschemastate, field: first_failed_at

📚 How to Deploy These Changes Safely

AddField:

This operation acquires a brief lock but doesn't rewrite the table.

Deployment uses lock timeouts with automatic retries, so lock contention will cause retries rather than connection pile-up.

Last updated: 2026-07-09 20:03 UTC (befaf94)

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff (1)

  1. products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/duckgres/backfill.py, line 344-350 (link)

    P2 Superseded NEEDS_RESYNC schemas inflate stuck count but have no age

    _failing_q() classifies all NEEDS_RESYNC schemas as failing regardless of streak, which is intentional. But when a schema enters NEEDS_RESYNC via supersession by a replace run (tested in test_reconcile_supersession_is_streak_neutral), it has first_failed_at=None. Min("first_failed_at") ignores NULLs, so if every stuck schema is a superseded one, STUCK_BACKFILL_GAUGE shows N but STUCK_BACKFILL_OLDEST_AGE_GAUGE stays 0. An operator looking at the dashboard sees "N stuck schemas" with "oldest age = 0 seconds", which is misleading — it looks like the gauge is broken rather than signalling that those schemas are awaiting a replace run. Consider stamping first_failed_at on the supersession path, or excluding NEEDS_RESYNC-without-streak from the age calculation.

Reviews (1): Last reviewed commit: "feat(data-warehouse): split failing back..." | Re-trigger Greptile

Comment on lines +474 to 489
def _revert_to_pending(state_id: Any, error: str | None = None, *, record_failure: bool = False) -> None:
"""record_failure distinguishes a failed attempt from a neutral revert
(capacity re-check lost the race) — capacity is pacing, not failure, and
must not start a streak."""
now = timezone.now()
updates: dict[str, Any] = {
"state": DuckgresSinkSchemaState.State.PENDING_BACKFILL,
"updated_at": timezone.now(),
"updated_at": now,
}
if error is not None:
updates["last_error"] = error
if record_failure:
updates.update(_failure_streak_updates(now=now))
DuckgresSinkSchemaState.objects.filter(
id=state_id, state=DuckgresSinkSchemaState.State.BACKFILLING, backfill_run_uuid__isnull=True
).update(**updates)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Capacity revert resets the backoff anchor

_revert_to_pending always sets updated_at = now, even on the neutral capacity path (record_failure=False). The backoff window in _plan_pending is computed as timezone.now() < state.updated_at + backoff_seconds, so a schema that just passed its 30-minute backoff window, got capacity-reverted, and went back to PENDING will see its anchor reset to NOW — meaning it must wait another full backoff window before the planner will attempt it again.

In a consistently capacity-constrained environment (pre-check passes, post-claim check fails repeatedly) a schema near the cap could have its 30-minute window restarted on every planner tick, preventing it from ever retrying despite the failure streak counting up. The fix is to skip the updated_at write in the neutral-revert path when the schema already has an active streak (consecutive_failures > 0), or to track the backoff anchor separately from updated_at.

@trunk-io

trunk-io Bot commented Jul 8, 2026

Copy link
Copy Markdown

Static BadgeStatic BadgeStatic BadgeStatic Badge

View Full Report ↗︎Docs

EDsCODE and others added 3 commits July 9, 2026 09:35
…k tests

Satisfies the test-datetime-now-without-freeze semgrep rule, which landed on
master after this branch was cut.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…and tests

updated_at is nullable to the type checker (auto_now + null=True), so guard it
before the backoff arithmetic. Route the fake queue connection through a typed
factory instead of passing it straight to _reconcile_one.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@EDsCODE
EDsCODE marked this pull request as ready for review July 9, 2026 17:50
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team July 9, 2026 17:50
@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Reviews (2): Last reviewed commit: "fix(data-warehouse): satisfy mypy in duc..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

Playwright — all passed

All tests passed.

View test results →

@tests-posthog

tests-posthog Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

⏭️ Skipped snapshot commit because branch advanced to 23cdb1f while workflow was testing ceb93f9.

The new commit will trigger its own snapshot update workflow.

If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:

  • Merge master into your branch, or
  • Push an empty commit: git commit --allow-empty -m 'trigger CI' && git push

@EDsCODE
EDsCODE merged commit d4891c2 into master Jul 9, 2026
239 checks passed
@EDsCODE
EDsCODE deleted the feat/duckgres-sink-failing-backfill-tracking branch July 9, 2026 21:53
@deployment-status-posthog

deployment-status-posthog Bot commented Jul 9, 2026

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-07-09 22:28 UTC Run
prod-us ✅ Deployed 2026-07-09 22:41 UTC Run
prod-eu ✅ Deployed 2026-07-09 22:44 UTC Run

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.

2 participants