Skip to content

fix(dags): read duckling backfill bucket from stored catalog - #64909

Merged
fuziontech merged 4 commits into
masterfrom
fix/duckling-backfill-bucket-from-catalog
Jun 19, 2026
Merged

fuziontech merged 4 commits into
masterfrom
fix/duckling-backfill-bucket-from-catalog

Conversation

@fuziontech

Copy link
Copy Markdown
Member

Problem

The duckling events backfill DAG started failing with a ClickHouse S3 error during the Parquet export step:

Code: 499. DB::Exception: The specified bucket does not exist.

The regression traces to #63834 (merged the day before this fix). That PR replaced DuckLakeCatalog with DuckgresServer in the backfill connection path and, as part of it, stopped reading the org's bucket name from the stored DuckLakeCatalog.bucket field and instead recomputed it deterministically as posthog-duckling-{org}-{suffix} via derive_duckling_bucket.

DuckgresServer has no bucket field, so the new code had nothing to read from and reconstructed the name from the org id. When an org's actually-provisioned bucket name doesn't match that exact pattern (or the env suffix resolves differently than expected in the Dagster runtime), ClickHouse is handed a bucket that doesn't exist and the export blows up — surfacing as a raw ClickHouse 499 deep inside the op rather than a readable error.

Changes

_resolve_duckling_target now reads bucket / bucket_region from the org's stored DuckLakeCatalog row — the source of truth written at provisioning time — and only falls back to the deterministic derive_duckling_bucket derivation when there is no catalog row (e.g. dev mode, where the lookup returns None).

This restores the pre-#63834 behavior for the bucket while keeping the new duckgres-backed connection path intact. The catalog lookup reuses the existing get_ducklake_catalog_for_organization helper.

How did you test this code?

I'm an agent (Claude Opus 4.8). Automated checks I actually ran:

  • isolated ruff check and ruff format --check (line-length 120) on the modified file — both clean
  • python3 -c "import ast; ast.parse(...)" syntax check

I did not run the full Python suite locally (the local venv is stale and hogli/flox were unavailable). Relying on CI for the full lint + test run. No automated test was added — the change is a narrow source-of-truth swap in an existing resolver; a unit test asserting "stored catalog bucket wins over derived" would be a reasonable follow-up.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Debugged from a Dagster stack trace the user pasted. Walked the call chain (duckling_events_backfill → export_events_to_duckling_s3 → INSERT INTO FUNCTION s3(...)) to the ClickHouse 499, then used git log/git show to bisect the bucket-resolution change to #63834. Confirmed DuckgresServer carries no bucket field while DuckLakeCatalog.bucket is still populated, so the fix reads from the catalog and falls back to the derived name only when no row exists.

No skills were applicable (Dagster DAG / ClickHouse export, not a DRF/migration/frontend surface).

🤖 Generated with Claude Code

#63834 swapped the duckling events backfill from reading the org's
provisioned bucket name off DuckLakeCatalog.bucket to recomputing it as
posthog-duckling-{org}-{suffix}. When the stored name doesn't match that
deterministic pattern, ClickHouse writes to a nonexistent bucket and the
export fails with S3 error 499 "The specified bucket does not exist".

Resolve the bucket from the stored DuckLakeCatalog row (the provisioning
source of truth), falling back to the derived name only when no catalog
row exists (e.g. dev mode).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 19, 2026 16:10
@github-actions

Copy link
Copy Markdown
Contributor

Hey @fuziontech! 👋

It looks like your git author email on this PR isn't your @posthog.com address (fuziontech@gmail.com). Since you're on the PostHog team, it's worth pointing your local git author email at your @posthog.com address. Why it matters:

  • Consistent work identity in git history — internal tooling that attributes commits to team members keys off your @posthog.com address.
  • Keeps team contributions easy to tell apart from external community ones when scanning history.

You can fix it for this repo with:

git config user.email "you@posthog.com"

Or set it globally with git config --global user.email "you@posthog.com". No need to redo this PR — just a nudge for next time. 🙂

Copilot AI left a comment

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.

Pull request overview

Fixes the duckling events backfill Dagster job’s S3 Parquet export failures by restoring the bucket/region source-of-truth to the organization’s persisted DuckLakeCatalog row (written at provisioning time), while keeping the newer duckgres-backed connection path.

Changes:

  • Update _resolve_duckling_target to prefer DuckLakeCatalog.bucket / bucket_region for the org.
  • Fall back to derive_duckling_bucket(org_id) only when no catalog row exists (e.g. dev mode).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +202 to +206
catalog = get_ducklake_catalog_for_organization(org_id)
if catalog is not None and catalog.bucket:
bucket, bucket_region = catalog.bucket, catalog.bucket_region
else:
bucket, bucket_region = derive_duckling_bucket(org_id)
The derive_duckling_bucket fallback only fires for orgs with no stored
DuckLakeCatalog row (dev mode, or orgs provisioned with only a
DuckgresServer). That derived name is an unverified guess at the real
bucket — the exact case that produces ClickHouse 499. Log it so the
failure mode is observable instead of silent.

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

greptile-apps Bot commented Jun 19, 2026 •

Copy link
Copy Markdown
Contributor

Comments Outside Diff (1)

  1. posthog/dags/events_backfill_to_duckling.py, line 177-183 (link)

    P2 The DucklingTarget docstring now contradicts the fix: _resolve_duckling_target does make a catalog-DB lookup (via get_ducklake_catalog_for_organization) and the bucket is preferentially read from that stored row, not deterministically derived. Any developer reading the class docstring will have the wrong mental model.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: posthog/dags/events_backfill_to_duckling.py
    Line: 177-183
    
    Comment:
    The `DucklingTarget` docstring now contradicts the fix: `_resolve_duckling_target` does make a catalog-DB lookup (via `get_ducklake_catalog_for_organization`) and the bucket is preferentially read from that stored row, not deterministically derived. Any developer reading the class docstring will have the wrong mental model.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.

    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!

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
posthog/dags/events_backfill_to_duckling.py:177-183
The `DucklingTarget` docstring now contradicts the fix: `_resolve_duckling_target` does make a catalog-DB lookup (via `get_ducklake_catalog_for_organization`) and the bucket is preferentially read from that stored row, not deterministically derived. Any developer reading the class docstring will have the wrong mental model.

```suggestion
    """Resolved per-org duckling backfill target: connection identity + S3 storage.

    Built once per run from the team's organization id. The duckgres connection is driven
    by make_duckgres_conninfo; the S3 bucket is read from the org's stored DuckLakeCatalog
    row (provisioning source of truth) and only falls back to deterministic derivation when
    no catalog row exists (e.g. dev mode).
    """
```

Reviews (1): Last reviewed commit: "chore(dags): warn when duckling bucket f..." | Re-trigger Greptile

@fuziontech
fuziontech requested a review from a team June 19, 2026 16:19
fuziontech and others added 2 commits June 19, 2026 09:23
The class docstring and module header still described the bucket as
deterministically derived with no catalog-DB lookup. _resolve_duckling_target
now reads it from the stored DuckLakeCatalog row, so update both to match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collapse the multi-line import to a single line to match the repo ruff
isort config (I001).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fuziontech
fuziontech merged commit 8ffcb4d into master Jun 19, 2026
226 checks passed
@fuziontech
fuziontech deleted the fix/duckling-backfill-bucket-from-catalog branch June 19, 2026 16:51
@deployment-status-posthog

deployment-status-posthog Bot commented Jun 19, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-06-19 17:43 UTC Run
prod-us ✅ Deployed 2026-06-19 17:54 UTC Run
prod-eu ✅ Deployed 2026-06-19 17:57 UTC Run

fuziontech added a commit that referenced this pull request Jun 19, 2026
…fill-admin

* origin/master: (203 commits)
  fix(experiments): add missing event properties to the backend events on the recalculation workflow. (#64923)
  refactor(capture): decouple v1 RequestContext from analytics Context (#64184)
  feat(data-warehouse): add direct_query_enabled toggle to data sources (#64685)
  feat(data-warehouse): persist duckling bucket on DuckgresServer (#64919)
  fix(dev): strip read-denied $HOME dirs from sandbox PATH (#64768)
  fix(mcp-analytics): keep muted text readable inside quill subtrees (#64898)
  feat(skills): add Scouts tab backed by a generic skill category (#64796)
  fix(ducklake): make get_instance_region a lazy import in common helpers (#64913)
  fix(replay-vision): drop vestigial scanner_type from VisionAction selection (#64737)
  feat(warehouse): add xmin read path for postgres source (#64721)
  feat(warehouse): add per-table xmin sync radio (flag-gated) (#64718)
  feat(warehouse): validate xmin sync type and surface availability (#64723)
  fix(data-warehouse): CDC improvements — fix dead orphan-slot sweeper (#64725)
  feat(data-warehouse): scaffold Razorpay source (#64484)
  fix(dags): read duckling backfill bucket from stored catalog (#64909)
  feat(inbox): add archived tab with restore (#64876)
  feat(signals): surface a banner when inbox filters are active (#64811)
  fix(lemon-dialog): isolate nested form dialogs with unique kea logic keys (#62113)
  feat(session-replay): show event triggers on mobile replay settings tab (#64859)
  fix(preflight): restore kafka connectivity probe (#58743)
  ...
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