Skip to content

fix(attachments): Delete based on date_expires in cleanup job#111955

Open
matt-codecov wants to merge 4 commits intomasterfrom
matth/attachments-variable-expiry
Open

fix(attachments): Delete based on date_expires in cleanup job#111955
matt-codecov wants to merge 4 commits intomasterfrom
matth/attachments-variable-expiry

Conversation

@matt-codecov
Copy link
Copy Markdown
Contributor

@matt-codecov matt-codecov commented Mar 31, 2026

Depends on #111881 + #111980.

Attachments now store a per-record date_expires set by the application based on the organization's plan retention. The cleanup job previously deleted based on date_added, so this PR switches to date_expires.

Self-hosted problem: Self-hosted typically runs one cleanup job with a fixed --days=N. With date_expires as the cutoff field, the query becomes date_expires < now() - N days, which delays deletion by N days past expiry. For a 90-day plan with --days=90, attachments would live for 180 days — twice as long as intended.

Fix: Introduce a separate models_which_use_expiry_deletions() code path that always runs with days=0, independent of --days. It runs alongside the existing age-based path. No operator config changes needed — the job keeps running as-is and attachments are now deleted exactly when their date_expires is reached.

Closes FS-328

@matt-codecov matt-codecov requested a review from jan-auer March 31, 2026 21:26
@linear-code
Copy link
Copy Markdown

linear-code bot commented Mar 31, 2026

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 31, 2026
@jan-auer
Copy link
Copy Markdown
Member

jan-auer commented Apr 1, 2026

Looks good for production.

I also checked self-hosted and there's a single cleanup job running with event_retention_days (here) - which means we would be retaining event attachments too long there. Curious about your thoughts, which would be the better approach:

  • Try to separate out attachments in the self-hosted deployment.
  • Perform a minimal code change to ignore --days for expiry-based deletions.

…ariable-expiry

* origin/master: (96 commits)
  ref: update TanStack Query (#111884)
  feat(stacktrace): Add frontend hook and UI for on-demand SCM source context (#110327)
  ref(dynamic-sampling): remove all usage of custom ds rules in code (v2) (#111982)
  Remove experimental from logs (#112088)
  feat(onboarding): Add Litestar as a Python onboarding platform (FE) (#111607)
  ref(snapshots): Add skill and add InputGroup snapshot tests to test skill (#112074)
  fix(explorer): use issue.id filter for event timeseries (#112078)
  fix(preprod): Speed up error page on snapshots and improve error UI (#112076)
  chore(ACI): Add example data to payload for GitHub action in workflow creation (#112068)
  chore: Re-run `codeowners-coverage baseline` to trim output (#112062)
  fix(issues): Redesign new stack trace grid (#112059)
  feat(aci): Add types for DetectorInput and DataSourceInput (#112050)
  chore(ACI): Add a single feature flag to control issue alert rule backwards compatible endpoint flags (#112063)
  fix(ui): Increase node memory limit locally, decrease ci (#112066)
  feat(preprod): Add snapshot approval UI in header (#111977)
  ref(github): Do not return sentinel install option via API (#112038)
  feat(preprod): Add approval API endpoint and approval info in snapshot response (#111976)
  feat(coding integrations): add github copilot as an integration (frontend) (#111851)
  fix(grouping): Fix IPv6 parameterization (#111979)
  feat(autofix): Add seerDrawer to location (#112048)
  ...
Self-hosted runs a single cleanup job with a fixed --days value. Since
EventAttachment now deletes based on date_expires, using --days=30 would
delay deletion by 30 days past the attachment's expiry, effectively
doubling storage duration for plans with matching retention periods.

Introduce models_which_use_expiry_deletions() for models with per-record
expiry dates, and run that path with days=0 unconditionally alongside the
existing age-based path. No operator config changes needed.
@jan-auer jan-auer marked this pull request as ready for review April 2, 2026 09:07
Comment thread src/sentry/runner/commands/cleanup.py
Copy link
Copy Markdown
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go.

Blocking merge until backfill is complete. See #112095 (comment)

…piry test

Match the existing pattern used in other cleanup tests that pass
SynchronousTaskQueue to functions expecting _WorkQueue.
@jan-auer jan-auer requested a review from a team as a code owner April 2, 2026 12:17
@jan-auer jan-auer force-pushed the matth/attachments-variable-expiry branch from 223ecdb to e4c2fa6 Compare April 2, 2026 12:18
@jan-auer
Copy link
Copy Markdown
Member

jan-auer commented Apr 8, 2026

This PR can be merged on or after Apr 30, 20:00 UTC

@armenzg armenzg removed the request for review from a team April 13, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants