Conversation
Changed Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
Adds the routing and assignment substrate for A/B testing revenue recovery retry
implementations, so that competing implementations can be run side by side on live recovery
traffic and compared.
Superposition configuration
Two new keys, declared in
crates/router/src/consts.rswithconfig!blocks incrates/router/src/core/configs/dimension_config.rs:revenue_recovery.ab_enabledboolfalserevenue_recovery.ab_algorithmString""GlobalPaymentIdab_enabledis a plain gate, resolved per merchant/connector, that decides whether A/B routingparticipates at all.
ab_algorithmis the only bucketed config in the revenue recovery set —its targeting key is the invoice, so an experiment on it splits traffic per invoice rather than
per merchant. Both carry a
{merchant_id}_{key}database fallback, consistent with theneighbouring revenue recovery configs.
The gate is a separate key rather than a field inside the bucketed value so that it cannot be
overridden by an experiment variant: variant overrides are themselves contexts keyed on
variantIds, and matching contexts are merged in server-delivered order, so a gate embedded inthe experiment-driven value could not be reliably forced off for a specific merchant.
TargetingKeyforGlobalPaymentIdcrates/common_utils/src/id_type/global_id/payment.rs—GlobalPaymentIdis generated byglobal_id_type!, which does not emit this impl (unlikeimpl_id_type_methods!). Without itthe bucketed config does not compile, and a blank targeting key makes the bucket lookup return
no applicable experiment, so every invoice would silently resolve to the default.
Persisted assignment
recovery_routingadded toPaymentRevenueRecoveryMetadatain bothapi_modelsanddiesel_models, with#[serde(default)]so existing rows deserialize unchanged. The fieldrides
payment_intent.feature_metadata, which is JSONB, so there is no migration. TheApiModelToDieselModelConvertorimplementations pass it through in both directions, andPaymentIntent::get_updated_feature_metadatacarries the existing value forward so thatrebuilding the metadata on a new failed attempt does not discard the assignment.
Routing in the CALCULATE workflow
crates/router/src/workflows/revenue_recovery.rsnow resolves both keys at the top of theSmartarm and branches on the gate. When A/B routing is enabled, a stored assignment isreplayed; when none is stored, the implementation is resolved from Superposition using the
invoice as the targeting key and recorded on the intent's feature metadata so subsequent
retries replay it. When the gate is off, the existing selection path runs unchanged.
payment_intentis threaded as&mutthroughperform_calculate_workflowandget_token_with_schedule_time_based_on_retry_algorithm_typeso the assignment can be recorded;it reaches the database via
reset_connector_transmission_and_active_attempt_id_before_pushing_to_execute_workflow, whichbuilds its update request from that object. One borrow adjustment was needed in
crates/router/src/core/revenue_recovery.rs:active_payment_attempt_idis read on both sidesof the now-mutable call, so the later read is re-derived rather than reusing the earlier
binding, which would otherwise hold a shared borrow across it.
Additional Changes
API contract:
recovery_routing: Option<String>is added toapi_models::payments::PaymentRevenueRecoveryMetadata. Additive and optional, so existingclients are unaffected.
Database schema: no migration. The field lives inside the existing
feature_metadataJSONBcolumn on
payment_intent.Configuration: no file under
config/,crates/router/src/configs/orloadtest/config/changed. The two new keys are Superposition configs declared in code
(
crates/router/src/consts.rs), and they must be created in the Superposition workspace beforethe feature can be switched on — with both keys absent, the resolves miss, the database
fallback misses, and the declared defaults (
falseand"") apply, which is the existingbehaviour.
Motivation and Context
The retry implementation used inside the
Smartarm is currently selected from a Superpositionflag that is re-read on every retry. That has two consequences:
two implementations cannot be compared against each other on live traffic.
part-way through its retry chain, which makes its outcome unattributable to either.
Superposition owns the bucketing, but bucketing alone is not sufficient for stickiness. The
bucket is
hash((targeting_key, group_id)) % 100, and for user-created groups the toss isrescaled by the experiment's traffic percentage — so changing the ramp re-buckets invoices
that are already in flight. Recovery spans the grace window (30 days by default), which makes
a ramp change during an invoice's life likely rather than exceptional. The assignment therefore
has to be resolved once, persisted on the invoice, and replayed.
Storing it on the intent rather than in
process_tracker.tracking_datais deliberate:process-tracker rows are keyed one per
(runner, task, invoice)and are finished and laterrecreated, so the assignment would reset mid-recovery; and
tracking_datacannot be joined topayment_intent.status, which the measurement query needs.How did you test it?
Compile and lint only — this PR is the routing substrate and is inert by default.
just check_v2— cleanjust clippy_v2— cleanNo functional testing yet.
ab_enableddefaults tofalse, and both keys are absent from theSuperposition workspace, so every existing code path resolves exactly as it did before this
change and behaviour is unchanged.
The A/B branch is not yet wired to dispatch to an implementation, so end-to-end verification
(traffic split, stickiness across retries, and confirming the assigned implementation actually
runs) is deferred to the follow-up that completes the dispatch. That verification needs a live
stack with Superposition plus a seeded experiment, and is not meaningful until then.
Checklist
cargo +nightly fmt --allcargo clippy