fix(data-warehouse): stop deleted schemas and dead lock holders from wedging the duckgres planner - #69103
Conversation
…wedging the duckgres planner Two planner resource leaks found in a deep QA pass: Soft-deleted schemas: deletion has no signal into the sink state machine. A deleted schema's chunks are unclaimable (the eligibility gate excludes deleted schemas), so its BACKFILLING row sat forever with zero failures on any gauge while consuming one of the MAX_CONCURRENT_BACKFILLS_GLOBAL slots — five routine source deletions would halt all backfill planning fleet-wide — and the reconciler re-enqueued its chunks after every queue prune. The reconciler now retires such runs and drops the state rows (bootstrap re-creates PENDING_BACKFILL if a schema comes back), and _plan_one skips deleted schemas instead of planning them through the default manager. Advisory-lock waits: enqueue_chunks took a session advisory lock with no lock_timeout on a connection with no keepalives. A pod dying without FIN leaves the server session (and lock) alive for hours, and every other pod's reconciler replay then blocks indefinitely — inline in the consumer fetch path, halting all duckgres claiming, the same shape as the July delta-loader fleet stall. The lock wait is now bounded at 10s (timeout skips the replay for one tick), and the planner's queue connections get connect_timeout and keepalives matching the duckgres connection's rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r-deleted-schemas-lock-timeout
…r-deleted-schemas-lock-timeout
|
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| with psycopg.connect( | ||
| settings.WAREHOUSE_SOURCES_DATABASE_URL, | ||
| autocommit=True, | ||
| # These run inline in the consumer fetch path: a half-open connection | ||
| # must fail in minutes, not the OS TCP timeout (hours). | ||
| connect_timeout=30, | ||
| keepalives=1, | ||
| keepalives_idle=60, | ||
| keepalives_interval=15, | ||
| keepalives_count=4, | ||
| ) as conn: | ||
| retire_backfill_run(conn, run_uuid=state.backfill_run_uuid) |
There was a problem hiding this comment.
replan_backfill, _plan_one, and _reconcile. A single module-level dict extracted once would satisfy the OnceAndOnlyOnce rule and make future tuning a one-line change.
| with psycopg.connect( | |
| settings.WAREHOUSE_SOURCES_DATABASE_URL, | |
| autocommit=True, | |
| # These run inline in the consumer fetch path: a half-open connection | |
| # must fail in minutes, not the OS TCP timeout (hours). | |
| connect_timeout=30, | |
| keepalives=1, | |
| keepalives_idle=60, | |
| keepalives_interval=15, | |
| keepalives_count=4, | |
| ) as conn: | |
| retire_backfill_run(conn, run_uuid=state.backfill_run_uuid) | |
| with psycopg.connect( | |
| settings.WAREHOUSE_SOURCES_DATABASE_URL, | |
| autocommit=True, | |
| **_PLANNER_CONNECT_KWARGS, | |
| ) as conn: | |
| retire_backfill_run(conn, run_uuid=state.backfill_run_uuid) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| conn.execute("SET lock_timeout = '10s'") | ||
| try: | ||
| conn.execute( | ||
| "SELECT pg_advisory_lock(%s, hashtext(%s))", | ||
| [BACKFILL_ENQUEUE_LOCK_NAMESPACE, run_uuid], | ||
| ) | ||
| except psycopg.errors.LockNotAvailable: | ||
| conn.execute("RESET lock_timeout") | ||
| logger.warning("duckgres_backfill_enqueue_lock_timeout", run_uuid=run_uuid) | ||
| return 0 |
There was a problem hiding this comment.
SET lock_timeout = '10s' is a session-level setting, so it remains active for the entire second try block that follows — covering all the INSERT INTO and SELECT pg_advisory_unlock calls, not just the advisory lock acquisition. If a row-level lock wait on an INSERT exceeds 10 s, PostgreSQL raises LockNotAvailable too. The finally block still runs (releasing the advisory lock and calling RESET lock_timeout), so cleanup is correct, but the partial-insertion failure propagates to the caller. Since partial insertion is safe (idempotency via chunk.index in existing), the behaviour is fine on the next tick — but the intent reads as "bound only the advisory-lock wait", and a reader might not realise the timeout is wider. Consider narrowing the scope with RESET lock_timeout immediately after the advisory lock is acquired.
2 participants
Problem
Two resource leaks in the duckgres backfill planner, both of the "quietly wedge the fleet" shape:
_global_at_capacitycounts it againstMAX_CONCURRENT_BACKFILLS_GLOBAL = 5. Five routine deletions halt all backfill planning for every org. The reconciler also re-enqueues the orphaned chunks after each 7-day queue prune, and_plan_onereads schemas through the default manager, which includes soft-deleted rows, so a deleted schema could still be planned fresh.enqueue_chunksserializes on a sessionpg_advisory_lockwith nolock_timeout, on a connection with no keepalives. A pod that dies without FIN leaves the server session (and its lock) alive for hours; every other pod's reconciler replay then blocks on the lock inline in the consumer's fetch path - the whole duckgres sink stops claiming, pod by pod. Same failure shape as a previous fleet-wide delta-loader stall.Changes
_purge_deleted_schema_statessweep in the reconciler: retires the runs of deleted schemas and drops their state rows (bootstrap re-creates PENDING_BACKFILL if a schema is ever restored)_plan_oneskips deleted schemas instead of planning themenqueue_chunksbounds the advisory-lock wait at 10s - on timeout it skips the replay for this tick and logs; the next tick retries once the dead session is reapedconnect_timeout+ keepalives, matching the duckgres connection's bounded-liveness rationaleHow did you test this code?
I (Claude) added four tests to
test_backfill.py, each catching a regression no existing test covered:_plan_onerefuses to plan a deleted schema (the default-manager hole)enqueue_chunksskips the replay, resetslock_timeout, and inserts nothingpytest .../duckgres/test_backfill.py: 15 passed.Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Deep QA session on the duckgres consumer (Claude Code). The deleted-schema leak came from an adversarial state-machine review ("what happens when a user deletes a source mid-backfill"); the unbounded lock from a reliability review that pattern-matched it to a prior fleet stall. I chose deleting the state row over a terminal state value to keep the state machine's surface unchanged; the tradeoff (bootstrap re-primes a restored schema from scratch) is the already-correct behavior for a table whose duckgres copy may have aged out anyway.