JITSU-227: Add Reverse ETL writer protocol and lifecycle core - #1509
Merged
Merged
Conversation
![jitsu-code-review[bot]](https://avatars.githubusercontent.com/in/3391383?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new reverse-ETL protocol, metadata validation, deterministic identity/store handling, and runner lifecycle (batching, checkpointing, recovery, cancellation, and finish behavior).
No additional actionable findings beyond the two existing unresolved inline threads.
There was a problem hiding this comment.
Reviewed the new Reverse ETL protocol, lifecycle, validation, identity helpers, and recovery tests. Findings: a durable-acknowledgement failure can still abort an already accepted batch; cursorless completed runs persist a state that ordinary subsequent runs reject.
There was a problem hiding this comment.
Reviewed the Reverse ETL protocol, lifecycle/error-recovery paths, registry and package surface. I also checked the existing review threads; the earlier concerns are resolved in this head. No additional actionable correctness, security, or user-visible regression findings.
absorbb
added a commit
that referenced
this pull request
Sep 12, 2026
## Scope First implementation slice of `JITSU-227`: reusable Reverse ETL Models and warehouse readers. Scheduling, destination delivery, state commits and billing follow in separate PRs. - Add feature-gated Models UI with warehouse selection, SQL preview, primary keys, cursor/lookback and tombstone settings. - Reuse configuration-object CRUD, workspace roles and audit logging; store warehouse references, not copied credentials. - Add read-only Postgres and HTTP(S) ClickHouse readers with bounded previews, lossless composite checkpoints and duplicate/null-key failures. - Protect warehouse/model references with transactional checks; keep reverse-sync creation unavailable until the runtime and destination contracts are implemented. - Share model/reverse-sync schemas; permanent row error policy is fixed to `fail`. ## Updated architecture and release plan (September 12) Snapshot-based audience mirroring belongs to the Node runner core and is required for the first production audience release. The core owns desired/effective membership, bounded diff/removal planning, recovery and generation promotion; destination writers have no snapshot context API. This PR's cursorless full-query readers are the source foundation for that core. Next slices: #1509 writer contracts without `ctx.snapshot`; Node PostgreSQL journal/snapshot persistence and migrations; core mirror planner and recovery tests; executable Node runner + syncctl/CronJobs; verified Google upsert/mirror and UI. Native replacement is a later optional provider strategy. This PR's code scope and feature gates are unchanged. ## Rollout and limits - Creating, updating and previewing models requires the workspace `reverse-etl` feature flag. Existing models remain readable/deletable for authorized cleanup when the flag is off; no automatic flag enablement. - Postgres password authentication and ClickHouse HTTP(S) only. Use restricted read-only warehouse credentials. - ClickHouse SQL initially supports the parser's portable SELECT subset. Unsupported syntax fails closed. - Duplicate-key validation uses a database window over the query before incremental filtering; this can require a full scan. - Preview is limited to 100 rows / 2 MB and 30 seconds, and requires edit permission. - Monthly-active-sync billing remains the approved policy, but this PR adds no billing events or scheduled delivery. ## Reader / SQL dialect refactor - Split PostgreSQL and ClickHouse readers into dedicated warehouse files, including their parsing, quoting, cursor type validation, parameter binding, and lookback rules. - Compose `WarehouseReader.sql: WarehouseSqlDialect` with pure `validateQuery`, `validateColumns`, and `compileModel` methods. `getWarehouseSqlDialect` enables validation without credentials or a client. - Keep shared safety checks, checkpoint planning, row decoding, and preview limits in common modules. - Document why PostgreSQL `losslessTypes` preserves exact scalar text for both direct queries and cursors; add precision and dialect-isolation regressions. - Verified installed `node-sql-parser` 5.4.0 has no ClickHouse dialect. Existing portable SELECT support is unchanged; native ClickHouse parsing is a separate compatibility change. ## ClickHouse checkpoint compatibility - Validate every bound cursor/key type before saving a model or beginning its first read, using the same allowlist as checkpoint binding. - Support `FixedString(N)` and outer `LowCardinality` wrappers, preserving nullable types and exact key values, including NUL padding. - Add real ClickHouse save/read/resume regressions and reject unsupported checkpoint key types before persistence. Unbound full-query and lookback keys are checked separately for scalar decoding compatibility, without applying checkpoint-only binding restrictions. ## PostgreSQL preview byte guard - Size the first 100 rows in PostgreSQL before transferring native field values. If their combined text output exceeds 2 MB, return only NULLs and an oversized flag. - Use row 101 solely as a truncation marker, without transferring its field values. - Materialize source expressions once (PostgreSQL 12+); retain native type decoding and the final JSON byte cap. This protects console memory, not source-query memory inside PostgreSQL. - Add raw SQL boundary regressions for oversized first/overflow rows, aggregate and multibyte payloads, JSON escaping, precision, empty results, and volatile expression evaluation. ## Rollout cleanup and primary-key validation - Keep Models navigation visible while existing models need cleanup, even with the rollout flag disabled. The model form is read-only; role-authorized deletion remains available. - Preserve workspace authorization, deletion roles, transactional reference guards and audit logging. A reverse sync must still be removed before its model, and models before their warehouse. - Validate every primary-key column against warehouse-owned scalar decoding rules before create/update or streaming, including full-query and lookback models with zero source rows. - Reject PostgreSQL binary/JSON/array and ClickHouse Array/Map/Tuple keys before persistence, with explicit-cast guidance. PostgreSQL uses a conservative known scalar OID allowlist. - Keep decoding compatibility distinct from checkpoint binding: unbound scalar enums, booleans, IP addresses and timestamp keys with numeric timezone names remain supported. Runtime null/duplicate failures and strict checkpoint binding remain unchanged. - Add role/reference/audit cleanup regressions, read-only editor coverage, and real PostgreSQL/ClickHouse rejection and supported-scalar/cast save/read tests. ## Zero lookback and final rollout gate - An explicitly configured zero-second timestamp lookback replays all rows at or after the saved timestamp, with cursor-only parameter binding; omitted lookback keeps strict composite resume semantics. - Non-timestamp cursors reject any explicit lookback, including zero. - Create/update recheck the rollout flag inside the final write transaction after warehouse inspection. A workspace row SHARE lock keeps the flag stable through commit even against ordinary flag updates that do not take the model advisory lock. - Warehouse I/O remains outside the transaction; flag-off cleanup remains available. - Add PostgreSQL/ClickHouse resume-boundary tests, create/update flag-disablement-during-inspection tests, failed-write audit checks and a competing flag-update locking regression. - Verified the schema-composition review concern is a false positive: ModelDefinition remains a ZodObject and both ModelConfig.parse/safeParse succeed; refinements are on nested fields only. ## Numeric timezones and delete-column compatibility - Allow digits in the strict ClickHouse checkpoint timezone grammar, supporting DateTime/DateTime64 cursors and keys such as Etc/GMT+3 without weakening the metadata interpolation boundary. - Validate delete-column warehouse types before persistence and stream compilation. Preserve boolean, numeric and text 0/1 representations; reject incompatible binary/structured/date/time types with boolean-expression guidance. - Expose the warehouse-owned predicate through WarehouseSqlDialect.supportsDeleteType. Console preview adds optional supportsDelete metadata and the delete-column picker filters its options without importing warehouse clients. - Keep row decoding unchanged: only true/false, numeric 0/1, exact strings "0"/"1", or null (keep) are valid. Metadata is not a guarantee of valid row values; bad values still fail immediately. - Cover real ClickHouse timezone save/read/resume, PostgreSQL/ClickHouse incompatible delete types and rejected updates, preserved valid representations, preview metadata and picker filtering. ## Validation - 277 console unit/integration tests pass, including real Postgres and ClickHouse tests and an editor lookback-preservation regression test. - 164 warehouse-query unit tests pass. - Warehouse-query and console TypeScript checks pass. - ESLint on changed console files and formatting checks pass. - The earlier preview-guard baseline passed Next.js production compilation (`next build --webpack --experimental-build-mode compile`); this follow-up was verified with tests, type-checks and lint. - Local large/risky review: reader and console finders, adversarial verification, fixes and fresh final review before push. No deployment has been triggered. Automatic beta deployment on merge is not enabled unless explicitly approved.
absorbb
dismissed
jitsu-code-review[bot]’s stale review
September 12, 2026 12:20
The base branch was changed.
absorbb
dismissed
jitsu-code-review[bot]’s stale review
September 12, 2026 12:23
The merge-base changed after approval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Second slice of
JITSU-227, building on #1507 (now merged). This PR targetsnewjitsu.The ClickHouse multi-host reader finding inherited from #1507 is addressed separately in #1512. That fix validates and tries all configured endpoints before delivery, keeps metadata/data on the same host per attempt, and never switches replicas after a streamed row has been emitted. It is separate from this PR's writer protocol and still needs to merge to affect production code.
Adds a provider-neutral Reverse ETL writer protocol and an upsert lifecycle library, ready for an in-process PostgreSQL-backed Node persistence module. It does not yet run a CronJob or send advertising API requests.
types/protocols/reverse-etl.d.ts.builtin.reverse.*registry. Existing event destinations remain unchanged.Explicit boundaries
Per the owner's September 10 architecture decision, Reverse ETL uses a single Node runner, not a Go sidecar or Unix-socket RPC. The caller must supply a fenced PostgreSQL-backed journal, with durable encrypted manifests, transactional receipts/effective membership/state, and monthly-active-sync activation outbox. There is no production in-memory journal. That Node backend, recovery, executable runner, syncctl/CronJobs and provider adapters follow in subsequent slices. Writers receive persistence interfaces, not database clients or credentials. syncctl/Kubernetes remain responsible for independent crash/stall detection; existing connector sidecars are unchanged.
Per the owner's September 12 decision, snapshot-based mirroring belongs to the runner core and the first production audience release. This PR removes
ctx.snapshot, unused provider-facingSnapshotStore/SnapshotEntrytypes and their identity generics. The core will own desired/effective membership, deterministic identity projection, bounded diff/removal planning and atomic promotion. Writers handle normalization and remote operations;finish()does not compute diffs or promote generations. The provider KVstore.snapshot()is unrelated and remains.Mirror execution is deliberately rejected in this foundation until the core planner, persistence and recovery tests exist. It is no longer deferred to phase 2 of the production rollout; verified native replacement remains a later optional strategy. No provider capability or workspace feature is enabled here. No billing invoice meter is introduced; accepted operations will activate a monthly sync once, not create row charges.
Full-query runs only checkpoint on completion. Explicit full refresh ignores saved extraction cursors after recovery, without erasing accepted receipts or provider state. Once provider finalization starts, failures/cancellation are left to recovery rather than invoking abort on potentially accepted delivery. Writer construction is inside the prepared init boundary. Unacknowledged init/batch calls also remain recovery-only; after durable acknowledgement, cleanup is limited to unaccepted staging and may never undo accepted delivery or erase recovery evidence. Cursorless input restarts at sequence zero after recovery admission, including subsequent normal runs; saved sequence remains receipt accounting, not an extraction offset. Earlier cleanup requires fenced authorization; already-authorized provider requests still need reconciliation if a lease expires in flight.
Validation
September 12 validation: both package typechecks, the full destination suite, changed-file ESLint and formatting pass. Small-tier incremental review of
3395956c2..6b29dc46bis clean. The installed pnpm version launcher was unavailable; checks used installed binaries, and the exact staged-format hook script was run directly before committing.September 12 lifecycle follow-up:
8df0c91b1fixes interrupted full-refresh cursor reuse and unsafe cleanup after finalization begins. Five added regression tests bring the suite to 126 tests (41 Reverse ETL). Risky-tier incremental review of6b29dc46b..8df0c91b1is clean after independent finders, adversarial verification and a fresh final review.September 12 review-thread follow-up:
8a8204a20moves init preparation before writer construction, suppresses cleanup for unacknowledged init/batch calls, and restarts cursorless scans after recovery admission. Ten additional regression cases bring the suite to 136 tests (51 Reverse ETL). Both package typechecks, changed-file lint/formatting and diff checks pass. Risky-tier incremental review of8df0c91b1..8a8204a20is clean after independent finders, adversarial verification and a fresh final review.Next: Node PostgreSQL operation/snapshot/membership persistence + migrations, core snapshot planner + recovery tests, then executable runner/syncctl/CronJobs and verified Google upsert/mirror UI rollout.
Latest cancellation fix:
539e7b30dobserves cancellation after durable finish acknowledgement for both accepted and pending results. Two regression cases (cancellation during finish or acknowledgement) failed before the fix and pass afterward. The full destination suite passes 138 tests, with 53 Reverse ETL tests; destination-functions typecheck, changed-file lint and formatting pass. Pending job recovery is preserved without abort or completion checkpoint.Risky-tier incremental review of
8a8204a20..539e7b30dis clean after independent finders, adversarial verification and a fresh final review.Packaging follow-up:
7af4ce7d9includessrc/reverse-etl/**in the npm package, documents server and browser-metadata deep imports for TypeScript-aware consumers, and adds an offline script-free package smoke test. The test failed before the allowlist change and passes afterward; all four runtime modules and README are present in the pack manifest. No exports map, dependency changes, or unrelated event-module packaging changes. Full suite: 139 tests / 19 files; destination-functions typecheck, changed-file lint and formatting pass.Small-tier incremental review of
539e7b30d..7af4ce7d9is clean.No deployment, merge, database migration or live advertising write performed.