Skip to content

fix(pump): separate read routing from tenant webhook writes - #134

Open
olavurellefsen wants to merge 1 commit into
mainfrom
fix/separate-pump-read-routing-20261101
Open

olavurellefsen wants to merge 1 commit into
mainfrom
fix/separate-pump-read-routing-20261101

Conversation

@olavurellefsen

Copy link
Copy Markdown
Contributor

Compute must keep its tenant-specific webhook URL for ingestion. Upgrading its pump passes that same URL to SDK catalog/event-source commands, which fail even though the credentials and dedicated service hosts work. A read-only probe with the new data-pump package reproduced the failure in both existing compute deployments; using SDK read hosts succeeds for both identities.

Add pumpBaseUrl independently of the existing webhook baseUrl: omitted preserves legacy single-host routing, null uses SDK service defaults, and a string selects a read proxy. Webhook writes, provisioning and checkpoints retain their existing behaviour. This complements PR125's optional webhook URL approach for callers that must retain an explicit tenant-specific webhook URL.

Tests capture the real data-pump creation options and public webhook write URL in all three modes. Full PostgreSQL-backed Deno suite: 103 passed (256 steps), zero failures. Lint, typecheck and npm ESM/CJS build pass.

Explore recovery is already verified on backend1.43.1 with all four events acknowledged, empty buffers and no duplicate audit or voucher projections. Compute remains on its recovered deployed version while this compatibility change is reviewed; the 3.0.3 consumer upgrade has not been deployed.

@ramattraomnicable

Copy link
Copy Markdown

Ramattra review

Summary: No blocking issues found — small, well-scoped, backward-compatible change.

This PR adds an optional pumpBaseUrl to PathwaysBuilderConfig so pump reads (catalog / event-source) can be routed independently of the tenant-specific webhook write baseUrl. The routing resolves at both pump configure() call sites as:

this.pumpBaseUrl === null ? "" : this.pumpBaseUrl ?? this.baseUrl

  • omitted → this.baseUrl — identical to legacy single-host behaviour ✅
  • null → "" → baseUrlOverride: "" → SDK dedicated read hosts
  • string → that URL — a separate read proxy

What I checked

  • Webhook writes untouched. webhookBuilderFactory (src/pathways/builder.ts:622) still takes the webhook baseUrl, so ingestion URLs are unchanged — the test asserts the write URL stays .../event/.../sample.0/created.0 in all three modes. ✅
  • Both pump paths updated consistently — the non-leader configure() (builder.ts:2659) and the main one (builder.ts:2672) use the identical expression, and PathwayPump.configure forwards it to FlowcoreDataPump.create as baseUrlOverride (src/pathways/pump/pathway-pump.ts:369). ✅
  • No clone path drops the field. The with* builder methods mutate and return this (no new PathwaysBuilder reconstruction), so the new private pumpBaseUrl survives — no latent "lost on clone" bug despite the "stored for cloning" comment. ✅
  • Provisioning / checkpoints intentionally keep baseUrl per the description — consistent with the diff.

Notes (non-blocking)

  • The test stubs FlowcoreDataPump.create and asserts baseUrlOverride === "" for the null mode. That verifies our routing value is passed through, but not that the real @flowcore/data-pump treats baseUrlOverride: "" as "use SDK default service hosts" rather than an empty/relative base. That semantic is the crux of the null mode and currently rests on the external package's behaviour plus the manual read-probe mentioned in the description — worth keeping in mind if the pump dependency is ever bumped.
  • Minor: an explicit pumpBaseUrl: "" coalesces to "" (same as null) via ??. Harmless and arguably intended, just noting the empty-string case isn't distinct from null.

Automated review by Ramattra — comments only, a human decides on merge.

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