diff --git a/lib/billing.ts b/lib/billing.ts index 8c2fc2a..082001e 100644 --- a/lib/billing.ts +++ b/lib/billing.ts @@ -113,11 +113,30 @@ export function buildBillingAccessWhereInput(now: Date = new Date()): Prisma.Use export function buildExpiredBillingWhereInput(now: Date = new Date()): Prisma.UserWhereInput { const cleanupCutoff = new Date(now.getTime() - STORAGE_CLEANUP_GRACE_DAYS * 24 * 60 * 60 * 1000); + // Without billing nothing can expire, so nobody is eligible. This used to fall through to + // `NOT: {}`, which Prisma drops entirely, leaving a filter that matched on the grace period + // alone: a self-hosted deployment running the cleanup script would delete the workspaces of + // users it never charged. + if (!isStripeFeatureEnabled()) { + return { id: { in: [] } }; + } + + // Spelled out as positive AND branches instead of `NOT: buildBillingAccessWhereInput(now)`. + // Prisma renders that NOT as `NOT (status IN (...) OR "trialEndsAt" > $1 OR + // "stripeCurrentPeriodEnd" > $2)`, and SQL comparisons against NULL are unknown rather than + // false, so for a row with both dates empty the OR evaluates to NULL and NOT NULL is still + // NULL: the row is never returned. Both columns empty is exactly what a canceled subscriber + // looks like (markSubscriptionCanceledByCustomerId clears trialEndsAt, and Stripe no longer + // reports current_period_end on the subscription), so the cleanup silently matched nobody. return { AND: [ { - NOT: buildBillingAccessWhereInput(now), + subscriptionStatus: { + notIn: [BillingSubscriptionStatus.ACTIVE, BillingSubscriptionStatus.TRIALING], + }, }, + { OR: [{ trialEndsAt: null }, { trialEndsAt: { lte: now } }] }, + { OR: [{ stripeCurrentPeriodEnd: null }, { stripeCurrentPeriodEnd: { lte: now } }] }, { OR: [ { billingAccessEndedAt: { lte: cleanupCutoff } }, diff --git a/scripts/bunny-orphan-cleanup.ts b/scripts/bunny-orphan-cleanup.ts index 0a44248..8874482 100644 --- a/scripts/bunny-orphan-cleanup.ts +++ b/scripts/bunny-orphan-cleanup.ts @@ -319,6 +319,7 @@ async function main() { console.log(`[bunny-orphan-cleanup] Grace period: ${graceHours}h`); const expiredBillingCleanup = await cleanupExpiredBillingWorkspaces({ dryRun }); + console.log(`[bunny-orphan-cleanup] Expired owners past grace: ${expiredBillingCleanup.owners}`); console.log( `[bunny-orphan-cleanup] Expired owner workspaces scanned: ${expiredBillingCleanup.scanned}` ); diff --git a/scripts/expired-billing-cleanup.ts b/scripts/expired-billing-cleanup.ts index a3ce1e6..29efdac 100644 --- a/scripts/expired-billing-cleanup.ts +++ b/scripts/expired-billing-cleanup.ts @@ -2,6 +2,7 @@ import { db } from '../lib/db'; import { buildExpiredBillingWhereInput } from '../lib/billing'; import { collectWorkspaceMediaUrls, deleteMediaFilesBestEffort } from '../lib/r2-cleanup'; import { cleanupBunnyStreamVideosBestEffort } from '../lib/bunny-stream-cleanup'; +import { logCleanupWarnings } from '../lib/cleanup-warnings'; type ExpiredWorkspaceTarget = { id: string; @@ -9,46 +10,58 @@ type ExpiredWorkspaceTarget = { ownerEmail: string | null; }; -async function getExpiredWorkspaceTargets(): Promise { +/** + * The owners past their grace period, and the workspaces they own. + * + * Both counts are reported, because they answer different questions and a single number + * conflated them: zero workspaces can mean nobody expired, or that everyone who expired owns + * nothing. The first is normal, the second means media is being kept alive by rows the + * cleanup cannot reach, and telling them apart used to require a hand-written query. + */ +async function getExpiredWorkspaceTargets(): Promise<{ + owners: number; + workspaces: ExpiredWorkspaceTarget[]; +}> { const expiredOwners = await db.user.findMany({ where: buildExpiredBillingWhereInput(), select: { id: true }, }); if (expiredOwners.length === 0) { - return []; + return { owners: 0, workspaces: [] }; } - return db.workspace - .findMany({ - where: { - ownerId: { in: expiredOwners.map((owner) => owner.id) }, - }, - select: { - id: true, - ownerId: true, - owner: { - select: { - email: true, - }, + const workspaces = await db.workspace.findMany({ + where: { + ownerId: { in: expiredOwners.map((owner) => owner.id) }, + }, + select: { + id: true, + ownerId: true, + owner: { + select: { + email: true, }, }, - }) - .then((workspaces) => - workspaces.map((workspace) => ({ - id: workspace.id, - ownerId: workspace.ownerId, - ownerEmail: workspace.owner.email, - })) - ); + }, + }); + + return { + owners: expiredOwners.length, + workspaces: workspaces.map((workspace) => ({ + id: workspace.id, + ownerId: workspace.ownerId, + ownerEmail: workspace.owner.email, + })), + }; } export async function cleanupExpiredBillingWorkspaces(options?: { dryRun?: boolean }) { const dryRun = options?.dryRun ?? false; - const workspaces = await getExpiredWorkspaceTargets(); + const { owners, workspaces } = await getExpiredWorkspaceTargets(); if (workspaces.length === 0) { - return { scanned: 0, deleted: 0 }; + return { owners, scanned: 0, deleted: 0 }; } let deleted = 0; @@ -102,12 +115,15 @@ export async function cleanupExpiredBillingWorkspaces(options?: { dryRun?: boole ]; await db.workspace.delete({ where: { id: workspace.id } }); - await Promise.all([ + const [bunny, r2] = await Promise.all([ cleanupBunnyStreamVideosBestEffort(bunnyRefs), deleteMediaFilesBestEffort(mediaUrls), ]); + // The rows are already gone, so a refused delete leaves media nothing points at. The + // orphan sweep in the calling script picks those up, but only a log says it happened. + logCleanupWarnings({ entityType: 'workspace', entityId: workspace.id }, { bunny, r2 }); deleted += 1; } - return { scanned: workspaces.length, deleted }; + return { owners, scanned: workspaces.length, deleted }; } diff --git a/scripts/r2-orphan-cleanup.ts b/scripts/r2-orphan-cleanup.ts index 99dde3b..e35bc98 100644 --- a/scripts/r2-orphan-cleanup.ts +++ b/scripts/r2-orphan-cleanup.ts @@ -209,6 +209,7 @@ async function main() { console.log(`[r2-orphan-cleanup] Starting (${dryRun ? 'dry-run' : 'delete mode'})`); const expiredBillingCleanup = await cleanupExpiredBillingWorkspaces({ dryRun }); + console.log(`[r2-orphan-cleanup] Expired owners past grace: ${expiredBillingCleanup.owners}`); console.log( `[r2-orphan-cleanup] Expired owner workspaces scanned: ${expiredBillingCleanup.scanned}` ); diff --git a/tests/api/expired-billing-cleanup.test.ts b/tests/api/expired-billing-cleanup.test.ts new file mode 100644 index 0000000..750bfe1 --- /dev/null +++ b/tests/api/expired-billing-cleanup.test.ts @@ -0,0 +1,157 @@ +// Exercises scripts/expired-billing-cleanup.ts against a real database, because the bug this +// file exists for could not be seen any other way. +// +// The filter used to be written as `NOT: buildBillingAccessWhereInput(now)`, which the unit +// tests happily asserted on: as an object it reads correctly. Prisma renders it as +// `NOT (status IN (...) OR "trialEndsAt" > $1 OR "stripeCurrentPeriodEnd" > $2)`, and SQL +// comparisons against NULL are unknown rather than false, so a row with both dates empty +// evaluates to NOT NULL and is dropped. A canceled subscriber is exactly that row, so the +// scheduled cleanup deleted nothing at all while reporting success. Only a query against +// Postgres shows it, which is why these tests live here and not in tests/unit. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { BillingSubscriptionStatus } from '@prisma/client'; +import { db } from '@/lib/db'; +import { cleanupExpiredBillingWorkspaces } from '../../scripts/expired-billing-cleanup'; +import { createUser, createVersion, createVideo, seedProject } from '../factories'; + +const DAY_MS = 24 * 60 * 60 * 1000; + +/** Every Bunny video id the cleanup asked Bunny to delete, in call order. */ +let bunnyDeletes: string[] = []; + +beforeEach(() => { + bunnyDeletes = []; + vi.stubEnv('BUNNY_STREAM_API_KEY', 'test-bunny-key'); + vi.stubEnv('BUNNY_STREAM_LIBRARY_ID', '999999'); + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string | URL, init?: { method?: string }) => { + // Matched on the parsed host rather than a substring of the href, so a request to some + // other service that merely mentions the Bunny host cannot be recorded as a Bunny + // delete. The recorder decides what the assertions see, so a loose match here would + // make a test pass for the wrong reason. + const target = new URL(typeof url === 'string' ? url : url.toString()); + if (init?.method === 'DELETE' && target.host === 'video.bunnycdn.com') { + bunnyDeletes.push(target.pathname.split('/videos/')[1] ?? ''); + return new Response(null, { status: 200 }); + } + throw new Error(`Unexpected fetch in test: ${init?.method ?? 'GET'} ${target.href}`); + }) + ); +}); + +// tests/setup/api.ts already undoes stubbed envs after every test, but not globals, and a +// leaked fetch stub would swallow the next file's HTTP calls. +afterEach(() => { + vi.unstubAllGlobals(); +}); + +/** + * An owner whose access ended `endedDaysAgo` ago, shaped the way Stripe actually leaves the + * row: the status is CANCELED and both date columns are empty, because + * markSubscriptionCanceledByCustomerId() clears trialEndsAt and Stripe no longer reports + * current_period_end on the subscription itself. + */ +async function seedCanceledOwner(endedDaysAgo: number) { + const owner = await createUser({ + subscriptionStatus: BillingSubscriptionStatus.CANCELED, + trialEndsAt: null, + stripeCurrentPeriodEnd: null, + billingAccessEndedAt: new Date(Date.now() - endedDaysAgo * DAY_MS), + }); + const { workspace, project } = await seedProject({ ownerUser: owner }); + const video = await createVideo({ projectId: project.id }); + const version = await createVersion({ + videoParentId: video.id, + providerId: 'bunny', + providerVideoId: `bunny-video-${workspace.id.slice(0, 12)}`, + }); + + return { owner, workspace, project, video, version }; +} + +describe('cleanupExpiredBillingWorkspaces', () => { + it('deletes the workspace of a canceled owner whose trial and period columns are null', async () => { + const { workspace, version } = await seedCanceledOwner(30); + + const result = await cleanupExpiredBillingWorkspaces(); + + expect(result).toEqual({ owners: 1, scanned: 1, deleted: 1 }); + expect(await db.workspace.findUnique({ where: { id: workspace.id } })).toBeNull(); + expect(bunnyDeletes).toEqual([version.videoId]); + }); + + it('leaves an owner inside the fifteen day grace period alone', async () => { + const { workspace } = await seedCanceledOwner(5); + + const result = await cleanupExpiredBillingWorkspaces(); + + expect(result).toEqual({ owners: 0, scanned: 0, deleted: 0 }); + expect(await db.workspace.findUnique({ where: { id: workspace.id } })).not.toBeNull(); + expect(bunnyDeletes).toEqual([]); + }); + + it('leaves an owner with billing access alone', async () => { + const { workspace } = await seedProject(); + + const result = await cleanupExpiredBillingWorkspaces(); + + expect(result).toEqual({ owners: 0, scanned: 0, deleted: 0 }); + expect(await db.workspace.findUnique({ where: { id: workspace.id } })).not.toBeNull(); + }); + + it('collects an owner whose trial lapsed without Stripe ever setting an end date', async () => { + const owner = await createUser({ + subscriptionStatus: BillingSubscriptionStatus.FREE, + trialEndsAt: new Date(Date.now() - 30 * DAY_MS), + billingAccessEndedAt: null, + }); + const { workspace } = await seedProject({ ownerUser: owner }); + + const result = await cleanupExpiredBillingWorkspaces(); + + expect(result).toEqual({ owners: 1, scanned: 1, deleted: 1 }); + expect(await db.workspace.findUnique({ where: { id: workspace.id } })).toBeNull(); + }); + + it('reports what it would delete without touching anything on a dry run', async () => { + const { workspace } = await seedCanceledOwner(30); + + const result = await cleanupExpiredBillingWorkspaces({ dryRun: true }); + + expect(result).toEqual({ owners: 1, scanned: 1, deleted: 0 }); + expect(await db.workspace.findUnique({ where: { id: workspace.id } })).not.toBeNull(); + expect(bunnyDeletes).toEqual([]); + }); + + // The guard is `{ id: { in: [] } }`, and an empty IN list is only safe if Prisma renders it + // as a contradiction rather than dropping the filter. It renders `WHERE 1=0`, but a + // regression there would delete every workspace on a self-hosted deployment, so it is worth + // a test rather than trust. + it('deletes nothing when Stripe is disabled, since nothing can expire without billing', async () => { + vi.stubEnv('OPENFRAME_ENABLE_STRIPE', 'false'); + const { workspace } = await seedCanceledOwner(30); + + const result = await cleanupExpiredBillingWorkspaces(); + + expect(result).toEqual({ owners: 0, scanned: 0, deleted: 0 }); + expect(await db.workspace.findUnique({ where: { id: workspace.id } })).not.toBeNull(); + expect(bunnyDeletes).toEqual([]); + }); + + it('counts an expired owner who owns no workspace, so an empty scan is not mistaken for an empty user table', async () => { + await createUser({ + subscriptionStatus: BillingSubscriptionStatus.CANCELED, + trialEndsAt: null, + stripeCurrentPeriodEnd: null, + billingAccessEndedAt: new Date(Date.now() - 30 * DAY_MS), + }); + + expect(await cleanupExpiredBillingWorkspaces()).toEqual({ + owners: 1, + scanned: 0, + deleted: 0, + }); + }); +}); diff --git a/tests/unit/lib/billing.test.ts b/tests/unit/lib/billing.test.ts index 09c5521..b06e0a3 100644 --- a/tests/unit/lib/billing.test.ts +++ b/tests/unit/lib/billing.test.ts @@ -332,20 +332,14 @@ describe('buildBillingAccessWhereInput', () => { }); describe('buildExpiredBillingWhereInput', () => { - it('negates the access filter and requires the fifteen day grace to have elapsed', () => { + it('states the lack of access positively and requires the fifteen day grace to have elapsed', () => { const cutoff = new Date('2025-12-31T00:00:00.000Z'); expect(buildExpiredBillingWhereInput(NOW)).toEqual({ AND: [ - { - NOT: { - OR: [ - { subscriptionStatus: { in: ['ACTIVE', 'TRIALING'] } }, - { trialEndsAt: { gt: NOW } }, - { stripeCurrentPeriodEnd: { gt: NOW } }, - ], - }, - }, + { subscriptionStatus: { notIn: ['ACTIVE', 'TRIALING'] } }, + { OR: [{ trialEndsAt: null }, { trialEndsAt: { lte: NOW } }] }, + { OR: [{ stripeCurrentPeriodEnd: null }, { stripeCurrentPeriodEnd: { lte: NOW } }] }, { OR: [ { billingAccessEndedAt: { lte: cutoff } }, @@ -356,10 +350,21 @@ describe('buildExpiredBillingWhereInput', () => { }); }); - it('keeps the grace clause when Stripe is disabled even though NOT {} matches nobody', () => { + // The NOT form this replaced could not express "no access" for a row whose date columns are + // empty, because SQL turns a comparison against NULL into unknown rather than false. Every + // branch has to name NULL explicitly instead. tests/api/expired-billing-cleanup.test.ts + // proves it against a real database; this only guards the shape. + it('admits a null trial and a null period end as expired rather than skipping the row', () => { + const where = buildExpiredBillingWhereInput(NOW) as { + AND: Array<{ OR?: Array> }>; + }; + expect(where.AND[1].OR).toContainEqual({ trialEndsAt: null }); + expect(where.AND[2].OR).toContainEqual({ stripeCurrentPeriodEnd: null }); + }); + + it('matches nobody when Stripe is disabled, because nothing can expire without billing', () => { vi.stubEnv('OPENFRAME_ENABLE_STRIPE', 'false'); - const where = buildExpiredBillingWhereInput(NOW) as { AND: Array<{ NOT?: object }> }; - expect(where.AND[0].NOT).toEqual({}); + expect(buildExpiredBillingWhereInput(NOW)).toEqual({ id: { in: [] } }); }); });