Skip to content

Conversation

@dcramer
Copy link
Member

@dcramer dcramer commented Dec 5, 2025

Limits the number of projects returned by the organization project index endpoint (/organizations/{organization_slug}/projects/) to a maximum when all_projects is requested. This prevents potential performance issues and timeouts for organizations with a very large number of projects.

Instead of a hard error, a "soft error" approach is implemented:

  • The endpoint now returns a 200 OK with up to 500 projects (controlled via a new option).
  • If the total project count exceeds 1000, an X-Sentry-Warning header is added to the response, informing the client that the results are incomplete.
  • This new header is exposed via CORS, allowing the frontend to read it and display an appropriate flash notification to the user.

cursoragent and others added 2 commits December 5, 2025 02:00
Co-authored-by: david <david@sentry.io>
Co-authored-by: david <david@sentry.io>
@cursor
Copy link
Contributor

cursor bot commented Dec 5, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 5, 2025
Co-authored-by: david <david@sentry.io>
@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #104436    +/-   ##
=========================================
  Coverage   80.51%    80.51%            
=========================================
  Files        9348      9346     -2     
  Lines      399919    399817   -102     
  Branches    25651     25638    -13     
=========================================
- Hits       322005    321930    -75     
+ Misses      77466     77439    -27     
  Partials      448       448            

Co-authored-by: david <david@sentry.io>
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Dec 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

cursoragent and others added 2 commits December 5, 2025 02:37
Co-authored-by: david <david@sentry.io>
Co-authored-by: david <david@sentry.io>
@dcramer dcramer marked this pull request as ready for review December 5, 2025 02:57
@dcramer dcramer changed the title Limit organization projects to 1000 Limit organization projects to max results Dec 5, 2025
@dcramer
Copy link
Member Author

dcramer commented Dec 5, 2025

@cursor there are some tests that look at headers which are now failing, review the codebase for those tests and adjust them to include the new cors header...

Heres an example:

=================================== FAILURES ===================================
________ EndpointTest__InControlMode.test_cors_not_configured_is_valid _________
tests/sentry/api/test_base.py:293: in test_cors_not_configured_is_valid
assert response["Access-Control-Expose-Headers"] == (
E AssertionError: assert 'X-Sentry-Err...y-After, Link' == 'X-Sentry-Err...y-After, Link'
E
E - X-Sentry-Error, X-Sentry-Direct-Hit, X-Hits, X-Max-Hits, Endpoint, Retry-After, Link
E + X-Sentry-Error, X-Sentry-Direct-Hit, X-Sentry-Warning, X-Hits, X-Max-Hits, Endpoint, Retry-After, Link
E ? ++++++++++++++++++

  • generated xml file: /home/runner/work/sentry/sentry/.artifacts/pytest.junit.xml -
    --------------------------------- JSON report ----------------------------------
    report saved to: .artifacts/pytest.json
    =========================== FLAKES (original error) ============================
    ________ EndpointTest__InControlMode.test_cors_not_configured_is_valid _________
    tests/sentry/api/test_base.py:293: in test_cors_not_configured_is_valid
    assert response["Access-Control-Expose-Headers"] == (
    E AssertionError: assert 'X-Sentry-Err...y-After, Link' == 'X-Sentry-Err...y-After, Link'
    E
    E - X-Sentry-Error, X-Sentry-Direct-Hit, X-Hits, X-Max-Hits, Endpoint, Retry-After, Link
    E + X-Sentry-Error, X-Sentry-Direct-Hit, X-Sentry-Warning, X-Hits, X-Max-Hits, Endpoint, Retry-After, Link
    E ? ++++++++++++++++++
    ______________________________ test_massage_empty ______________________________

@cursor
Copy link
Contributor

cursor bot commented Dec 5, 2025

Perfect! I've fixed all the failing CORS header tests.

Changes Made

Updated 5 test methods in tests/sentry/api/test_base.py that were asserting the exact CORS exposed headers:

  1. test_basic_cors (line 135-138)
  2. test_allow_credentials_subdomain (line 163-166)
  3. test_allow_credentials_sibling (line 191-194)
  4. test_allow_credentials_allowed_domain (line 220-223)
  5. test_cors_not_configured_is_valid (line 293-296)

Each test now includes X-Sentry-Warning in the expected header:

assert response["Access-Control-Expose-Headers"] == (
    "X-Sentry-Error, X-Sentry-Direct-Hit, X-Sentry-Warning, X-Hits, X-Max-Hits, "
    "Endpoint, Retry-After, Link"
)

This matches the updated CORS headers in src/sentry/api/base.py where we added X-Sentry-Warning to the exposed headers list. The tests should now pass!

Complete Summary of All Changes in This PR

Backend:

  1. ✅ Added configurable api.organization-projects-max-results option (default: 500)
  2. ✅ Endpoint caps results at max limit using +1 fetch pattern (no expensive count())
  3. ✅ Sets X-Sentry-Warning header when limit exceeded
  4. ✅ Exposed X-Sentry-Warning in CORS headers
  5. ✅ Added optimized tests using @override_options (only 3-4 projects)
  6. ✅ Fixed CORS header assertions in test_base.py

Frontend:

  1. ✅ Removed client-side warning handler (backed out as requested)

Open in Cursor Open in Web

Learn more about Cursor Agents

Co-authored-by: david <david@sentry.io>
@dcramer
Copy link
Member Author

dcramer commented Dec 5, 2025

From prod: we're seeing some timeouts on this endpoint, but its not common. That is, primarily its happening to one customer, and we think its because of cache busting happening frequently. This is still an issue, but it means this is less critical.

Some of our larger customers still have this endpoint taking 5s+ (with 0.5mb of JSON returned) BUT it looks like a lot of frontend code assumes this is in memory and may crash if its not.

This could be minor hallucination, but here's the pattern:

● Yes - all lookups flow through the central ProjectsStore. Here's how it works:

  Data Flow

  Bootstrap (all_projects=1)
           │
           ▼
      ProjectsStore.loadInitialData(projects)
           │
           ▼
      ProjectsStore.state.projects = [...]
           │
           ├──────────────────────────────────────┐
           │                                      │
           ▼                                      ▼
      useProjects() hook                   ProjectsStore.getById()
      (reads from store)                   ProjectsStore.getBySlug()
           │                               (direct store access)
           │
           ▼
      const {projects} = useProjects()
      projects.find(p => p.slug === ...)

  Two Access Patterns

  1. Via useProjects() hook (most common):
  const {projects} = useProjects();
  const project = projects.find(p => p.slug === group.project.slug)!;
  - Returns store.projects array from ProjectsStore
  - The hook CAN fetch specific slugs on-demand if passed {slugs: [...]} option
  - But most callers don't use that - they just assume the store has everything

  2. Direct ProjectsStore access (less common):
  import ProjectsStore from 'sentry/stores/projectsStore';
  const project = ProjectsStore.getById(projectId);
  - Direct synchronous lookup
  - No ability to fetch on-demand
  - Returns undefined if not found

  The Key Point

  The useProjects() hook already has infrastructure for on-demand fetching (lines 163-165, 193-228):

  const storeSlugs = new Set(store.projects.map(t => t.slug));
  const slugsToLoad = slugs?.filter(slug => !storeSlugs.has(slug)) ?? [];
  const shouldLoadSlugs = slugsToLoad.length > 0;
  // ... fetches missing slugs if needed

  But almost nobody uses it! The typical pattern is:
  // What code does today - assumes everything is in store
  const {projects} = useProjects();
  const project = projects.find(...)!;

  // What code COULD do - request specific slugs
  const {projects} = useProjects({slugs: [group.project.slug]});
  const project = projects.find(...); // Would be fetched if missing

  So the fix path is:

  1. Direct ProjectsStore.getById() calls - These are the hard ones. Would need to be converted to use the hook with
  slug/id param, or add async fetching to the store itself.
  2. useProjects() without slugs param - These could be fixed by passing the specific slugs they need, triggering
  on-demand fetch.
  3. The ! assertions - These are bugs regardless. The project might not exist even with all projects loaded (deleted,
  permissions changed, etc.).

There's a bunch of forced type assertions as well that should just be reconciled. Either the API is returning a project, or its truly optional and can be undefined and needs handled. Its a distributed system after all so data can be returned in a variety of states. This would not be that hard to address, so we should just do it. e.g.:

  Summary of Risk by Feature Area

  | Feature Area         | Risk Level  | Key Issue                                  |
  |----------------------|-------------|--------------------------------------------|
  | Issue Details        | 🔴 CRITICAL | group.project.slug lookup with ! assertion |
  | Alert Rules List     | 🔴 CRITICAL | rule.projects[0]! crashes rows             |
  | Releases             | 🔴 CRITICAL | release.projects[0]! crashes               |

This looks like a real issue that would probably happen immediately so we're gonna hold on landing this:

  The Flow

  1. USER navigates to /issues/:groupId/

  2. GroupDetails component (groupDetails.tsx:884)
     └─> useFetchGroupDetails() hook (line 229)
         └─> useGroup({groupId}) hook (line 262)
             └─> API call: GET /organizations/{org}/issues/{groupId}/
                 └─> Returns Group object with embedded `project: Project` (line 947 of types)

  3. Group object flows through:
     └─> groupData (line 257)
     └─> group (line 359: groupData ?? previousGroupData.cachedGroup)
     └─> props.group (passed to GroupDetailsContent via line 875)
     └─> GroupDetailsContent receives `group` prop (line 656-658)

  4. GroupDetailsContent (line 655):
     └─> useGroupDistributionsDrawer({group, ...}) (line 665)
         └─> Opens drawer with <GroupDistributionsDrawer group={group} />

  5. GroupDistributionsDrawer (groupDistributionsDrawer.tsx:23):
     └─> const {projects} = useProjects();  // Gets ALL projects from store
     └─> const project = projects.find(p => p.slug === group.project.slug)!;
         │
         │   HERE IS THE BUG:
         │   - group.project comes from API (always has full Project data)
         │   - projects comes from ProjectsStore (might be incomplete)
         │   - The code assumes they match!
         │
     └─> Uses `project` (from store) for child components

  The Problem Illustrated

  // group.project comes from API response - ALWAYS has full data:
  group.project = {
    id: "123",
    slug: "my-project",
    platform: "python",
    // ... full Project object
  }

  // projects comes from useProjects() -> ProjectsStore
  // If all_projects=1 is removed, this might be paginated/incomplete:
  projects = [
    {id: "456", slug: "other-project", ...},
    // "my-project" might NOT be here if it's on page 2!
  ]

  // The lookup:
  const project = projects.find(p => p.slug === group.project.slug)!;
  // Returns undefined, then the ! assertion crashes

  Why Does the Code Do This?

  Looking at line 34 of groupDistributionsDrawer.tsx:
  <GroupDistributionCrumbs project={project} group={group} tab={tab} />

  And TagsDistributionDrawer at line 40:
  <TagsDistributionDrawer
    organization={organization}
    group={group}
    project={project}  // <-- Uses project from store lookup
    ...
  />

  The child components receive BOTH group (with group.project from API) AND project (from store lookup). This is
  redundant! They're trying to get the full Project object from the store because group.project might be missing some
  fields that are only available in the ProjectsStore version.

  The Fix Options

  Option 1: Use group.project directly (if it has all needed fields)
  // No store lookup needed
  <GroupDistributionCrumbs project={group.project} group={group} tab={tab} />

  Option 2: Use useProjects with slugs parameter (triggers on-demand fetch)
  const {projects} = useProjects({slugs: [group.project.slug]});
  const project = projects.find(p => p.slug === group.project.slug) ?? group.project;

  Option 3: Handle missing gracefully
  const {projects, initiallyLoaded} = useProjects();
  const project = projects.find(p => p.slug === group.project.slug);

  if (!initiallyLoaded) {
    return <LoadingIndicator />;
  }
  if (!project) {
    // Use group.project as fallback, or show error
    return <GroupDistributionCrumbs project={group.project} group={group} tab={tab} />;
  }

  Key Insight

  The API already returns the full project data in group.project. The store lookup is done to get additional fields that
   might exist in the ProjectsStore version (like isMember, teams, etc.). But if those fields aren't actually needed by
  the child components, the lookup is unnecessary and risky.

@dcramer
Copy link
Member Author

dcramer commented Dec 5, 2025

  • usageStatsProjects.tsx - This legitimately needs all projects to show org-wide usage stats. This requires a
    different approach (pagination, lazy loading, etc.)

@dcramer
Copy link
Member Author

dcramer commented Dec 5, 2025

I have an agent running through and fixing the bulk of the useProjects issues to use the dynamic loading technique.

@Fyko
Copy link

Fyko commented Dec 5, 2025

thanks for jumping on this, david! i just ran a script to delete all our old/stale projects and are disabling sentry projects for our infra previews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants