Skip to content

fix(backend): fix review findings on the task manager retention settings [INFP-507] - #11253

Merged
saltas888 merged 16 commits into
task-history-retention-infp-507from
feat/hopeful-bardeen-o76qm4
Oct 28, 2026
Merged

saltas888 merged 16 commits into
task-history-retention-infp-507from
feat/hopeful-bardeen-o76qm4

Conversation

@saltas888

@saltas888 saltas888 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes for review findings on #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:

What changed

Behavioral changes:

  • The standalone task manager reads only the task_manager section of infrahub.toml and the INFRAHUB_TASK_MANAGER_* variables, plus INFRAHUB_LOG_LEVEL and INFRAHUB_PRODUCTION for 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_ONLY is set, the task manager warns that the process that runs Prefect's background services does not read task_manager.retention.
  • prefect_own_events longer than activity_log is 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 carry prefect_own_events as set. This reverses decision D5 (warn and cap) of the design doc.
  • An empty PREFECT_SERVER_SERVICES_DB_VACUUM_ENABLED environment variable no longer counts as set. Before, Prefect read it as false, so no task runs were deleted.
  • A preset PREFECT_SERVER_SERVICES_DB_VACUUM_ENABLED is no longer reported as overriding task_history. The task manager now warns for each cleanup that the switch leaves off (flow_runs or events) and names the retention that is not applied.
  • A retention set in Prefect's .env, prefect.toml, [tool.prefect] in pyproject.toml or 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.started and prefect.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.py runs 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:

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

  1. backend/infrahub/prefect_server/retention_settings.py (new: the task_manager section reader and the validator), then retention.py and app.py.
  2. Tests: backend/tests/unit/prefect_server/ and backend/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 though activity_log is not in effect then.
  • The source scan in test_prefect_event_types.py finds 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

Stack impact

PRs 2 to 6 need a rebase after this lands:

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


Generated by Claude Code

View guided diff

claude added 15 commits October 27, 2026 23:59
… in the task manager

The standalone task manager loaded and validated the whole Infrahub
configuration, so a setting it does not use, such as an invalid storage
driver, stopped it. It now reads only the task_manager section of the
configuration file and its INFRAHUB_TASK_MANAGER_* variables. An invalid
retention or an unreadable configuration file still stops it, and
distributed mode loads the whole configuration as before.

The retention settings move to a module that imports nothing else from
Infrahub, and infrahub.config imports them from there. The task manager
still sets up Infrahub's logging before Prefect builds its app, so the
retention lines stay visible.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…round services

With PREFECT__SERVER_WEBSERVER_ONLY set, the task manager runs only the
web server, and the process that runs Prefect's background services
does not read task_manager.retention, so it deletes no task runs. The
task manager now logs a warning at startup that says so, instead of only
logging the retention as if it were in effect.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…ity_log

A prefect_own_events longer than activity_log has no effect, because
every event older than activity_log is deleted anyway. Infrahub used to
accept it, cap it at runtime and log a warning, and the cap used
activity_log even when PREFECT_SERVER_EVENTS_RETENTION_PERIOD was set
and activity_log was not in effect. The settings now refuse such a value
at startup, and the per-type overrides carry prefect_own_events as
configured, since Prefect caps each override by the event retention it
uses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
A compose or Helm file that passes PREFECT_SERVER_SERVICES_DB_VACUUM_ENABLED
through from an unset host variable gives the task manager an empty value.
Infrahub kept it as an operator setting, and Prefect read it as false, so
finished task runs were never deleted. An empty or blank retention variable
is now removed and replaced with the value derived from the task manager
retention, with no warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
… turns off

A preset PREFECT_SERVER_SERVICES_DB_VACUUM_ENABLED was reported as
overriding task_history, although task_history still set the run
retention, and a value without events silently stopped all event
deletion. The switch is now kept without that warning. After Prefect's
settings are refreshed, the task manager warns when the switch turns off
the deletion of task runs or events, and names the retention that is
not applied.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…g the values Prefect applies

Prefect also reads its settings from .env, prefect.toml, pyproject.toml
and the active profile, and an environment variable wins over all of
them. A retention set in one of those files was silently replaced by the
value Infrahub wrote. The task manager now writes a variable only when no
Prefect source sets that setting, warns about the ones it leaves, and
logs the values from Prefect's parsed settings after the refresh.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…retention

With the default 7-day activity log and own-event retention, the task
manager wrote a per-type override for every listed Prefect event type,
so Prefect ran over 200 delete statements every hour that removed
nothing its general event cleanup does not remove. The overrides are now
written as an empty mapping when the own-event retention equals the
activity log retention and Infrahub sets the event retention. The rows
deleted do not change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…list against Prefect's source

Prefect's per-type event retention matches exact names, and the list of
Prefect event types missed the two events that prefect transfer sends.
They are now listed, with the run events of the Rerunning state name
that Prefect's server sets on task runs. New unit tests read the
installed Prefect package and fail when it sends an event name, builds a
name pattern or defines a status, lifecycle record kind or state name
that the list does not cover, so a Prefect upgrade that adds one fails
CI instead of keeping those events for the activity log retention.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
The test ran Prefect's event cleanup on the database that every
component test in the same worker shares, so it deleted any old event
another test had seeded there. It now runs on an empty, migrated SQLite
database in its own temporary directory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…ests

The state constructors were typed as returning object, so ty, which CI
runs, reported an invalid argument where their states are added to the
list of Prefect states. They are now typed as returning Prefect's client
or server state.

The test docstring and research.md also said the tests read every name
built from the known patterns. They do not read the lifecycle actions,
the concurrency limit phases, the asset materialization statuses or a
name passed through a helper with another parameter name, so the text
now says so. plan.md now names the unit tests that check the event type
list against Prefect's source.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
… an empty retention variable does

An empty PREFECT_SERVER_SERVICES_DB_VACUUM_RETENTION_PERIOD or
PREFECT_SERVER_EVENTS_RETENTION_PERIOD makes the import of prefect.context
fail with a time_delta_parsing error, and an empty
PREFECT_SERVER_SERVICES_DB_VACUUM_EVENT_RETENTION_OVERRIDES with a
SettingsError. Both happen when the task manager app module or the infrahub
command line is imported, before any Infrahub code can remove the variable,
so removing them never took effect. Only the vacuum switch is now removed
when it is empty.

The changelog and the specs now say that an empty vacuum switch gives way to
a value from Prefect's settings files or profile, with the usual warning for
each cleanup it turns off, and otherwise to events,flow_runs, and that an
empty retention variable still stops the task manager at startup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…an activity_log

The refusal said the task manager deletes every event older than
activity_log, which is false when a Prefect setting already sets the
event retention or the vacuum switch turns off the event cleanup. The
message now asks the operator to set prefect_own_events to at most the
activity_log value.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
The loader also raises tomllib.TOMLDecodeError, UnicodeDecodeError and
OSError when the configuration file cannot be parsed, decoded or read,
so its Raises section now names them. The configuration contract now
says that an invalid INFRAHUB_LOG_LEVEL or INFRAHUB_PRODUCTION still
stops the standalone task manager, because it reads both variables to
set up its logging.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
…eady asserts

The equal_retentions case wrote the 30d, 7d and 7d retentions with nothing
preset and checked that the per-type overrides are empty. The test that
applies every missing variable uses the same settings and checks the whole
environment, overrides included, so it already pinned this. Both failed
together when the skip of equal overrides was reverted. The case is
removed and the apply test now says why its overrides are empty.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
Say that refusing a prefect_own_events longer than activity_log is about the
case where the task manager sets the event retention, that only an empty
vacuum switch in the environment is removed, that an empty retention variable
stops the task manager only under the name Prefect reads, and that the
standalone task manager also reads the two logging variables.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwJNFw8rccyxc4Wp9eZFbH
@github-actions github-actions Bot added type/documentation Improvements or additions to documentation group/backend API server, GraphQL, task worker (Prefect), database, Python code type/spec A specification for an upcoming change to the project labels Oct 28, 2026
@codspeed

codspeed Bot commented Oct 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 1 benchmark is dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 13 untouched benchmarks


Comparing feat/hopeful-bardeen-o76qm4 (b5964c4) with task-history-retention-infp-507 (ff6a8ce)1

Open in CodSpeed

Footnotes

  1. No successful run was found on task-history-retention-infp-507 (a8c7fc9) during the generation of this report, so 2bb5899 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@saltas888
saltas888 merged commit b5964c4 into task-history-retention-infp-507 Oct 28, 2026
55 checks passed
@saltas888
saltas888 deleted the feat/hopeful-bardeen-o76qm4 branch October 28, 2026 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend API server, GraphQL, task worker (Prefect), database, Python code type/documentation Improvements or additions to documentation type/spec A specification for an upcoming change to the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants