fix(uploads): direct-to-upload workspace files + shared transport (#4407)

* fix(uploads): direct-to-S3 workspace files + shared transport

* chore(testing): centralize posthog and storage-service mocks

* fix(uploads): address PR review — abort propagation, orphan cleanup, error handling

- Throw immediately on AbortError in KB retry loop (no useless 14s backoff)
- Cleanup S3/Blob object on quota or size-cap rejection in registerUploadedWorkspaceFile
- Enforce MAX_WORKSPACE_FILE_SIZE at registration (defense vs presigned PUT lying about size)
- Handle non-OK / non-JSON responses in workspace-files upload paths

* fix(uploads): add Zod contracts for workspace presigned/register routes

* fix(uploads): correct BlobServiceClient type name in headBlobObject

* fix(uploads): address PR review — typo, complete-failure cleanup, double-increment

* fix(uploads): preserve fallback size and reuse existing display name on re-register

* fix(uploads): surface server error message and bypass quota for local-storage fallback

* fix(uploads): align register response schema with UserFile; skip presigned for KB large files

- registerWorkspaceFileResponseSchema now matches the UserFile shape the route actually returns; previous schema required workspace DB-row fields that were never populated, causing requestJson validation to reject successful uploads.
- KB batch presigned fetch now skips files >= LARGE_FILE_THRESHOLD since multipart bypasses the per-file presigned URL anyway.

* fix(uploads): idempotent register skips duplicate audit/posthog; add edge-case tests

- registerUploadedWorkspaceFile now returns { file, created } so the route can skip captureServerEvent and recordAudit on idempotent re-register (existing metadata reused). Previously a re-register fired duplicate analytics + audit log entries.
- Add tests covering: idempotent re-register skips audit/analytics, isNetworkError matches econnreset/timeout/etc keywords, multipart complete failure fires action=abort cleanup.

* fix(uploads): include 50MiB boundary in batch presigned fetch

* fix(uploads): trust HEAD size to prevent quota inflation

The head.size > 0 fallback let a client PUT 0 bytes and register
with an inflated size, debiting quota without storing data. HEAD
on an existing object always returns the true byte count, so trust
it directly — a genuine 0-byte file correctly contributes 0.

* fix(uploads): audit verified file size, not client-supplied

* fix(uploads): handle register retries and name-collision races

Two bugs in registerUploadedWorkspaceFile:

1. Register retry could orphan storage. When a successful response
   was lost on the wire and the client retried, the quota check saw
   the bytes already counted, failed, and cleanupOrphan deleted the
   already-registered storage object — leaving the DB row pointing
   to nothing. Fix: check getFileMetadataByKey before quota guard
   and short-circuit on existing record.

2. Concurrent same-named uploads could lose data. allocateUniqueWorkspaceFileName
   is best-effort; two racing uploads can pass it and both attempt
   the same display name. The loser's insert hits 23505, the catch
   block called cleanupOrphan, and successfully-uploaded bytes
   were deleted. Fix: retry on 23505 with a fresh allocateUniqueWorkspaceFileName,
   matching the pattern in uploadWorkspaceFile. Throw FileConflictError
   after exhaustion.

* fix(uploads): retry transient DirectUploadErrors at outer KB level

The KB outer retry only triggered on isNetworkError, missing
transient 5xx from S3/Azure (DirectUploadError code
DIRECT_UPLOAD_ERROR or MULTIPART_ERROR). Adds isTransientUploadError
and retries on it, restoring resilience for small-file presigned
PUTs against flaky cloud storage.

* fix(uploads): only retry transient 5xx, not deterministic 4xx

DirectUploadError now carries the HTTP status. isTransientUploadError
gates on 5xx so callers don't loop on 400/403/404 (e.g., malformed
request, expired signature). Multipart per-part retry also short-circuits
on 4xx — same reasoning.

* refactor(uploads): collapse getFileContentType into resolveFileType

The two helpers differed only in whether application/octet-stream
falls back to the extension map. Add an option flag to resolveFileType
and keep getFileContentType as a thin wrapper for direct-PUT callers
that need to preserve the exact browser-reported content-type.

* chore(uploads): trim verbose comments

Drop inline comments that restate code ("Use the full storageKey as fileName"),
collapse a multi-line block comment into a tighter TSDoc on the existence
check, and prune verbose vitest file headers — describe blocks already
document what's tested.

* fix(uploads): regenerate fileId per insert retry; require cloud storage for register

* fix(uploads): cap formdata fallback at 100MB; drop unused size param

* fix(uploads): abort multipart on get-part-urls failure; retry register on transient errors

* fix(uploads): drop vestigial size field from register contract

* fix(uploads): abort multipart on complete-fetch throw

* fix(uploads): set kb presignedEndpoint fallback; race-safe blob HEAD

* fix(uploads): include ?type=knowledge-base on kb presigned fallback

* fix(uploads): remove abort listener on xhr timeout

* fix(uploads): add timeout/abort to kb api fallback upload
This commit is contained in:
Waleed
2026-05-02 17:59:40 -07:00
committed by GitHub
parent 64642d4f65
commit af55bad491
33 changed files with 2775 additions and 1186 deletions
+5 -1
View File
@@ -86,6 +86,8 @@ export {
} from './logging-session.mock'
// Permission mocks
export { permissionsMock, permissionsMockFns } from './permissions.mock'
// PostHog server mocks (for @/lib/posthog/server)
export { posthogServerMock, posthogServerMockFns } from './posthog-server.mock'
// Redis client mocks (for Redis client objects)
export { clearRedisMocks, createMockRedis, type MockRedis } from './redis.mock'
// Redis config mocks (for @/lib/core/config/redis)
@@ -106,8 +108,10 @@ export {
type MockSocket,
type MockSocketServer,
} from './socket.mock'
// Storage mocks
// Storage mocks (browser localStorage/sessionStorage)
export { clearStorageMocks, createMockStorage, setupGlobalStorageMocks } from './storage.mock'
// Storage service mocks (for @/lib/uploads/core/storage-service)
export { storageServiceMock, storageServiceMockFns } from './storage-service.mock'
// Stripe mocks
export {
createMockStripeEvent,
@@ -0,0 +1,30 @@
import { vi } from 'vitest'
/**
* Controllable mock functions for `@/lib/posthog/server`.
* All defaults are bare `vi.fn()` — configure per-test as needed.
*
* @example
* ```ts
* import { posthogServerMockFns } from '@sim/testing'
*
* expect(posthogServerMockFns.mockCaptureServerEvent).toHaveBeenCalledWith(...)
* ```
*/
export const posthogServerMockFns = {
mockCaptureServerEvent: vi.fn(),
mockGetPostHogClient: vi.fn(() => null),
}
/**
* Static mock module for `@/lib/posthog/server`.
*
* @example
* ```ts
* vi.mock('@/lib/posthog/server', () => posthogServerMock)
* ```
*/
export const posthogServerMock = {
captureServerEvent: posthogServerMockFns.mockCaptureServerEvent,
getPostHogClient: posthogServerMockFns.mockGetPostHogClient,
}
@@ -0,0 +1,47 @@
import { vi } from 'vitest'
/**
* Controllable mock functions for `@/lib/uploads/core/storage-service`.
* All defaults are bare `vi.fn()` — configure per-test as needed.
*
* @example
* ```ts
* import { storageServiceMockFns } from '@sim/testing'
*
* storageServiceMockFns.mockHasCloudStorage.mockReturnValue(true)
* storageServiceMockFns.mockGeneratePresignedUploadUrl.mockResolvedValue({
* uploadUrl: 'https://s3/test', key: 'workspace/x/y', ...
* })
* ```
*/
export const storageServiceMockFns = {
mockUploadFile: vi.fn(),
mockDownloadFile: vi.fn(),
mockDeleteFile: vi.fn(),
mockHeadObject: vi.fn(),
mockGeneratePresignedUploadUrl: vi.fn(),
mockGenerateBatchPresignedUploadUrls: vi.fn(),
mockGeneratePresignedDownloadUrl: vi.fn(),
mockHasCloudStorage: vi.fn(() => false),
mockGetS3InfoForKey: vi.fn(),
}
/**
* Static mock module for `@/lib/uploads/core/storage-service`.
*
* @example
* ```ts
* vi.mock('@/lib/uploads/core/storage-service', () => storageServiceMock)
* ```
*/
export const storageServiceMock = {
uploadFile: storageServiceMockFns.mockUploadFile,
downloadFile: storageServiceMockFns.mockDownloadFile,
deleteFile: storageServiceMockFns.mockDeleteFile,
headObject: storageServiceMockFns.mockHeadObject,
generatePresignedUploadUrl: storageServiceMockFns.mockGeneratePresignedUploadUrl,
generateBatchPresignedUploadUrls: storageServiceMockFns.mockGenerateBatchPresignedUploadUrls,
generatePresignedDownloadUrl: storageServiceMockFns.mockGeneratePresignedDownloadUrl,
hasCloudStorage: storageServiceMockFns.mockHasCloudStorage,
getS3InfoForKey: storageServiceMockFns.mockGetS3InfoForKey,
}