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.
This commit is contained in:
yusufipk
2026-07-30 19:25:47 +07:00
parent debfd73214
commit a3036f1a52
3 changed files with 176 additions and 14 deletions
+20 -1
View File
@@ -113,11 +113,30 @@ export function buildBillingAccessWhereInput(now: Date = new Date()): Prisma.Use
export function buildExpiredBillingWhereInput(now: Date = new Date()): Prisma.UserWhereInput { export function buildExpiredBillingWhereInput(now: Date = new Date()): Prisma.UserWhereInput {
const cleanupCutoff = new Date(now.getTime() - STORAGE_CLEANUP_GRACE_DAYS * 24 * 60 * 60 * 1000); 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 { return {
AND: [ AND: [
{ {
NOT: buildBillingAccessWhereInput(now), subscriptionStatus: {
notIn: [BillingSubscriptionStatus.ACTIVE, BillingSubscriptionStatus.TRIALING],
},
}, },
{ OR: [{ trialEndsAt: null }, { trialEndsAt: { lte: now } }] },
{ OR: [{ stripeCurrentPeriodEnd: null }, { stripeCurrentPeriodEnd: { lte: now } }] },
{ {
OR: [ OR: [
{ billingAccessEndedAt: { lte: cleanupCutoff } }, { billingAccessEndedAt: { lte: cleanupCutoff } },
+138
View File
@@ -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,
});
});
});
+18 -13
View File
@@ -332,20 +332,14 @@ describe('buildBillingAccessWhereInput', () => {
}); });
describe('buildExpiredBillingWhereInput', () => { 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'); const cutoff = new Date('2025-12-31T00:00:00.000Z');
expect(buildExpiredBillingWhereInput(NOW)).toEqual({ expect(buildExpiredBillingWhereInput(NOW)).toEqual({
AND: [ AND: [
{ { subscriptionStatus: { notIn: ['ACTIVE', 'TRIALING'] } },
NOT: { { OR: [{ trialEndsAt: null }, { trialEndsAt: { lte: NOW } }] },
OR: [ { OR: [{ stripeCurrentPeriodEnd: null }, { stripeCurrentPeriodEnd: { lte: NOW } }] },
{ subscriptionStatus: { in: ['ACTIVE', 'TRIALING'] } },
{ trialEndsAt: { gt: NOW } },
{ stripeCurrentPeriodEnd: { gt: NOW } },
],
},
},
{ {
OR: [ OR: [
{ billingAccessEndedAt: { lte: cutoff } }, { 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<Record<string, unknown>> }>;
};
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'); vi.stubEnv('OPENFRAME_ENABLE_STRIPE', 'false');
const where = buildExpiredBillingWhereInput(NOW) as { AND: Array<{ NOT?: object }> }; expect(buildExpiredBillingWhereInput(NOW)).toEqual({ id: { in: [] } });
expect(where.AND[0].NOT).toEqual({});
}); });
}); });