feat(routing): sticky routing storage — per-customer success counts - #419
prajjwalkumar17 wants to merge 1 commit into
Conversation
dfa47ea to
67dcf67
Compare
prajjwalkumar17
changed the title
67dcf67 to
64bdc9e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The sticky field encoding/lookup currently doesn’t case-fold PM/PMT as designed, and the failure-decrement path can recreate/prune state in ways that violate the intended “failures don’t create state / cap is stable” invariants.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces the Phase 1 sticky-routing Redis storage layer, enabling per-(merchant, customer) “net habit” counts per PM:PMT:connector for later use by the decide/feedback paths. It also extends the Redis wrapper with hash helpers needed to keep sticky hashes TTL’d and manageable.
Changes:
- Added
src/sticky_routing.rswith write (success/failure), read, delete, pruning, and admission-budget logic for per-customer sticky hashes. - Added Redis wrapper commands for sticky storage (
hincrby_with_expire,hincrby,hgetall_map,hdel_field). - Exported the new sticky routing module from
src/lib.rs.
File summaries
| File | Description |
|---|---|
| src/sticky_routing.rs | New sticky-routing Redis data model + read/write/prune/admission logic and unit tests. |
| src/redis/commands.rs | Adds Redis hash helpers, including atomic HINCRBY+EXPIRE via MULTI. |
| src/lib.rs | Exposes the sticky_routing module. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let is_new_customer = !app_state.redis_conn.exists(&key).await?; | ||
| if is_new_customer && !admit_new_customer(merchant_id, ttl).await { |
There was a problem hiding this comment.
Deliberate trade-off. The admission budget is a coarse runaway-growth guard (default 1M new customers per 90-day window), not an exact quota — a small racy overshoot is harmless at that scale, and the counter deliberately errs on the side of admitting. The atomic INCR-then-compare alternative counts attempts rather than creations: under a failure storm or bot traffic that never succeeds, denied attempts would inflate the window counter and lock out legitimate new customers within budget — a worse failure mode than admitting a few extra ~150-byte keys. Same reasoning applies to the double-count race: two first successes racing inflate the counter by one, in the safe direction. Documented on the counter helper; can revisit with a Lua CAS if real-world numbers show meaningful overshoot.
| let new_count = app_state.redis_conn.hincrby(&key, &field, -1).await?; | ||
| // Concurrent decrements can race past the floor; repair best-effort. | ||
| if new_count < 0 { | ||
| let _ = app_state.redis_conn.hincrby(&key, &field, -new_count).await; | ||
| } |
There was a problem hiding this comment.
A decrement landing ≤0 now HDELs the field instead of repairing to zero, so only positive counts are ever stored, zeros no longer consume the combo cap, and the HGET→HINCRBY recreation race self-cleans through the same path (a raced recreation lands ≤0 and is deleted best-effort). Fixed in 575d249.
| format!( | ||
| "{}:{}", | ||
| sanitize(payment_method), | ||
| sanitize(payment_method_type) | ||
| ) |
There was a problem hiding this comment.
The case-folding existed but lived one PR up the stack (#420), which made this PR's key construction incomplete on its own. Moved canonical_dimension (trim + uppercase for pm/pmt; connector deliberately verbatim for the decide-time eligible-list comparison) plus its unit test down into this PR, so #419 is self-consistent. Fixed in 575d249.
64bdc9e to
575d249
Compare
| let key = sticky_key(merchant_id, customer_id); | ||
| let ttl = key_ttl_secs().await; | ||
|
|
||
| let is_new_customer = !app_state.redis_conn.exists(&key).await?; |
There was a problem hiding this comment.
Two problems: it's an extra Redis RTT on every successful payment forever (hot feedback path), and it's TOCTOU — two concurrent first-payments both read false, both get admitted, and the admission counter double-counts.
Both fix with one change: put TTL key as the first command inside the existing MULTI. It returns -2 when the key doesn't exist, so you get the new-key signal atomically, in the write you're already making, with zero extra round trips.
There was a problem hiding this comment.
Implemented as suggested: TTL is now the first command inside the existing MULTI (TTL → HINCRBY → EXPIRE), and hincrby_with_expire returns (prev_ttl, new_count). prev_ttl == -2 is the new-customer signal — atomic, so exactly one of two concurrent first-payments sees it (no admission double-count), and the standalone EXISTS round trip is gone from every success. The budget check moved after the write for the new-customer case only: when the budget is exhausted the just-created key is deleted (refusing the customer beats breaching the budget), so enforcement semantics are unchanged. Fixed in 72f13eb.
| /// state: no key (key-creation stays tied to successful payments, which is what bounds the | ||
| /// footprint) and no field. Deliberately no TTL re-arm — failing activity must not extend a | ||
| /// habit's lifetime. | ||
| pub async fn record_failure( |
There was a problem hiding this comment.
- a dead combo permanently occupies one of the 30 cap slots;
- the next success on it returns new_count == 1, which triggers prune_if_over_cap → a full HGETALL + prune scan. A customer whose payments oscillate pays that scan on every recovery.
HDEL the field when it hits zero, or gate the prune on hash length rather than new_count == 1.
There was a problem hiding this comment.
Zero-count fields no longer exist: a decrement landing ≤0 HDELs the field (landed for the earlier review pass in this same commit chain), so dead combos free their cap slot immediately and a recovery success is a genuine new-field write. The prune trigger additionally gates on HLEN before the HGETALL scan, so the full read+sort only runs when the hash is actually over the cap — an oscillating customer now pays one HLEN, not a scan, per recovery. Fixed in 72f13eb.
…r scores Storage layer only; feedback writes and the decide-side read come in the stacked follow-ups. - One Redis hash per (merchant, customer): fields pm:pmt:connector hold a NET count — successes increment, gateway failures decrement (floored at zero; merchant feedback is the source of truth in both directions). Failures never create a key or field, so key-creation stays tied to successful payments and the footprint stays bounded. Only positive counts qualify as pin candidates. Read is a single HGETALL returning counts sorted per exact combo; deliberately no cross-combo fallback (transaction-level fallbacks already cover a missing combo). - Eviction safety: every key volatile with a sliding TTL re-armed only on success; per-customer combo cap with lowest-count pruning that drains full excess; per-merchant two-window admission budget so new-key growth is bounded up front. Defaults overridable via service config. - Sticky on/off is a merchant-level feature flag (sticky_routing_enabled FeatureConf, dashboard plumbing in a follow-up) — deliberately NOT part of the euclid rule store, so toggling never touches the algorithm lifecycle. - New wrapper commands: hincrby_with_expire, hincrby, hgetall_map, hdel_field. Measured: 2-combo customer = 136B, 30-combo = 952B, listpack-encoded (Redis 7.2.7). Refs #393. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
575d249 to
72f13eb
Compare
Phase 1 of the sticky routing stack (#393). Merge order: this → #420 → #412 → #422 → #423 → #424. Each PR body names its own commit(s); later branches contain their predecessors until rebased post-merge.
Closes #413.
What this adds
The Redis store (
src/sticky_routing.rs): one hash per(merchant, customer)holding NET counts per combo (successes add, gateway failures subtract, floored at zero; failures never create state) —sticky_gw_{mid}_{cid}with fields{PM}:{PMT}:{connector} → countviaHINCRBY(atomic under concurrent feedback tasks). Only positive counts qualify as pin candidates. PM/PMT are case-folded and trimmed at the key boundary (payload- and snapshot-sourced writes must land on one field), connector verbatim. Failures delete a field at ≤0 — only positive counts are ever stored. Measured: 136 B for a 2-combo customer, 952 B at the cap, listpack-encoded.Eviction safety — sticky keys can never crowd out SR/elimination state:
STICKY_ROUTING_KEY_TTL, default 90 d) — all keys volatile, idle customers self-evict;STICKY_ROUTING_MAX_COMBOS_PER_CUSTOMER, default 30);STICKY_ROUTING_MAX_CUSTOMERS_{mid}, default 1M/window) — growth bounded up front.New wrapper commands:
hincrby_with_expire(MULTI HINCRBY+EXPIRE so a hash can never exist without a TTL),hgetall_map,hdel_field.Sticky's on/off is a merchant-level feature flag (
sticky_routing_enabled, plumbed in #422 and surfaced in the dashboard in #424) — deliberately independent of the euclid rule store, so toggling never touches the algorithm lifecycle.Unit tests cover field encode/decode, case-fold convergence, prune-victim selection, and nested-map assembly.
cargo build --no-default-features --features postgres, mysqlcargo check, and nightly rustfmt all green.🤖 Generated with Claude Code