From a3036f1a5243e0209f58f0415f3709f7d51d8030 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Thu, 30 Jul 2026 19:25:47 +0700 Subject: [PATCH 1/4] fix(billing): select expired owners whose billing dates are null The expired-owner filter expressed "no billing access" as NOT: buildBillingAccessWhereInput(now). Prisma renders that as NOT (status IN ('ACTIVE','TRIALING') OR "trialEndsAt" > $1 OR "stripeCurrentPeriodEnd" > $2), and a SQL comparison against NULL is unknown rather than false, so for a row with both dates empty the OR is NULL and NOT NULL is NULL: the row is never returned. Both dates empty is exactly what a canceled subscriber looks like, since markSubscriptionCanceledByCustomerId clears trialEndsAt and Stripe no longer reports current_period_end on the subscription. The scheduled cleanup therefore matched nobody at all while reporting success, and media of owners fifteen days past their grace period stayed in Bunny and R2 indefinitely. Each branch now names NULL explicitly. Disabling Stripe also selects nobody instead of falling through to NOT {}, which Prisma drops entirely: that left a filter keyed on the grace period alone, so a self-hosted deployment running the cleanup would delete workspaces of users it never charged. The unit tests could not catch this, because as an object the old filter reads correctly and no SQL is produced. The new coverage lives in tests/api and runs against Postgres. --- lib/billing.ts | 21 +++- tests/api/expired-billing-cleanup.test.ts | 138 ++++++++++++++++++++++ tests/unit/lib/billing.test.ts | 31 +++-- 3 files changed, 176 insertions(+), 14 deletions(-) create mode 100644 tests/api/expired-billing-cleanup.test.ts 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/tests/api/expired-billing-cleanup.test.ts b/tests/api/expired-billing-cleanup.test.ts new file mode 100644 index 0000000..af72720 --- /dev/null +++ b/tests/api/expired-billing-cleanup.test.ts @@ -0,0 +1,138 @@ +// 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 }) => { + const href = typeof url === 'string' ? url : url.toString(); + if (init?.method === 'DELETE' && href.includes('video.bunnycdn.com')) { + bunnyDeletes.push(href.split('/videos/')[1] ?? ''); + return new Response(null, { status: 200 }); + } + throw new Error(`Unexpected fetch in test: ${init?.method ?? 'GET'} ${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([]); + }); + + 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: [] } }); }); }); From 80ef29f7873e2a93fa80440eaa641e43ab660874 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Thu, 30 Jul 2026 19:26:06 +0700 Subject: [PATCH 2/4] fix(scripts): tell an empty cleanup scan apart from an unreachable one The cleanup printed one number for expired owners, the count of workspaces it found, so zero meant either that nobody had passed the grace period or that everyone who had owns nothing. The first is normal and the second means media is held alive by rows the cleanup cannot reach, and telling them apart took a hand-written query against production. Both counts are reported now. Bunny and R2 results were also discarded. The workspace row is deleted first, so a refused storage delete leaves media that nothing points at, and nothing recorded that it happened. logCleanupWarnings already exists for this and is now called with the per-workspace result. --- scripts/bunny-orphan-cleanup.ts | 1 + scripts/expired-billing-cleanup.ts | 68 ++++++++++++++++++------------ scripts/r2-orphan-cleanup.ts | 1 + 3 files changed, 44 insertions(+), 26 deletions(-) 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}` ); From 38d829597a153fa3b9397b52573f20727bd601ca Mon Sep 17 00:00:00 2001 From: yusufipk Date: Thu, 30 Jul 2026 19:30:34 +0700 Subject: [PATCH 3/4] test(api): pin the stripe-disabled guard to a real empty result The guard is `{ id: { in: [] } }`, which is only safe because Prisma renders an empty IN list as `WHERE 1=0` instead of dropping the filter. A regression there would delete every workspace on a self-hosted deployment, which is too expensive to leave resting on that assumption. --- tests/api/expired-billing-cleanup.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/api/expired-billing-cleanup.test.ts b/tests/api/expired-billing-cleanup.test.ts index af72720..01d1989 100644 --- a/tests/api/expired-billing-cleanup.test.ts +++ b/tests/api/expired-billing-cleanup.test.ts @@ -121,6 +121,21 @@ describe('cleanupExpiredBillingWorkspaces', () => { 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, From 733ac43172b7daa3f0d017a9f952a7f8e25893c0 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Thu, 30 Jul 2026 19:33:19 +0700 Subject: [PATCH 4/4] test(api): match the bunny host instead of a substring of the url CodeQL flags the substring form (js/incomplete-url-substring-sanitization) because a host check on an unparsed url matches when the host appears anywhere in it. Nothing untrusted reaches this recorder, but a loose match could still record a delete aimed elsewhere as a Bunny delete and pass an assertion for the wrong reason. --- tests/api/expired-billing-cleanup.test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/api/expired-billing-cleanup.test.ts b/tests/api/expired-billing-cleanup.test.ts index 01d1989..750bfe1 100644 --- a/tests/api/expired-billing-cleanup.test.ts +++ b/tests/api/expired-billing-cleanup.test.ts @@ -27,12 +27,16 @@ beforeEach(() => { vi.stubGlobal( 'fetch', vi.fn(async (url: string | URL, init?: { method?: string }) => { - const href = typeof url === 'string' ? url : url.toString(); - if (init?.method === 'DELETE' && href.includes('video.bunnycdn.com')) { - bunnyDeletes.push(href.split('/videos/')[1] ?? ''); + // 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'} ${href}`); + throw new Error(`Unexpected fetch in test: ${init?.method ?? 'GET'} ${target.href}`); }) ); });