Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/migrations.yml
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ jobs:
echo "ERROR: db:push needs an interactive rename decision; land it as a versioned migration instead of relying on push." >&2
exit 1
fi
bun run ./scripts/apply-dev-workspace-file-size-cutover.ts
else
echo "Applying versioned migrations (db:migrate)"
bun run ./scripts/migrate.ts
Expand Down
20 changes: 0 additions & 20 deletions apps/sim/background/cleanup-soft-deletes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,26 +198,6 @@ describe('cleanup soft deletes', () => {
expect(mockDecrementStorageUsageForBillingContextInTx).not.toHaveBeenCalled()
})

it('fails before deleting storage when canonical size metadata is missing', async () => {
mockSelectRowsByIdChunks
.mockResolvedValueOnce([])
.mockResolvedValueOnce([])
.mockResolvedValueOnce([
{
id: 'file-missing-size',
key: 'workspace/ws-1/file-missing-size',
workspaceId: 'ws-1',
context: 'workspace',
sizeBytes: null,
},
])

await expect(runCleanupSoftDeletes(basePayload)).rejects.toThrow(
'Workspace file is missing canonical size_bytes metadata'
)
expect(mockDeleteFiles).not.toHaveBeenCalled()
})

it('hard-deletes retained documents before deleting an expired knowledge base', async () => {
mockChunkedBatchDelete.mockImplementationOnce(
async (options: {
Expand Down
16 changes: 2 additions & 14 deletions apps/sim/ee/workspace-forking/lib/copy/storage-quota.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ import {
} from '@/ee/workspace-forking/lib/copy/storage-quota'
import { ForkError } from '@/ee/workspace-forking/lib/lineage/authz'

function makeExecutor(total: number | string | null) {
function makeExecutor(total: number | string) {
const execute = vi.fn((_query: unknown) => Promise.resolve([{ total }]))
return { executor: { execute } as unknown as DbOrTx, execute }
}
Expand All @@ -71,8 +71,7 @@ describe('sumForkCopyBytes', () => {
const compiled = outerQuery.toSQL()
expect(compiled.sql).toBe('SELECT (? + ?)::bigint AS total')
const [fileBytes, kbBytes] = compiled.params
expect(fileBytes.toSQL().sql).toContain('count(*) FILTER')
expect(fileBytes.toSQL().sql).toContain('IS NULL')
expect(fileBytes.toSQL().sql).toContain('coalesce(sum')
expect(fileBytes.toSQL().params).toContainEqual({
type: 'and',
conditions: [
Expand Down Expand Up @@ -103,17 +102,6 @@ describe('sumForkCopyBytes', () => {
expect(bytes).toBe(1024)
})

it('fails closed when a selected workspace file lacks canonical size metadata', async () => {
const { executor } = makeExecutor(null)

await expect(
sumForkCopyBytes(executor, 'src-ws', { fileIds: ['wf-missing-size'] })
).rejects.toMatchObject({
message: 'Storage calculation is temporarily unavailable',
statusCode: 503,
})
})

it('runs no query for an empty selection', async () => {
const { executor, execute } = makeExecutor(0)

Expand Down
11 changes: 4 additions & 7 deletions apps/sim/ee/workspace-forking/lib/copy/storage-quota.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,8 @@ export async function sumForkCopyBytes(
const fileBytes =
fileSelectors.length === 0
? sql<number>`0`
: sql<number | null>`(
SELECT CASE
WHEN count(*) FILTER (WHERE ${workspaceFiles.sizeBytes} IS NULL) > 0 THEN NULL
ELSE coalesce(sum(${workspaceFiles.sizeBytes}), 0)
END
: sql<number>`(
SELECT coalesce(sum(${workspaceFiles.sizeBytes}), 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When a selected workspace_files row still has size_bytes = NULL, PostgreSQL ignores it in sum, and this coalesce turns an all-NULL selection into zero, allowing quota admission to undercount copied bytes. Preserve the NULL-detection aggregate and make the 503 path handle a null total.

(Based on your team's feedback about failing closed on missing size_bytes.) [3f8e6e9d-39ca-4a4a-b8b4-0e6be9b461e0].

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/ee/workspace-forking/lib/copy/storage-quota.ts, line 51:

<comment>When a selected `workspace_files` row still has `size_bytes = NULL`, PostgreSQL ignores it in `sum`, and this `coalesce` turns an all-NULL selection into zero, allowing quota admission to undercount copied bytes. Preserve the NULL-detection aggregate and make the 503 path handle a null total.

(Based on your team's feedback about failing closed on missing `size_bytes`.) [3f8e6e9d-39ca-4a4a-b8b4-0e6be9b461e0].</comment>

<file context>
@@ -47,11 +47,8 @@ export async function sumForkCopyBytes(
-            ELSE coalesce(sum(${workspaceFiles.sizeBytes}), 0)
-          END
+      : sql<number>`(
+          SELECT coalesce(sum(${workspaceFiles.sizeBytes}), 0)
           FROM ${workspaceFiles}
           WHERE ${and(
</file context>

FROM ${workspaceFiles}
WHERE ${and(
fileSelectors.length === 1 ? fileSelectors[0] : or(...fileSelectors),
Expand All @@ -77,10 +74,10 @@ export async function sumForkCopyBytes(
isNotNull(document.storageKey)
)}
)`
const [row] = await executor.execute<{ total: number | string | null }>(
const [row] = await executor.execute<{ total: number | string }>(
sql`SELECT (${fileBytes} + ${kbBytes})::bigint AS total`
)
if (row?.total == null) {
if (!row) {
throw new ForkError('Storage calculation is temporarily unavailable', 503)
}
return Number(row.total)
Expand Down
58 changes: 1 addition & 57 deletions apps/sim/lib/billing/storage/payer-transfer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ interface FakeTransferState {
users?: Record<string, number>
workspace: FakeWorkspace
workspaceFileBytes?: number
workspaceFileMissingSizeCount?: number
}

function createFakeTx(state: FakeTransferState) {
Expand Down Expand Up @@ -166,7 +165,6 @@ function createFakeTx(state: FakeTransferState) {
{
document_bytes: state.documentBytes ?? 0,
workspace_file_bytes: state.workspaceFileBytes ?? 0,
workspace_file_missing_size_count: state.workspaceFileMissingSizeCount ?? 0,
},
]
})
Expand All @@ -188,7 +186,6 @@ function updateFor(

interface FakeBatchTransferState {
exactBytes: Record<string, number>
missingSizeCounts?: Record<string, number>
organizations?: Record<string, number>
users?: Record<string, number>
workspaces: FakeWorkspace[]
Expand Down Expand Up @@ -217,7 +214,6 @@ function createFakeBatchTx(state: FakeBatchTransferState) {
workspace_id: workspaceId,
document_bytes: 0,
workspace_file_bytes: bytes,
workspace_file_missing_size_count: state.missingSizeCounts?.[workspaceId] ?? 0,
}))
)

Expand Down Expand Up @@ -535,31 +531,7 @@ describe('changeWorkspaceStoragePayerInTx', () => {
expect(query.values).not.toContain('workspaceFiles.deletedAt')
expect(query.values).toContain('document.connectorId')
expect(query.values).toContain('document.deletedAt')
expect(query.values.filter((value) => value === 'workspace-1')).toHaveLength(3)
})

it('fails closed when a billable file is missing canonical size metadata', async () => {
const fake = createFakeTx({
workspace: {
id: 'workspace-1',
billedAccountUserId: 'user-1',
organizationId: null,
storageUsedBytes: 10,
},
workspaceFileBytes: 10,
workspaceFileMissingSizeCount: 1,
users: { 'user-1': 10, 'user-2': 0 },
})

await expect(
changeWorkspaceStoragePayerInTx(fake.tx, {
workspaceId: 'workspace-1',
organizationId: null,
billedAccountUserId: 'user-2',
})
).rejects.toThrow('Workspace workspace-1 has files missing canonical size_bytes metadata')

expect(fake.updates).toEqual([])
expect(query.values.filter((value) => value === 'workspace-1')).toHaveLength(2)
})
})

Expand Down Expand Up @@ -712,34 +684,6 @@ describe('changeWorkspaceStoragePayersInTx', () => {
expect(fake.updates).toEqual([])
expect(fake.locks).toEqual([{ ids: ['workspace-a'], table: 'workspace' }])
})

it('fails the batch before payer writes when canonical size metadata is missing', async () => {
const fake = createFakeBatchTx({
exactBytes: { 'workspace-a': 10 },
missingSizeCounts: { 'workspace-a': 1 },
users: { current: 10, destination: 0 },
workspaces: [
{
id: 'workspace-a',
billedAccountUserId: 'current',
organizationId: null,
storageUsedBytes: 10,
},
],
})

await expect(
changeWorkspaceStoragePayersInTx(fake.tx, [
{
workspaceId: 'workspace-a',
organizationId: null,
billedAccountUserId: 'destination',
},
])
).rejects.toThrow('Workspace workspace-a has files missing canonical size_bytes metadata')

expect(fake.updates).toEqual([])
})
})

describe('changeOrganizationWorkspaceBilledAccountsInTx', () => {
Expand Down
28 changes: 3 additions & 25 deletions apps/sim/lib/billing/storage/payer-transfer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ interface ExactWorkspaceStorageRow {
[key: string]: unknown
document_bytes: number | string
workspace_file_bytes: number | string
workspace_file_missing_size_count: number | string
}

interface BatchExactWorkspaceStorageRow extends ExactWorkspaceStorageRow {
Expand Down Expand Up @@ -88,13 +87,6 @@ async function getExactWorkspaceStorageBytes(tx: DbOrTx, workspaceId: string): P
WHERE ${workspaceFiles.workspaceId} = ${workspaceId}
AND ${workspaceFiles.context} = 'workspace'
), 0)::bigint AS workspace_file_bytes,
(
SELECT COUNT(*)
FROM ${workspaceFiles}
WHERE ${workspaceFiles.workspaceId} = ${workspaceId}
AND ${workspaceFiles.context} = 'workspace'
AND ${workspaceFiles.sizeBytes} IS NULL
)::bigint AS workspace_file_missing_size_count,
COALESCE((
SELECT SUM(${document.fileSize}::bigint)
FROM ${document}
Expand All @@ -109,10 +101,6 @@ async function getExactWorkspaceStorageBytes(tx: DbOrTx, workspaceId: string): P
if (!row) {
throw new Error(`Could not recompute storage for workspace ${workspaceId}`)
}
if (parseExactBytes(row.workspace_file_missing_size_count, 'missing workspace file size') > 0) {
throw new Error(`Workspace ${workspaceId} has files missing canonical size_bytes metadata`)
}

const workspaceFileBytes = parseExactBytes(row.workspace_file_bytes, 'workspace file')
const documentBytes = parseExactBytes(row.document_bytes, 'knowledge document')
const total = workspaceFileBytes + documentBytes
Expand Down Expand Up @@ -178,16 +166,12 @@ async function getExactWorkspaceStorageBytesBatch(
COALESCE(SUM(storage_by_workspace.workspace_file_bytes), 0)::bigint
AS workspace_file_bytes,
COALESCE(SUM(storage_by_workspace.document_bytes), 0)::bigint
AS document_bytes,
COALESCE(SUM(storage_by_workspace.workspace_file_missing_size_count), 0)::bigint
AS workspace_file_missing_size_count
AS document_bytes
FROM (
SELECT
${workspaceFiles.workspaceId} AS workspace_id,
SUM(${workspaceFiles.sizeBytes}) AS workspace_file_bytes,
0::bigint AS document_bytes,
COUNT(*) FILTER (WHERE ${workspaceFiles.sizeBytes} IS NULL)::bigint
AS workspace_file_missing_size_count
0::bigint AS document_bytes
FROM ${workspaceFiles}
WHERE ${inArray(workspaceFiles.workspaceId, workspaceIds)}
AND ${workspaceFiles.context} = 'workspace'
Expand All @@ -198,8 +182,7 @@ async function getExactWorkspaceStorageBytesBatch(
SELECT
${knowledgeBase.workspaceId} AS workspace_id,
0::bigint AS workspace_file_bytes,
SUM(${document.fileSize}::bigint) AS document_bytes,
0::bigint AS workspace_file_missing_size_count
SUM(${document.fileSize}::bigint) AS document_bytes
FROM ${document}
INNER JOIN ${knowledgeBase}
ON ${knowledgeBase.id} = ${document.knowledgeBaseId}
Expand All @@ -213,11 +196,6 @@ async function getExactWorkspaceStorageBytesBatch(
`)

for (const row of rows) {
if (parseExactBytes(row.workspace_file_missing_size_count, 'missing workspace file size') > 0) {
throw new Error(
`Workspace ${row.workspace_id} has files missing canonical size_bytes metadata`
)
}
const workspaceFileBytes = parseExactBytes(row.workspace_file_bytes, 'workspace file')
const documentBytes = parseExactBytes(row.document_bytes, 'knowledge document')
const total = workspaceFileBytes + documentBytes
Expand Down
13 changes: 2 additions & 11 deletions apps/sim/lib/uploads/shared/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,8 @@
*/
export const MAX_WORKSPACE_FILE_SIZE = 5 * 1024 * 1024 * 1024

/**
* Returns the canonical workspace-file byte size after the `size_bytes` cutover.
*
* The migration backfills every existing row before the new application image is
* promoted, and its compatibility trigger fills the column for writes from an old
* image during rollout. A null therefore indicates migration drift, not a legacy row.
*/
export function getWorkspaceFileSize(file: { sizeBytes: number | null }): number {
if (file.sizeBytes === null) {
throw new Error('Workspace file is missing canonical size_bytes metadata')
}
/** Returns a validated workspace-file byte size. */
export function getWorkspaceFileSize(file: { sizeBytes: number }): number {
if (!Number.isSafeInteger(file.sizeBytes) || file.sizeBytes < 0) {
throw new Error(`Invalid workspace file size: ${file.sizeBytes}`)
}
Expand Down
14 changes: 14 additions & 0 deletions packages/db/migrations/0309_contract_workspace_file_size.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
ALTER TABLE "workspace_files"
ADD CONSTRAINT "workspace_files_size_bytes_not_null_check"
CHECK ("size_bytes" IS NOT NULL) NOT VALID;--> statement-breakpoint
ALTER TABLE "workspace_files"
VALIDATE CONSTRAINT "workspace_files_size_bytes_not_null_check";--> statement-breakpoint
-- migration-safe: contract of #7112 and #7123 — application reads and writes use size_bytes, the backfill is complete, and this PR must merge only after the compatibility release fully drains
Comment on lines +5 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When an existing database has rows from before size_bytes was added, this validation aborts because the migration does not backfill NULLs. Add an idempotent update from the still-present non-null size column before validation so self-hosted upgrades do not depend on the removed runner.

(Based on your team's feedback about deployable schema/data migrations.)

View Feedback

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/migrations/0309_contract_workspace_file_size.sql, line 5:

<comment>When an existing database has rows from before `size_bytes` was added, this validation aborts because the migration does not backfill NULLs. Add an idempotent update from the still-present non-null `size` column before validation so self-hosted upgrades do not depend on the removed runner.

(Based on your team's feedback about deployable schema/data migrations.) </comment>

<file context>
@@ -0,0 +1,14 @@
+	ADD CONSTRAINT "workspace_files_size_bytes_not_null_check"
+	CHECK ("size_bytes" IS NOT NULL) NOT VALID;--> statement-breakpoint
+ALTER TABLE "workspace_files"
+	VALIDATE CONSTRAINT "workspace_files_size_bytes_not_null_check";--> statement-breakpoint
+-- migration-safe: contract of #7112 and #7123 — application reads and writes use size_bytes, the backfill is complete, and this PR must merge only after the compatibility release fully drains
+ALTER TABLE "workspace_files" ALTER COLUMN "size_bytes" SET NOT NULL;--> statement-breakpoint
</file context>
Suggested change
VALIDATE CONSTRAINT "workspace_files_size_bytes_not_null_check";--> statement-breakpoint
-- migration-safe: contract of #7112 and #7123 — application reads and writes use size_bytes, the backfill is complete, and this PR must merge only after the compatibility release fully drains
UPDATE "workspace_files"
SET "size_bytes" = "size"
WHERE "size_bytes" IS NULL;--> statement-breakpoint
ALTER TABLE "workspace_files"
VALIDATE CONSTRAINT "workspace_files_size_bytes_not_null_check";--> statement-breakpoint

ALTER TABLE "workspace_files" ALTER COLUMN "size_bytes" SET NOT NULL;--> statement-breakpoint
-- migration-safe: removes the temporary proof constraint created and validated above after PostgreSQL records the equivalent column-level NOT NULL invariant
ALTER TABLE "workspace_files"
DROP CONSTRAINT "workspace_files_size_bytes_not_null_check";--> statement-breakpoint
DROP TRIGGER IF EXISTS "workspace_files_sync_size_columns" ON "workspace_files";--> statement-breakpoint
DROP FUNCTION IF EXISTS "sync_workspace_file_size_columns"();--> statement-breakpoint
-- migration-safe: contract of #7112 and #7123 — no deployed application reader or writer depends on size, and this PR must merge only after the compatibility release fully drains
ALTER TABLE "workspace_files" DROP COLUMN "size";
Loading
Loading