Skip to main content

max / makenotwork

payments: close refund crash-window, seal enforcement_mode enum, license-key idempotency Drive Payments to A (ultra-fuzz Run 7). Pay-S1: split claim from completion in pending_refunds (mig 154 adds completed_at). The refund work marks completed_at only on success; the stale sweep now filters completed_at IS NULL, so a claimed-but-crashed row surfaces for human reconciliation instead of being silently dropped or auto-retried (double-refund risk). Crash-window regression test added. enforcement_mode -> sealed SyncEnforcementMode enum (mirrors SyncBillingStatus): DbSyncAppBilling, the DB writers, blob-confirm, and the warning scan carry the enum; monthly_price_cents now matches exhaustively, so the unknown-mode -> floor fail-open (Pay-S2) is gone by construction. validate_knobs is the single parse point. Dead is_empty draft-sentinel removed (column is NOT NULL DEFAULT 'bulk'). Pay-M1: create_license_key branches on the violated constraint — a transaction_id collision returns the existing key (idempotent finalize re-run); only a key_code collision regenerates and retries. Pay-M2: try_create_activation reads max_activations from the FOR UPDATE-locked row rather than a caller-supplied arg, closing the TOCTOU. Pay-M3: cart subtotal widened to i64 so a large cart can't overflow. create_tip guards amount_cents > 0; Cents::new documented as intentionally unchecked.
Co-Authored-By
Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-24 21:47 UTC
Signed with PGP, not checked
Commit: d69642cee9bd2b2268e86d71d8c45e214c6c83bf
Parent: e2b6070
21 files changed, +298 insertions, -131 deletions
@@ -46,24 +46,27 @@
46 46 /// Floors at `BASE_FLOOR_CENTS` so we never invoice below the Stripe-fee
47 47 /// break-even amount.
48 48 pub fn monthly_price_cents(
49 - enforcement_mode: &str,
49 + enforcement_mode: crate::db::SyncEnforcementMode,
50 50 storage_gb_cap: Option<u32>,
51 51 key_cap: Option<u32>,
52 52 gb_per_key: Option<u32>,
53 53 ) -> i64 {
54 + use crate::db::SyncEnforcementMode::{Bulk, PerKey};
54 55 // Pure integer-cents arithmetic. The rate is a whole number of cents and
55 56 // the caps are whole GB, so there is no fractional money to round; the old
56 57 // `(gb as f64 * 3.0).ceil()` was an unnecessary trip through f64. Saturating
57 58 // multiplies keep absurd admin-set caps from overflowing i64 instead of
58 - // wrapping to a negative invoice.
59 + // wrapping to a negative invoice. The match is exhaustive over the sealed
60 + // enum — there is no unknown-mode arm that could silently price at the floor
61 + // (Pay-S2); an invalid mode can't reach here because the column is
62 + // CHECK-constrained and the type is parsed at the API boundary.
59 63 let gb: i64 = match enforcement_mode {
60 - "bulk" => i64::from(storage_gb_cap.unwrap_or(0)),
61 - "per_key" => {
64 + Bulk => i64::from(storage_gb_cap.unwrap_or(0)),
65 + PerKey => {
62 66 let k = i64::from(key_cap.unwrap_or(0));
63 67 let g = i64::from(gb_per_key.unwrap_or(0));
64 68 k.saturating_mul(g)
65 69 }
66 - _ => 0,
67 70 };
68 71 let raw = gb.saturating_mul(STORAGE_RATE_CENTS_PER_GB);
69 72 raw.max(BASE_FLOOR_CENTS)
@@ -77,55 +80,54 @@
77 80 #[cfg(test)]
78 81 mod tests {
79 82 use super::*;
83 + use crate::db::SyncEnforcementMode::{Bulk, PerKey};
80 84
81 85 #[test]
82 86 fn bulk_mode_pricing() {
83 87 // 100 GB bulk → 100 × 3 = 300 cents.
84 - assert_eq!(monthly_price_cents("bulk", Some(100), None, None), 300);
88 + assert_eq!(monthly_price_cents(Bulk, Some(100), None, None), 300);
85 89 // 1000 GB → $30.
86 - assert_eq!(monthly_price_cents("bulk", Some(1000), None, None), 3000);
90 + assert_eq!(monthly_price_cents(Bulk, Some(1000), None, None), 3000);
87 91 }
88 92
89 93 #[test]
90 94 fn per_key_mode_pricing() {
91 95 // 50 keys × 2 GB = 100 GB equivalent → 300 cents. Matches 100 GB bulk.
92 - assert_eq!(monthly_price_cents("per_key", None, Some(50), Some(2)), 300);
96 + assert_eq!(monthly_price_cents(PerKey, None, Some(50), Some(2)), 300);
93 97 // 1000 keys × 1 GB → $30.
94 - assert_eq!(monthly_price_cents("per_key", None, Some(1000), Some(1)), 3000);
98 + assert_eq!(monthly_price_cents(PerKey, None, Some(1000), Some(1)), 3000);
95 99 }
96 100
97 101 #[test]
98 102 fn floor_kicks_in_for_small_accounts() {
99 103 // 1 GB bulk → 3¢ raw, floored to 31¢.
100 - assert_eq!(monthly_price_cents("bulk", Some(1), None, None), 31);
104 + assert_eq!(monthly_price_cents(Bulk, Some(1), None, None), 31);
101 105 // 10 GB → 30¢, also floored to 31¢ (one cent short).
102 - assert_eq!(monthly_price_cents("bulk", Some(10), None, None), 31);
106 + assert_eq!(monthly_price_cents(Bulk, Some(10), None, None), 31);
103 107 // 11 GB → 33¢, above floor.
104 - assert_eq!(monthly_price_cents("bulk", Some(11), None, None), 33);
108 + assert_eq!(monthly_price_cents(Bulk, Some(11), None, None), 33);
105 109 // 1 key × 1 GB → 3¢ raw, floored.
106 - assert_eq!(monthly_price_cents("per_key", None, Some(1), Some(1)), 31);
110 + assert_eq!(monthly_price_cents(PerKey, None, Some(1), Some(1)), 31);
107 111 }
108 112
109 113 #[test]
110 114 fn heavy_workload_pricing() {
111 115 // 10 TB bulk → 10240 × 3 = 30720¢ = $307.20.
112 - assert_eq!(monthly_price_cents("bulk", Some(10_240), None, None), 30_720);
116 + assert_eq!(monthly_price_cents(Bulk, Some(10_240), None, None), 30_720);
113 117 // 10k keys × 1 GB → same.
114 - assert_eq!(monthly_price_cents("per_key", None, Some(10_000), Some(1)), 30_000);
118 + assert_eq!(monthly_price_cents(PerKey, None, Some(10_000), Some(1)), 30_000);
115 119 }
116 120
117 121 #[test]
118 122 fn missing_knobs_drop_to_floor() {
119 123 // Mode is set but no knobs provided — should hit the floor.
120 - assert_eq!(monthly_price_cents("bulk", None, None, None), BASE_FLOOR_CENTS);
121 - assert_eq!(monthly_price_cents("per_key", None, None, None), BASE_FLOOR_CENTS);
124 + assert_eq!(monthly_price_cents(Bulk, None, None, None), BASE_FLOOR_CENTS);
125 + assert_eq!(monthly_price_cents(PerKey, None, None, None), BASE_FLOOR_CENTS);
122 126 }
123 127
124 - #[test]
125 - fn unknown_mode_drops_to_floor() {
126 - // Defensive: an unrecognized mode shouldn't blow up; it lands at the floor.
127 - assert_eq!(monthly_price_cents("unknown", Some(100), None, None), BASE_FLOOR_CENTS);
128 - }
128 + // (The former `unknown_mode_drops_to_floor` test is gone: `enforcement_mode`
129 + // is now a sealed enum, so an unrecognized mode is unrepresentable — the
130 + // Pay-S2 fail-open it guarded against can no longer be written.)
129 131
130 132 #[test]
131 133 fn floor_amount_covers_stripe_fee() {
@@ -146,7 +148,7 @@
146 148 #[test]
147 149 fn pricing_at_u32_max_does_not_panic() {
148 150 // u32::MAX GB × 3¢ ≈ 1.3e10 cents, fits in i64. The cast must not panic.
149 - let p = monthly_price_cents("bulk", Some(u32::MAX), None, None);
151 + let p = monthly_price_cents(Bulk, Some(u32::MAX), None, None);
150 152 assert!(p > 0, "huge price should be positive, got {p}");
151 153 }
152 154
@@ -154,7 +156,7 @@
154 156 fn per_key_pricing_at_u32_max_saturates_cleanly() {
155 157 // u32::MAX × u32::MAX overflows f64 precision but Rust's f64-as-i64 cast
156 158 // saturates at i64::MAX rather than UB. Must not panic.
157 - let p = monthly_price_cents("per_key", None, Some(u32::MAX), Some(u32::MAX));
159 + let p = monthly_price_cents(PerKey, None, Some(u32::MAX), Some(u32::MAX));
158 160 assert!(p > 0, "saturated price should still be positive, got {p}");
159 161 }
160 162
@@ -170,13 +172,13 @@
170 172 fn bulk_with_zero_gb_drops_to_floor() {
171 173 // Defensive: validate_knobs rejects gb=0 at the route layer, but the
172 174 // pure function should still produce the floor rather than 0.
173 - assert_eq!(monthly_price_cents("bulk", Some(0), None, None), BASE_FLOOR_CENTS);
175 + assert_eq!(monthly_price_cents(Bulk, Some(0), None, None), BASE_FLOOR_CENTS);
174 176 }
175 177
176 178 #[test]
177 179 fn per_key_one_dimension_zero_drops_to_floor() {
178 180 // If only one of key_cap/gb_per_key is 0, the product is 0 → floor.
179 - assert_eq!(monthly_price_cents("per_key", None, Some(0), Some(10)), BASE_FLOOR_CENTS);
180 - assert_eq!(monthly_price_cents("per_key", None, Some(10), Some(0)), BASE_FLOOR_CENTS);
181 + assert_eq!(monthly_price_cents(PerKey, None, Some(0), Some(10)), BASE_FLOOR_CENTS);
182 + assert_eq!(monthly_price_cents(PerKey, None, Some(10), Some(0)), BASE_FLOOR_CENTS);
181 183 }
182 184 }
@@ -212,9 +212,7 @@
212 212 /// Lifecycle of a SyncKit developer app's billing record (the `sync_apps.billing_status`
213 213 /// TEXT column, CHECK-constrained in migration 117). Replaces the raw string the
214 214 /// `DbSyncAppBilling` model used to carry, so a status comparison can't drift from the
215 - /// CHECK set. (The sibling `enforcement_mode` column stays a string: the pricing layer
216 - /// and the DB use two vocabularies — `bulk` vs `app_wide` — whose reconciliation is a
217 - /// separate, deliberately deferred refactor.)
215 + /// CHECK set.
218 216 #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
219 217 pub enum SyncBillingStatus {
220 218 #[serde(rename = "draft")]
@@ -234,6 +232,25 @@
234 232 Canceled => "canceled",
235 233 });
236 234
235 + /// How a SyncKit developer app's storage billing is enforced (the
236 + /// `sync_apps.enforcement_mode` TEXT column, CHECK-constrained to `('per_key','bulk')`
237 + /// in migration 118). Replaces the raw string the `DbSyncAppBilling` model used to
238 + /// carry. Lifting this to an enum makes `monthly_price_cents` match exhaustively, so an
239 + /// unrecognized mode is no longer silently priced at the floor (Pay-S2). The historical
240 + /// `app_wide` value was renamed to `bulk` in migration 118; only these two are live.
241 + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
242 + pub enum SyncEnforcementMode {
243 + #[serde(rename = "per_key")]
244 + PerKey,
245 + #[serde(rename = "bulk")]
246 + Bulk,
247 + }
248 +
249 + impl_str_enum!(SyncEnforcementMode {
250 + PerKey => "per_key",
251 + Bulk => "bulk",
252 + });
253 +
237 254 // ── Git repository visibility ──
238 255
239 256 #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
@@ -45,8 +45,27 @@
45 45 match first {
46 46 Ok(key) => Ok(key),
47 47 Err(sqlx::Error::Database(e)) if e.code().as_deref() == Some("23505") => {
48 + // Distinguish which unique index fired. A `transaction_id` collision
49 + // (mig 151's partial unique index) means this purchase already minted
50 + // its one key — a duplicate finalize from crash-recovery redelivery.
51 + // Return the existing key as idempotent success; retrying with a fresh
52 + // code would only collide on the same index again (Pay-M1). A `key_code`
53 + // collision is a random clash — regenerate and retry once.
54 + let constraint = e.constraint().map(str::to_string);
55 + if let (Some("license_keys_transaction_id_key"), Some(tx_id)) =
56 + (constraint.as_deref(), transaction_id)
57 + {
58 + return get_license_key_by_transaction_id(pool, tx_id)
59 + .await?
60 + .ok_or_else(|| {
61 + crate::error::AppError::Internal(anyhow::anyhow!(
62 + "license_keys transaction_id unique violation but no existing \
63 + row found for {tx_id:?}"
64 + ))
65 + });
66 + }
48 67 let retry_code = crate::helpers::generate_key_code();
49 - tracing::warn!(item_id = %item_id, "license key 23505 collision; retrying once");
68 + tracing::warn!(item_id = %item_id, "license key key_code 23505 collision; retrying once");
50 69 let key = sqlx::query_as!(
51 70 DbLicenseKey,
52 71 r#"
@@ -281,24 +300,25 @@
281 300 license_key_id: LicenseKeyId,
282 301 machine_id: &str,
283 302 label: Option<&str>,
284 - max_activations: Option<i32>,
285 303 ) -> Result<Option<DbLicenseActivation>> {
286 304 let mut tx = pool.begin().await?;
287 305
288 - // Lock the license key row to serialize concurrent activations, and re-check
289 - // revocation INSIDE the lock (MINOR TOCTOU, Run #23): a charge.refunded that
290 - // revokes the key between the caller's pre-check and here must block the
291 - // activation. No eligible (non-revoked) row => no activation.
292 - let locked = sqlx::query_scalar!(
293 - r#"SELECT 1 AS "one!" FROM license_keys WHERE id = $1 AND revoked_at IS NULL FOR UPDATE"#,
306 + // Lock the license key row to serialize concurrent activations, re-check
307 + // revocation, AND read `max_activations` from the locked row — not from a
308 + // caller-supplied argument (Pay-M2). The caller's value was read before the
309 + // lock; if an admin lowered the cap in between, enforcing the stale arg would
310 + // let an extra machine activate. Reading the column here makes the limit the
311 + // authoritative one. No eligible (non-revoked) row => no activation.
312 + let locked: Option<Option<i32>> = sqlx::query_scalar!(
313 + r#"SELECT max_activations FROM license_keys WHERE id = $1 AND revoked_at IS NULL FOR UPDATE"#,
294 314 license_key_id as LicenseKeyId,
295 315 )
296 316 .fetch_optional(&mut *tx)
297 317 .await?;
298 - if locked.is_none() {
318 + let Some(max_activations) = locked else {
299 319 tx.rollback().await?;
300 320 return Ok(None);
301 - }
321 + };
302 322
303 323 // Check if this machine already has an activation (re-activation is always OK)
304 324 let existing: Option<DbLicenseActivation> = sqlx::query_as!(
@@ -46,8 +46,11 @@
46 46
47 47 /// Claim a pending refund matching a payment intent ID.
48 48 ///
49 - /// Atomically marks it as matched (so it is only processed once).
50 - /// Returns `None` if no unmatched pending refund exists.
49 + /// Atomically marks it as matched (so it is only claimed once) but NOT completed —
50 + /// completion is recorded separately by [`mark_refund_completed`] only after the
51 + /// fallible refund work succeeds. A claim that is never completed (process killed
52 + /// mid-refund) leaves `completed_at IS NULL`, so the stale-refund sweep surfaces it
53 + /// for human escalation (PAY-S1). Returns `None` if no unmatched pending refund exists.
51 54 pub async fn claim_pending_refund(
52 55 pool: &PgPool,
53 56 payment_intent_id: &str,
@@ -73,17 +76,26 @@
73 76 Ok(row)
74 77 }
75 78
76 - /// Release a claimed pending refund back to the queue (`matched_at` → NULL) after
77 - /// its processing failed.
79 + /// Record that a claimed pending refund's processing finished successfully.
78 80 ///
79 - /// `claim_pending_refund` marks the row matched BEFORE the fallible refund work
80 - /// runs. If that work errors, the claim must be released or the row is lost
81 - /// forever: `get_stale_refunds` only sees `matched_at IS NULL` rows, and the
82 - /// original `charge.refunded` event was already marked processed, so Stripe will
83 - /// not redeliver it (PAY-S1). Releasing the claim re-enters the row into the
84 - /// stale-refund sweep (human escalation) and lets a later delivery re-claim and
85 - /// retry — safe because the refund handler is atomic, so a failed attempt
86 - /// committed nothing. Idempotent.
81 + /// Sets `completed_at`; only after this is the row considered fully handled. A
82 + /// claimed row without a `completed_at` (the process died between claim and this
83 + /// call) is surfaced by [`get_stale_refunds`] for manual reconciliation instead of
84 + /// being auto-retried — re-issuing a refund that may already have reached Stripe
85 + /// could double-refund (PAY-S1).
86 + pub async fn mark_refund_completed(pool: &PgPool, id: uuid::Uuid) -> Result<()> {
87 + sqlx::query!("UPDATE pending_refunds SET completed_at = NOW() WHERE id = $1", id)
88 + .execute(pool)
89 + .await?;
90 + Ok(())
91 + }
92 +
93 + /// Release a claimed pending refund back to the queue (`matched_at` → NULL) after a
94 + /// *graceful* processing failure (a transient error where the handler committed
95 + /// nothing — it is atomic). Releasing re-opens the row so a later webhook delivery
96 + /// can re-claim and retry. A non-graceful failure (process killed) cannot reach
97 + /// here; that row stays matched-but-incomplete and is escalated by the sweep
98 + /// instead (PAY-S1). Idempotent.
87 99 pub async fn unclaim_pending_refund(pool: &PgPool, id: uuid::Uuid) -> Result<()> {
88 100 sqlx::query!("UPDATE pending_refunds SET matched_at = NULL WHERE id = $1", id)
89 101 .execute(pool)
@@ -107,8 +119,12 @@
107 119 /// letting one unbounded query stall the tick (PERF-S2, Run #23).
108 120 pub const STALE_REFUND_BATCH: i64 = 100;
109 121
110 - /// Get up to [`STALE_REFUND_BATCH`] pending refunds older than `age` that have
111 - /// not been matched or escalated, oldest first.
122 + /// Get up to [`STALE_REFUND_BATCH`] pending refunds older than `age` that still
123 + /// need attention and have not been escalated, oldest first. "Need attention" means
124 + /// the refund work never completed: either the row was never matched to a payment
125 + /// (`matched_at IS NULL`), or it was claimed but the process died before recording
126 + /// completion (`matched_at IS NOT NULL AND completed_at IS NULL`) — the crash-window
127 + /// case (PAY-S1). Both surface here for human reconciliation.
112 128 pub async fn get_stale_refunds(
113 129 pool: &PgPool,
114 130 age: chrono::Duration,
@@ -121,7 +137,7 @@
121 137 r#"
122 138 SELECT id, payment_intent_id, amount, amount_refunded, created_at
123 139 FROM pending_refunds
124 - WHERE matched_at IS NULL
140 + WHERE completed_at IS NULL
125 141 AND escalated_at IS NULL
126 142 AND created_at < $1
127 143 ORDER BY created_at
@@ -15,7 +15,7 @@
15 15 //! ('draft','active','suspended_unpaid','canceled')
16 16 //! storage_gb_cap INT
17 17 //! egress_multiple NUMERIC(6,2)
18 - //! enforcement_mode TEXT NOT NULL CHECK IN ('per_key','app_wide')
18 + //! enforcement_mode TEXT NOT NULL CHECK IN ('per_key','bulk')
19 19 //! key_cap INT
20 20 //! current_period_start TIMESTAMPTZ
21 21 //! current_period_end TIMESTAMPTZ
@@ -82,7 +82,7 @@
82 82 pub async fn activate_billing(
83 83 pool: &PgPool,
84 84 app_id: SyncAppId,
85 - enforcement_mode: &str,
85 + enforcement_mode: super::SyncEnforcementMode,
86 86 storage_gb_cap: Option<i32>,
87 87 key_cap: Option<i32>,
88 88 gb_per_key: Option<i32>,
@@ -122,7 +122,7 @@
122 122 pub async fn update_knobs(
123 123 pool: &PgPool,
124 124 app_id: SyncAppId,
125 - enforcement_mode: &str,
125 + enforcement_mode: super::SyncEnforcementMode,
126 126 storage_gb_cap: Option<i32>,
127 127 key_cap: Option<i32>,
128 128 gb_per_key: Option<i32>,
@@ -634,7 +634,7 @@
634 634 creator_id: UserId,
635 635 creator_email: String,
636 636 app_name: String,
637 - enforcement_mode: String,
637 + enforcement_mode: super::SyncEnforcementMode,
638 638 storage_gb_cap: Option<i32>,
639 639 key_cap: Option<i32>,
640 640 gb_per_key: Option<i32>,
@@ -671,8 +671,8 @@
671 671 let mut out = Vec::new();
672 672 let mut per_key_apps: Vec<(SyncAppId, UserId, String, String, i32)> = Vec::new();
673 673 for r in rows {
674 - match r.enforcement_mode.as_str() {
675 - "bulk" => {
674 + match r.enforcement_mode {
675 + super::SyncEnforcementMode::Bulk => {
676 676 let Some(gb) = r.storage_gb_cap else { continue };
677 677 let limit = crate::synckit_billing::storage_cap_bytes(gb as u32);
678 678 if let Some(pct) =
@@ -691,13 +691,12 @@
691 691 });
692 692 }
693 693 }
694 - "per_key" => {
694 + super::SyncEnforcementMode::PerKey => {
695 695 // Defer to a second query that fans out per (app, key); the
696 696 // per-key counter — not the app aggregate — is what we warn on.
697 697 let (Some(_), Some(g)) = (r.key_cap, r.gb_per_key) else { continue };
698 698 per_key_apps.push((r.app_id, r.creator_id, r.creator_email, r.app_name, g));
699 699 }
700 - _ => {}
701 700 }
702 701 }
703 702
@@ -18,6 +18,12 @@
18 18 message: Option<&str>,
19 19 stripe_checkout_session_id: &str,
20 20 ) -> Result<DbTip> {
21 + // Defense in depth: the route handler enforces a $1 minimum and the column
22 + // carries CHECK (amount_cents > 0), but guard here too so any future caller
23 + // gets a clean validation error instead of a raw constraint violation.
24 + if amount_cents <= 0 {
25 + return Err(crate::error::AppError::validation("Tip amount must be positive"));
26 + }
21 27 let tip = sqlx::query_as!(
22 28 DbTip,
23 29 r#"
@@ -244,6 +244,11 @@
244 244 impl Cents {
245 245 pub const ZERO: Self = Self(0);
246 246
247 + /// Wrap a raw cents value. Intentionally unchecked: `Cents` is a general money
248 + /// quantity that legitimately holds zero (free items, empty balances) and
249 + /// negative values (refund deltas, split adjustments). Positivity is a
250 + /// *domain* constraint, so it's enforced at the domain boundary that needs it
251 + /// (e.g. `PriceCents::new`, `create_tip`) rather than blanket-rejected here.
247 252 pub fn new(cents: i64) -> Self {
248 253 Self(cents)
249 254 }