Harden the platform against deploy races and request storms#901
Merged
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (41)
📝 WalkthroughWalkthroughThis PR reduces the maximum pagination page size from 100 to 50 across backend and frontend, adds strict integer validation for the mission ChangesPagination limits and mission filter validation
Advisory locking for migrations and leaderboard ranks
Frontend request dedup and reCAPTCHA
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Startup as startup.sh
participant Command as migrate_with_lock
participant DB as PostgreSQL
Startup->>Command: run migrate_with_lock --noinput
Command->>DB: ensure_connection()
alt vendor is postgresql
Command->>DB: SET lock_timeout = '900s'
Command->>DB: SELECT pg_advisory_lock(namespace, key)
Command->>DB: SET lock_timeout = 0
Command->>Command: run migrate
else non-postgres
Command->>Command: run migrate without lock
end
sequenceDiagram
participant Component
participant apiJs as api.js
participant Backend
Component->>apiJs: getAllMissions(params)
apiJs->>Backend: GET /missions/?page=1&page_size=50
Backend-->>apiJs: results, next
loop while next is not null
apiJs->>Backend: GET /missions/?page=N&page_size=50
Backend-->>apiJs: results, next
end
apiJs-->>Component: { data: aggregated results }
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Database migrations and leaderboard rank updates now run under PostgreSQL advisory locks so concurrent App Runner instances can no longer race each other during deploys or contribution bursts; rank updates also iterate leaderboards in sorted order to keep lock acquisition deadlock-free. Deploys can skip startup migrations via a RUN_MIGRATIONS_ON_STARTUP switch. API page sizes are capped at 50, and the mission contribution_type filter rejects non-numeric values instead of erroring server-side. The frontend stops amplifying load: catalog-style endpoints (missions, contribution types, partners) are fetched through a bounded paginated helper instead of oversized page_size requests, metrics endpoints share a 30-second cache, session refresh / user load / what's-new refresh each collapse concurrent callers into one in-flight request, session refresh pauses in hidden tabs, and literal "undefined"/"null" query params are stripped before hitting the API. - backend/utils/management/commands/migrate_with_lock.py: New management command wrapping migrate in a pg_try_advisory_lock poll loop (timeout/poll configurable via flags or env); no-ops to plain migrate on non-Postgres. - backend/startup.sh: Uses migrate_with_lock and honors RUN_MIGRATIONS_ON_STARTUP (default true). - backend/deploy-apprunner.sh, backend/deploy-apprunner-dev.sh: Pass RUN_MIGRATIONS_ON_STARTUP through to App Runner env vars. - backend/github-actions-role-setup.sh: create-role is now idempotent (updates trust policy when the role exists). - backend/leaderboard/models.py: update_leaderboard_ranks takes a pg_advisory_xact_lock keyed by leaderboard type inside a transaction; callers iterate leaderboard types sorted; recalculate_all_leaderboards ranks all configured leaderboards instead of a hardcoded list. - backend/utils/pagination.py: max_page_size 100 -> 50. - backend/contributions/views.py: MissionViewSet validates contribution_type query param, returning 400 on junk like "undefined". - frontend/src/lib/api.js: Adds sanitizeRequestParams request interceptor, fetchSmallCatalogPages helper with getAllContributionTypes/getAllMissions/partnersAPI.listAll, and a shared 30s metrics request cache for overview + network-activity. - frontend/src/lib/auth.js: refreshSession single-flight; periodic refresh skips hidden tabs. - frontend/src/lib/userStore.js, frontend/src/lib/whatsNewStore.js, frontend/src/lib/missionsStore.js: loadUser/refresh single-flight; missions store uses getAllMissions. - frontend/src/components/*, frontend/src/routes/*: Swapped page_size=100/200/1000 requests for the bounded fetch-all helpers or page_size<=50. - backend/*/tests/*, frontend/src/tests/*: Updated for the 50 max page size and new helpers; new tests for migrate_with_lock, the param sanitizer, auth session refresh, and whats-new store dedup.
The contribution submit, edit, and selection flows now load contribution types through the bounded fetch-all helper instead of raw page_size requests. The submit form recovers the reCAPTCHA token after expiry by reading it back from the widget rather than failing with a stale one, resets it on widget expiry/error, and keeps reCAPTCHA prompts out of the global error banner. Deploy scripts no longer inject a migrations toggle into App Runner; startup keeps the RUN_MIGRATIONS_ON_STARTUP switch (default on) around the advisory-locked migrate, and the GitHub Actions role setup returns to plain role creation. - frontend/src/components/portal/submit-contribution/SubmitContribution.svelte: getAllContributionTypes; expired/error callbacks reset recaptchaToken; getRecaptchaToken() falls back to grecaptcha.getResponse; reCAPTCHA errors excluded from the banner. - frontend/src/lib/components/ContributionSelection.svelte, frontend/src/routes/EditSubmission.svelte: getAllContributionTypes instead of paginated getContributionTypes. - backend/deploy-apprunner.sh, backend/deploy-apprunner-dev.sh: reverted APP_RUNNER_RUN_MIGRATIONS_ON_STARTUP plumbing; startup.sh documents setting RUN_MIGRATIONS_ON_STARTUP=false once deploy-time migrations exist. - backend/github-actions-role-setup.sh: reverted to plain create-role. - backend/CLAUDE.md, CHANGELOG.md: dropped the extra doc bullets and changelog entry.
Page-size dropdowns no longer offer 100: the server caps pages at 50, so picking 100 silently halved the reachable rows because total pages were computed from the requested size. The submissions, steward submissions, and Discord XP tables now stop at 50, as does the Pagination component default. Dead code flagged in review is removed: the unpaginated contribution-type, mission, and partner list helpers (all callers use the bounded fetch-all variants), the unused URLSearchParams and array branches of the request-param sanitizer, and the one-caller mission filter helper is inlined. The migration lock command drops its unused CLI passthrough and tuning knobs and lets Postgres do the waiting via lock_timeout plus a blocking advisory lock; the rank-update wrapper loses its duplicate vendor check; unrelated Mee6 fixture churn is reverted. ## Claude Implementation Notes - frontend/src/components/Pagination.svelte, MySubmissions.svelte, StewardSubmissions.svelte, StewardDiscordXP.svelte: pageSizeOptions capped at 50 (two paginators per route). - frontend/src/lib/api.js: deleted contributionsAPI.getContributionTypes, contributionsAPI.getMissions, partnersAPI.list; sanitizeRequestParams reduced to the plain-object path. - frontend/src/tests/setupTests.js, routes.test.js: removed/updated mocks for the deleted helpers. - backend/utils/management/commands/migrate_with_lock.py: rewritten to SET lock_timeout '900s' + blocking pg_advisory_lock (session lock auto-releases on disconnect); only flag left is --noinput; test file rewritten to match. - backend/leaderboard/models.py: update_leaderboard_ranks always locks inside transaction.atomic (helper already no-ops off Postgres). - backend/contributions/views.py: inlined _get_valid_contribution_type_param into get_queryset. - backend/startup.sh: boolean case collapsed to a single [ = "false" ] test. - backend/api/tests.py, community_xp/tests/test_mee6_sync.py, leaderboard/tests/test_stats.py: Mee6SyncRun fixture page_size back to 1000 (MEE6 fetch size, unrelated to DRF max_page_size). - frontend/CLAUDE.md: partnersAPI doc now lists listAll; PartnerLogoBanner note updated.
JoaquinBN
force-pushed
the
JoaquinBN/poap-outage-triage
branch
from
8ed25aa to
b9f0393
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@backend/contributions/views.py`:
- Around line 3380-3388: The contribution_type validation in the queryset
filtering logic should preserve exception context when re-raising a
ValidationError. Update the exception handling in the code path that parses
contribution_type in the relevant view method so the serializers.ValidationError
is chained from the caught TypeError/ValueError using from err, keeping the
original traceback available for debugging.
In `@backend/utils/management/commands/migrate_with_lock.py`:
- Around line 12-16: The `_signed_int32` helper is duplicated and should be
centralized to avoid drift. Move the shared 32-bit signed conversion logic into
a utility under backend/utils/ (for example a locks helper) and update
migrate_with_lock.py and leaderboard/models.py to import and use that shared
function. If the advisory-lock code is also duplicated, consider extracting the
common lock-acquisition helper there as well so both call sites use the same
implementation.
- Around line 48-59: The `migrate_with_lock` command uses an f-string for
`cursor.execute("SET lock_timeout = ...")`, which static analysis may flag even
though `LOCK_TIMEOUT_SECONDS` is a fixed module constant. Update the
`migrate_with_lock` flow around `connection.ensure_connection()` and the `with
connection.cursor()` block to either cast the timeout to `int()` at use or add a
brief inline comment explaining that the value is hardcoded and not
user-controlled, so the SQL is safe. Keep the `pg_advisory_lock` logic
unchanged.
In `@backend/utils/tests/test_migrate_with_lock.py`:
- Around line 41-44: The test setup in the migrate_with_lock helper uses nested
with blocks that can be combined, so merge the patch.object calls into a single
with statement to satisfy Ruff SIM117. Update the helper that calls
command.handle to keep the same mocked connections and call_command behavior
while reducing the nested context managers.
In `@frontend/src/lib/api.js`:
- Around line 24-51: The cached response in getCachedMetricsRequest is being
reused by reference, so mutations by one caller can leak to others. Update the
caching path in getCachedMetricsRequest to store and return an immutable or
cloned response object instead of the original axios response, and ensure any
nested data used by callers cannot be mutated through the cache. Keep the
promise deduping behavior intact while making the cached value safe to share
across callers.
- Around line 53-83: The hard page cap in fetchSmallCatalogPages can turn normal
catalog growth into a runtime failure once the result set exceeds 1,000 records.
Update fetchSmallCatalogPages to avoid throwing on SMALL_CATALOG_MAX_PAGES in a
way that breaks callers like getAllContributionTypes, getAllMissions, and
listAll; either increase the cap to leave headroom for expected catalog growth
or return the accumulated results and surface a warning/log when the limit is
reached. Keep the existing paging logic and unique symbols intact while making
the overflow behavior graceful.
In `@frontend/src/lib/auth.js`:
- Line 733: The auth refresh logic in the visibility guard only skips work while
hidden, but it never rechecks immediately when the tab becomes visible again.
Update the session refresh flow in auth.js around the state.isAuthenticated /
document.hidden check to listen for visibilitychange and trigger the existing
refresh path as soon as visibility is restored, so the next timer tick is not
the only catch-up mechanism.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 651d3098-ffa3-40ef-8210-70d47ac23e0a
📒 Files selected for processing (41)
backend/contributions/tests/test_pagination.pybackend/contributions/tests/test_security_boundaries.pybackend/contributions/tests/test_submission_limits.pybackend/contributions/views.pybackend/leaderboard/models.pybackend/leaderboard/tests/test_recalculation.pybackend/partners/tests/test_partners.pybackend/startup.shbackend/utils/management/__init__.pybackend/utils/management/commands/__init__.pybackend/utils/management/commands/migrate_with_lock.pybackend/utils/pagination.pybackend/utils/tests/test_migrate_with_lock.pyfrontend/CLAUDE.mdfrontend/src/components/Missions.sveltefrontend/src/components/Pagination.sveltefrontend/src/components/Sidebar.sveltefrontend/src/components/portal/PartnerLogoBanner.sveltefrontend/src/components/portal/submit-contribution/SubmitContribution.sveltefrontend/src/lib/api.jsfrontend/src/lib/auth.jsfrontend/src/lib/components/ContributionSelection.sveltefrontend/src/lib/missionsStore.jsfrontend/src/lib/userStore.jsfrontend/src/lib/whatsNewStore.jsfrontend/src/routes/AllContributions.sveltefrontend/src/routes/CommunityPoaps.sveltefrontend/src/routes/ContributionTypeDetail.sveltefrontend/src/routes/EcosystemPartners.sveltefrontend/src/routes/EditSubmission.sveltefrontend/src/routes/Metrics.sveltefrontend/src/routes/MySubmissions.sveltefrontend/src/routes/ProjectPageEditor.sveltefrontend/src/routes/StewardDiscordXP.sveltefrontend/src/routes/StewardSubmissions.sveltefrontend/src/tests/apiClient.test.jsfrontend/src/tests/authSession.test.jsfrontend/src/tests/routes.test.jsfrontend/src/tests/setupTests.jsfrontend/src/tests/userStore.test.jsfrontend/src/tests/whatsNewStore.test.js
Comment on lines
+12
to
+16
| def _signed_int32(value): | ||
| return value - 2**32 if value >= 2**31 else value | ||
|
|
||
|
|
||
| MIGRATION_LOCK_KEY = _signed_int32(zlib.crc32(b'tally:migrations:default')) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate _signed_int32 helper across files.
The identical _signed_int32 conversion helper is redefined in backend/leaderboard/models.py (used for the leaderboard rank advisory lock key). Consider extracting this (and possibly a shared "acquire advisory lock" helper) into backend/utils/ to avoid divergence if one copy is changed later.
♻️ Suggested consolidation
# backend/utils/locks.py
def signed_int32(value: int) -> int:
return value - 2**32 if value >= 2**31 else valueThen import signed_int32 from both migrate_with_lock.py and leaderboard/models.py.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 12-12: Missing return type annotation for private function _signed_int32
(ANN202)
🤖 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 `@backend/utils/management/commands/migrate_with_lock.py` around lines 12 - 16,
The `_signed_int32` helper is duplicated and should be centralized to avoid
drift. Move the shared 32-bit signed conversion logic into a utility under
backend/utils/ (for example a locks helper) and update migrate_with_lock.py and
leaderboard/models.py to import and use that shared function. If the
advisory-lock code is also duplicated, consider extracting the common
lock-acquisition helper there as well so both call sites use the same
implementation.
Comment on lines
+24
to
+51
| function getCachedMetricsRequest(key, requestFn) { | ||
| const cached = metricsRequestCache.get(key); | ||
| const now = Date.now(); | ||
|
|
||
| if (cached?.response && now - cached.timestamp < METRICS_CACHE_TTL_MS) { | ||
| return Promise.resolve(cached.response); | ||
| } | ||
|
|
||
| if (cached?.promise) { | ||
| return cached.promise; | ||
| } | ||
|
|
||
| const promise = requestFn() | ||
| .then((response) => { | ||
| metricsRequestCache.set(key, { | ||
| response, | ||
| timestamp: Date.now(), | ||
| }); | ||
| return response; | ||
| }) | ||
| .catch((error) => { | ||
| metricsRequestCache.delete(key); | ||
| throw error; | ||
| }); | ||
|
|
||
| metricsRequestCache.set(key, { promise, timestamp: now }); | ||
| return promise; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Cached response object is shared by reference across callers.
cached.response is returned as-is to every caller during the TTL window; any downstream mutation of the axios response (or its .data) would silently corrupt the cache for all other callers until it expires.
🤖 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 `@frontend/src/lib/api.js` around lines 24 - 51, The cached response in
getCachedMetricsRequest is being reused by reference, so mutations by one caller
can leak to others. Update the caching path in getCachedMetricsRequest to store
and return an immutable or cloned response object instead of the original axios
response, and ensure any nested data used by callers cannot be mutated through
the cache. Keep the promise deduping behavior intact while making the cached
value safe to share across callers.
The mission contribution_type validation error is chained from the original parse error so tracebacks stay debuggable. The bounded catalog fetch no longer throws when a catalog outgrows its page cap; it logs a warning and returns what it has, so dropdowns and catalog pages degrade instead of crashing. Session refresh now also fires the moment a hidden tab becomes visible again, since the periodic refresh skips hidden tabs. The migration lock timeout is cast to int at the SQL boundary and the lock test collapses its nested context managers. ## Claude Implementation Notes - backend/contributions/views.py: raise ... from err on the contribution_type ValidationError. - frontend/src/lib/api.js: fetchSmallCatalogPages returns accumulated results with a console.warn instead of throwing at SMALL_CATALOG_MAX_PAGES. - frontend/src/lib/auth.js: visibilitychange listener triggers refreshSession when the tab becomes visible while authenticated. - backend/utils/management/commands/migrate_with_lock.py: int() cast on LOCK_TIMEOUT_SECONDS in the SET lock_timeout f-string. - backend/utils/tests/test_migrate_with_lock.py: merged nested patch.object with-blocks (SIM117). - Skipped: _signed_int32 dedup (2-line pure function in two unrelated lock domains; a shared module is more surface than the duplication) and metrics cache cloning (all consumers are read-only and DecisionsChart already copies arrays before chart.js mutates them).
…triage # Conflicts: # frontend/src/lib/userStore.js
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
1 participant
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.
Startup migrations now run through a new
migrate_with_lockcommand that holds a PostgreSQL advisory lock (with aRUN_MIGRATIONS_ON_STARTUPopt-out), so concurrent App Runner instances can no longer race each other during deploys. Leaderboard rank updates take a per-type advisory lock and iterate leaderboards in sorted order, keeping concurrent recalculations deadlock-free. API page sizes are capped at 50, the missioncontribution_typefilter rejects non-numeric values, and the frontend fetches catalog endpoints (contribution types, missions, partners) through bounded fetch-all helpers instead of oversizedpage_sizerequests. The portal also stops amplifying load: metrics responses share a 30s cache, session refresh / user load / what's-new checks collapse concurrent callers into a single request, refresh pauses in hidden tabs, and literalundefined/nullquery params are stripped. The submit form additionally recovers expired reCAPTCHA tokens from the widget instead of blocking submission.Summary by CodeRabbit
New Features
Bug Fixes