Skip to content

Commit 9a96054

Browse files
committed
fix(uploads): commit local objects atomically, so "it exists" means "it finished"
headObject now answers local storage from `stat` instead of reporting every object as missing. That is the honest answer and the byte-range paths need it, but it also brought ten existing callers to life on self-hosted and dev deployments — including three that read "the object is there" as "the copy finished" and skip the work: the workspace fork copier, the KB document copier, and the table snapshot cache. Those guards were written against cloud backends, where an interrupted PUT leaves no object at all. The local path was a bare `writeFile`, which truncates before it writes, so a crash mid-write would have left a prefix under a key those callers now treat as complete — a fork retry marking a torn file copied. Writes now land on a sibling `.partial` path and are renamed onto the target; `rename` within a directory is atomic on POSIX and replaces in one step, so the key names either the previous bytes or the whole new ones. A failed write or rename removes the temp file and rethrows. Local multipart accumulates and commits through this same path, so snapshots are covered too. Reviewed the other seven callers: all strictly improve. materialize-file now charges quota against the real on-disk size rather than trusting the row, the CSV import progress bar is no longer permanently indeterminate on local, and the TikTok size probe skips its counting pass. Their comments claimed local returns null; refreshed. Tests assert the call sequence rather than an observed torn read — a timing test passes against a plain `writeFile` too, since even a multi-megabyte write resolves inside one macrotask, so it would be a test that cannot fail. Confirmed all four go red when the temp-and-rename is reverted.
1 parent 52e43bb commit 9a96054

4 files changed

Lines changed: 149 additions & 7 deletions

File tree

apps/sim/app/api/tools/tiktok/upload-video-draft/upload.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,9 @@ async function countStoredFileBytes(options: StoredFileOptions): Promise<number>
138138
}
139139

140140
/**
141-
* Resolves the authoritative object size. Cloud storage uses provider metadata; local storage
142-
* and providers without HEAD support are counted with a bounded, zero-accumulation pass.
141+
* Resolves the authoritative object size. Cloud storage uses provider metadata and local
142+
* storage stats the file; a provider that reports no size at all falls back to a bounded,
143+
* zero-accumulation counting pass.
143144
*/
144145
export async function getStoredVideoSize(options: StoredFileOptions): Promise<number> {
145146
throwIfAborted(options.signal)

apps/sim/lib/table/import-runner.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,9 @@ export async function runTableImport(payload: TableImportPayload): Promise<void>
129129
return fresh ?? undefined
130130
}
131131

132-
// Total byte size for the progress estimate — a cheap HEAD, no download. May be null on
133-
// the local dev provider, in which case the bar stays indeterminate (rows still show).
132+
// Total byte size for the progress estimate — a cheap HEAD, no download. Every backend
133+
// answers it, including local storage (which stats the file), so the bar is real; a
134+
// genuinely absent object still yields 0 and leaves it indeterminate.
134135
const totalBytes = (await headObject(fileKey, 'workspace'))?.size ?? 0
135136

136137
// Stream the file rather than buffering it — a ~1M-row import must never be held in memory.
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/**
2+
* The local-filesystem backend must commit an object atomically, the way every
3+
* cloud backend does.
4+
*
5+
* `headObject` answers local storage from `stat` rather than reporting it as
6+
* missing, so callers that read "the object is there" as "the copy finished" —
7+
* the workspace fork copier, the KB document copier, the table snapshot cache —
8+
* are live on self-hosted and dev deployments. That inference is only sound if a
9+
* half-written file can never appear under the final key, which means the bytes
10+
* must land on a sibling path and be renamed over the target: `rename` within a
11+
* directory is atomic on POSIX, `writeFile` truncates first and is not.
12+
*
13+
* The assertion is on the call sequence rather than on an observed torn read.
14+
* A timing-based test here passes against a plain `writeFile` too — the write of
15+
* even a multi-megabyte buffer resolves well inside one macrotask — so it would
16+
* be a test that cannot fail. This one fails the moment the temp-and-rename is
17+
* replaced by a direct write.
18+
*
19+
* @vitest-environment node
20+
*
21+
* Under `isolate: false` the storage-service module may already be cached and
22+
* bound to the real `@/lib/uploads/config` namespace, so a per-file `vi.mock` of
23+
* that path would never reach it. This file patches the real namespace in place
24+
* (the `USE_*` flags are value exports read at call time) and restores it after,
25+
* matching `storage-service.blob-connection-string.test.ts`.
26+
*/
27+
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
28+
29+
const { fs } = vi.hoisted(() => ({
30+
fs: {
31+
writeFile: vi.fn(async () => {}),
32+
rename: vi.fn(async () => {}),
33+
mkdir: vi.fn(async () => undefined),
34+
rm: vi.fn(async () => {}),
35+
stat: vi.fn(async () => ({ size: 11, isFile: () => true })),
36+
readFile: vi.fn(async () => Buffer.alloc(0)),
37+
unlink: vi.fn(async () => {}),
38+
readdir: vi.fn(async () => []),
39+
access: vi.fn(async () => {}),
40+
},
41+
}))
42+
43+
vi.mock('fs/promises', () => ({ ...fs, default: fs }))
44+
vi.mock('node:fs/promises', () => ({ ...fs, default: fs }))
45+
46+
import * as uploadsConfig from '@/lib/uploads/config'
47+
import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server'
48+
import { uploadFile } from '@/lib/uploads/core/storage-service'
49+
50+
const KEY = 'workspace/ws-1/doc.txt'
51+
const TARGET = `${UPLOAD_DIR_SERVER}/${KEY}`
52+
53+
const CLOUD_FLAGS = ['USE_S3_STORAGE', 'USE_BLOB_STORAGE', 'USE_GCS_STORAGE'] as const
54+
const originalFlags = new Map(
55+
CLOUD_FLAGS.map((flag) => [flag, uploadsConfig[flag] as boolean] as const)
56+
)
57+
58+
function setCloudFlags(value: boolean) {
59+
for (const flag of CLOUD_FLAGS) {
60+
Object.defineProperty(uploadsConfig, flag, { value, configurable: true })
61+
}
62+
}
63+
64+
beforeEach(() => {
65+
vi.clearAllMocks()
66+
setCloudFlags(false)
67+
})
68+
69+
afterAll(() => {
70+
for (const [flag, value] of originalFlags) {
71+
Object.defineProperty(uploadsConfig, flag, { value, configurable: true })
72+
}
73+
})
74+
75+
function upload() {
76+
return uploadFile({
77+
file: Buffer.from('hello world', 'utf8'),
78+
fileName: 'doc.txt',
79+
contentType: 'text/plain',
80+
context: 'workspace',
81+
customKey: KEY,
82+
preserveKey: true,
83+
})
84+
}
85+
86+
describe('local filesystem uploads', () => {
87+
it('writes to a sibling temp path and renames it onto the target', async () => {
88+
await upload()
89+
90+
expect(fs.writeFile).toHaveBeenCalledTimes(1)
91+
const [writtenPath] = fs.writeFile.mock.calls[0] as [string]
92+
expect(writtenPath).toMatch(new RegExp(`^${TARGET}\\.[A-Za-z0-9_-]+\\.partial$`))
93+
expect(fs.rename).toHaveBeenCalledWith(writtenPath, TARGET)
94+
})
95+
96+
it('never writes directly to the key readers resolve', async () => {
97+
await upload()
98+
99+
const targets = fs.writeFile.mock.calls.map(([path]) => path)
100+
expect(targets).not.toContain(TARGET)
101+
})
102+
103+
it('removes the temp file and surfaces the error when the write fails', async () => {
104+
const failure = new Error('ENOSPC: no space left on device')
105+
fs.writeFile.mockRejectedValueOnce(failure)
106+
107+
await expect(upload()).rejects.toThrow(failure)
108+
109+
const [writtenPath] = fs.writeFile.mock.calls[0] as [string]
110+
expect(fs.rename).not.toHaveBeenCalled()
111+
expect(fs.rm).toHaveBeenCalledWith(writtenPath, { force: true })
112+
})
113+
114+
it('removes the temp file when the rename itself fails', async () => {
115+
fs.rename.mockRejectedValueOnce(new Error('EXDEV: cross-device link'))
116+
117+
await expect(upload()).rejects.toThrow('EXDEV')
118+
119+
const [writtenPath] = fs.writeFile.mock.calls[0] as [string]
120+
expect(fs.rm).toHaveBeenCalledWith(writtenPath, { force: true })
121+
})
122+
})

apps/sim/lib/uploads/core/storage-service.ts

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { Readable } from 'node:stream'
22
import { randomBytes } from 'crypto'
33
import { createLogger } from '@sim/logger'
44
import { getErrorMessage } from '@sim/utils/errors'
5-
import { generateId } from '@sim/utils/id'
5+
import { generateId, generateShortId } from '@sim/utils/id'
66
import { assertKnownSizeWithinLimit } from '@/lib/core/utils/stream-limits'
77
import {
88
getStorageConfig,
@@ -210,7 +210,7 @@ export async function uploadFile(options: UploadFileOptions): Promise<FileInfo>
210210
return uploadResult
211211
}
212212

213-
const { writeFile, mkdir } = await import('fs/promises')
213+
const { writeFile, mkdir, rename, rm } = await import('fs/promises')
214214
const { join, dirname } = await import('path')
215215
const { UPLOAD_DIR_SERVER } = await import('./setup.server')
216216

@@ -220,7 +220,25 @@ export async function uploadFile(options: UploadFileOptions): Promise<FileInfo>
220220

221221
await mkdir(dirname(filesystemPath), { recursive: true })
222222

223-
await writeFile(filesystemPath, file)
223+
/**
224+
* Write to a sibling temp file, then rename over the target.
225+
*
226+
* Every cloud backend commits an object atomically — an interrupted PUT leaves
227+
* nothing behind — and callers rely on that: the fork copier and the KB
228+
* document copier both treat "the object is there" as "the copy finished" and
229+
* skip re-copying. A bare `writeFile` here would let a crash mid-write leave a
230+
* truncated file that satisfies that check forever. `rename` within the same
231+
* directory is atomic on POSIX and replaces the target in one step, so the key
232+
* either names the previous bytes or the complete new ones and never a prefix.
233+
*/
234+
const pendingPath = `${filesystemPath}.${generateShortId(12)}.partial`
235+
try {
236+
await writeFile(pendingPath, file)
237+
await rename(pendingPath, filesystemPath)
238+
} catch (error) {
239+
await rm(pendingPath, { force: true }).catch(() => {})
240+
throw error
241+
}
224242

225243
if (metadata && persistMetadata) {
226244
await insertFileMetadataHelper(

0 commit comments

Comments
 (0)