fix(runs): honor Offset in ListActions so WatchActions loads all child actions by pingsutw · Pull Request #7585 · flyteorg/flyte

pingsutw · 2026-06-23T23:56:25Z

Why are the changes needed?

WatchActions returned children_phase_counts capped at 100 no matter the real child count (reproduced with a 10,000-element map task).

Root cause is a pagination contract mismatch:

WatchActions.listAndSendAllActions (runs/service/run_service.go:1262) loads the initial snapshot by offset paging:

const pageSize = 100
for {
    batch := ListActions(ctx, ListResourceInput{Filter: run, Limit: pageSize, Offset: offset, sort created_at ASC})
    rsm.upsertActions(batch)            // feeds the run-state tree -> ChildPhaseCounts
    if len(batch) < pageSize { return }
    offset += pageSize
}

but the repo ListActions (runs/repository/impl/action.go) only implemented keyset paging (CursorToken) and silently ignored Offset — there was no OFFSET clause in the query.

So every iteration ran the identical query and re-read the same first page. Consequences:

Only the first pageSize (100) actions ever entered the run-state tree, so attachChild/ChildPhaseCounts only ever counted 100 children → children_phase_counts capped at 100.
For any run with more than one page of actions, len(batch) < pageSize was never true, so listAndSendAllActions never returned and the live-update loop (updatesCh) was never reached — the watch got stuck replaying the first page. (Runs with <100 actions worked, which is why this only showed up at scale.)

The ListResourceInput struct already documents Offset as a supported, cursor-exclusive option (runs/repository/interfaces/common.go); it just wasn't wired into the query.

What changes were proposed in this pull request?

Add the OFFSET clause to ListActions when Offset > 0:

queryBuilder.WriteString(" LIMIT ?")
args = append(args, input.Limit+1)

if input.Offset > 0 {
    queryBuilder.WriteString(" OFFSET ?")
    args = append(args, input.Offset)
}

This makes the existing offset loop walk all actions and terminate. (Longer term the snapshot loop could move to the keyset CursorToken the repo also supports, but that needs the cursor direction fixed for the ASC sort plus a (created_at, name) tiebreaker — out of scope here.)

How was this patch tested?

Added TestListActionsOffsetPagination (runs/repository/impl/action_test.go): creates 150 root actions and asserts that

the page at Offset=50 starts at r050 (not r000), and
walking by offset covers all 150 actions exactly once and terminates.

Verified it fails without the fix (the Offset=50 page returns the first page → r000 != r050) and passes with it. Full runs/repository/impl suite passes.

Labels

fixed

Check all the applicable boxes

All new and existing tests passed.
All commits are signed-off.

…d actions ListActions only implemented keyset (CursorToken) pagination and silently ignored the Offset field. WatchActions.listAndSendAllActions pages by Offset (offset += pageSize), so every iteration ran the identical query and re-read the same first page. Effects: - only the first pageSize (100) actions ever entered the run-state tree, capping children_phase_counts at 100 regardless of the real child count (observed with a 10k-element map task). - for runs with more than one page, len(batch) < pageSize was never true, so the initial-snapshot loop never returned and the live-update loop was never reached. Add the OFFSET clause to ListActions when Offset > 0. Regression test asserts offset shifts the page window and that walking by offset covers all actions and terminates. Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot

Pull request overview

This PR fixes WatchActions getting stuck and undercounting children_phase_counts by making the runs action repository honor ListResourceInput.Offset during ListActions pagination.

Changes:

Add OFFSET support to the ListActions SQL query when input.Offset > 0.
Add a regression test ensuring offset-based pagination returns the expected window and can walk the full result set.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`runs/repository/impl/action.go`	Applies `OFFSET` in the `ListActions` query so offset pagination actually advances across pages.
`runs/repository/impl/action_test.go`	Adds a regression test validating `Offset` shifts the page window and supports full traversal.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin Su <pingsutw@gmail.com>

Signed-off-by: Kevin Su <pingsutw@apache.org>

…s-offset

Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+	if input.CursorToken != "" && input.Offset > 0 {
+		return nil, fmt.Errorf("CursorToken and Offset are mutually exclusive")
+	}
+	if input.Offset > 0 {
+		queryBuilder.WriteString(" OFFSET ?")
+		args = append(args, input.Offset)
+	}


…tions A map task bulk-creates thousands of children with identical created_at. listAndSendAllActions paged the snapshot by Offset sorted on created_at alone; OFFSET over a non-unique ORDER BY has undefined order among ties, so pages overlap/skip and the run-state tree loses rows -- children_phase_counts came up short (e.g. 4.8K instead of 20000). Add a deterministic 'name' tiebreaker (unique within a run) to the sort so offset paging is stable. Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

 			SortParameters: []interfaces.SortParameter{
 				impl.NewSortParameter("created_at", interfaces.SortOrderAscending),
+				impl.NewSortParameter("name", interfaces.SortOrderAscending),
 			},


+	// Offset-based pagination. Callers either page by CursorToken (keyset) or by
+	// Offset; the two are mutually exclusive.
+	if input.CursorToken != "" && input.Offset > 0 {
+		return nil, fmt.Errorf("CursorToken and Offset are mutually exclusive")
+	}
+	if input.Offset > 0 {
+		queryBuilder.WriteString(" OFFSET ?")
+		args = append(args, input.Offset)
+	}


+	assert.Len(t, seen, total, "offset paging over tied created_at must cover all actions exactly once")
+	assert.True(t, sort1.IsSorted(sort1.StringSlice(ordered)),
+		"the name tiebreaker must give a total order so pages don't overlap/skip")


listAndSendAllActions now passes a (created_at ASC, name ASC) sort; update TestListAndSendAllActionsUsesAscendingSort to assert both parameters. Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot AI review requested due to automatic review settings June 23, 2026 23:56

github-actions Bot added the flyte2 label Jun 23, 2026

Copilot started reviewing on behalf of pingsutw June 23, 2026 23:56 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread runs/repository/impl/action.go

Comment thread runs/repository/impl/action_test.go

pingsutw marked this pull request as draft June 24, 2026 00:06

pingsutw self-assigned this Jun 24, 2026

pingsutw added this to the V2 GA milestone Jun 24, 2026

Apply suggestions from code review

09b63c1

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin Su <pingsutw@gmail.com>

Copilot AI review requested due to automatic review settings June 24, 2026 06:55

Copilot started reviewing on behalf of pingsutw June 24, 2026 06:55 View session

pingsutw added 4 commits June 23, 2026 23:56

nit

2db9a4a

Signed-off-by: Kevin Su <pingsutw@apache.org>

Merge branch 'main' of github.com:flyteorg/flyte into fix/list-action…

98499af

…s-offset

nit

70ae611

Signed-off-by: Kevin Su <pingsutw@apache.org>

nit

7986b10

Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 24, 2026 07:17

Copilot started reviewing on behalf of pingsutw June 24, 2026 07:17 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

pingsutw mentioned this pull request Jun 24, 2026

fix(runs): seed WatchActions child phase counts so large map-task counts are correct #7589

Draft

2 tasks

test: expect name tiebreaker in WatchActions ascending sort

2e50035

listAndSendAllActions now passes a (created_at ASC, name ASC) sort; update TestListAndSendAllActionsUsesAscendingSort to assert both parameters. Signed-off-by: Kevin Su <pingsutw@apache.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(runs): honor Offset in ListActions so WatchActions loads all child actions#7585

fix(runs): honor Offset in ListActions so WatchActions loads all child actions#7585
pingsutw wants to merge 8 commits into
mainfrom
fix/list-actions-offset

pingsutw commented Jun 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pingsutw commented Jun 23, 2026

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Check all the applicable boxes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants