Merge pull request #48 from yusufipk/fix/expired-billing-cleanup-null-dates

fix(billing): select expired owners whose billing dates are null

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 } },

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}`
   );

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<ExpiredWorkspaceTarget[]> {
+/**
+ * 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 };
 }

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}`
   );

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,
+    });
+  });
+});

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<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');
-    const where = buildExpiredBillingWhereInput(NOW) as { AND: Array<{ NOT?: object }> };
-    expect(where.AND[0].NOT).toEqual({});
+    expect(buildExpiredBillingWhereInput(NOW)).toEqual({ id: { in: [] } });
   });
 });