Skip to content

fix(web): enforce the comment media quota atomically - #4165

Open
richiemcilroy wants to merge 8 commits into
mainfrom
fix/comment-media-quota-atomic
Open

richiemcilroy wants to merge 8 commits into
mainfrom
fix/comment-media-quota-atomic

Conversation

@richiemcilroy

@richiemcilroy richiemcilroy commented Nov 13, 2026 •

Copy link
Copy Markdown
Member

Comment media uploads land in the video owner's bucket, which can be their own S3 or Drive. /initiate capped 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 with SELECT ... FOR UPDATE in 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.
  • Reservations live in 6 hour windows, the lifetime of the comment media upload token. The check adds the current window and the two before it, so a reservation counts for at least 12 hours. That is longer than any token can be used, even when the bucket listing, the reservation and the token fall on different sides of a window boundary.
  • It reuses the existing media_processing_budgets counter table under a hashed comment-media key, so there is no schema change. Rows expire on their own and are removed by the existing recover-failed-video-processing cron.
  • A per-viewer limit now applies to initiates as well as finished comments: rl_comment_media_upload, 60 per hour per user.
  • The reservation runs after the content type check, so a refused type never takes a slot. Over-limit responses keep the existing 429 bodies.

Limits chosen

  • 400 objects per video is unchanged. An upload counts against it from the moment it starts, both while in flight and after it lands, until its reservation ages out.
  • 60 initiates per viewer per hour is twice the existing 30 media comments per hour, leaving room for retries after a failed upload.

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 /initiate route 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 --noEmit for apps/web: no new errors. The one error, in organization-settings-ownership.test.ts, is already on main.
  • __tests__/integration/comment-media-quota-database.test.ts drives the real /initiate handler 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 unless CAP_INFRA_TEST_DATABASE_URL (a local cap_infra_* database) and CAP_COMMENT_MEDIA_TEST_S3_ENDPOINT (a local bucket) are set, so CI is unaffected. Run locally against a scratch database and local MinIO:
    • A normal video comment with a thumbnail and a normal audio comment initiate, upload with PUT to the issued targets and land in the bucket (3 objects, sizes checked).
    • One viewer starts 30 media uploads on a video (the existing per-hour comment limit) and uploads each: none is refused.
    • With 398 objects already in the bucket, 8 viewers start a video with a thumbnail at the same moment: exactly one gets 200 and seven get the existing 429 "reached its media comment limit". With comment-media.ts from main and the same test, all eight get 200, so the test fails on main.
  • Whole apps/web unit 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.ts conflicted with main's rl_organization_create and rl_space_create. Both sides are kept; against main the file only adds COMMENT_MEDIA_UPLOAD and 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.

RetriggerView in GreptileConfidence 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.

  • Comment uploads count against one shared object cap per video.
  • Comment upload starts are limited to 60 per viewer each hour.

Reviews (4) · Last reviewed commit: "test(web): send comment media test uploa..." · Reviewed by Greptile

@capy-ai capy-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Capy found no issues.

View 1 other finding in Capy.

Open in Capy

Comment thread apps/web/lib/comment-media-quota.ts Resolved
@sandchest-com

sandchest-com Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

Sandchest · Sandchest couldn't finish reviewing 38e7b6d. Comment /sandchest review to try again; nothing was charged.

Details · earlier results on this pull request no longer apply to the latest commit.

@vercel

vercel Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
cap-status Skipped Skipped Nov 13, 2026 1:33pm UTC

@vercel
vercel Bot temporarily deployed to Preview – cap-status November 13, 2026 05:33 Inactive
@richiemcilroy

Copy link
Copy Markdown
Member Author

hey @greptileai, please re-review the PR

@capy-ai capy-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Capy found no issues.

View 2 other findings in Capy.

Open in Capy

@richiemcilroy

Copy link
Copy Markdown
Member Author

@greptileai review

@capy-ai capy-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Capy found no issues.

View 1 other finding in Capy.

Open in Capy

@vercel
vercel Bot temporarily deployed to Preview – cap-status November 13, 2026 13:33 Inactive
@richiemcilroy

Copy link
Copy Markdown
Member Author

@greptileai review

@capy-ai capy-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Capy found no issues.

View 1 other finding in Capy.

Open in Capy

This branch was successfully deployed

1 active and 1 inactive deployments
Preview – cap-web — 38e7b6d4 Deployed Nov 13, 2026 by vercel[bot]
Preview – cap-status — 38e7b6d4 Deployed Nov 13, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant