-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Make ReleaseFile
TTI by tracking its last access time
#95867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This brings the legacy `ReleaseFile`s in line with how debug files and `ArtifactBundle`s are being expired. My hope was to just be able to completely deprecate and delete all of `ReleaseFile`, but even though the usage of these is well below <1%, the deprecation isn’t really going anywhere. So might as well just give these a access-time-based TTI and move on. Most of the code is just a straight up copy of how this was done for debug files, except for the actual deletion job, which still needs to be implemented.
This PR has a migration; here is the generated SQL for for --
-- Add field date_accessed to releasefile
--
ALTER TABLE "sentry_releasefile" ADD COLUMN "date_accessed" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks good.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #95867 +/- ##
==========================================
- Coverage 87.74% 79.25% -8.50%
==========================================
Files 10582 10583 +1
Lines 610032 610124 +92
Branches 23976 23976
==========================================
- Hits 535276 483548 -51728
- Misses 74472 126292 +51820
Partials 284 284 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incorrect Metadata Update Affects File Cleanup
The ReleaseFile.update()
method incorrectly updates the date_accessed
field when the name
is changed and ident
is not provided. This field is intended for TTI (Time To Idle) to track read access, not metadata modifications. This behavior incorrectly resets the TTI timer, preventing proper cleanup of files that have not been genuinely accessed.
src/sentry/models/releasefile.py#L117-L118
sentry/src/sentry/models/releasefile.py
Lines 117 to 118 in 8d27d60
kwargs["ident"] = self.ident = type(self).get_ident(kwargs["name"], dist_name) | |
kwargs["date_accessed"] = timezone.now() |
Bug: Inconsistent Timestamps in File Renewal
A race condition exists due to inconsistent now
and threshold_date
calculations. The maybe_renew_releasefiles
function identifies files for renewal based on a computed threshold_date
, but renew_releasefiles_by_id
recomputes these values independently. This breaks the intended timestamp consistency, potentially causing files selected for renewal to not be updated if the threshold_date
changes between the two function calls.
src/sentry/debug_files/release_files.py#L15-L32
sentry/src/sentry/debug_files/release_files.py
Lines 15 to 32 in 8d27d60
def maybe_renew_releasefiles(releasefiles: list[ReleaseFile]): | |
# We take a snapshot in time that MUST be consistent across all updates. | |
now = timezone.now() | |
# We compute the threshold used to determine whether we want to renew the specific bundle. | |
threshold_date = now - timedelta(days=AVAILABLE_FOR_RENEWAL_DAYS) | |
# We first check if any file needs renewal, before going to the database. | |
needs_bump = [rf.id for rf in releasefiles if rf.date_accessed <= threshold_date] | |
if not needs_bump: | |
return | |
renew_releasefiles_by_id(needs_bump) | |
def renew_releasefiles_by_id(releasefile_ids: list[int]): | |
now = timezone.now() | |
threshold_date = now - timedelta(days=AVAILABLE_FOR_RENEWAL_DAYS) |
Was this report helpful? Give feedback by reacting with 👍 or 👎
This brings the legacy
ReleaseFile
s in line with how debug files andArtifactBundle
s are being expired.My hope was to just be able to completely deprecate and delete all of
ReleaseFile
, but even though the usage of these is well below <1%, the deprecation isn’t really going anywhere.So might as well just give these a access-time-based TTI and move on.
Most of the code is just a straight up copy of how this was done for debug files (#52257) over 2 years ago, except for the actual deletion job, which still needs to be implemented.