fix(run-storage): validate cursor order_by column

[jmoldow]

Summary & Motivation

Cursor pagination is invalid when the order_by doesn't match the cursor and/or the cursor doesn't completely cover the index. Validate that this doesn't happen, and raise if it ever does happen.

_add_cursor_limit_to_query in SqlRunStorage previously allowed semantically invalid cursor pagination with a non-id order_by column. This would silently apply id-based cursor filtering while sorting by the requested column — producing incorrect, page-skipping results with no error.

This change adds explicit guards:

• If cursor is provided and order_by maps to a non-unique column (e.g. status), a ParameterCheckError is raised immediately, since cursor pagination on a non-unique column is undefined.
• If cursor is provided and order_by maps to a unique non-id column (e.g. run_id), a NotImplementedCheckError is raised, since stable single-column cursor pagination over an arbitrary unique key is not yet implemented.
• The existing id-based cursor path is unchanged.

Test Plan

Two new test cases in TestRunStorage cover both error branches: test_cursor_pagination_with_non_unique_order_by_raises and test_cursor_pagination_with_unique_non_id_order_by_raises.

Changelog

DagsterInstance.get_run_records, RunStorage.get_runs, and RunStorage.get_run_records: passing cursor together with an order_by column other than id now raises an explicit error instead of silently returning wrong results. This is a user-facing behavior change for these @public methods. Previously, callers that pass cursor with a non-id order_by get results back, even if those results may be semantically incorrect. After this change, the same call raises.

[greptile-apps]

Greptile Summary

This PR fixes a silent data correctness bug in SqlRunStorage._add_cursor_limit_to_query where providing cursor with a non-id order_by column would silently apply id-based cursor filtering while sorting by the requested column, producing page-skipping results. Explicit guards are now added that raise descriptive errors instead.

• Non-unique order_by + cursor: raises ParameterCheckError immediately, since cursor pagination on a non-unique column is undefined.
• Unique non-id order_by + cursor: raises NotImplementedCheckError, since single-column cursor pagination over an arbitrary unique key is not yet implemented.
• Two new tests cover both error branches; both correctly skip InMemoryRunStorage which doesn't exercise this code path.

Confidence Score: 5/5

Safe to merge — the change only adds early-exit validation before any DB query is executed, leaving the existing id-based cursor path untouched.

The fix is minimal and well-scoped: it moves one line of column resolution above the cursor guard and adds two branches that raise on previously silently-broken inputs. The id-based cursor path is structurally unchanged. Both new error paths are exercised by new tests that correctly skip in-memory storage. The sorting_column is RunsTable.c.id identity check is safe because SQLAlchemy column objects on a Table.c are singletons, and Column.unique accurately reflects the schema-level unique=True attribute used in RunsTable.

No files require special attention.

Important Files Changed

Filename	Overview
python_modules/dagster/dagster/_core/storage/runs/sql_run_storage.py	Moves sorting_column computation before the if cursor: block and adds two explicit guard branches that raise ParameterCheckError or NotImplementedCheckError when cursor pagination is requested with a non-id sort column; the existing id-based cursor path is unchanged.
python_modules/dagster/dagster_tests/storage_tests/utils/run_storage.py	Adds two new test cases covering the non-unique and unique-non-id error branches; both tests import the correct error types and correctly skip in-memory storage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_add_cursor_limit_to_query(cursor, order_by, ...)"] --> B["sorting_column = getattr(RunsTable.c, order_by)\nor RunsTable.c.id if order_by is None"]
    B --> C{cursor provided?}
    C -- No --> G["Apply limit + ORDER BY sorting_column"]
    C -- Yes --> D{sorting_column\nis RunsTable.c.id?}
    D -- Yes --> E["Apply id-based cursor filter\n(WHERE id > or < subquery)"]
    E --> G
    D -- No --> F{sorting_column.unique?}
    F -- Yes --> H["raise NotImplementedCheckError\n'Cursor pagination for this\nRunsTable unique column: ...'"]
    F -- No --> I["raise ParameterCheckError\n'Cursor pagination requires\na unique sort column'"]
    G --> J["Return query"]

Loading

_{Reviews (2): Last reviewed commit: "Update python_modules/dagster/dagster/_c..." | Re-trigger Greptile}

[cmpadden]

This looks technically correct to me, but I think we should be careful that this is a user-facing behavior change. Today, callers that pass cursor with a non-id order_by get results back, even if those results may be semantically incorrect. After this change, the same call raises.

I agree explicit failure is better than silently wrong pagination, but since this affects public APIs like DagsterInstance.get_run_records, RunStorage.get_runs, and RunStorage.get_run_records, we may want to consider whether this should be:

• gated behind an opt-in / feature flag first,
• introduced with a deprecation warning period,
• or held for a major release if we consider it breaking behavior.

At minimum, I think we should explicitly call out the compatibility impact in the changelog/release notes.

[jmoldow]

@cmpadden I updated the changelog part of the PR description to add two of the sentences from your comment:

DagsterInstance.get_run_records, RunStorage.get_runs, and RunStorage.get_run_records: passing cursor together with an order_by column other than id now raises an explicit error instead of silently returning wrong results. This is a user-facing behavior change for these @public methods. Previously, callers that pass cursor with a non-id order_by get results back, even if those results may be semantically incorrect. After this change, the same call raises.

As for the three options you called out: I'm fine with any of the options you laid out. Though I think "gated behind an opt-in / feature flag first" is possibly the lowest ROI, because if someone wanted to enable that flag, they could just fix their calling code instead.

Let me know if you want this changed to emit a deprecation warning instead of throwing exceptions. Or feel free to update the PR as you see fit.