RPL-6780: fan out non-natural-key deletes to real natural keys (leo-connector-common)

[johnnyC9000]

Stacked on #249 — please merge that first. This depends on combine-records.js, which only exists on that branch.

Scope: leo-connector-common only. This PR adds the delete fan-out that rstreams-connector-datalake needs. It changes no connector implementation, so Redshift behavior is unchanged.

Problem

A delete keyed by something other than the row's natural key — a parent FK like shipment_id, or a source mongo_id — is synthesized as a _del_<value> marker. combine() groups by natural key, so that marker lands in a group of its own and is never compared against the same-batch writes for the rows it affects.

The outcome is then decided by the connector's fixed flush-before-merge order rather than by event order. Net effect: if any write for the row exists in the batch, the delete is reverted, regardless of which event was actually later on the bus.

Confirmed live in production on f_shipment_item, f_order_item_activity and f_warehouse; structurally present on 18 tables.

Approach

The ordering logic was never missing. combineRecords already implements last-event-wins correctly — the markers just never reached it. So resolve the delete to the natural keys it currently matches and emit one marker per row. Those flow through the existing logic unchanged.

• common/datawarehouse/delete-fanout.js (new) — the fan-out, extracted for testability, same reason combine-records.js was split out of combine.js.
• common/datawarehouse/load.js — checkforDelete delegates to it.

Resolution is an optional client capability

load.js is shared by every connector — postgres, mysql, oracle, sqlserver, mongo, elasticsearch. Raw SQL there would break or misbehave for all of them, so the fan-out asks the client for an optional resolveDeleteKeys(table, field, nk, ids, cb).

A connector that does not implement it — plus any table with a composite natural key, or one absent from tableConfig — falls back to today's _del_ marker. That is every connector in this repo, including postgres, so nothing here changes behavior for existing consumers.

The Databricks connector implements the capability in its own repo (rstreams-connector-datalake fix/rpl-6780-resolve-delete-keys), so only that path changes.

Also fixes a latent case

When a table's natural key is not named id (f_shipping_label_package, NK package_id), the old marker left the natural key unset. Every such delete hashed to the same empty combine key and all but the last were silently dropped — data loss, not just misordering. No producer targets that table today, so this was latent.

Tests

16 new, all passing. Includes RPL-6780's five ordering scenarios replayed through a resolved marker; rows 1 and 4 — the two the ticket calls provably wrong today — now come out correct. Also covers every fallback path, resolver errors, and the non-id natural key case.

Suite is 23 passing / 1 failing. That failure (test/checksum/index.test.js) is pre-existing on this branch's base — not introduced here. Lint error counts are identical to base on both files touched.

Not in this PR

• Any connector implementation. The Databricks resolveDeleteKeys lives in its own repo and PR. Until it lands and the datalake pin moves off 4.1.0-rc.32056762820, nothing in production changes.
• Residual gap. A row created in the same batch as the delete does not exist when the target is queried, so it is not resolved. Not a regression — the previous FK-keyed UPDATE also ran before the merge and matched nothing. Closing it needs the reconciliation sweep described on RPL-6780.
• End-to-end verification. RPL-6780 asks for positive exercise across all 18 tables in non-prod. These are unit tests; they prove the fan-out logic, not end-to-end behavior. That gate is still open.

Note on the commit list

The branch history includes DPT-2586 numeric-validation commits and a postgres resolveDeleteKeys implementation, both since reverted in full — DPT-2586 is being issued separately, and the postgres work was dropped to keep this branch focused on the fan-out. common/utils/validation.js and postgres/lib/dwconnect.js are byte-identical to the base branch. The net diff is three files. History was preserved rather than rewritten because the branch was already pushed.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes the shared DW load delete path used by every connector; behavior stays on the old path until a client implements resolveDeleteKeys, but async fan-out and new failure modes affect all delete events once connectors opt in.

Overview
Fixes RPL-6780, where deletes keyed by a parent FK or other non–natural-key column were emitted as _del_<value> markers that never joined the same combine() group as that row’s writes, so batch flush order could silently undo a delete even when the delete was later on the bus.

load.js now routes type: 'delete' events through a new delete-fanout.js module instead of inlining marker synthesis. The fan-out emits one delete marker per affected row. When the delete field is already the table’s single-column natural key, behavior is unchanged. Otherwise it optionally calls client.resolveDeleteKeys(table, field, nk, ids, cb) to look up current natural keys and build markers keyed by the real NK (so combineRecords last-event-wins applies). Connectors without that hook, composite NKs, or unknown tables still get the historical _del_ marker. Resolver errors propagate instead of dropping the delete. The same change also fixes a latent bug for NKs not named id (e.g. package_id), where markers previously collapsed to one combine group.

Adds unit tests for resolution, fallbacks, errors, and the five ordering scenarios from the ticket. The release workflow also pushes from the feature branch for RC publishes.

^{Reviewed by Cursor Bugbot for commit 3df52d3. Bugbot is set up for automated code reviews on this repo. Configure here.}

[ch-snyk-sa]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9fc336f. Configure here.}

[pmogren]

Review summary

The delete fan-out design is correct: keying resolved deletes by the row's real natural key so combine() groups them with that row's writes is the right fix for RPL-6780's collision. Traced the mechanism end to end (delete-fanout.js → combine.js → combine-records.js) to confirm exactly one winning record per natural key survives per batch — that holds regardless of which connector eventually implements resolveDeleteKeys.

Two things worth addressing before merge, plus two minor cleanups.

1. Resolver errors now abort the whole batch (load.js, checkforDelete)

deleteFanout(obj, tableNks, client, (err, records) => {
    if (err) {
        return done(err);
    }
    ...

done(err) propagates through ls.pipe(validateData, checkforDelete, combine(tableNks), ls.write(...)) and fails the whole batch's callback. Every other malformed record in this same file — see validateData's handleFailedValidation — is quarantined to a separate _error queue instead, and the stream continues. A transient resolveDeleteKeys failure (timeout, connectivity blip) now takes down every other valid write/delete in the batch with it. Before this PR, checkforDelete was synchronous and had no failure mode at all.

Worth at least a decision: is fail-loud intentional here, or should resolver errors get the same quarantine treatment as other invalid records?

2. Composite natural-key tables are permanently excluded, not just unimplemented

const nk = nks.length === 1 ? nks[0] : null;

For any table with a multi-column NK, nk is always null, so it always takes the unresolved() fallback — unresolved() only ever sets data.id, never the real NK columns, so combine.js's tableIds[table].map(f => values[f]) hashes every such delete on that table to the same key regardless of connector or resolver support. This is the exact RPL-6780 collision, and it's structural: there's no path to close it later the way there is for the "no connector implements the resolver yet" gap. It's documented in an inline comment but not called out in the PR description's own "Not in this PR" list — worth adding there so it isn't lost once the resolver-adoption gap closes.

3. Minor: unneeded serialization (delete-fanout.js)

async.eachSeries(entities, (entity, entityDone) => { ... client.resolveDeleteKeys(...) ... })

Entities resolve against independent tables with no shared state (confirmed by the test asserting each entity hits its own table). Once a connector implements the resolver, an event naming N entities pays N sequential round-trips instead of running concurrently, and checkforDelete's done() — and the whole stream's forward progress — waits on it. async.eachLimit(entities, limit, ...) removes the serialization while still bounding concurrent DB load (plain async.each is riskier since no connector's client has been confirmed safe under unbounded concurrent calls). Would need the one order-asserting test updated to be order-independent.

4. Minor: dead self = this alias (load.js)

let self = this;
deleteFanout(obj, tableNks, client, (err, records) => {
    ...
    records.forEach(record => self.push(record));

The callback is an arrow function, so it already inherits this lexically from the enclosing function(obj, done) — same this self points to. Harmless, but this repo's self = this convention (e.g. dol.js) exists specifically for non-arrow callbacks that get their own rebound this; copying it here where it isn't needed makes it look load-bearing.

---

Also traced (and ruled out as a non-issue): postgres/mysql's deleteHandler in dwconnect.js diverts any record carrying __leo_delete__ away from the staging-table merge before it's ever seen. That looked at first like it could bypass this PR's combine-group fix entirely, but combine.js/combine-records.js already resolve to exactly one winning record per natural key before that diversion runs — so it doesn't re-decide anything, and the fix's premise holds for those connectors too. It's just dormant for them today because neither implements resolveDeleteKeys, which the PR description already discloses.