fix(backend): fix review findings on the task manager retention settings [INFP-507]
Fixes for review findings on opsmill/infrahub#10862 (PR 1 of the INFP-507 stack). This PR targets task-history-retention-infp-507, so the author can merge it into #10862.
Why
A code review of #10862 found cases where the retention settings do not apply, or where task runs or events are deleted earlier than the operator expects. This PR fixes the must-fix and should-fix findings. Each finding was checked against the code and the Prefect 3.8.6 source before it was fixed, and each fix was checked again afterwards.
Non-goals:
- No change to PRs 2 to 6. They need a rebase after this lands (see "Stack impact").
- The
infrahub tasks background-servicescommand stays in #10878.
What changed
Behavioral changes:
- The standalone task manager reads only the
task_managersection ofinfrahub.tomland theINFRAHUB_TASK_MANAGER_*variables, plusINFRAHUB_LOG_LEVELandINFRAHUB_PRODUCTIONfor its logging. An invalid value in another section no longer stops it. Distributed mode loads the whole configuration, as before. - Webserver-only warning: when
PREFECT__SERVER_WEBSERVER_ONLYis set, the task manager warns that the process that runs Prefect's background services does not readtask_manager.retention. prefect_own_eventslonger thanactivity_logis refused at startup (@model_validator) instead of being capped at runtime. Prefect already caps each override by the event retention it uses, so the overrides now carryprefect_own_eventsas set. This reverses decision D5 (warn and cap) of the design doc.- An empty
PREFECT_SERVER_SERVICES_DB_VACUUM_ENABLEDenvironment variable no longer counts as set. Before, Prefect read it asfalse, so no task runs were deleted. - A preset
PREFECT_SERVER_SERVICES_DB_VACUUM_ENABLEDis no longer reported as overridingtask_history. The task manager now warns for each cleanup that the switch leaves off (flow_runsorevents) and names the retention that is not applied. - A retention set in Prefect's
.env,prefect.toml,[tool.prefect]inpyproject.tomlor the active profile now takes precedence, with a warning, like an environment variable. Before, Infrahub replaced it without a warning. The startup log shows the values Prefect parsed. - Per-type event overrides are written only when they shorten the event retention. With the defaults (7d and 7d) the overrides are
{}, so Prefect no longer runs 103 per-type delete passes every hour. - Prefect event list: it adds
prefect.workspace.transfer.startedandprefect.workspace.transfer.completed. New unit tests scan the installed Prefect source for event names, so a Prefect upgrade that adds an event fails CI. test_event_retention.pyruns on a database of its own, so it no longer deletes events seeded by other component tests in the same worker.- The changelog fragment, the spec set and the generated configuration reference are updated to match.
Findings not fixed:
- "Importing
infrahub.configchanges the standalone task manager logging" was not reproduced. Theprefect.collectionsentry point already importedinfrahub.logbefore #10862. With #10862 the root logger stays at Prefect's WARNING level. - The minor findings (duplicated logic in
tasks/docs.py, schema regeneration on every call, constants copied intest_retention.py) are not in this PR.
What stayed the same: no Prefect database schema change, Infrahub events are untouched, and the setting names, defaults and variable names are unchanged.
How to review
backend/infrahub/prefect_server/retention_settings.py(new: thetask_managersection reader and the validator), thenretention.pyandapp.py.- Tests:
backend/tests/unit/prefect_server/andbackend/tests/unit/config/test_task_manager_retention_settings.py.
Worth extra scrutiny:
- The refusal also applies when the operator presets
PREFECT_SERVER_EVENTS_RETENTION_PERIOD, even thoughactivity_logis not in effect then. - The source scan in
test_prefect_event_types.pyfinds event names written as string literals or f-strings only.
How to test
uv run pytest backend/tests/unit/config/test_task_manager_retention_settings.py backend/tests/unit/prefect_server/ -n 0
uv run pytest backend/tests/component/task_manager/test_event_retention.py -n 0
Local results: 163 unit tests pass, and ruff, mypy, ty check . and docs.validate pass. The component test was not run (no Docker in the environment), so CI is its first run.
Impact & rollout
- Backward compatibility: a configuration with
prefect_own_eventslonger thanactivity_lognow stops the task manager. Loweringactivity_logbelow 7 days needsprefect_own_eventsset no longer than it. - Performance: with the defaults, Prefect runs 103 fewer delete passes every hour.
- Deployment notes: when the background services run in their own process, that process must run
infrahub tasks background-services(#10878). The Helm chart also needs opsmill/infrahub-helm#100.
Stack impact
PRs 2 to 6 need a rebase after this lands:
- #10869:
task_history.pyreads the retention period without checking thatflow_runsis in the vacuum switch. The cleanup command should refuse, or say that deletion is off, when it is not. - #10877: the
app.pyimport line and the lifespan setup conflict. The startup check for unlisted event types should skip when no override is in effect. The integration test assumes overrides under the 7d/7d defaults. - #10878:
apply_infrahub_settings_to_prefecttakes a config file path. The test that expectsP7D for <count> event typesnow gets{}. - #10879: the precedence section in
tasks.mdx, andactivity-log.mdx("capped with a warning" is now a refusal at startup).
Suggested change to the description of #10862: "Ship with #10869 and #10878 in the same release", because without #10878 a separate background-services process deletes no task runs.
Checklist
- Tests added/updated
- Changelog entry added
- External docs updated (#10879 needs the updates listed above)
- Internal .md docs updated (spec set)
- I have reviewed AI generated content
Generated by Claude Code