refactor(storage): route timestamp/interval SQL through the dialect

[Kiran01bm]

Summary

Continues the state-store dialect abstraction by moving the timestamp and relative-time SQL in mysqlstore behind the Dialect provider, so a future Postgres store can supply its own syntax. No behavior change for MySQL/Vitess.

What

• Add CurrentTimestamp and RelativeTime to Dialect, with MySQLDialect rendering the exact expressions the store used before (NOW()/NOW(6), NOW() - INTERVAL n UNIT, DATE_ADD(NOW(6), INTERVAL ? MICROSECOND)).
• Route the stale-claim and retry-freshness cutoffs in applies.go and apply_operations.go, and all four NOW(6)/DATE_ADD sites in webhook_events.go (FindNext claim + predicate, Heartbeat, and the redelivery-reopen predicate), through the provider.
• Wire MySQLDialect into the apply, apply-operation, and webhook-event stores.
• Bare NOW() wall-clock stamps are intentionally left as-is.

Why

Relative-time arithmetic is one of the last dialect-specific SQL shapes hardcoded in the state store. Centralizing it on the provider keeps the store logic engine-neutral and lets the Postgres store implement the same intent (NOW() - INTERVAL, now() - make_interval(...), etc.) without editing every call site. Unit tests pin the generated SQL and placeholder counts so the MySQL output is provably unchanged.

Before / after

  before                                    after
  ╭────────────────────────────────────╮   ╭─────────────────────────────────────────╮
  │ applies.go / apply_operations.go / │   │ applies.go / apply_operations.go /      │
  │ webhook_events.go                  │   │ webhook_events.go                       │
  │   "… NOW(6) …"          (hardcoded)│   │   "… "+dialect.CurrentTimestamp(µs)+" …"│
  │   "… NOW() - INTERVAL 1 MINUTE …"  │   │   "… "+dialect.RelativeTime(…)+" …"     │
  │   "DATE_ADD(NOW(6), INTERVAL ? …)" │   │                                         │
  ╰────────────────────────────────────╯   ╰──────────────────┬──────────────────────╯
                                                              ▼
                                            ╭─────────────────────────────────────────╮
                                            │ Dialect provider                        │
                                            │   MySQLDialect → identical MySQL SQL    │
                                            │   (future) PostgresDialect → pg syntax  │
                                            ╰─────────────────────────────────────────╯

Known follow-ups (out of scope, deliberately deferred)

• Placeholder rebinding for Postgres. The entire mysqlstore emits MySQL-style ? placeholders (e.g. placeholders() in applies.go, and the parameterized RelativeTime). Postgres via database/sql needs numbered placeholders ($1, $2, …). The intended approach is a single ?→$n rebind at the store boundary (e.g. sqlx.Rebind) when the Postgres store lands — so the Dialect seam deliberately keeps emitting ? rather than growing an ordinal-placeholder API. Tracked as DB-13 in the DB-agnostic workstream; not part of this MySQL-only, no-behavior-change PR.

[Copilot]

Pull request overview

This PR continues the mysqlstore dialect abstraction by routing current-timestamp and relative-time SQL expressions through the Dialect provider, so time arithmetic can be implemented per-database family (with MySQL output intended to remain unchanged).

Changes:

• Extend Dialect with CurrentTimestamp and RelativeTime, and implement them in MySQLDialect to render the existing MySQL/Vitess expressions.
• Thread the dialect provider into applyStore, applyOperationStore, and webhookEventStore, and switch staleness / lease-expiry SQL to use the dialect-rendered expressions.
• Add unit tests that pin the generated MySQL expressions (including placeholder counts) for the new dialect methods.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/storage/mysqlstore/dialect.go	Adds CurrentTimestamp / RelativeTime and related enums/types; implements MySQL rendering.
pkg/storage/mysqlstore/dialect_test.go	Adds tests pinning MySQL timestamp/interval SQL output and placeholder counts.
pkg/storage/mysqlstore/storage.go	Wires MySQLDialect{} into stores that now require dialect-provided time SQL.
pkg/storage/mysqlstore/applies.go	Routes stale-claim and retry freshness cutoffs (and a couple other relative-time predicates) through the dialect.
pkg/storage/mysqlstore/apply_operations.go	Routes staleness / retry freshness cutoffs through the dialect and updates the staleness constant to a typed minute count.
pkg/storage/mysqlstore/webhook_events.go	Routes NOW(6)/DATE_ADD usage for webhook-event claiming/heartbeat/reopen predicates through the dialect.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review (posted on Armand's behalf by his AI agent).

Verdict: clean — approve. No correctness findings. CI is green (including the integration suites that hammer the claim queries, which is the real proof for a no-behavior-change claim), and Copilot's two findings were both handled sensibly (doc honesty reword done; the ?-placeholder concern correctly scoped to the store boundary and deferred to the tracked DB-13 rebind).

The one thing that could have broken, verified byte-for-byte

The dangerous part of this refactor is positional fmt.Sprintf: FindNextApplyOperation now interleaves 8 %s verbs (columns, three state IN lists, and four time expressions), and the diff hunks hide part of the query. I read the full query on the PR head and mapped every verb in appearance order — cols, active, stale, active, stale, retry, active, stale — against the argument list: exact match. Same for FindNextApply/ClaimApplyByID (cols, active, stale, retry, stale). And since the parameterized cutoff renders its ? at the same character position the literal ? occupied before, query-argument order is untouched.

All nine rendered sites are byte-identical to the originals: NOW() - INTERVAL 1 MINUTE, NOW() - INTERVAL ? DAY, NOW() - INTERVAL 1 HOUR, NOW(6) in the reopen and FindNext predicates, and DATE_ADD(NOW(6), INTERVAL ? MICROSECOND) in claim and Heartbeat — and the dialect tests pin each rendering plus the exactly-one-placeholder invariant. I also grepped for struct constructions bypassing New() that would leave a nil dialect: none exist.

Notes (not blocking)

• Merge-order coordination with add webhook inbox reconciler with stuck-row sweep #724 and feat(observability): durable webhook inbox depth and backlog metrics #726. Both open PRs touch webhook_events.go with raw NOW(6) — and feat(observability): durable webhook inbox depth and backlog metrics #726 rewrites FindNext's predicate into the shared webhookClaimablePredicate const, exactly the lines this PR routes through the dialect. Whichever lands second will conflict, and the risk in resolution is silently dropping the dialect routing (or leaving the new TerminateStuckProcessing / InboxStats SQL unrouted). Worth an explicit heads-up on the stack rather than trusting conflict resolution to preserve the seam.
• retry_after <= NOW() is left raw while its sibling predicate is routed. In the same FindNext WHERE clause, lease_expires_at <= NOW(6) now goes through CurrentTimestamp, but retry_after <= NOW() doesn't. The stated "bare NOW() wall-clock stamps are intentionally left as-is" rationale covers assignments (updated_at = NOW()), but this is a comparison predicate — the same category as the routed ones. Either route it or extend the stated rule to say comparisons at default precision stay raw (Postgres accepts NOW(), so it does work — it's a consistency-of-intent gap, not a bug).

Verified correct

• The applyOperationHeartbeatStaleness const migration (string "1 MINUTE" → uint64 minutes + IntervalMinute) renders identically at both former splice sites.
• The dialect enums use panic-on-unknown defaults — acceptable here since every call site passes compile-time constants; a bad value is a programmer error, not runtime input.
• ExpireRetryable still passes retryableRecoveryFreshnessDays as the bound arg for the parameterized day cutoff — arg list untouched at every parameterized site.
• The seam's doc comment now honestly describes it as incremental (store stays in mysqlstore, still emits ?, only family-varying syntax routes through) with the rebind forward-reference — matching what the code actually does.

This review was generated by Claude Code (claude-fable-5).

[Kiran01bm]

🤖 Adversarial correctness review (posted on Armand's behalf by his AI agent).

Verdict: clean — approve. No correctness findings. CI is green (including the integration suites that hammer the claim queries, which is the real proof for a no-behavior-change claim), and Copilot's two findings were both handled sensibly (doc honesty reword done; the ?-placeholder concern correctly scoped to the store boundary and deferred to the tracked DB-13 rebind).

Review response from Kiran's (@Kiran01bm) AI code review assessment agent

Summary: Clean approve — 0 correctness findings; two non-blocking notes, both already addressed in the #732 rebase, so nothing new is pending.

#	Finding	Status	Explanation
1	Merge-order coordination with #724/#726 — whoever lands second risks silently dropping the dialect routing or leaving new TerminateStuckProcessing/InboxStats SQL unrouted	fixed	The rebase onto current main preserved the routing and routed the new sites (TerminateStuckProcessing, InboxStats, ReclaimStaleSummaryClaim); oracle verified no routing was dropped.
2	retry_after <= NOW() left raw while its sibling lease_expires_at <= NOW(6) is routed (consistency-of-intent gap, not a bug)	fixed	In the rebased branch webhookClaimablePredicate now routes retry_after via CurrentTimestamp(TimestampPrecisionDefault); all relative-time/NOW(6) sites in mysqlstore now go through the dialect.

The "Verified correct" section raised no findings — all confirmations, no action.