Skip to content

Remove scheduler-side retry policy re-validation - #5090

Open
dejanzele wants to merge 1 commit into
armadaproject:masterfrom
dejanzele:retry-policy-single-validation-home
Open

Remove scheduler-side retry policy re-validation#5090
dejanzele wants to merge 1 commit into
armadaproject:masterfrom
dejanzele:retry-policy-single-validation-home

Conversation

@dejanzele

Copy link
Copy Markdown
Member

Retry policies were validated in two places: the CRUD service at write time, and again in the scheduler when ConvertPolicy compiled a stored policy. The two copies checked the same rules (name set, action valid, category set). A rule update had to land in both places, and it was easy to miss one.

This PR makes write-time validation the single home. ConvertPolicy now only parses. It still returns an error for data it cannot map, for example an unknown action enum value. That is corruption handling, not re-validation: the fail-open policy cache logs and skips a policy that fails to convert. The engine-side ValidatePolicy and its tests are removed. The server-side validation and its full test table are unchanged.

The retry policy feature is not live, so no stored policy predates the write-time rules. Nothing needs a migration.

…tion is the single home

Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR consolidates retry-policy validation in the CRUD service by making scheduler conversion responsible only for parsing values it can map.

  • Removes the scheduler-side ValidatePolicy call and validation helpers.
  • Removes the corresponding engine validation tests.
  • Retains conversion errors for nil policies, nil rules, and unknown or unspecified action values.

Confidence Score: 5/5

The PR appears safe to merge because all current policy writes remain validated and scheduler conversion still rejects values it cannot map.

Production retry policies flow through validated create or update operations before the cache invokes ConvertPolicy, and no alternate production constructor or unvalidated storage path was found.

Important Files Changed

Filename Overview
internal/scheduler/retry/convert.go Removes redundant semantic re-validation while retaining errors for policy data that cannot be converted.
internal/scheduler/retry/types.go Removes the now-unused scheduler-side policy and rule validation helpers.
internal/scheduler/retry/engine_test.go Removes tests dedicated to the deleted validation helpers without reducing engine evaluation coverage.

Reviews (1): Last reviewed commit: "Remove scheduler-side retry policy re-va..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant