fix(actions): retry rejected action records by 1fanwang · Pull Request #7892 · flyteorg/flyte

1fanwang · 2026-08-20T20:19:06Z

Tracking issue

N/A

Why are the changes needed?

An action can disappear from Flyte's run history after a repository failure. RecordAction puts application failures in resp.Msg.Status while returning err == nil. The old client adds the failed action to recordedFilter, so it is never retried.

It surfaced while checking whether #7852 applied to v2. V2 handles duplicates idempotently; its failure path was broken instead.

What changes were proposed in this pull request?

Only memoize an action after a non-nil response reports Status.Code == OK. Keep every other result retryable and log rejected statuses.

How was this patch tested?

A real RunService, Connect handler, migrations, and embedded PostgreSQL returned Status=INTERNAL with no Connect error. The merge-base client stopped after one RPC; the PR retried until OK.

Commands and raw output

$ go test ./runs/test/api -run '^TestRecordActionReturnsRepositoryFailureInBody$' -count=1 -v
DB-FAILURE transportErr=<nil> status.code=13 status.message="failed to create action: ERROR: relation \"actions\" does not exist (SQLSTATE 42P01)"
--- PASS: TestRecordActionReturnsRepositoryFailureInBody (0.01s)

$ BASE=$(git merge-base HEAD upstream/main)
$ git restore --source="$BASE" -- actions/k8s/client.go
$ go test ./actions/k8s/... -run TestNotifyRunService_InternalFailureAllowsRetry -count=1 -v
    Expected number of calls (3) of method RecordAction does not match the actual number of calls (1).
--- FAIL: TestNotifyRunService_InternalFailureAllowsRetry (0.00s)

$ git restore --source=HEAD -- actions/k8s/client.go
$ go test ./actions/k8s/... -run TestNotifyRunService_InternalFailureAllowsRetry -count=1 -v
--- PASS: TestNotifyRunService_InternalFailureAllowsRetry (0.00s)

Labels

fixed

Setup process

N/A

Screenshots

N/A

Check all the applicable boxes

I updated the documentation accordingly. Not applicable: no documented behavior changed.
All new and existing tests passed.
All commits are signed-off.

Related PRs

#7852

Stack

Docs link

N/A

github-actions · 2026-08-20T20:19:18Z

This PR was flagged by our automated quality checks. If you're a genuine
contributor, please reply here and a maintainer will review your PR.

We appreciate your contribution and apologize if this is a false positive!

Copilot

Pull request overview

This PR fixes a reliability gap in the Actions k8s client where InternalRunService.RecordAction failures can be returned as a successful transport response with a non-OK google.rpc.Status in the response body—previously causing rejected actions to be incorrectly deduplicated and never retried.

Changes:

Capture RecordAction responses in notifyRunService and only memoize the action key when the response-body status is OK.
Log response-body rejection status details when the run service rejects an action.
Add unit tests covering both “rejected stays retryable” and “accepted deduplicates” behavior.

Reviewed changes

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

File	Description
actions/k8s/client.go	Checks `RecordAction` response-body status before adding to the dedup filter; logs rejections.
actions/k8s/client_test.go	Adds tests ensuring rejected records are retried and accepted records are deduplicated.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

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

RecordAction reports application rejections in its body while returning a nil transport error. Memoizing those keys drops all retries, leaving rejected actions unrecorded. Only an explicit OK response is safe to memoize. Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot

Pull request overview

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

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot

Pull request overview

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

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot

🟢 Approval recommended

The change is narrowly scoped, addresses the stated failure mode directly, and is covered by targeted new/updated tests for both rejection and missing-response scenarios.

Review details

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Lite

kumare3 · 2026-09-05T23:47:54Z

+		resp, err := c.runClient.RecordAction(ctx, connect.NewRequest(recordReq))
+		if err != nil {
 			logger.Warnf(ctx, "Failed to record action in run service for %s: %v", update.ActionID.Name, err)
+		} else if resp == nil || resp.Msg == nil || resp.Msg.GetStatus() == nil {


Is it that - buf does not always return errors

Not a general Buf/Connect behavior. This handler returns application failures in resp.Msg.Status with err == nil; err only covers RPC-level failures. We therefore memoize only Status.Code == OK.

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot

🟢 Approval recommended

The change is targeted and well-covered by tests, and the only remaining feedback is a minor readability concern (identifier shadowing) rather than correctness.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot

🟡 Changes recommended

The new SQL-building test helper is likely to be flagged by gosec without an explicit suppression/justification, which can block CI.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Lite

…ode import Alias the googleapis rpc code package so it no longer shadows the local code variables in this file, and correct the comment to say a rejected action is memoized as recorded rather than never recorded. Document why the test table rename concatenates its identifiers. Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot

🟢 Approval recommended

The change directly addresses the described retry/dedup bug, matches RunService’s in-body status behavior, and is covered by focused new/updated tests for the failure and success paths.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot AI lite review requested due to automatic review settings August 20, 2026 20:19

github-actions Bot added the flyte2 label Aug 20, 2026

github-actions Bot added the Code agent slop label Aug 20, 2026

Copilot started reviewing on behalf of 1fanwang August 20, 2026 20:19 View session

Copilot AI reviewed Aug 20, 2026

View reviewed changes

Comment thread actions/k8s/client.go Outdated

Copilot AI review requested due to automatic review settings August 20, 2026 23:37

Copilot AI reviewed Aug 20, 2026

View reviewed changes

1fanwang changed the title ~~Don't mark an action recorded when the run service rejected it~~ fix(actions): retry rejected action records Aug 20, 2026

Copilot started reviewing on behalf of 1fanwang August 21, 2026 00:20 View session

Copilot AI review requested due to automatic review settings August 21, 2026 18:49

1fanwang force-pushed the record-action-check-body-status branch from 215e037 to 3d0cb91 Compare August 21, 2026 18:49

Copilot started reviewing on behalf of 1fanwang August 21, 2026 18:49 View session

Copilot AI reviewed Aug 21, 2026

View reviewed changes

Comment thread actions/k8s/client_test.go Outdated

Sovietaced removed the Code agent slop label Aug 25, 2026

fix(actions): use valid RecordAction response fixtures

12c4764

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot AI review requested due to automatic review settings August 26, 2026 07:31

Copilot started reviewing on behalf of 1fanwang August 26, 2026 07:32 View session

Copilot AI reviewed Aug 26, 2026

View reviewed changes

Comment thread actions/k8s/client_test.go Outdated

test(actions): match RecordAction response IDs

d8aaad1

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot AI review requested due to automatic review settings September 5, 2026 15:56

Copilot started reviewing on behalf of 1fanwang September 5, 2026 15:57 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

kumare3 reviewed Sep 5, 2026

View reviewed changes

test(actions): model a repository failure response

60d0815

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot AI review requested due to automatic review settings September 9, 2026 10:29

Copilot started reviewing on behalf of 1fanwang September 9, 2026 10:30 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread actions/k8s/client.go Outdated

test(runs): pin RecordAction repository error contract

55c61bb

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Copilot AI review requested due to automatic review settings September 9, 2026 10:37

Copilot started reviewing on behalf of 1fanwang September 9, 2026 10:37 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread actions/k8s/client.go Outdated

Comment thread runs/test/api/record_action_status_test.go

Copilot AI review requested due to automatic review settings September 9, 2026 10:58

Copilot started reviewing on behalf of 1fanwang September 9, 2026 10:58 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Uh oh!

Conversation

1fanwang commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Stack

Docs link

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

kumare3 Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

1fanwang Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1fanwang commented Aug 20, 2026 •

edited

Loading

1fanwang Sep 6, 2026 •

edited

Loading