fix: 3-day creation grace before charging an app's base fee#46
Closed
I-am-nothing wants to merge 2 commits into
Closed
fix: 3-day creation grace before charging an app's base fee#46I-am-nothing wants to merge 2 commits into
I-am-nothing wants to merge 2 commits into
Conversation
A newly created app is no longer charged its creation-period base synchronously in RegisterApp. It enters a GraceDays (=3) grace window and is charged only once it has SURVIVED it, so an app soft-deleted within grace is NEVER billed. A survivor pays the SAME creation-period proration (identical ProratedBaseMicros math, anchored to the TRUE created_at) — grace delays WHEN the charge fires, never WHAT it covers. - RegisterApp now ONLY mirrors the ms_billing.apps row (account, created_at, module_count). It never calls Stripe. Response fields kept for wire back-compat (ProrationCents always 0; ProrationInvoiceID echoes the armed guard on a post-sweep retry). - New Service.ChargeCreationProration(appID): the extracted charge leg (same Stripe plumbing, micros→cents boundary, invoice mirror, and migration-028 base snapshot the old inline RegisterApp charge used), keyed by app id with the same one-shot proration_invoice_id guard. - New Service.SweepCreationProrations(at): lists apps past grace (created_at <= at − GraceDays, guard unarmed, deleted_at IS NULL) and charges each. Wired into cmd/billing-cycle alongside the per-account boundary loop (both transports). - Race safety: ChargeProrationLocked runs the not-deleted re-check, the Stripe charge, and the guard-arm inside ONE transaction holding a SELECT ... FOR UPDATE row lock, so a concurrent soft-delete and the charge are mutually exclusive (no refund path, D1e). Whichever commits first wins; the loser no-ops. - Delayed billing correctly still charges an app whose creation period has since closed: that period is billed by NO other leg (the boundary advance leg only ever bills an app's SUBSEQUENT periods), so it never double-bills. - Migration 029: partial index apps_pending_proration_idx on (created_at) WHERE deleted_at IS NULL AND proration_invoice_id IS NULL — the sweep's work-list stays a fraction of the roster. - GraceDays const in internal/account/usage/bill.go alongside the pricing tier consts. Tests: RegisterApp mirror-only (never charges); sweep grace-holds / deleted-within-grace-never-charged / survivor-charged-once-across-reruns; proration $ math unchanged (200¢, 1300¢ overage, full-base-on-boundary); gates + idempotency; integration (build-tagged) for the AppsPendingProration filter and the FOR UPDATE ChargeProrationLocked semantics. make test / go vet / go build / gofmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three confirmed, adversarially-verified findings on the creation-grace charge (PR #46, money-charging code; invariant D1d: no retroactive catch-up): 1. HIGH — ChargeCreationProration priced the ENTIRE historical creation-period window off module_count read FRESH at sweep time, days after creation. A customer installing/uninstalling modules via SyncAppModules during the mandatory grace window could retroactively move the tier applied to days that never had that module count. Fixed: migration 030 adds apps.created_module_count, frozen once at RegisterApp (InsertAppMirror stamps it from the SAME value as module_count) and never touched by any other writer. ChargeCreationProration now prices off created_module_count; module_count remains the live value the boundary advance leg (and the display estimate for future periods) uses. 2. HIGH — removing the "skip if the creation period already closed" gate reintroduced the retroactive catch-up billing D1d forbids: an app created while its account is unactivated sits correctly unbilled every sweep, but the moment the account activates months later, the next sweep charged it for a period that closed long ago. Fixed: migration 031 adds apps.proration_skipped_at, a one-shot PERMANENT marker. ChargeCreationProration now compares the account's activation anchor against the app's anchored creation-period end (not "now" — grace + ordinary sweep cadence can legitimately push a healthy, already-activated account's charge a few days past its own period end, and that must still charge normally); when the account only activated at/after the period closed, the charge is permanently skipped and the app never resurfaces on a later sweep (AppsPendingProration now also excludes proration_skipped_at). 3. MEDIUM — ChargeProrationLocked held a SELECT ... FOR UPDATE row lock across two live Stripe HTTP calls (up to ~160s), blocking any concurrent SyncAppModules/AppDeleted write to the same row. Its deferred tx.Rollback(ctx) also reused the request context, which can silently fail against an already-cancelled/expired ctx. Fixed: split into three phases — lock+read+release (a short tx), the Stripe calls OUTSIDE any lock, then persist (a second short tx). A concurrent delete that races in during the Stripe call no longer blocks, and — since the charge has already succeeded in Stripe by then — is recorded regardless (D1e already forbids refunds; the money moved). The deferred rollback now uses a short-lived detached context (context.WithoutCancel + a fresh timeout) so cleanup reaches Postgres even when the caller's context has died. Tests: unit regression tests for all three (frozen-count pricing both directions, permanent-skip vs. still-charges-when-activated-in-time, verified to fail without the fix by reverting each in isolation); DB integration tests for the SQL-layer freeze/skip semantics and the lock-not-held-across-Stripe-call + dead-context-rollback behavior (verified to fail/hang against the pre-fix code). Full migration up/down/up round-trip validated against a real Postgres. make test / go vet / go build / gofmt / integration suite (-race) all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
I-am-nothing
added a commit
that referenced
this pull request
Jul 6, 2026
Replaces the account-wide pooled overage model with per-module-instance overage timers (migration 033), merges in the app-creation grace period and the auto-collect disclosure flag, and unifies base fee + module overage into one charge concept. Supersedes #45, #46, #47. Key mechanics: 3-day creation grace (deleted WITHIN grace = never charged; after = still owes); 5 included modules account-wide with $3/module beyond, one install-anchored timer per module instance with live FIFO re-evaluation (over->included only); coverage contract — every grace charge covers install/creation day through the END of the period its grace elapses into, boundary legs count only entities whose (immutable) install + grace-expiry predate the new period; all Stripe charges via draft->pinned-items->finalize with ms_charge_ref metadata; migration 035/036 markers + FindInvoiceByRef recovery for retries past Stripe's idempotency-key window (gates bypassed only when a finalized invoice actually exists); per-app advisory-locked timer synthesis and deletion. Migrations 029-036. Squash of the 32-commit PR branch, including two adversarial-review fix waves (23 + 12 confirmed findings fixed; full record on PR #48 and in docs-temp/pr48-review/findings.md). go build/vet/gofmt clean; full unit + integration suites green against real Postgres (migrations 001->036). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Billing policy change (owner spec 2026-07-05, D1e follow-up). A newly created app must not be charged its creation-period base immediately: it gets a 3-day grace window. Deleted within grace → never charged. Survives grace → charged the same creation-period proration that exists today (identical pricing math, just delayed timing).
Behavior change
RegisterAppno longer charges Stripe. It only mirrors thems_billing.appsrow (account linkage,created_at,module_count) exactly as before. The response fields (ProrationInvoiceID/ProrationCents) are retained for wire back-compat:ProrationCentsis always 0 from this RPC, andProrationInvoiceIDechoes the armed guard only on a retry that lands after the sweep already charged. api-platform'sAppCreatedcaller discards the response body (checks only the error), so no api-platform change is needed.Service.ChargeCreationProration(ctx, appID)— the extracted charge leg. SameProratedBaseMicrospricing, same Stripe idem keys (app-ii-<app>/app-inv-<app>), same invoice mirror + migration-028 base snapshot the old inlineRegisterAppcharge produced, same one-shotproration_invoice_idguard. Anchored to the truecreated_at(nevernow), so the amount is deterministic regardless of when the sweep fires.Service.SweepCreationProrations(ctx, at)— lists apps past grace (created_at <= at − GraceDays AND proration_invoice_id IS NULL AND deleted_at IS NULL) and charges each. Wired intocmd/billing-cyclealongside the existing per-account boundary loop (both the local one-shot and the Lambda/EventBridge transport). Idempotent + resumable: a charged app drops out of the work list; a per-app failure is counted but never aborts the batch.Race safety
ChargeProrationLockedruns the not-deleted re-check, the Stripe charge, and the guard-arm inside one transaction holding aSELECT ... FOR UPDATErow lock. A concurrent soft-delete (MarkAppDeleted) and the charge are thus mutually exclusive — whichever commits first wins and the loser sees the other's write and no-ops. Holding the lock across the Stripe call is deliberate: it's the only way to make "charge" and "delete-for-free" mutually exclusive given the no-refund policy (D1e).Design note: delayed billing past a closed period
If grace pushes the charge past the app's creation-period end (an app created within
GraceDaysof its period boundary), the creation-period proration still fires (anchored tocreated_at). That period is billed by no other leg — the boundary advance leg only ever bills an app's subsequent periods, never the one containing its creation — so charging it is correct and can never double-bill. This intentionally replaces the old synchronous-path "skip if the creation period already closed" behavior (which was a defense against a retry-recompute-from-nowbug that no longer applies), matching point 5 of the spec: grace only delays when the charge fires.Schema
apps_pending_proration_idx ON ms_billing.apps (created_at) WHERE deleted_at IS NULL AND proration_invoice_id IS NULL. Chosen over a plain(deleted_at, proration_invoice_id, created_at)composite because almost every row is eventually charged and drops out of the partial index, keeping it a fraction of the table. No new columns (created_at/proration_invoice_id/deleted_atalready exist on 027).GraceDays = 3const added alongside the pricing/tier consts ininternal/account/usage/bill.go.AccountHasLiveApps/ live-app gateUnaffected: it counts live roster rows regardless of charge state, so ungraced (day 0–2) apps still count as "live" for the boundary gate — they just aren't charged their creation base yet.
Tests
RegisterAppmakes zero Stripe calls; a sweep before grace elapses finds nothing pending (charging within grace is impossible).make test-integration): theAppsPendingProrationSQL filter and theFOR UPDATEChargeProrationLockedsemantics (charged/already-armed/deleted/declined outcomes) against real Postgres.make test/go vet/go build/gofmtclean. sqlc regenerated (v1.30.0).Follow-up (not this PR)
mirrorstack-docs/db/ms_billing/may need a companion doc update for migration 029 + the new charge timing (RegisterApp no longer charges; the sweep does). Flagging only — doc updates across the three related PRs will be consolidated afterward.🤖 Generated with Claude Code