Repository navigation
fix(web): enforce the comment media quota atomically - #4165
Open
richiemcilroy wants to merge 8 commits into
Open
richiemcilroy wants to merge 8 commits into
richiemcilroy wants to merge 8 commits into
Conversation
|
Sandchest · Sandchest couldn't finish reviewing Details · earlier results on this pull request no longer apply to the latest commit. |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Member
Author
|
hey @greptileai, please re-review the PR |
added 3 commits
November 13, 2026 08:14
…ta-atomic # Conflicts: # apps/web/lib/rate-limit.ts
Member
Author
|
@greptileai review |
Member
Author
|
@greptileai review |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comment media uploads land in the video owner's bucket, which can be their own S3 or Drive.
/initiatecapped each video at 400 comment media objects, but it counted only objects already in the bucket. Several initiates sent at the same moment all saw the same count, so together they could hand out more upload targets than the cap allows. Uploads that had been started but not finished were not counted either.Fix
reserveCommentMediaObjects(apps/web/lib/comment-media-quota.ts) reserves the objects an upload will create (1, or 2 with a thumbnail) before any upload target is issued. It upserts a counter row and takes it withSELECT ... FOR UPDATEin one transaction, so concurrent initiates for a video run one after another. A request is refused when objects in the bucket plus reserved objects plus its own would pass 400.media_processing_budgetscounter table under a hashedcomment-mediakey, so there is no schema change. Rows expire on their own and are removed by the existingrecover-failed-video-processingcron.rl_comment_media_upload, 60 per hour per user.Limits chosen
Why normal use is unaffected
Normal videos carry a handful of media comments, far from 400 even when a finished upload is counted twice, once in the bucket and once in its reservation. A viewer who stays within the existing 30 media comments per hour never reaches 60 initiates. The upload, presign, complete and abort paths are unchanged.
Tests
comment-media-quota.test.ts: a fake database that enforces row locks. 500 concurrent initiates get exactly 400 slots, and 300 initiates with thumbnails get exactly 200. Stored objects count. A reservation taken at the last moment of a window still counts after its token expires and is free three windows later. Videos are independent. A control run with the lock disabled lets more than 400 through, which shows the test depends on the lock.comment-media-initiate-limits.test.ts: the/initiateroute reserves 2 objects for a video with a thumbnail and 1 for audio. It answers 429 with the existing messages when the video is full or the viewer is rate limited, issues no upload target in either case, and reserves nothing for an unsupported content type.tsc --noEmitforapps/web: no new errors. The one error, inorganization-settings-ownership.test.ts, is already on main.__tests__/integration/comment-media-quota-database.test.tsdrives the real/initiatehandler against a real MySQL database and a real S3-compatible bucket. Only auth, the video lookup and the plan check are stubbed; the comment throttle, the rate limiter, the reservation and the bucket listing run for real. It skips unlessCAP_INFRA_TEST_DATABASE_URL(a localcap_infra_*database) andCAP_COMMENT_MEDIA_TEST_S3_ENDPOINT(a local bucket) are set, so CI is unaffected. Run locally against a scratch database and local MinIO:comment-media.tsfrom main and the same test, all eight get 200, so the test fails on main.apps/webunit suite (npx vitest run, local Node 26): this branch 9553 passed, 17 failed, 241 skipped; main at 18d84db 9541 passed, 17 failed, 238 skipped. The 17 failures are the same tests in the same 8 files on both (local Node 26 and environment noise: share-stage-identity, sso-login-pages, linkified-text and others). The difference is the 12 new unit tests and the 3 database tests skipped without their environment.Merge with main
lib/rate-limit.tsconflicted with main'srl_organization_createandrl_space_create. Both sides are kept; against main the file only addsCOMMENT_MEDIA_UPLOADand its fallback (60 per hour). No existing export or limit changes.Rollout
Optional: add a Vercel Firewall rate limit rule
rl_comment_media_upload(60 per hour, keyed). Until it exists, the in-process fallback applies the same number on each instance.

Confidence Score: 5/5
The PR appears safe to merge; no new actionable issues were found.Summary
The PR reserves comment media slots before issuing upload targets and adds a per-viewer upload limit.
Reviews (4) · Last reviewed commit: "test(web): send comment media test uploa..." · Reviewed by Greptile