Skip to content

feat(revenue-recovery): Add retry stats migration API - #13908

Open
AnuthaDev wants to merge 1 commit into
mainfrom
revrec-migrate
Open

feat(revenue-recovery): Add retry stats migration API#13908
AnuthaDev wants to merge 1 commit into
mainfrom
revrec-migrate

Conversation

@AnuthaDev

@AnuthaDev AnuthaDev commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD
    Description

Adds an admin-only migration API to backfill the revenue_recovery_retry_stats table from historical payment/billing data, so the revenue-recovery retry decider has informed statistics from day one instead of starting from an empty table.
Endpoint: POST /v2/recovery/data-backfill/retry-stats (multipart CSV, field file), authenticated with the existing V2AdminApiAuth, v2-gated.
Design:

  • The CSV is a converted form of the offline-generated backfill SQL (scripts/generate_retry_stats_sql.py): one row per cluster key as cluster_key,stats, where stats is the pre-aggregated document JSON ({dow/dom/hod} slot counters). Error-code standardisation, attempt pairing, slot bucketing, and aggregation all happen offline — the API performs no GSM lookups and no bucketing.
  • Batch atomicity: CSV parsing is strict — a single malformed row rejects the whole upload (400 via the standard API error envelope, nothing is written). All rows are then validated up front (cluster-key shape via RetryStatsClusterKey::from_db_string, document deserialisation, and StatsDocument::validate_invariants — k<=n per slot and Σn/Σk consistency across slot families) before any row is persisted, so a bad row can never leave a partially migrated batch.
  • Write semantics: whole-document REPLACE per cluster key (insert on first sight, overwrite thereafter — idempotent reruns), serialized through the same per-cluster-key Redis lock (.redis_locking_key()) as the live recording path, per the data-model "every writer takes the lock" invariant.
    Supporting refactors (behavior of the live recording path is unchanged):
  • RetryOutcomeEvent::record_strict / RetryStatsRecordOutcome — error/outcome-propagating variant of the live record() (which keeps its best-effort log-and-swallow behavior).
  • replace_retry_stats_document loader + with_retry_stats_lock helper — the per-key SETNX lock loop is no longer duplicated.
  • RetryStatsClusterKey::redis_locking_key() on the domain key type.
  • StatsDocument::validate_invariants() on the domain document type.
  • New Flow::RecoveryRetryStatsMigration for telemetry/lock mapping.
    Additional Changes
  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Adds a new endpoint (admin-only): crates/router/src/routes/app.rs, crates/router/src/routes/revenue_recovery_data_backfill.rs; request/response types in crates/api_models/src/revenue_recovery_data_backfill.rs. The revenue_recovery_retry_stats table already exists (v2_compatible_migrations/2026-08-13-000001) and is not modified; no config changes (reuses AdminApiKey auth and existing revenue_recovery.retry_stats_lock settings).
Motivation and Context

Retry-decision quality for revenue recovery depends on retry-outcome statistics aggregated per cluster key (standardised error code) and time slot (docs/revenue-recovery-data-model.md). A fresh deployment records these only from live webhook ingestion, so the decider has no evidence for weeks. Merchants/ops already hold historical billing exports (e.g., Chargebee/Stripe transaction CSVs); preprocessing them with scripts/generate_retry_stats_sql.py produces the same document shape the live recorder writes. This API provides the safe load path for those precomputed documents.
How did you test it?

Manually, on a local app with --features v2:

  • just clippy_v2 — clean.
  • Happy path: converted scripts/revenue_recovery_retry_stats_backfill.sql to CSV and uploaded — {"processed_records":21,"failed_records":0,"row_errors":[]}; select cluster_key, stats from revenue_recovery_retry_stats shows all 21 documents, byte-identical to the SQL payload.
  • Idempotency: re-upload of the same file returns identical tallies; n/k totals in the table unchanged (whole-document replace).
  • Atomic parsing: a file with one malformed row → 400 through the standard error envelope; table empty afterwards.
  • Atomic validation: a valid-CSV file with one bad row (invalid JSON / k>n / Σn mismatch) → 200 with that row in row_errors, processed_records: 0, nothing persisted.
  • Live record() path re-verified compilable and behavior-identical after the record_strict refactor.
    Checklist
  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible

@AnuthaDev
AnuthaDev requested review from a team as code owners August 28, 2026 12:15
@semanticdiff-com

semanticdiff-com Bot commented Aug 28, 2026

Copy link
Copy Markdown

@XyneSpaces

Copy link
Copy Markdown
Contributor

[should-fix] Please reject duplicate cluster_key values during the migration validation phase before any writes. The new migration writes each validated row as a whole-document replace, so a CSV containing the same cluster_key twice will report both rows as processed while the later document silently overwrites the earlier one.

[should-fix] Please make the validate_invariants totals use checked addition and return a validation error on overflow. A supplied stats document with very large u64 counters can overflow the .sum::<u64>() totals, which either panics in debug builds or wraps in release and makes the cross-family invariant check unreliable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants