Skip to content

Fix ListQueues: iterator leak, unbounded CQL round-trips, and PageState after Close - #9523

Open
mykaul wants to merge 1 commit into
temporalio:mainfrom
mykaul:fix-cassandra-paging
Open

Fix ListQueues: iterator leak, unbounded CQL round-trips, and PageState after Close#9523
mykaul wants to merge 1 commit into
temporalio:mainfrom
mykaul:fix-cassandra-paging

Conversation

@mykaul

@mykaul mykaul commented Mar 15, 2026

Copy link
Copy Markdown
Contributor

What changed?

  • Set explicit CQL PageSize for Cassandra/Scylla queue reads.
  • Fix Cassandra/Scylla iterator lifecycle: close iterators on processing errors and preserve close errors.
  • Fix GetTaskQueuesByBuildId to recreate iterators with PageState and consume all pages. There is no artificial 10,000-result cap.
  • Fix ListQueues paging for under-filled ALLOW FILTERING pages, while bounding each request to 10 CQL round-trips and returning NextPageToken when more results remain.
  • Capture PageState before closing iterators.
  • Pre-allocate the ListQueues result slice.
  • Add regression coverage for queue-name query errors and task-queue mappings spanning multiple pages.

Why?

Cassandra/Scylla can return under-filled pages for ALLOW FILTERING queries, so a single CQL page does not necessarily satisfy the requested page size. Manual page traversal must also preserve the page token before iterator close. Iterator errors must not leak resources or be discarded.

Validation

  • ScyllaDB: TestCassandraTaskQueueUserDataSuite
  • ScyllaDB: TestCassandraQueueV2Persistence
  • make lint-code

The 10-page ListQueues bound is intentional because that API is paginated; GetTaskQueuesByBuildId remains unbounded and returns all mappings as required by its API contract.

@mykaul
mykaul requested review from a team as code owners March 15, 2026 15:03
@CLAassistant

CLAassistant commented Mar 15, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@mykaul mykaul changed the title Fix cassandra paging Fix ListQueues: iterator leak, unbounded CQL round-trips, and PageState after Close Mar 15, 2026
@mykaul
mykaul requested review from a team as code owners April 5, 2026 18:39
@prathyushpv

Copy link
Copy Markdown
Contributor

Code review

Found 1 issue:

  1. ListQueues inner scan loop can return more rows than PageSize. The CQL page size is set to request.PageSize on every iteration (line 492), but after the first page partially fills queues, subsequent pages can still return up to request.PageSize rows. The inner loop (lines 494-524) appends all of them without checking len(queues) >= request.PageSize. For example: if PageSize=100 and the first page returns 90 rows, the second page fetches another 100, producing 190 total. Fix: either use request.PageSize - len(queues) as the CQL page size, or add a len(queues) >= request.PageSize break inside the inner loop.

https://github.com/temporalio/temporal/blob/b6fdcdbe31f900c54eb75f8a13ed9d641630ec4b/common/persistence/cassandra/queue_v2_store.go#L487-L524

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@mykaul
mykaul force-pushed the fix-cassandra-paging branch from b6fdcdb to 23493b7 Compare April 20, 2026 10:03
@mykaul

mykaul commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue:

  1. ListQueues inner scan loop can return more rows than PageSize. The CQL page size is set to request.PageSize on every iteration (line 492), but after the first page partially fills queues, subsequent pages can still return up to request.PageSize rows. The inner loop (lines 494-524) appends all of them without checking len(queues) >= request.PageSize. For example: if PageSize=100 and the first page returns 90 rows, the second page fetches another 100, producing 190 total. Fix: either use request.PageSize - len(queues) as the CQL page size, or add a len(queues) >= request.PageSize break inside the inner loop.

https://github.com/temporalio/temporal/blob/b6fdcdbe31f900c54eb75f8a13ed9d641630ec4b/common/persistence/cassandra/queue_v2_store.go#L487-L524

🤖 Generated with Claude Code

  • If this code review was useful, please react with 👍. Otherwise, react with 👎.

Fixed with the first option: PageSize is now set to request.PageSize - len(queues) .
Also rebased on top of origin/main.
Ran go test -tags test_dep -run TestCassandraQueueV2 on the change.

Comment thread common/persistence/cassandra/matching_task_store_user_data.go Outdated
Comment thread common/persistence/cassandra/matching_task_store_user_data.go Outdated
Comment thread common/persistence/cassandra/queue_v2_store.go Outdated
@mykaul
mykaul force-pushed the fix-cassandra-paging branch from 45da0de to 3910aa8 Compare May 20, 2026 19:51
@mykaul

mykaul commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

v2 — Review feedback addressed + additional fixes

Review comments addressed:

  1. Loop condition — changed to for len(taskQueues) < maxTaskQueuesByBuildIdResults
  2. Break instead of early return — inner loop now uses break when hitting cap
  3. Don't ignore iter.Close() error — uses errors.Join(err, iter.Close()) on all early-return paths

Additional fixes found during implementation:

  1. Recreate iterator per page with PageState(pageToken) — gocql does not auto-paginate when PageState is used manually; the old code silently returned only the first page
  2. Capture PageState() before Close()Close() invalidates page state in gocql
  3. Same iter.Close() error handling fix applied to ListQueues — had the same _ = iter.Close() pattern
  4. Simplified pageToken logic in ListQueues — removed redundant if/else
  5. SQL backends: added ORDER BY task_queue_name LIMIT 10000 — aligns MySQL, PostgreSQL, and SQLite to the same deterministic truncation behavior as Cassandra/ScyllaDB
  6. Comment + CONSIDER item documenting the shared 10k cap across all backends

Rebased on origin/main, squashed into a single commit.

@mykaul

mykaul commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

8. SQL backends: added ORDER BY task_queue_name LIMIT 10000 — aligns MySQL, PostgreSQL, and SQLite to the same deterministic truncation behavior as Cassandra/ScyllaDB

Reviewers - take a note of the above please.

@mykaul

mykaul commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

ping - is there anything I need to do to move this fix forward?

@rodrigozhou rodrigozhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR lgtm.
Just wondering if there's any place that assumes the function would return all task queues. Since your changes you limit to 10k, it could potentially break something.
cc: @dnr

return nil, gocql.ConvertError("QueueV2ListQueues", err)
}
if len(pageToken) == 0 {
if pageToken == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from len(pageToken) == 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, fixed.

@mykaul
mykaul force-pushed the fix-cassandra-paging branch from 3910aa8 to 3be6b22 Compare August 24, 2026 18:36
@mykaul
mykaul requested a review from a team August 24, 2026 18:36
@mykaul

mykaul commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

The PR lgtm. Just wondering if there's any place that assumes the function would return all task queues. Since your changes you limit to 10k, it could potentially break something. cc: @dnr

OK, reverted this 10K, now just using paging.

- Set explicit PageSize for Cassandra queue ReadMessages queries
- Fix iterator leak and add round-trip cap in ListQueues
- Fix GetTaskQueuesByBuildId: recreate iterator per page with PageState
- Don't discard iter.Close() errors; use errors.Join on early-return paths
- Capture PageState before Close (Close invalidates page state)
- Add ORDER BY + LIMIT 10000 to SQL backends for consistent truncation
@mykaul
mykaul force-pushed the fix-cassandra-paging branch from 3be6b22 to 79e4d67 Compare August 25, 2026 08:18
@mykaul

mykaul commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Updated PR description!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants