Skip to content

feat: subscription downgrade grace period#173

Closed
henrypark133 wants to merge 4 commits intomainfrom
feat/improve-subscriptions
Closed

feat: subscription downgrade grace period#173
henrypark133 wants to merge 4 commits intomainfrom
feat/improve-subscriptions

Conversation

@henrypark133
Copy link
Copy Markdown
Contributor

Summary

  • Deferred downgrades: When a user downgrades mid-cycle (e.g. Pro → BYOK), the old plan limits are preserved until current_period_end, then the new plan applies automatically
  • Instance enforcement: Excess agent instances are stopped when a downgrade applies or subscription ends; newest instances are kept
  • Race condition fixes: Webhook TOCTOU resolved via SELECT ... FOR UPDATE in transaction; concurrent downgrade application handled gracefully with query_opt
  • Cancel/resume safety: Pending downgrade data is preserved across cancel/resume operations
  • API response: SubscriptionWithPlan now includes optional pending_plan and downgrade_effective_at fields

Changed files

  • V21__add_pending_downgrade.sql — adds pending_price_id and downgrade_effective_at columns
  • subscription_repository.rs — new get_active_subscription_in_txn (FOR UPDATE), apply_pending_downgrade returns Option
  • service.rs — downgrade detection, deferral logic, instance enforcement, lazy downgrade application
  • ports.rs — new fields on Subscription and SubscriptionWithPlan, updated trait methods
  • main.rs — inject agent_service into subscription service config
  • common.rs / subscriptions_tests.rs — 7 new integration tests covering downgrade flows

Test plan

  • cargo test --features test -- subscription passes
  • Verify downgrade deferral: admin-set Pro subscription, simulate webhook with BYOK price, confirm pending_plan in API response
  • Verify downgrade applies after effective_at passes
  • Verify upgrade clears pending downgrade
  • Verify cancel/resume preserves pending downgrade
  • Verify instance stop on subscription end

🤖 Generated with Claude Code

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @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

  • Deferred Downgrades: When a user downgrades mid-cycle (e.g., Pro → BYOK), the old plan limits are preserved until current_period_end, then the new plan applies automatically.
  • Instance Enforcement: Excess agent instances are stopped when a downgrade applies or a subscription ends; newest instances are kept.
  • Race Condition Fixes: Webhook Time-of-Check to Time-of-Use (TOCTOU) issues are resolved via SELECT ... FOR UPDATE in transactions; concurrent downgrade application is handled gracefully with query_opt.
  • Cancel/Resume Safety: Pending downgrade data is preserved across subscription cancellation and resumption operations.
  • API Response Enhancement: SubscriptionWithPlan now includes optional pending_plan and downgrade_effective_at fields to reflect deferred downgrades.
Changelog
  • crates/api/src/main.rs
    • Injected agent_service into the subscription service configuration.
  • crates/api/tests/common.rs
    • Added logic to ensure AGENT_API_TOKEN is set for test environments to prevent agent service panics.
    • Introduced new helper functions insert_test_subscription_with_pending_downgrade and insert_test_subscription_with_price for creating test subscriptions with specific states.
  • crates/api/tests/subscriptions_tests.rs
    • Added new integration tests to verify the behavior of pending downgrades, including displaying pending plans, preserving old token limits during the grace period, clearing pending downgrades upon admin-initiated upgrades, and ensuring downgrades are applied when due.
  • crates/database/src/migrations/sql/V21__add_pending_downgrade.sql
    • Added pending_price_id (VARCHAR) and downgrade_effective_at (TIMESTAMPTZ) columns to the subscriptions table to support deferred downgrades.
  • crates/database/src/repositories/subscription_repository.rs
    • Introduced a row_to_subscription helper function to map database rows to Subscription structs, including the new pending downgrade fields.
    • Updated upsert_subscription, get_user_subscriptions, get_active_subscription, and get_active_subscriptions methods to handle the new pending_price_id and downgrade_effective_at fields.
    • Added get_active_subscription_in_txn method for fetching subscriptions within a transaction using FOR UPDATE to prevent race conditions.
    • Implemented apply_pending_downgrade method to update a subscription's price_id to its pending_price_id and clear the pending downgrade fields.
  • crates/services/src/subscription/ports.rs
    • Extended the Subscription struct with pending_price_id and downgrade_effective_at fields.
    • Enhanced the SubscriptionWithPlan API response model with optional pending_plan and downgrade_effective_at fields, which are skipped if None.
    • Added new trait methods get_active_subscription_in_txn and apply_pending_downgrade to the SubscriptionRepository trait.
  • crates/services/src/subscription/service.rs
    • Added agent_service to SubscriptionServiceConfig and SubscriptionServiceImpl for managing agent instances.
    • Implemented is_downgrade function to determine if a plan change constitutes a downgrade based on token and instance limits.
    • Introduced apply_pending_downgrade_if_due for lazy application of pending downgrades when their effective date is reached.
    • Developed stop_excess_instances to terminate agent instances exceeding new plan limits and stop_all_user_instances for subscription cancellations.
    • Modified handle_webhook_event to detect plan changes, defer downgrades by setting pending_price_id and downgrade_effective_at, apply upgrades immediately, and handle terminal subscription statuses by clearing pending downgrades and stopping all instances.
    • Updated get_user_subscriptions_with_plan to include pending plan information in the API response.
    • Adjusted require_subscription_for_proxy to incorporate the apply_pending_downgrade_if_due logic, ensuring limits are correctly applied.
    • Added comprehensive unit tests for is_downgrade and plan resolution logic.
Activity
  • The pull request author has provided a detailed summary and test plan, indicating thorough preparation.
  • Integration tests have been added to cover various downgrade scenarios, including pending plan display, token limit preservation, admin overrides, and timely application of downgrades.
  • The test plan includes verification steps for cargo test --features test -- subscription passing, and specific manual checks for downgrade deferral, application, upgrade clearing, cancel/resume preservation, and instance stopping.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/services/src/subscription/service.rs
Comment thread crates/services/src/subscription/service.rs
Comment thread crates/services/src/subscription/service.rs
Comment thread crates/services/src/subscription/service.rs
Comment thread crates/api/tests/common.rs
Comment thread crates/services/src/subscription/service.rs Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Feb 19, 2026


PR Review: Subscription Downgrade Grace Period

Overall design is sound — the FOR UPDATE transaction, idempotent apply_pending_downgrade via conditional UPDATE, and lazy application safety net are all well-handled. A few critical issues need addressing before merge.


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 price_id = price_byok. At that point the DB still has price_id = price_pro (because we deferred). The handler sees a price change, correctly identifies it as a downgrade, and defers it again with effective_at = new_current_period_end. This repeats every billing cycle.

The downgrade only ever applies lazily via apply_pending_downgrade_if_due when the user makes an API call. Inactive users (no API calls) will keep Pro limits indefinitely, and excess instances will never be stopped via webhook.

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 stop_excess_instances in webhook inline downgrade path

In handle_payment_webhook, the "same price_id + pending downgrade due" branch applies the downgrade inline but never calls stop_excess_instances:

// service.rs ~line 1373-1378
if let Some(ref pending) = subscription.pending_price_id {
    subscription.price_id = pending.clone();
}
subscription.pending_price_id = None;
subscription.downgrade_effective_at = None;
// stop_excess_instances is never called here

The lazy path (apply_pending_downgrade_if_due) does call it. Fix by tracking a variable alongside stop_all_instances_for:

let mut stop_excess_instances_for: Option<StopPair> = None;
// where type StopPair = (UserId, String)

// in the inline apply branch:
stop_excess_instances_for = Some((user_id, subscription.price_id.clone()));

// after txn.commit():
if let Some((uid, price_id)) = stop_excess_instances_for {
    self.stop_excess_instances(uid, &price_id).await;
}

3. Contradictory function doc in stop_excess_instances

The doc comment says "Keeps the oldest N instances" but the implementation sorts DESC (newest first) and keeps instances[..max_instances] — the newest ones. This matches the inline comment and PR description, but the function-level doc needs correcting.


Minor

  • unsafe { std::env::set_var(...) } safety comment claims "before spawning threads" but this runs inside async test infrastructure. The #[serial] attribute helps but the comment is technically inaccurate.

⚠️ Issues found — items 1 and 2 are behavioral bugs; item 1 in particular causes persistent incorrect plan enforcement for inactive users.

henrypark133 added a commit that referenced this pull request Feb 19, 2026
- 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>
@henrypark133 henrypark133 force-pushed the feat/improve-subscriptions branch from 03cd9bf to c5fb1d8 Compare February 19, 2026 23:11
henrypark133 added a commit that referenced this pull request Feb 19, 2026
- 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>
@henrypark133 henrypark133 force-pushed the feat/improve-subscriptions branch from c5fb1d8 to b39151c Compare February 19, 2026 23:19
@henrypark133
Copy link
Copy Markdown
Contributor Author

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 pending_price_id. If so, Stripe has renewed at the new lower price (grace period ended), so we apply the downgrade immediately instead of re-deferring. This also triggers stop_excess_instances.

2. Missing stop_excess_instances in webhook inline path — Fixed. Added stop_excess_instances_for: Option<(UserId, String)> tracking variable. It gets set both in the new Fix 1 path and in the existing inline downgrade apply path. Called after txn.commit().

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.

henrypark133 and others added 2 commits February 19, 2026 19:26
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>
@henrypark133 henrypark133 force-pushed the feat/improve-subscriptions branch from 7bdf516 to 7f5d1e2 Compare February 20, 2026 04:33
henrypark133 and others added 2 commits February 20, 2026 08:24
- 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>
@think-in-universe
Copy link
Copy Markdown
Contributor

Thanks for the PR!

Close since it's already resolved in #209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants