-
Notifications
You must be signed in to change notification settings - Fork 429
logs: keep paging in all-workflows mode when full batches filter to zero runs #39741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,20 @@ type LogsDownloadOptions struct { | |
| After string | ||
| } | ||
|
|
||
| func shouldStopPagination(totalFetched, batchSize int) bool { | ||
| return totalFetched < batchSize | ||
| } | ||
|
|
||
| func selectPaginationCursorDate(filteredRuns []WorkflowRun, oldestFetchedCreatedAt time.Time) (string, bool) { | ||
| if !oldestFetchedCreatedAt.IsZero() { | ||
| return oldestFetchedCreatedAt.Format(time.RFC3339), true | ||
| } | ||
| if len(filteredRuns) == 0 { | ||
| return "", false | ||
| } | ||
| return filteredRuns[len(filteredRuns)-1].CreatedAt.Format(time.RFC3339), true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] 💡 SuggestionAdd a brief comment to document the contract: // filteredRuns must be ordered newest-first (GitHub API default);
// the last element is treated as the oldest.The same assumption applies in |
||
| } | ||
|
pelikhan marked this conversation as resolved.
|
||
|
|
||
| // DownloadWorkflowLogs downloads and analyzes workflow logs with metrics | ||
| func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error { | ||
| workflowName := opts.WorkflowName | ||
|
|
@@ -239,29 +253,47 @@ func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error { | |
| } | ||
| } | ||
|
|
||
| var oldestFetchedCreatedAt time.Time | ||
| runs, totalFetched, err := listWorkflowRunsWithPagination(ListWorkflowRunsOptions{ | ||
| WorkflowName: workflowName, | ||
| Limit: batchSize, | ||
| StartDate: startDate, | ||
| EndDate: endDate, | ||
| BeforeDate: beforeDate, | ||
| Ref: ref, | ||
| BeforeRunID: beforeRunID, | ||
| AfterRunID: afterRunID, | ||
| RepoOverride: repoOverride, | ||
| ProcessedCount: len(processedRuns), | ||
| TargetCount: count, | ||
| Verbose: verbose, | ||
| WorkflowName: workflowName, | ||
| Limit: batchSize, | ||
| StartDate: startDate, | ||
| EndDate: endDate, | ||
| BeforeDate: beforeDate, | ||
| Ref: ref, | ||
| BeforeRunID: beforeRunID, | ||
| AfterRunID: afterRunID, | ||
| RepoOverride: repoOverride, | ||
| OldestFetchedCreatedAt: &oldestFetchedCreatedAt, | ||
| ProcessedCount: len(processedRuns), | ||
| TargetCount: count, | ||
| Verbose: verbose, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if len(runs) == 0 { | ||
| if shouldStopPagination(totalFetched, batchSize) { | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("No more workflow runs found, stopping iteration")) | ||
| } | ||
| break | ||
| } | ||
|
|
||
| cursor, ok := selectPaginationCursorDate(nil, oldestFetchedCreatedAt) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] No guard against cursor stagnation: if GitHub returns a full batch where all runs share the same 💡 Suggested safeguardTrack the previous cursor and break (or log a warning) if it hasn't advanced: if cursor == beforeDate {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Pagination cursor did not advance; stopping to prevent infinite loop"))
}
break
}This is a degenerate case, but in high-volume repos with many same-second runs it is reachable. |
||
| if !ok { | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Workflow batch filtered to zero runs but no pagination cursor was found, stopping iteration")) | ||
| } | ||
| break | ||
| } | ||
|
|
||
| beforeDate = cursor | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("No more workflow runs found, stopping iteration")) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Batch filtered to zero runs; advancing pagination cursor and continuing")) | ||
| } | ||
| break | ||
| continue | ||
| } | ||
|
|
||
| if verbose { | ||
|
|
@@ -515,10 +547,12 @@ func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error { | |
| } | ||
| } | ||
|
|
||
| // Prepare for next iteration: set beforeDate to the oldest processed run from this batch | ||
| if len(runs) > 0 && len(runsRemaining) == 0 { | ||
| oldestRun := runs[len(runs)-1] // runs are typically ordered by creation date descending | ||
| beforeDate = oldestRun.CreatedAt.Format(time.RFC3339) | ||
| // Prepare for next iteration: set beforeDate to the oldest run from the raw API batch. | ||
| // This guarantees pagination moves forward even when filtered runs are sparse. | ||
| if len(runsRemaining) == 0 { | ||
| if cursor, ok := selectPaginationCursorDate(runs, oldestFetchedCreatedAt); ok { | ||
| beforeDate = cursor | ||
| } | ||
| } | ||
|
|
||
| // If we got fewer runs than requested in this batch, we've likely hit the end | ||
|
|
@@ -528,7 +562,7 @@ func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error { | |
| // Example: API returns 250 total runs, but only 5 are agentic workflows after filtering. | ||
| // Old buggy logic: len(runs)=5 < batchSize=250, stop iteration (WRONG - misses more agentic workflows!) | ||
| // Fixed logic: totalFetched=250 < batchSize=250 is false, continue iteration (CORRECT) | ||
| if totalFetched < batchSize { | ||
| if shouldStopPagination(totalFetched, batchSize) { | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Received fewer runs than requested, likely reached end of available runs")) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| func TestShouldStopPagination(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| totalFetched int | ||
| batchSize int | ||
| want bool | ||
| }{ | ||
| { | ||
| name: "stop when raw batch is smaller than requested", | ||
| totalFetched: 249, | ||
| batchSize: 250, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "continue when raw batch is full", | ||
| totalFetched: 250, | ||
| batchSize: 250, | ||
| want: false, | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] 💡 Suggested addition{
name: "stop when no runs fetched",
totalFetched: 0,
batchSize: 250,
want: true,
}, |
||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := shouldStopPagination(tt.totalFetched, tt.batchSize); got != tt.want { | ||
| t.Fatalf("shouldStopPagination(%d, %d) = %v, want %v", tt.totalFetched, tt.batchSize, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestSelectPaginationCursorDate(t *testing.T) { | ||
| oldestFetched := time.Date(2026, 6, 16, 10, 0, 0, 0, time.UTC) | ||
| filteredRuns := []WorkflowRun{ | ||
| {CreatedAt: time.Date(2026, 6, 16, 12, 0, 0, 0, time.UTC)}, | ||
| {CreatedAt: time.Date(2026, 6, 16, 11, 0, 0, 0, time.UTC)}, | ||
| } | ||
|
|
||
| cursor, ok := selectPaginationCursorDate(filteredRuns, oldestFetched) | ||
| if !ok { | ||
| t.Fatal("expected cursor to be set when raw oldest fetched run is available") | ||
| } | ||
| if cursor != oldestFetched.Format(time.RFC3339) { | ||
| t.Fatalf("expected cursor %s, got %s", oldestFetched.Format(time.RFC3339), cursor) | ||
| } | ||
| } | ||
|
|
||
| func TestSelectPaginationCursorDateFallsBackToFilteredRuns(t *testing.T) { | ||
| filteredOldest := time.Date(2026, 6, 15, 18, 30, 0, 0, time.UTC) | ||
| filteredRuns := []WorkflowRun{ | ||
| {CreatedAt: time.Date(2026, 6, 15, 19, 0, 0, 0, time.UTC)}, | ||
| {CreatedAt: filteredOldest}, | ||
| } | ||
|
|
||
| cursor, ok := selectPaginationCursorDate(filteredRuns, time.Time{}) | ||
| if !ok { | ||
| t.Fatal("expected cursor to be set from filtered runs") | ||
| } | ||
| if cursor != filteredOldest.Format(time.RFC3339) { | ||
| t.Fatalf("expected fallback cursor %s, got %s", filteredOldest.Format(time.RFC3339), cursor) | ||
| } | ||
| } | ||
|
|
||
| func TestSelectPaginationCursorDateNoCursor(t *testing.T) { | ||
| cursor, ok := selectPaginationCursorDate(nil, time.Time{}) | ||
| if ok { | ||
| t.Fatalf("expected no cursor, got %s", cursor) | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] The helpers are tested in isolation, but the orchestrator loop's 💡 Suggested approachAdd a test that exercises the orchestrator with a mock
Assert the API is called at least twice and |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnose]
OldestFetchedCreatedAtis an output field on an options struct, mixing input and output concerns inListWorkflowRunsOptions. Future callers reading the struct type won't immediately know this field is populated by the function, not consumed by it.💡 Alternative
Return it as an additional named return value alongside
totalFetched:This keeps the options struct as pure input and makes the out-param visible at every call site.