Chore/refactor and rate limit - #14
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if user_id is not None: | ||
| await self._enforce_daily_upload_quota( | ||
| user_id=user_id, bytes_added=len(data) | ||
| ) |
There was a problem hiding this comment.
Daily upload quota is bumped before any storage or database work and never rolled back. _enforce_daily_upload_quota runs as soon as the file is read, but if write_bytes_async, the DB insert/commit, or downstream processing fails, the Redis counter remains incremented and the user loses daily quota even though no asset was stored. This can lock users out after transient upload errors; defer the increment until after successful persistence or wrap later steps in try/except to decrement on failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex can you backup this expected edge case with a test to prove your point? Or will the redis stubs make it hard to do so?
There was a problem hiding this comment.
Summary
- Added rollback handling around asset upload persistence so daily upload quota counters are reversed if downstream storage or database work fails after incrementing the Redis counter. app/services/assets.pyL116-L336
- Added regression test that simulates a storage failure and asserts the per-user upload quota is not consumed when the upload aborts. tests/test_assets.pyL42-L99
Testing
⚠️ pytest tests/test_assets.py -q(fails: Postgres not running in container)
There was a problem hiding this comment.
@codex That was a good call.
I had to copy the patch and propagate the failure with:
except Exception as exc:
if quota_charged and user_id is not None:
await self._rollback_daily_upload_quota(
user_id=user_id, bytes_removed=quota_bytes
)
if isinstance(exc, HTTPException):
raise
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail="Upload failed, please retry",
) from exc
Just as personal feedback. I am able to run tests in CI github environment by using simple commands that I have prepared like setting up the repo with poetry install --with dev and make db-up and make test-auto.
What is blocking you from doing that so that I can fix it?
There was a problem hiding this comment.
Summary
- Added quota rollback and database rollback around asset uploads so failed storage or database operations do not consume a user’s daily upload allowance and return a consistent 500 error. app/services/assets.pyL116-L211
- Added a regression test that simulates a storage failure and verifies the upload quota remains available for a subsequent retry. tests/test_assets.pyL40-L101
Testing
⚠️ pytest tests/test_assets.py::test_upload_rolls_back_quota_on_failure -q(fails: Postgres not running locally)
1 participant
that unauthenticated routes behave as expected.