fix(requests): enforce the quota when editing a request - #3378
Conversation
|
Important Review skippedThe saved review history does not include the base for the last reviewed commit. This saved history cannot establish the base for an incremental review. Comment You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe request handlers now serialize updates, deletion, retries, and status changes with request locks. PUT updates enforce movie and TV quotas during edits and reassignment. Tests cover quota rules, concurrency, override services, and TV season restoration. ChangesRequest update enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant RequestRoute
participant RequestLock
participant RequestStore
participant RadarrSonarr
Caller->>RequestRoute: Update or retry request
RequestRoute->>RequestLock: Acquire request lock
RequestLock->>RequestStore: Reload and validate request
RequestStore-->>RequestRoute: Current request state
RequestRoute->>RequestStore: Save authorized mutation
RequestRoute->>RadarrSonarr: Dispatch approved retry
RequestRoute-->>Caller: Return updated request
Merge Risk: ⚪ Minimal · up to Request edits now enforce quotas safely, while concurrent lifecycle actions are serialized so stale updates do not overwrite one another. The covered request and quota changes are ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The quota enforcement changes are in scope for issue Resolution Remove the unrelated request-lifecycle concurrency changes, or link separate issues that justify request creation, approval, decline, retry, and delete serialization. Keep the quota enforcement and edit-request locking changes in this pull request. A rabbit locks each request tight, Comment |
455c4c4 to
62f6a45
Compare
fallenbagel
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the /request/:requestId PUT handler to enforce quota constraints correctly (including during reassignment) and to serialize quota-sensitive edits via a per-user lock, with accompanying test coverage for quota edge cases.
Changes:
- Wrap request edits in a user-keyed
requestLock.dispatch(...)to prevent concurrent quota/race issues during PUT updates. - Add quota enforcement for TV season edits (delta vs full charge on reassignment) and for movie reassignment when the target user is at quota.
- Refactor and expand request route tests with seed helpers and new quota-focused test cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/routes/request.ts |
Adds user-keyed locking and new quota/reassignment checks in the PUT request update flow. |
server/routes/request.test.ts |
Adds seed helpers and introduces quota-specific PUT tests for TV seasons and reassignment scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
62f6a45 to
ba51a06
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ba51a06 to
ff14bb2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ff14bb2 to
0624bb8
Compare
0624bb8 to
7594577
Compare
fa9c79b to
9f0c3aa
Compare
9f0c3aa to
f89be60
Compare
f89be60 to
5ae8b52
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
9d79f82 to
a665fc7
Compare
|
Do not merge from this PR. Merge from #3380 once all upstack PRs are approved to avoid dismissal of approvals. |
a665fc7 to
9ba1d58
Compare
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Editing a request never checked the requester's quota, so a user could add seasons past their limit on an existing request and an admin could move a request onto a user with no room for it. An edit is charged for every season getQuota did not already count against the owner, so reassignment and a request older than the quota window pay in full while an ordinary edit pays the difference. The handler serializes on the request id from the url so both parties to a reassignment queue behind each other, then takes the owner lock around the quota check and save. Fixes #633
The lock that serializes request creation is keyed on requestBody.userId, which only callers who may reassign a request are allowed to set, but the check enforcing that runs inside the lock. Any unauthenticated caller could name a userId and queue on that user's key before being rejected. Although today, the rejection throws before the callback awaits anything, so the lock is held for a microtask with no query behind it, the key should still follow the same permission as the field it comes from, so it is now the caller's own id unless they may set the other one.
Each of these read the request, check its status, then saved the result, with nothing holding the row across the three steps. Two calls landing together both read the same pre-transition state and both passed their guard, so an approve and a decline could both return 200, the later save quietly overwriting the other while both notification hooks fired.
9ba1d58 to
a41e38c
Compare
github-actions
Bot
removed
the
merge conflict
Description
Editing a request never checked the requester's quota at all. A user could add seasons beyond their limit on a request that already existed, which is #633, and an admin could move a request onto a user with no room for it.
An edit is charged for every season
getQuotadid not already count against the owner. Reassignment pays in full because none of the request's seasons count towards the new owner yet, a declined request pays in full because declined requests are excluded from the count, and a request older than the quota window pays in full because the window filters on when the request was created. An ordinary edit inside the window pays only the difference. Requests created with the quota bypass keep it, and users whose quota is unlimited are unaffected.The handler serializes on the request id from the url so both parties to a reassignment queue behind each other, then takes the owner lock from the PR below around the quota check and save. The prefix on that first key is load bearing, since a request id and a user id would otherwise stringify to the same key on the same lock instance.
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Bug Fixes