fix(files): cache TUS upload access checks#2258
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds a caching layer for TUS upload write-access decisions to avoid repeated authorization checks, updates the stats endpoint to unconditionally allow deleted app versions in fallback scenarios, optimizes a database lookup with a new index, and includes comprehensive test coverage for both features. ChangesAttachment Write Access Caching
Stats Endpoint Deleted Version Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.1.0)supabase/migrations/20260513093535_app_versions_r2_path_index.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/files-upload-access-cache.unit.test.ts`:
- Around line 83-97: The beforeEach and afterEach hooks are out of conventional
order in the test file; move the beforeEach block (which clears mocks and sets
up getPgClientMock, getUserIdFromApikeyMock, getAppByAppIdPgMock,
getAppByIdPgMock, checkPermissionPgMock) so it appears before the afterEach
block that restores globalThis.caches, ensuring hooks read: beforeEach(...) then
afterEach(...); update only the ordering around the beforeEach and afterEach
symbols in tests/files-upload-access-cache.unit.test.ts.
- Around line 11-13: The test uses the global Buffer in encodeMetadataValue
which ESLint flags; fix it by adding an explicit import from the buffer module
(import { Buffer } from 'buffer') at the top of the file and keep using Buffer
inside encodeMetadataValue so the symbol is resolved from the import rather than
the global; update any other test helpers in the same file that reference Buffer
to use the imported symbol as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ff99e34-9460-44cc-a7e6-88e522549155
📒 Files selected for processing (5)
supabase/functions/_backend/files/files.tssupabase/functions/_backend/plugins/stats.tssupabase/migrations/20260513093535_app_versions_r2_path_index.sqltests/files-upload-access-cache.unit.test.tstests/stats.test.ts
| function encodeMetadataValue(value: string) { | ||
| return Buffer.from(value).toString('base64') | ||
| } |
There was a problem hiding this comment.
Import Buffer explicitly instead of using the global.
ESLint flags using the global Buffer variable. Import it explicitly for clarity and Node.js module compatibility.
Proposed fix
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { Buffer } from 'node:buffer'
const fileId = 'orgs/test-org/apps/test-app/cache-test.zip'🧰 Tools
🪛 ESLint
[error] 12-12: Unexpected use of the global variable 'Buffer'. Use 'require("buffer").Buffer' instead.
(node/prefer-global/buffer)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/files-upload-access-cache.unit.test.ts` around lines 11 - 13, The test
uses the global Buffer in encodeMetadataValue which ESLint flags; fix it by
adding an explicit import from the buffer module (import { Buffer } from
'buffer') at the top of the file and keep using Buffer inside
encodeMetadataValue so the symbol is resolved from the import rather than the
global; update any other test helpers in the same file that reference Buffer to
use the imported symbol as well.
| afterEach(() => { | ||
| globalThis.caches = originalCaches | ||
| }) | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| getPgClientMock.mockReturnValue({ | ||
| end: vi.fn(() => Promise.resolve()), | ||
| query: vi.fn(async () => ({ rows: [] })), | ||
| }) | ||
| getUserIdFromApikeyMock.mockResolvedValue('00000000-0000-0000-0000-000000000001') | ||
| getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) | ||
| getAppByIdPgMock.mockResolvedValue({ plan_valid: true }) | ||
| checkPermissionPgMock.mockResolvedValue(true) | ||
| }) |
There was a problem hiding this comment.
Place beforeEach before afterEach to follow testing conventions.
ESLint's test/prefer-hooks-in-order rule expects beforeEach hooks to appear before afterEach hooks for readability and conventional ordering.
Proposed fix: swap hook order
describe('files upload access cache', () => {
- afterEach(() => {
- globalThis.caches = originalCaches
- })
-
beforeEach(() => {
vi.clearAllMocks()
getPgClientMock.mockReturnValue({
end: vi.fn(() => Promise.resolve()),
query: vi.fn(async () => ({ rows: [] })),
})
getUserIdFromApikeyMock.mockResolvedValue('00000000-0000-0000-0000-000000000001')
getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' })
getAppByIdPgMock.mockResolvedValue({ plan_valid: true })
checkPermissionPgMock.mockResolvedValue(true)
})
+ afterEach(() => {
+ globalThis.caches = originalCaches
+ })
+
it('reuses cached TUS write access for repeated HEAD checks', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(() => { | |
| globalThis.caches = originalCaches | |
| }) | |
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| getPgClientMock.mockReturnValue({ | |
| end: vi.fn(() => Promise.resolve()), | |
| query: vi.fn(async () => ({ rows: [] })), | |
| }) | |
| getUserIdFromApikeyMock.mockResolvedValue('00000000-0000-0000-0000-000000000001') | |
| getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) | |
| getAppByIdPgMock.mockResolvedValue({ plan_valid: true }) | |
| checkPermissionPgMock.mockResolvedValue(true) | |
| }) | |
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| getPgClientMock.mockReturnValue({ | |
| end: vi.fn(() => Promise.resolve()), | |
| query: vi.fn(async () => ({ rows: [] })), | |
| }) | |
| getUserIdFromApikeyMock.mockResolvedValue('00000000-0000-0000-0000-000000000001') | |
| getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) | |
| getAppByIdPgMock.mockResolvedValue({ plan_valid: true }) | |
| checkPermissionPgMock.mockResolvedValue(true) | |
| }) | |
| afterEach(() => { | |
| globalThis.caches = originalCaches | |
| }) |
🧰 Tools
🪛 ESLint
[error] 87-97: beforeEach hooks should be before any afterEach hooks
(test/prefer-hooks-in-order)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/files-upload-access-cache.unit.test.ts` around lines 83 - 97, The
beforeEach and afterEach hooks are out of conventional order in the test file;
move the beforeEach block (which clears mocks and sets up getPgClientMock,
getUserIdFromApikeyMock, getAppByAppIdPgMock, getAppByIdPgMock,
checkPermissionPgMock) so it appears before the afterEach block that restores
globalThis.caches, ensuring hooks read: beforeEach(...) then afterEach(...);
update only the ordering around the beforeEach and afterEach symbols in
tests/files-upload-access-cache.unit.test.ts.
|
|
The allowed-write cache looks like it can bypass the ready-bundle immutability guard during the 60s TTL.
That seems to weaken the new ready-bundle guard exactly when the state changes from upload-in-progress to finalized. I would avoid caching |
2 participants
Summary (AI generated)
app_versions.r2_pathindex that Supabase recommended for the TUS/files ready-bundle guard.HEAD/PATCHcalls do not repeat the same DB work during an active upload./statsfallback version lookup so a deletedunknownplaceholder can be reused instead of repeatedly scheduling no-op placeholder inserts.unknownstats fallback.Motivation (AI generated)
Supabase reported heavy production volume from the ready-bundle
app_versions.r2_pathlookup. That lookup comes fromcheckWriteAppAccessin the files/TUS upload route. The repeated placeholder insert comes from/statsfalling back tounknownwith the wrong deleted filter, which can miss the placeholder and scheduleensurePlaceholderVersionsagain on every request.Business Impact (AI generated)
This reduces unnecessary database pressure on hot upload and stats paths, keeps the production index tracked in migrations, and lowers the risk of DB saturation from resumable-upload loops or stats traffic. Upload integrity stays intact because TUS create requests still perform the full primary DB validation and only repeated chunk/progress checks reuse a short-lived exact-match cache.
Test Plan (AI generated)
bun lintbun lint:backendbun test tests/files-upload-access-cache.unit.test.tsbun test tests/stats.test.ts --test-timeout 20000bun run cli:build && vue-tsc --noEmitbunx supabase migration list --localblocked because local Supabase Postgres is not running on127.0.0.1:54322in this worktree.Generated with AI
Summary by CodeRabbit
Performance Improvements
Bug Fixes