Fix incomplete fulfillment update on subscription plan upgrade

[rameerez]

🐛 The Bug

When upgrading between subscription plans with different fulfillment periods, only the metadata["plan"] field was being updated. The fulfillment_period and next_fulfillment_at fields remained stuck on the old plan's values.

Real-World Impact

Scenario that triggered discovery:

1. User subscribes to Starter plan (100 credits every 15 seconds)
2. User upgrades to Studio plan (1000 credits every 5 seconds)
3. ❌ User gets 1000 upgrade credits immediately ✓
4. ❌ But fulfillment_period still shows "15 seconds" instead of "5 seconds"
5. ❌ Future fulfillments remain on 15-second schedule instead of 5-second
6. ❌ Result: Delayed credit delivery, incorrect database state

Root Cause

The handle_plan_upgrade method (line 423-428 before fix) only updated the metadata["plan"] field:

# BEFORE (incomplete update)
fulfillment.update!(
  metadata: fulfillment.metadata
    .except("pending_plan_change", "plan_change_at")
    .merge("plan" => processor_plan)
)

This meant future fulfillments would:

• Use the correct plan for credit amounts (via metadata lookup) ✓
• But calculate next_fulfillment_at using the OLD fulfillment_period ❌
• Display incorrect fulfillment period in the database ❌

---

✅ The Fix

Updated handle_plan_upgrade to update ALL fulfillment fields (lines 467-473):

# AFTER (complete update)
fulfillment.update!(
  fulfillment_period: new_plan.fulfillment_period_display,  # ← NEW
  next_fulfillment_at: Time.current + new_plan.parsed_fulfillment_period,  # ← NEW
  metadata: fulfillment.metadata
    .except("pending_plan_change", "plan_change_at")
    .merge("plan" => processor_plan)
)

What This Fixes

1. fulfillment_period: Now immediately updated to match new plan
2. next_fulfillment_at: Recalculated from upgrade time using new plan's period
3. Future fulfillments: Run on correct schedule matching new plan

---

🔍 Debug Logging Added

Added comprehensive debug logging throughout the plan change flow (lines 347-420):

On Plan Change Detection

[UsageCredits] Plan change detected for subscription {id}
  Processor plan changed: [old_id, new_id]
  Current plan found: starter (100 credits)
  New plan found: studio (1000 credits)
  Comparing credits: 100 → 1000
  Action: UPGRADE detected

During Upgrade Process

  [UPGRADE] Starting upgrade process
  [UPGRADE] Current balance: 1200
  [UPGRADE] Credits to award: 1000
  [UPGRADE] Old fulfillment_period: 15 seconds
  [UPGRADE] New fulfillment_period: 5 seconds
  [UPGRADE] Old next_fulfillment_at: 2025-12-29 17:17:20 UTC
  [UPGRADE] New next_fulfillment_at: 2025-12-29 17:17:28 UTC
  [UPGRADE] Fulfillment updated successfully

This logging helps diagnose:

• Silent transaction rollbacks
• Race conditions with webhook processing
• Plan matching issues
• Timing problems

---

🧪 Testing

New Regression Tests

Added 2 comprehensive tests covering different upgrade scenarios:

1. test_REGRESSION_upgrade_updates_fulfillment_period_and_next_fulfillment_at

Tests upgrade between plans with significantly different periods (1 day → 7 days):

• ✅ Verifies fulfillment_period updates from "1.0 days" to "7.0 days"
• ✅ Confirms next_fulfillment_at is recalculated (~7 days from upgrade time)
• ✅ Ensures schedule change is significant (6+ day difference)

2. test_REGRESSION_upgrade_between_plans_with_same_period_only_updates_metadata

Tests upgrade between plans with the same period (both monthly):

• ✅ Verifies next_fulfillment_at is still recalculated from upgrade time
• ✅ Confirms fulfillment_period display string is updated
• ✅ Ensures metadata reflects new plan

Test Results

✅ All 614 tests pass (1364 assertions)
📊 Line Coverage: 88.29% (1169/1324)
📊 Branch Coverage: 82.13% (432/526)

Manual Testing (Production-like scenario)

Tested in separate Rails demo app with real Stripe webhooks:

Test Flow:

1. ✅ Subscribe to Starter yearly (100 credits/15s) → 1200 balance
2. ✅ Upgrade to Studio yearly (1000 credits/5s) → 2100 balance
3. ✅ Immediate 1000 upgrade credits awarded
4. ✅ Fulfillment period updated: "15 seconds" → "5 seconds"
5. ✅ Next fulfillment recalculated to 5-second schedule
6. ✅ Future fulfillments running on correct 5-second cadence

Debug logs confirmed:

• Plan change detection working ✓
• Upgrade credits awarded ✓
• All fulfillment fields updated ✓
• Transaction committed successfully ✓

---

📋 Files Changed

Modified Files

• lib/usage_credits/models/concerns/pay_subscription_extension.rb (+55, -2)
  • Updated handle_plan_upgrade to update all fulfillment fields
  • Added comprehensive debug logging throughout plan change flow
  • Enhanced error visibility for easier troubleshooting

Test Files

• test/models/concerns/pay_subscription_extension_test.rb (+144)
  • Added 2 regression tests for fulfillment period updates
  • Covers both different-period and same-period upgrade scenarios

---

🔗 Related PRs

This fix builds on recent improvements:

• PR Handle subscription plan changes (upgrades & downgrades) #20: Handle subscription plan changes (upgrades & downgrades)
• PR Add multi-period Stripe price support for subscription plans #22: Multi-period Stripe price support
• PR Fix a bug where very fast fulfillment periods would cause credits not to expire fast enough #23: Fix expiring credits piling up with fast fulfillment periods

---

⚡ Performance Impact

Minimal - Only adds:

• One additional field update (fulfillment_period)
• One timestamp calculation (next_fulfillment_at)
• Debug logging (negligible impact)

All within the existing transaction, no additional queries.

---

✨ Summary

Before: Upgrading between plans left fulfillment schedule on old plan's cadence
After: Upgrading immediately switches to new plan's fulfillment schedule

Impact: Ensures users get credits at the correct frequency after upgrading
Risk: Very low - only affects upgrade path, all tests passing, backward compatible

---

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

[rameerez]

@claude is this PR safe to merge?

[claude]

Claude finished @rameerez's task in 2m 3s —— View job

---

✅ Safe to Merge - Comprehensive Review Complete

Todo List:

• Read project context and guidelines
• Review changed files and implementation
• Check for bugs, edge cases, and security issues
• Assess test coverage and quality
• Provide merge safety recommendation

---

🎯 Executive Summary

Recommendation: SAFE TO MERGE ✅

This PR fixes a legitimate bug where upgrading subscription plans didn't update the fulfillment schedule. The fix is well-implemented, thoroughly tested, and has low risk.

---

🔍 Code Review

What Changed

Core Fix (lib/usage_credits/models/concerns/pay_subscription_extension.rb:467-473):

fulfillment.update!(
  fulfillment_period: new_plan.fulfillment_period_display,  # ← NEW
  next_fulfillment_at: Time.current + new_plan.parsed_fulfillment_period,  # ← NEW
  metadata: fulfillment.metadata
    .except("pending_plan_change", "plan_change_at")
    .merge("plan" => processor_plan)
)

Before: Only updated metadata["plan"]
After: Updates ALL fulfillment fields including period and next fulfillment time

✅ Strengths

1. Correct Fix: The bug was real - upgrades would award correct credits but keep old fulfillment schedule. This fix ensures the schedule matches the new plan.

2. Excellent Test Coverage:

   • 2 new regression tests covering different scenarios
   • Tests verify both different-period upgrades (1 day → 7 days) and same-period upgrades
   • All 614 tests passing (88.29% line coverage, 82.13% branch coverage)

3. Comprehensive Debug Logging: The extensive logging (lines 347-420) will make future issues much easier to diagnose in production.

4. Atomic Operations: All updates happen within transactions, preventing partial updates.

5. Backward Compatible: Only affects the upgrade path; no changes to existing stable functionality.

🔍 Potential Concerns (All Low Risk)

1. Log Verbosity

• The debug logging is quite verbose (13+ log statements per plan change)
• Risk: Low - Rails logger typically handles this fine, but might clutter logs in high-traffic systems
• Recommendation: Consider using Rails.logger.debug instead of .info for some of the detailed upgrade steps, or add a feature flag to control verbosity

2. next_fulfillment_at Calculation

• Line 455: next_fulfillment_at = Time.current + new_plan.parsed_fulfillment_period
• This recalculates from the upgrade moment, not from the original schedule
• Risk: Low - This is actually the correct behavior for an immediate upgrade, but it means users might get their next fulfillment slightly later than the original cadence
• Example: If user had fulfillment scheduled in 2 days on old plan, after upgrade it resets to the new plan's period from now
• This is mentioned in the PR description and is intentional behavior ✓

3. Edge Case: Very Fast Plan Changes

• If a user rapidly switches plans multiple times, each switch recalculates next_fulfillment_at
• Risk: Very Low - This is the correct behavior (latest upgrade wins), and the transaction locks prevent race conditions
• Tests don't cover rapid plan switching, but the code should handle it correctly

🛡️ Security Assessment

No security concerns identified:

• ✅ All database updates use parameterized queries (ActiveRecord)
• ✅ No user input directly affects credit amounts or timing
• ✅ Transaction locks prevent race conditions
• ✅ No SQL injection vectors
• ✅ No privilege escalation paths

🧪 Test Quality

The regression tests are well-designed:

Test 1 (pay_subscription_extension_test.rb:803-879):

• ✅ Tests significant period change (1 day → 7 days)
• ✅ Verifies fulfillment_period string is updated
• ✅ Confirms next_fulfillment_at is recalculated correctly
• ✅ Asserts timing is significantly different from old schedule
• ✅ Validates credits were awarded

Test 2 (pay_subscription_extension_test.rb:881-940):

• ✅ Tests same-period upgrade (both monthly)
• ✅ Confirms next_fulfillment_at still gets recalculated from upgrade time
• ✅ Validates metadata update
• ✅ Checks credit awarding

Minor suggestion: Could add a test for downgrade scenario to ensure it doesn't break fulfillment_period, but the existing downgrade tests probably cover this implicitly.

📊 CI Status

All checks passing ✅:

• ✅ Ruby tests (3 runs, all successful)
• ✅ Claude Code Review (passed)
• ✅ 614 tests, 1364 assertions, 0 failures

---

🎬 Final Verdict

SAFE TO MERGE with high confidence.

Why it's safe:

1. ✅ Fixes a real, documented bug with clear impact
2. ✅ Implementation is straightforward and correct
3. ✅ Excellent test coverage (2 new regression tests)
4. ✅ All existing tests still pass
5. ✅ Backward compatible (only affects upgrade path)
6. ✅ No security concerns
7. ✅ Atomic database operations prevent partial updates
8. ✅ CI passing
9. ✅ Manual testing confirmed in PR description

Optional follow-up (non-blocking):

• Consider reducing log verbosity by using Rails.logger.debug for some upgrade steps
• Could add a test for rapid plan switching, though current code should handle it

Risk Level: Very Low

The change is isolated to the upgrade flow, well-tested, and fixes incorrect behavior. Users will benefit from getting credits on the correct schedule after upgrading.

---

Merge with confidence! 🚀