fix(slack): stop retrying replies rejected with cannot_reply_to_message

Merged
KN
2mo ago
+307-1

What type of PR is this?

/kind bug

What this PR does / why we need it:

When Slack rejects a thread reply with cannot_reply_to_message (for example, when the thread is closed or the bot cannot reply to it), the kelos-slack-server reporting cycle currently retries the post forever. Because the failed post prevented the task's kelos.dev/slack-report-phase annotation from being persisted, the reporter re-posted on every 30s tick — observed in production as two self-triggered tasks wedged in the accepted phase with 198+ failed attempts in a single hour.

This PR treats cannot_reply_to_message as a permanent API error:

  • Phase transitions (ReportTaskStatus): log the failure once and still persist the report phase, so the reporting cycle stops retrying that reply on every tick. HTTP-level failures (timeouts, 5xx, rate limits) keep the existing retry behavior.
  • Progress snapshots (updateProgress): mark the snapshot as posted on a permanent error so it is not re-attempted every cycle.

The permanent-error detection is isolated in a small isPermanentSlackError helper, living alongside the other SlackReporter helpers in internal/reporting/slack.go, so the classification is easy to extend to other permanent Slack API errors later.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

  • The new behavior is covered by unit tests:
    • TestSlackTaskReporter_CannotReplyToMessageNotRetried — permanent error: one attempt, phase annotation persisted, no further attempts on the next cycle.
    • TestSlackTaskReporter_TransientReplyErrorStillRetried — transient error: error surfaced, annotation not set, retry behavior unchanged.
    • TestSlackTaskReporter_PermanentProgressErrorNotReattempted — permanent error on a progress snapshot: single attempt, not re-attempted next cycle.
    • TestIsPermanentSlackError — classification of wrapped errors, other Slack API errors, HTTP status errors, and plain errors. Includes rate_limited, asserting rate limits stay transient and keep their existing retry behavior.
  • isPermanentSlackError matches slack.SlackErrorResponse by value only. That is the shape slack-go v0.20.0 returns from SlackResponse.Err() (misc.go:180, misc.go:195), Error() has a value receiver, and no path in the library returns a pointer, so a *SlackErrorResponse branch would be unreachable.
  • The permanent-error path uses break rather than an early return, so it falls through to the existing tail of ReportTaskStatus: setActivityTarget is skipped (firstReplyTS is empty), and terminal phases still reach clearProgressCache/clearActivityState before the phase is persisted.
  • Verified: go build ./..., go vet, gofmt, and go test ./internal/reporting/ all pass. The new tests fail against the pre-fix code. The repo-wide failures (internal/controller entrypoint tests, test/e2e, test/integration) are pre-existing environment issues (kubebuilder envtest binaries and a git sandbox limitation) that fail identically without this change.
  • make test-integration requires envtest binaries not available in the sandbox environment.

Does this PR introduce a user-facing change?

Slack task reports that Slack permanently rejects with cannot_reply_to_message are no longer retried on every reporting cycle, preventing repeated duplicate posts.

Summary by cubic

Stop retrying Slack thread replies rejected with cannot_reply_to_message by classifying them as permanent API errors. This prevents 30s retry loops, wedged tasks, and duplicate posts by persisting the report phase and suppressing progress posts.

  • Adds isPermanentSlackError that matches slack.SlackErrorResponse.Err == "cannot_reply_to_message"; HTTP failures and rate limits remain retryable.
  • Phase transitions: on permanent error, log once with channel and thread timestamp, persist kelos.dev/slack-report-phase, and set a per-task progress block so the next cycle does not attempt progress posts.
  • Progress updates: if blocked, return before reading pod logs; on permanent error, set the block and log channel and thread timestamp on failures.
  • The progress block is in-memory: a restart/leader failover may trigger one extra rejected post before re-blocking; it clears via clearProgressCache and is swept in SweepProgressCache.
  • Unit tests cover permanent vs transient paths, progress suppression, and error classification. No config or migration changes.