From d56351206cd071de0fb19538c8a1db3a548cb804 Mon Sep 17 00:00:00 2001 From: Jakob Homan Date: Thu, 10 Sep 2026 13:49:43 -0700 Subject: [PATCH 1/2] fix(metrics): end remote catalog transactions eagerly with explicit BEGIN/COMMIT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The daemon's attached-Postgres connection sat 'idle in transaction' (REPEATABLE READ) on the shared megaduck catalog for the entire inter-query interval — minutes at a time, renewed every cycle, so effectively permanent. An idle-in-transaction snapshot pins vacuum and lengthens every writer's OCC conflict window on a catalog that is already the fleet's contention point. Root cause (verified live against megaduck, 2026-09-10): duckdb-postgres under autocommit ends the REMOTE transaction lazily — the connection stays parked in its transaction until the next statement reuses it. pg_pool_max_connections=0 does NOT fix this; the pool setting governs only the secondary connections, and the primary stays parked (also verified live). An explicit transaction commits the remote side eagerly. _run_query now brackets every query in BEGIN/COMMIT: - COMMIT fires as soon as the cursor is drained, so gauge bookkeeping cannot extend the transaction and a gauge-side exception cannot leak it - the server_sql -> local fallback reopens the bracket after the expected BinderException (rollback is version-tolerant: the failed statement either aborts the transaction or unwinds it) - the failure path rolls back best-effort; a dead connection stays the reconnect path's problem Three regression tests pin the no-open-transaction postcondition on the success, failure, and fallback paths (a leaked transaction makes the probe BEGIN raise 'cannot start a transaction within a transaction'). --- tests/unit/test_ducklake_metrics.py | 52 +++++++++++++++++++++++++++++ tools/ducklake_metrics.py | 39 ++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/tests/unit/test_ducklake_metrics.py b/tests/unit/test_ducklake_metrics.py index be581a3..c40afd8 100644 --- a/tests/unit/test_ducklake_metrics.py +++ b/tests/unit/test_ducklake_metrics.py @@ -269,6 +269,58 @@ def test_column_name_mismatch_raises_through_error_path(self, conn, registry): assert _gauge_value(registry, "ducklake_metrics_query_errors_total", {"query": "t_mismatch"}) == 1 + def test_success_leaves_no_open_transaction(self, conn, registry): + # Regression (2026-09-10): under autocommit, duckdb-postgres ended + # the remote transaction lazily, so the daemon's attached-Postgres + # connection sat "idle in transaction" on the shared catalog for + # the whole inter-query interval. _run_query now brackets every + # query in an explicit BEGIN/COMMIT. A leftover open transaction + # makes the BEGIN below raise ("cannot start a transaction within + # a transaction"). + q = dm.Query(name="t_txn", help="t", sql="SELECT 1 AS n", interval_seconds=60, labels=[], values=["n"]) + gauges = dm._build_query_gauges([q], registry=registry) + sm = dm._build_self_metrics(registry=registry) + + assert _run(conn, q, gauges[q.name], sm) is True + conn.execute("BEGIN") + conn.execute("COMMIT") + + def test_failure_leaves_no_open_transaction(self, conn, registry): + # The failure path must roll the bracket back, not leak it. + q = dm.Query( + name="t_txn_fail", help="t", sql="SELECT * FROM no_such_table", interval_seconds=60, labels=[], values=["n"] + ) + gauges = dm._build_query_gauges([q], registry=registry) + sm = dm._build_self_metrics(registry=registry) + + assert _run(conn, q, gauges[q.name], sm) is False + conn.execute("BEGIN") + conn.execute("COMMIT") + + def test_server_sql_fallback_still_works_inside_bracket(self, conn, registry): + # The server-side form fails on a lake without the pg attach + # (BinderException / CatalogException). The bracket must survive + # that failed statement and run the local fallback in a fresh + # transaction. + conn.execute("CREATE TABLE fb (n INTEGER)") + conn.execute("INSERT INTO fb VALUES (7)") + q = dm.Query( + name="t_fallback", + help="t", + sql="SELECT n FROM fb", + server_sql="SELECT n FROM fb", + interval_seconds=60, + labels=[], + values=["n"], + ) + gauges = dm._build_query_gauges([q], registry=registry) + sm = dm._build_self_metrics(registry=registry) + + assert _run(conn, q, gauges[q.name], sm) is True + assert _gauge_value(registry, "t_fallback_n") == 7.0 + conn.execute("BEGIN") + conn.execute("COMMIT") + # --------------------------------------------------------------------------- # Built-in: ducklake_pending_deletes — validate the SQL shape against an diff --git a/tools/ducklake_metrics.py b/tools/ducklake_metrics.py index 21f8f85..5242747 100644 --- a/tools/ducklake_metrics.py +++ b/tools/ducklake_metrics.py @@ -749,6 +749,23 @@ def _run_query( if liveness is not None: liveness.current_query_start = t0 try: + # Explicit transaction bracket, NOT for atomicity (these are + # single read-only statements) but for remote-connection hygiene. + # Under autocommit, duckdb-postgres ends the REMOTE transaction + # lazily: after a statement completes, the attached-Postgres + # connection sits in its REPEATABLE READ transaction ("idle in + # transaction" in pg_stat_activity) until the next statement + # reuses it — for this daemon, the entire inter-query interval + # (minutes). An idle-in-transaction snapshot on the shared + # catalog pins vacuum and lengthens every writer's OCC conflict + # window. An explicit COMMIT ends the remote transaction eagerly + # (verified against megaduck 2026-09-10: bare scan left the + # connection in-transaction for the full idle window; + # pg_pool_max_connections=0 did NOT help — the primary + # connection is exempt from the pool; a BEGIN/COMMIT bracket + # returned it clean). Safe here: the scheduler is strictly + # serial on this connection. + conn.execute("BEGIN") cur = None if q.server_sql is not None: # Server-side first (see Query.server_sql). Bind/catalog errors @@ -761,6 +778,17 @@ def _run_query( f"{ducklake_maintenance._sql_string_literal(q.server_sql)})" ) except (duckdb.BinderException, duckdb.CatalogException) as e: + # The failed statement may have aborted the explicit + # transaction; reopen it so the fallback runs cleanly. + # ROLLBACK is best-effort: depending on the duckdb + # version the failed statement either aborts the + # transaction (ROLLBACK required) or unwinds it + # (ROLLBACK raises "no transaction is active"). + try: + conn.execute("ROLLBACK") + except duckdb.Error: + pass + conn.execute("BEGIN") if q.name not in _SERVER_SQL_FALLBACK_LOGGED: _SERVER_SQL_FALLBACK_LOGGED.add(q.name) log.info("query %s: server-side form unavailable (%s); using local metadata attach", q.name, e) @@ -768,6 +796,10 @@ def _run_query( cur = conn.execute(q.sql) cols = [d[0] for d in cur.description] rows = cur.fetchall() + # Commit as soon as the cursor is drained: gauge bookkeeping + # below must not extend the remote transaction's lifetime, and a + # gauge-side exception must not leave the transaction open. + conn.execute("COMMIT") try: label_idx = [cols.index(name) for name in q.labels] value_idx = [cols.index(name) for name in q.values] @@ -795,6 +827,13 @@ def _run_query( log.debug("query %s: %d rows in %.3fs", q.name, len(rows), elapsed) return True except Exception: + # Close any transaction the failure left open (including the + # explicit bracket above). Best-effort: on a dead connection the + # ROLLBACK fails too, and the reconnect path owns recovery. + try: + conn.execute("ROLLBACK") + except Exception: # noqa: BLE001 + pass log.exception("query %s failed", q.name) self_metrics.errors.labels(tenant, q.name).inc() return False From 26432abaf0028df2b0cd3a49ac3143d128c8ba92 Mon Sep 17 00:00:00 2001 From: Jakob Homan Date: Thu, 10 Sep 2026 13:52:36 -0700 Subject: [PATCH 2/2] docs: state-file deploy + prod-promotion procedure in AGENT.md Millpond now deploys via the charts state-file mechanism (same as duckgres and viaduck): release dispatch -> state/millpond.yaml -> dev auto-promotes, prod behind the prod-promote-managed-warehouse approval gate. Documents the resolve/delta/fire/approve/verify procedure, that viaduck-metrics rides millpond's prod pin, and that the repo-local promote-to-prod retag now serves only mutable-tag CronJob consumers. --- AGENT.md | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/AGENT.md b/AGENT.md index a5efb96..890c7c5 100644 --- a/AGENT.md +++ b/AGENT.md @@ -446,6 +446,63 @@ Every merge to `main` triggers `.github/workflows/release.yaml`: The tarball is the primary artifact for external Docker builds (e.g. `posthog-cloud-infra`). It includes the lockfile so `uv sync --frozen` produces reproducible installs with pinned binary wheels. Do not distribute standalone wheels — they lack the lockfile and resolve unpinned deps from PyPI. +## Promoting to Prod + +Dev tracks head automatically: the `commit_state_update` dispatch above +writes `state.millpond.image.sha` in PostHog/charts and the deploy bot +promotes `image.dev` (mw-dev rolls immediately). Prod is manual by +policy — `state/millpond.yaml` sets `require_prod_approval: true`, so +the ONLY path to prod is the charts repo's `promote-to-prod.yml` +workflow, gated by required reviewers on the +`prod-promote-managed-warehouse` GitHub environment. Results post to +#alerts-managed-warehouse. (Same mechanism as duckgres and viaduck.) + +1. **Resolve the ref** — the promotable ref is the multi-arch manifest + digest recorded in the state file, never a per-arch digest: + + ```bash + STATE=$(gh api -H "Accept: application/vnd.github.raw" \ + /repos/PostHog/charts/contents/state/millpond.yaml) + DEV=$(echo "$STATE" | yq '.state.millpond.image.dev') + PROD=$(echo "$STATE" | yq '.state.millpond.image.prod') + echo "dev: $DEV"; echo "prod: $PROD" + # The prefix of $DEV must be the millpond main commit you + # intend to ship. If dev lags sha, the CD/deploy-bot hop hasn't + # landed yet — wait. + ``` + +2. **Show the delta going out**: `git log --oneline "${PROD%%@*}..${DEV%%@*}"` + +3. **Fire the promotion** (parks at the approval gate; nothing deploys yet): + + ```bash + gh workflow run promote-to-prod.yml -R PostHog/charts \ + -f app=millpond -f image="$DEV" + ``` + + Open the run page (`gh run list -R PostHog/charts + --workflow=promote-to-prod.yml --limit 1 --json url --jq '.[0].url'`) + and have a required reviewer approve the pending deployment. Never + self-approve programmatically on the operator's behalf. + +4. **Watch and verify**: `gh run watch -R PostHog/charts + --exit-status`, then confirm `image.prod` in the state file moved. + ArgoCD rolls the prod StatefulSets; `kubectl -n millpond get pods -w` + for verification beyond ArgoCD. Rollback = promote the previous + known-good ref (visible in `git log -- state/millpond.yaml` in the + charts repo) through the same workflow. + +Two millpond-specific notes: + +- The `viaduck-metrics` Deployment (viaduck namespace) runs THIS image + at millpond's prod pin — it hosts `tools/ducklake_metrics.py`. A + millpond prod promotion rolls that daemon too. +- The repo-local `.github/workflows/promote-to-prod.yaml` (registry + retag of `:prod`) is NOT the fleet deploy path anymore; the mutable + `:prod` tag remains only for consumers that deliberately pull it + fresh per run (maintenance CronJobs). The ingest StatefulSets follow + the digest-pinned state file only. + ## Deployment Strategy Rolling updates are a poor fit for static partition assignment — during the roll, pods run with different `REPLICA_COUNT` values, causing temporary double-assignment (duplicate writes) or gaps. Since Kafka is the durable buffer, a simpler strategy works: