[Detail Bug] Webhooks: `subscription.updated` sent on no-op seat update

[detail-app]

Detail Bug Report

https://app.detail.dev/org_ecfbc7cf-6d21-4ce1-8c61-cda1d650ccf7/bugs/bug_5c5cda90-2a31-4e1e-9bcf-d86f5d14050e

Introduced in #12155 by @frankie567 on Jun 4, 2026

Summary

• Context: The SubscriptionUpdateContext is an async context manager introduced in commit 0546fe1 (June 4, 2026) to "unify side-effects" across subscription update paths.
• Bug: When update_seats returns early because the seat count is unchanged (old_seats == seats) and there is no pending update to clear, a subscription.updated webhook is incorrectly sent even though no meaningful change occurred.
• Actual vs. expected: A webhook should NOT be sent when the seat count is unchanged (no-op), but the webhook IS being sent.
• Impact: External integrations listening to webhooks will receive spurious subscription.updated events when customers re-assert their current seat count, potentially triggering unnecessary processing, notifications, or errors.

Code with Bug

# In update_seats() - server/polar/subscription/service.py
async def update_seats(
    self,
    session: AsyncSession,
    ctx: SubscriptionUpdateContext,
    subscription: Subscription,
    *,
    seats: int,
    proration_behavior: SubscriptionProrationBehavior | None = None,
) -> Subscription:
    # ... validation code ...

    if old_seats == seats:
        # Re-asserting the current seat count cancels a pending seat
        # change. Drop the row if nothing else is scheduled on it.
        pending = subscription.pending_update
        if pending is not None and pending.seats is not None:
            if pending.product_id is None:
                await subscription_update_repository.soft_delete(pending)
                subscription.pending_update = None
            else:
                pending.seats = None
                await subscription_update_repository.update(pending)
        return subscription  # <-- BUG 🔴 early return w/ no-op still triggers webhook via context manager

    # ... normal path continues with ctx.add_event_metadata() ...

# In SubscriptionUpdateContext.__aexit__ - server/polar/subscription/service.py
async def __aexit__(
    self,
    type_: type[BaseException] | None,
    value: BaseException | None,
    traceback: TracebackType | None,
) -> None:
    # Don't trigger side effects if an exception was raised within the context
    if value is not None:
        return

    # ... billing effects ...

    if self._event_metadata:  # <-- Empty dict on early return, so no event
        await event_service.create_event(...)

    await self.service._after_subscription_updated(  # <-- BUG 🔴 called even when no meaningful change occurred
        self.session,
        self.subscription,
        previous_status=self._previous_status,
        previous_is_canceled=self._previous_is_canceled,
    )

# In _after_subscription_updated - server/polar/subscription/service.py
async def _after_subscription_updated(
    self,
    session: AsyncSession,
    subscription: Subscription,
    *,
    previous_status: SubscriptionStatus,
    previous_is_canceled: bool,
) -> None:
    await self._on_subscription_updated(session, subscription)  # <-- BUG 🔴 sends webhook unconditionally
    # ... state transition checks ...

Explanation

• update_seats has an intentional early-return path for old_seats == seats. When there is no pending update to clear, this path performs no DB mutation and does not call ctx.add_event_metadata().
• SubscriptionUpdateContext.__aexit__ correctly suppresses system events by checking if self._event_metadata, but it always calls _after_subscription_updated() on normal context exit.
• _after_subscription_updated() calls _on_subscription_updated(), which sends the subscription.updated webhook unconditionally.
• Net effect: a true no-op seat “update” (same seats, no pending update) still emits subscription.updated.

Codebase Inconsistency

• The docs table in docs/guides/seat-based-pricing.mdx states subscription.updated fires when “Seat count changes”, which contradicts sending subscription.updated in the no-op case.

Failing Test

# tests/subscription/test_bug_proof.py
@pytest.mark.asyncio
class TestBugProof:
    async def test_unchanged_seats_sends_webhook_no_pending(
        self,
        session: AsyncSession,
        save_fixture: SaveFixture,
        customer: Customer,
        organization: Organization,
        mocker: pytest.MockerFixture,
    ) -> None:
        """Prove that webhook is sent even when seats are unchanged with NO pending update."""

        webhook_send_mock = mocker.patch(
            "polar.subscription.service.webhook_service.send",
            new_callable=AsyncMock
        )

        product = await create_product(
            save_fixture,
            organization=organization,
            recurring_interval=SubscriptionRecurringInterval.month,
            prices=[("seat", 1000, "usd")],
        )
        subscription = await create_subscription_with_seats(
            save_fixture, product=product, customer=customer, seats=10
        )

        async with SubscriptionUpdateContext(
            session, subscription, subscription_service
        ) as ctx:
            updated = await subscription_service.update_seats(
                session,
                ctx,
                subscription,
                seats=10,  # Same as current seats - no-op
                proration_behavior=SubscriptionProrationBehavior.prorate,
            )
        await session.flush()

        webhook_send_mock.assert_not_called()

Test output:

AssertionError: Expected 'send' to not have been called. Called 1 times.
Calls: [call(<session>, Organization(id=UUID('...')), <WebhookEventType.subscription_updated: 'subscription.updated'>, Subscription(id=UUID('...')))]

Recommended Fix

Track whether any meaningful change occurred in the context and only send webhooks when changes were made:

class SubscriptionUpdateContext:
    def __init__(
        self,
        session: AsyncSession,
        subscription: Subscription,
        service: "SubscriptionService",
    ) -> None:
        # ... existing fields ...
        self._has_changes = False  # Track whether changes were made

    async def __aexit__(self, ...):
        if value is not None:
            return

        # ... billing effects ...

        if self._event_metadata:
            await event_service.create_event(...)

        # Only trigger webhooks if changes were made
        if self._has_changes:
            await self.service._after_subscription_updated(...)

Then in update_seats, set the flag only when it actually clears/modifies a pending update in the early-return path, and set it on the normal (seat-changing) path.

History

This bug was introduced in commit 0546fe1. The commit refactored subscription update operations to use a SubscriptionUpdateContext context manager to unify side-effects (events and webhooks). The context manager's __aexit__ unconditionally calls _after_subscription_updated() which sends webhooks. The refactoring failed to account for the existing early return path in update_seats that was designed to handle no-op cases without side effects. Before this commit, the early return happened before any event creation or webhook sending. After this commit, the context manager wraps all code paths including the early return, causing webhooks to fire even when no actual change occurs.