fix: allow paused actions to reach terminal phases

[anxkhn]

Tracking Issue

NA

Why are the changes needed?

actionRepo.UpdateActionPhase guards writes with phase <= $requested OR phase = ANY(retryablePhases), where the retryable set is FAILED and TIMED_OUT. ACTION_PHASE_PAUSED is enum value 9, while ACTION_PHASE_SUCCEEDED is 5 and ACTION_PHASE_TIMED_OUT is 8, so a paused condition action can never leave PAUSED: both the signal path and the timeout path fail the predicate. The statement updates zero rows, the repository still returns nil, and UpdateActionStatus reports success while the actions informer goes on to persist the signal output against a row that is still PAUSED.

The practical effect is that human-in-the-loop conditions stay listed and filtered as awaiting input after they have been answered or timed out, no end time is ever recorded, and historical action details plus paused-run listings are permanently wrong.

Reproduction: persist a condition action, move it QUEUED -> PAUSED, then call UpdateActionStatus with SUCCEEDED or TIMED_OUT. The RPC returns OK, but GetAction still returns PAUSED.

What changes were proposed in this pull request?

• Replace the ordinal phase comparison in UpdateActionPhase with an explicit map of allowed source phases per target phase. Forward progress through the running phases and the existing retry-from-FAILED/TIMED_OUT behavior are preserved, PAUSED becomes a valid source for SUCCEEDED, TIMED_OUT, ABORTED, FAILED and the retry phases, and completed actions can no longer be moved backwards.
• Stop reporting a rejected update as success. When the statement matches no row, the repository checks whether the action exists and returns ErrPhaseTransitionRejected (guard rejected the transition) or sql.ErrNoRows (no such action) instead of nil.
• updateSingleActionStatus treats ErrPhaseTransitionRejected as a no-op and returns before writing the output and run info, so a stale or out-of-order event can no longer overwrite the output of an action it was not allowed to transition.
• Tests: repository coverage for PAUSED -> SUCCEEDED/TIMED_OUT/ABORTED including the persisted end time, tightened assertions on the existing backwards-transition tests, and a condition service test asserting that a rejected transition does not persist the signal output.

Note for reviewers

The explicit table also rejects a few transitions that the ordinal check happened to permit, notably SUCCEEDED -> FAILED/ABORTED/TIMED_OUT and moving into RECOVERED from a terminal phase. That follows from treating completed actions as final, but if any of those are relied on in practice I am happy to add the specific entries back.

How was this patch tested?

go test ./runs/repository/impl ./runs/service

The new repository test drives the real QUEUED -> PAUSED -> terminal sequence against the test database and asserts both the stored phase and that ended_at is set. The service test uses the mocked action repo to assert that a rejected transition short-circuits before the output write and still returns an OK status to the caller.

Setup process

No setup or migration changes; the fix is confined to the update predicate and its error handling.

Screenshots

NA

Check all the applicable boxes

• All new and existing tests passed.
• All commits are signed-off.
• To the best of my knowledge, the proposed patch does not introduce new security risks.
• Documentation updates are needed (no user-facing docs cover this guard).

[Copilot]

Pull request overview

This PR fixes a phase-guard bug in the runs/actions repository that prevented condition actions from leaving PAUSED due to enum ordinal ordering, and makes rejected/ignored phase updates observable to callers so downstream writes (e.g., signal output) don’t persist against an unchanged action phase.

Changes:

• Replaces ordinal phase comparison in UpdateActionPhase with an explicit allowed-source transition table (including PAUSED -> terminal).
• Returns an explicit error on rejected transitions (vs silently succeeding) and short-circuits UpdateActionStatus output persistence on rejected transitions.
• Adds/updates repository and service tests around paused terminal transitions and rejected updates.

Reviewed changes

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

Show a summary per file

File	Description
runs/service/internal_run_service.go	Treats rejected phase transitions as no-ops to prevent persisting output/run info for stale events.
runs/service/condition_test.go	Adds a test ensuring rejected condition transitions don’t persist output.
runs/repository/interfaces/action.go	Introduces ErrPhaseTransitionRejected for explicit transition rejection signaling.
runs/repository/impl/action.go	Implements explicit allowed-source phase transition guard + distinguishes rejected transition vs missing action.
runs/repository/impl/action_test.go	Adds coverage for PAUSED -> terminal transitions and tightens backward-transition assertions.

Suppressed comments (1)

runs/repository/impl/action.go:35

• PAUSED is not included as an allowed source for transitioning into FAILED, even though the PR description calls out PAUSED as a valid source for FAILED. As written, a paused action still cannot be marked failed via UpdateActionPhase.

	common.ActionPhase_ACTION_PHASE_FAILED:                {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_INITIALIZING, common.ActionPhase_ACTION_PHASE_RUNNING, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT},

---

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

[pingsutw]

Do you have an example that I can repro?

[Copilot]

Pull request overview

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

Suppressed comments (2)

runs/service/internal_run_service.go:276

• sql.ErrNoRows (action not found) is treated as connect.CodeNotFound, but it is logged as a warning first. This will create noisy warning logs for an expected NotFound outcome; handle NotFound before logging the warning (or log at a lower level).

		logger.Warnf(ctx, "UpdateActionStatus: failed to update action %s: %v", req.GetActionId().GetName(), err)
		if errors.Is(err, sql.ErrNoRows) {
			return connect.NewError(connect.CodeNotFound, err)
		}
		return connect.NewError(connect.CodeInternal, err)

runs/repository/impl/action.go:457

• allowedActionPhaseSources[phase] is indexed without checking presence. If the ActionPhase enum gains a new value and the map isn’t updated, this will silently reject all transitions to that phase (via an empty ANY('{}')), returning ErrPhaseTransitionRejected and making the failure hard to diagnose. Add an explicit guard for unknown target phases.

	allowedSources := allowedActionPhaseSources[phase]
	allowedSourceValues := make([]int32, len(allowedSources))
	for i, source := range allowedSources {
		allowedSourceValues[i] = int32(source)
	}

[anxkhn]

@pingsutw thanks for the review. i also addressed the remaining findings: paused actions can enter retry phases and failed, missing actions return not found, and the transition matrix has direct coverage. the minimal repro is paused followed by succeeded, failed, or timed_out; previously the rpc returned ok while the stored phase remained paused. rebased onto the latest main and the focused repository and service tests pass.

[Copilot]

Pull request overview

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

Suppressed comments (1)

runs/repository/impl/action.go:34

• The transition table allows ACTION_PHASE_PAUSED as a source for non-terminal targets like QUEUED/WAITING_FOR_RESOURCES/INITIALIZING/RUNNING. This conflicts with the documented condition-action flow in flyteidl2/common/phase.proto (PAUSED -> terminal only) and the condition reconciler’s comment/state machine (created -> Paused -> {Succeeded|Failed}). If PAUSED is intended to be condition-only, consider restricting PAUSED to self+terminal transitions (or updating the docs to match the broader transition semantics).

	common.ActionPhase_ACTION_PHASE_QUEUED:                {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES: {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_INITIALIZING:          {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_INITIALIZING, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_RUNNING:               {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_INITIALIZING, common.ActionPhase_ACTION_PHASE_RUNNING, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_SUCCEEDED:             {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_INITIALIZING, common.ActionPhase_ACTION_PHASE_RUNNING, common.ActionPhase_ACTION_PHASE_SUCCEEDED, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},

[Copilot]

Pull request overview

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

Suppressed comments (4)

runs/repository/impl/action.go:457

• allowedActionPhaseSources[phase] assumes every possible common.ActionPhase value is present in the map. If a newer client sends an enum value unknown to this server, allowedSources will be nil/empty and the update will be treated as a rejected transition (and potentially ignored by callers that treat rejection as a no-op). It’s safer to detect missing keys and return an explicit error for unsupported target phases.

	now := time.Now()
	allowedSources := allowedActionPhaseSources[phase]
	allowedSourceValues := make([]int32, len(allowedSources))
	for i, source := range allowedSources {
		allowedSourceValues[i] = int32(source)
	}

runs/service/internal_run_service.go:283

• ErrPhaseTransitionRejected is treated as a no-op here, but mergeRunInfoOutput runs earlier in this function when Output/Principal are set. That means a rejected/out-of-order terminal update can still overwrite detailed_info even though the phase transition was rejected, contradicting the intended short-circuit behavior for stale events. Consider updating the phase first, and only persisting the output/run info after a successful phase update (and after handling sql.ErrNoRows / rejection).

		if errors.Is(err, interfaces.ErrPhaseTransitionRejected) {
			return nil
		}
		logger.Warnf(ctx, "UpdateActionStatus: failed to update action %s: %v", req.GetActionId().GetName(), err)
		if errors.Is(err, sql.ErrNoRows) {

runs/service/condition_test.go:138

• This test claims to cover rejected transitions, but it doesn’t include Output/Principal and therefore won’t catch regressions where a rejected transition still persists RunInfo (e.g., via GetAction/UpdateActionDetailedInfo). To validate the intended behavior, include an Output (or Principal) and ensure no RunInfo write calls occur when UpdateActionPhase returns ErrPhaseTransitionRejected.

func TestUpdateActionStatus_RejectedConditionTransitionReturnsSuccess(t *testing.T) {
	actionRepo, _, svc := newTestServiceWithTaskRepo(t)

	actionRepo.On("UpdateActionPhase", mock.Anything, testActionID,
		common.ActionPhase_ACTION_PHASE_SUCCEEDED, mock.Anything, mock.Anything, mock.Anything, mock.Anything).

runs/repository/impl/action.go:33

• The new transition table allows moving from PAUSED into non-terminal running phases (e.g. PAUSED -> RUNNING/INITIALIZING/...). This conflicts with the documented condition-action flow (PAUSED -> {SUCCEEDED|TIMED_OUT|ABORTED} in gen/go/flyteidl2/common/phase.pb.go) and re-opens the possibility of a stale non-terminal update “resuming” a paused condition. If PAUSED is intended only for condition actions, consider removing ACTION_PHASE_PAUSED from the allowed sources for non-terminal target phases.

	common.ActionPhase_ACTION_PHASE_QUEUED:                {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES: {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_INITIALIZING:          {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_INITIALIZING, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},
	common.ActionPhase_ACTION_PHASE_RUNNING:               {common.ActionPhase_ACTION_PHASE_UNSPECIFIED, common.ActionPhase_ACTION_PHASE_QUEUED, common.ActionPhase_ACTION_PHASE_WAITING_FOR_RESOURCES, common.ActionPhase_ACTION_PHASE_INITIALIZING, common.ActionPhase_ACTION_PHASE_RUNNING, common.ActionPhase_ACTION_PHASE_FAILED, common.ActionPhase_ACTION_PHASE_TIMED_OUT, common.ActionPhase_ACTION_PHASE_PAUSED},