Conversation
github-actions
Bot
added
the
group/backend
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
changelog/6906.added.md (1)
1-1: Enhance the changelog entry for clarity and completeness.The entry could be improved in several ways:
- Markdown formatting: Use backticks around the environment variable name for better readability.
- Completeness: The entry only mentions the environment variable but not the underlying configuration field (
WorkflowSettings.worker_events_retention_period) that it overrides.- Purpose: Explain what this setting controls (Prefect's event retention period to prevent unbounded growth of the event store).
- Constraints: Mention the minimum value of 1 day.
Since the referenced markdown guidelines (
dev/guidelines/markdown.md) were not provided, please verify this entry complies with the project's changelog formatting standards.📝 Suggested improvement
-Added new optional env var INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD (integer, days) with default of 7d. +Added configurable retention period for Prefect workflow events via `WorkflowSettings.worker_events_retention_period` (default: 7 days, minimum: 1 day). Can be overridden using the optional environment variable `INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD`. This prevents unbounded growth of Prefect's internal event store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changelog/6906.added.md` at line 1, Update the changelog entry to use markdown backticks for the environment variable name (`INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD`), state that it overrides the `WorkflowSettings.worker_events_retention_period` configuration field, explain that it controls Prefect's worker event retention period (to prevent unbounded growth of the event store), and include the default value (7 days) plus the minimum allowed value (1 day); ensure the phrasing matches the project's changelog style (brief, declarative sentence) and formatting conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@changelog/6906.added.md`:
- Line 1: Update the changelog entry to use markdown backticks for the
environment variable name (`INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD`),
state that it overrides the `WorkflowSettings.worker_events_retention_period`
configuration field, explain that it controls Prefect's worker event retention
period (to prevent unbounded growth of the event store), and include the default
value (7 days) plus the minimum allowed value (1 day); ensure the phrasing
matches the project's changelog style (brief, declarative sentence) and
formatting conventions.
| description="Threshold for caching flow run counts (0 to always cache, higher values to disable)", | ||
| ) | ||
| worker_events_retention_period: int = Field( | ||
| default=7, ge=1, description="Specify the number of days to retain events for (days)" |
There was a problem hiding this comment.
I think (days) can be removed
days is already specified earlier in the description
github-actions
Bot
added
the
type/documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/infrahub/config.py (1)
425-427: LGTM! Optional: improve description wording.The field definition is correct. Optionally, the description
"Specify the number of days to retain events for"(ends with a dangling preposition) reads better as"Specify the number of days to retain worker events"— this also propagates to the auto-generated docs.✏️ Proposed description wording improvement
worker_events_retention_period: int = Field( - default=7, ge=1, description="Specify the number of days to retain events for" + default=7, ge=1, description="Specify the number of days to retain worker events" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/config.py` around lines 425 - 427, Update the Field description for worker_events_retention_period to a clearer phrasing: replace "Specify the number of days to retain events for" with something like "Specify the number of days to retain worker events" so the auto-generated docs read cleanly; locate the Field declaration for worker_events_retention_period in config.py and update its description argument accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/prefect_server/app.py`:
- Around line 45-49: The code sets PREFECT_EVENTS_RETENTION_PERIOD by
concatenating events_retention_days with "d" but does not validate the fallback
env var INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD; change the fallback to
parse and validate using int(), e.g. attempt to int(os.environ.get(..., "7"))
and on ValueError fall back to 7 (or config.SETTINGS value), then set
os.environ["PREFECT_EVENTS_RETENTION_PERIOD"] = f"{validated_days}d"; apply the
same int-casting/validation fix in places that construct
PREFECT_EVENTS_RETENTION_PERIOD (e.g. the code that formats
events_retention_days in infrahub_async.py and any Dockerfile environment
construction).
---
Duplicate comments:
In `@development/Dockerfile`:
- Around line 90-91: Update the Dockerfile environment default for
PREFECT_EVENTS_RETENTION_PERIOD to use ISO 8601 duration format by replacing the
string "7d" with "P7D" so it matches the runtime change in app.py; locate the
PREFECT_EVENTS_RETENTION_PERIOD entry in the Dockerfile and set
PREFECT_EVENTS_RETENTION_PERIOD="P7D".
---
Nitpick comments:
In `@backend/infrahub/config.py`:
- Around line 425-427: Update the Field description for
worker_events_retention_period to a clearer phrasing: replace "Specify the
number of days to retain events for" with something like "Specify the number of
days to retain worker events" so the auto-generated docs read cleanly; locate
the Field declaration for worker_events_retention_period in config.py and update
its description argument accordingly.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/infrahub/prefect_server/app.py`:
- Around line 47-49: The code assigns events_retention_days via
int(os.environ.get("INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD", "7"))
which can raise ValueError for non-numeric input and allows zero/negative
values; change this to parse the env var safely (catch ValueError or use a
helper to safe-int) and enforce a minimum of 1 before setting
os.environ["PREFECT_EVENTS_RETENTION_PERIOD"]; reference the variable/events key
names events_retention_days, INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD
and the env output PREFECT_EVENTS_RETENTION_PERIOD when implementing the
validation and fallback logic.
| @@ -0,0 +1 @@ | |||
| Added new optional env var INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD (integer, days) with default of 7d. | |||
There was a problem hiding this comment.
I'd say this should be with default of 7 b/c the user does not include the d in their env var, we add that before sending to Prefect, right?
|
This PR has been inactive for 2 weeks and has been marked as stale. If you're still working on this, please:
The stale label will be removed automatically when there's new activity. |
| PREFECT_API_DATABASE_TIMEOUT=60 \ | ||
| PREFECT_FLOWS_HEARTBEAT_FREQUENCY=30 | ||
| PREFECT_FLOWS_HEARTBEAT_FREQUENCY=30 \ | ||
| PREFECT_EVENTS_RETENTION_PERIOD="7d" |
There was a problem hiding this comment.
I am surprised we have to add this to the Dockerfile.
BeArchiTek
left a comment
BeArchiTek
left a comment
There was a problem hiding this comment.
Could you run the invoke task to update the env variable in the docker compose files so INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD is present
|
This PR has been inactive for 2 weeks and has been marked as stale. If you're still working on this, please:
The stale label will be removed automatically when there's new activity. |
…-1702 Resolve development/Dockerfile conflict by taking develop's restructured version and dropping the PREFECT_EVENTS_RETENTION_PERIOD ENV line: the prefect server app sets it at startup from config, so a Dockerfile default is redundant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD to docker-compose.yml via 'invoke release.gen-config-env --update-docker-file' - Reword the setting description and regenerate the configuration reference - Clarify the changelog fragment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Superseded, I'd suggest we close this. This targets Worth flagging that the vacuum service doesn't solve #6906 on its own either. Overrides can only shorten retention, never extend it (effective retention is Tracked in IFC-2934. Thanks for the work here @solababs, the config surface is still roughly what we'll want, just applied at a different layer. |
4 participants
Why
Prefect's internal event store grows unbounded by default, which can lead to increased memory/disk usage on long-running Infrahub workers. There is currently no way to configure how long Prefect events are retained.
Goal: Expose a configurable retention period for Prefect events so anyone can control how long workflow event data is kept.
Non-goals: This does not change any other Prefect lifecycle or cleanup behavior.
Closes #6906
What changed
New config setting: Added worker_events_retention_period (integer, days, default 7, minimum 1) to WorkflowSettings.
Wire it into the worker: The new setting is passed to Prefect via PREFECT_EVENTS_RETENTION_PERIOD in the worker's temporary_settings block, formatted as "d" (e.g. "7d").
How to review
Check new Field definition on WorkflowSettings
Check passing the setting to Prefect's temporary_settings
How to test
Impact & rollout
Backward compatibility:
No breaking changes. The new setting has a sensible default (7 days); existing deployments require no configuration changes.
Performance:
Should improve long-running worker performance by bounding event store growth.
Config/env changes:
New optional env var INFRAHUB_WORKFLOW_WORKER_EVENTS_RETENTION_PERIOD (integer, days).
Deployment notes:
Safe to deploy without coordination. The setting takes effect on worker restart.
Checklist
uv run towncrier create ...)Summary by CodeRabbit
New Features
Documentation