fix(snowflake): deduplicate outcome-unknown truncates - #960
Conversation
356d497 to
ac04aae
Compare
There was a problem hiding this comment.
No bugs found, but this PR introduces a new correctness-critical protocol (deterministic UUIDv5 request IDs tied to channel lineage/row-progress) for reconciling outcome-unknown Snowflake truncates, plus a behavioral change to process each truncate boundary separately instead of collapsing consecutive truncates to the latest offset. This is exactly the kind of subtle distributed-recovery design decision that benefits from a human sign-off before merging.
Extended reasoning...
Overview
Modifies the Snowflake destination truncate path: derives a deterministic UUIDv5 requestId from a physical truncate attempt's identity (pipeline/table/db/schema/table name/truncate offset/channel creation timestamp/rows-inserted baseline), reuses it across retries of the same physical attempt, and issues a fresh ID once channel progress proves a new attempt is required. Also changes apply_truncate_events to process each consecutive Truncate event as its own boundary (via take_truncate_operations) instead of collapsing to only the latest offset per table, and threads an explicit request_id: Uuid through SqlClient::truncate_table/execute_ddl. Adds unit tests for the identity encoding (stable hash, per-component sensitivity, length-delimited framing) and extends the credentialed integration test to cover a second replay cycle.
Security risks
None identified. No user input reaches SQL string interpolation beyond existing patterns (table/column identifiers are already quoted elsewhere); the new requestId is a UUID placed in a query string, not attacker-controlled data. No auth/permission changes.
Level of scrutiny
This touches a production-critical, correctness-sensitive recovery path (Snowflake truncate replay after a lost SQL API outcome) and encodes a new "durable protocol" that future changes must not silently break (per the code's own comment: changing the namespace or identity encoding would break resumption for in-flight recoveries). That combination — subtle idempotency/fencing logic, an implicit forward-compatibility contract, and a change to how consecutive truncates are batched — is more than a mechanical fix and merits a human confirming the reasoning (e.g., that rows_inserted/channel_created_on_ms observed via open_at before the SQL call are the right fencing signal, and that per-boundary truncate processing doesn't regress throughput unacceptably for tables with many consecutive truncates).
Other factors
The PR has solid test coverage (deterministic-encoding unit tests, a credentialed cross-client idempotency test, and a two-cycle replay integration test), and the author's PR description shows the reasoning was worked through carefully, tying the design to Snowflake's documented resubmission protocol. The bug-hunting pass found nothing, and my own reading of the identity derivation and channel-progress invariants didn't surface a correctness gap. The main reason to defer is the design/critical-path judgment call, not a specific defect.
Context
The truncate replay recovery from #957 can resubmit a Snowflake
TRUNCATEafter losing its SQL API outcome. Without a stable request ID, an earlier request could finish after replay restored later rows and erase them.A concrete failing sequence is:
INSERT (1, 'before'),TRUNCATE, thenINSERT (2, 'after').requestId, ETL submits the same truncate as a separate request R2.(2, 'after')and advances the channel offset past that insert.The destination is now empty even though its channel offset records
(2, 'after')as applied, so normal recovery will not restore the missing row.This PR gives retries of the same fenced truncate attempt the same Snowflake
requestId, causing the retry to reconcile R1 instead of scheduling R2. A genuinely new truncate attempt still receives a new ID.Changes
requestIdfor the same unknown attempt, while deriving a new ID after channel progress. This follows Snowflake’s documented SQL API resubmission protocol: resubmit the same outcome-unknown truncate with its originalrequestIdandretry=true, while deriving a newrequestIdafter channel progress proves that a genuinely new truncate attempt is required.TRUNCATEseparately, resolving an earlier request with an unknown outcome before moving to a later one.Validation
Added identity and boundary unit tests, a credentialed cross-client idempotency test, and multi-cycle replay coverage proving post-truncate rows are restored exactly once.