feat: subscription downgrade grace period#173
Conversation
Summary of ChangesHello @henrypark133, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a grace period for subscription downgrades, ensuring that users retain their current plan's benefits until the end of their billing cycle. It also implements robust instance enforcement to align agent usage with new plan limits and addresses potential race conditions in webhook processing. These changes enhance the subscription management system's fairness and reliability, providing a smoother experience for users transitioning between plans. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a grace period for subscription downgrades, ensuring existing plan limits are maintained until the end of the billing cycle. While the implementation incorporates database schema updates, new repository methods with transaction-level locking (FOR UPDATE), and comprehensive updates to the subscription service logic to handle deferred downgrades, upgrades, and cancellations, and addresses several race conditions, there are significant logic flaws. Specifically, the instance cleanup logic is limited to 1000 items and can be bypassed, downgrade detection does not verify subscription IDs allowing for plan spoofing, and the token limit calculation assumes monthly billing, which breaks for yearly plans. Furthermore, pending downgrades are not correctly cleared if cancelled in the payment provider. The overall code quality is high, but minor improvements could be made regarding code duplication in test helpers and a misleading comment.
PR Review: Subscription Downgrade Grace PeriodOverall design is sound — the FOR UPDATE transaction, idempotent 1. Infinite downgrade re-deferral via webhook (Critical Logic Bug)When Stripe renews a subscription at the new (lower) price after the period ends, the renewal webhook delivers The downgrade only ever applies lazily via Fix: Before deferring, check if the incoming price already matches the pending downgrade target to avoid re-deferring: if Self::is_downgrade(&existing_sub.price_id, &subscription.price_id, ...) {
if existing_sub.pending_price_id.as_deref() == Some(&subscription.price_id) {
// Already pending the same downgrade — don't re-defer, preserve existing effective_at
subscription.pending_price_id = existing_sub.pending_price_id.clone();
subscription.downgrade_effective_at = existing_sub.downgrade_effective_at;
subscription.price_id = existing_sub.price_id.clone();
} else {
// New downgrade target — defer normally
subscription.pending_price_id = Some(subscription.price_id.clone());
subscription.downgrade_effective_at = Some(subscription.current_period_end);
subscription.price_id = existing_sub.price_id.clone();
}
}2. Missing
|
- Fix infinite downgrade re-deferral: detect when Stripe renews at pending price and apply downgrade immediately instead of re-deferring - Add stop_excess_instances call in webhook inline downgrade path - Fix doc comment: stop_excess_instances keeps newest N, not oldest - Add TODO for yearly plan period calculation - Add trial_period_days field to test plan configs (rebase fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
03cd9bf to
c5fb1d8
Compare
- Fix infinite downgrade re-deferral: detect when Stripe renews at pending price and apply downgrade immediately instead of re-deferring - Add stop_excess_instances call in webhook inline downgrade path - Fix doc comment: stop_excess_instances keeps newest N, not oldest - Add TODO for yearly plan period calculation - Add trial_period_days field to test plan configs (rebase fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c5fb1d8 to
b39151c
Compare
|
Thanks for the thorough review! All items addressed in b39151c: 1. Infinite downgrade re-deferral (Critical) — Fixed. Before deferring, we now check if the incoming price matches 2. Missing 3. Doc comment contradiction — Fixed. Changed to "Keeps the newest N instances (by created_at), stops the rest." Minor (unsafe env var comment) — Cosmetic, leaving as-is. |
Defer plan downgrades until current_period_end so users keep their existing limits for the remainder of their billing cycle. Apply the downgrade lazily on next proxy request or renewal webhook. Key changes: - Store pending_price_id and downgrade_effective_at in subscriptions table - Detect downgrades in webhook handler and defer price change - Enforce instance limits on downgrade (keep newest, stop oldest excess) - Stop all instances on subscription cancellation/expiry - Fix TOCTOU race: read subscription through webhook transaction (FOR UPDATE) - Fix concurrent downgrade: use query_opt to handle already-applied case - Preserve pending downgrade across cancel/resume operations - Add SubscriptionWithPlan.pending_plan and downgrade_effective_at to API Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix infinite downgrade re-deferral: detect when Stripe renews at pending price and apply downgrade immediately instead of re-deferring - Add stop_excess_instances call in webhook inline downgrade path - Fix doc comment: stop_excess_instances keeps newest N, not oldest - Add TODO for yearly plan period calculation - Add trial_period_days field to test plan configs (rebase fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7bdf516 to
7f5d1e2
Compare
- Move cache invalidation before txn.commit() in apply_pending_downgrade_if_due to match pattern used in all other code paths (cancel, resume, webhook) - Add overflow warning in stop_all_user_instances when total > 1000, consistent with the existing guard in stop_excess_instances - Renumber V21__add_pending_downgrade → V23 since V22 is already applied in production (refinery rejects out-of-order backfills) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the PR! Close since it's already resolved in #209 |
Summary
current_period_end, then the new plan applies automaticallySELECT ... FOR UPDATEin transaction; concurrent downgrade application handled gracefully withquery_optSubscriptionWithPlannow includes optionalpending_plananddowngrade_effective_atfieldsChanged files
V21__add_pending_downgrade.sql— addspending_price_idanddowngrade_effective_atcolumnssubscription_repository.rs— newget_active_subscription_in_txn(FOR UPDATE),apply_pending_downgradereturnsOptionservice.rs— downgrade detection, deferral logic, instance enforcement, lazy downgrade applicationports.rs— new fields onSubscriptionandSubscriptionWithPlan, updated trait methodsmain.rs— injectagent_serviceinto subscription service configcommon.rs/subscriptions_tests.rs— 7 new integration tests covering downgrade flowsTest plan
cargo test --features test -- subscriptionpassespending_planin API responseeffective_atpasses🤖 Generated with Claude Code