Repository navigation
Make S3 rollout for issue attachments time-driven (T1/T2) instead of … - #682
Merged
Merged
Conversation
…flag-driven
Previously, migrating a File attachment to S3 was a single atomic step:
upload bytes and immediately rewrite File.file.name to the S3 path in the
same command invocation. There was no record of *when* a file was
uploaded, and - contrary to the informal "day 1 upload, day 2 delete"
mental model - there was actually no code path that ever deleted a local
copy on a run after the one that uploaded it.
Add File.s3_uploaded_at (nullable). The management command's new two-phase
flow:
- Phase 1 (always runs on --execute): upload every File not yet on S3.
Does NOT touch file.name, so anytask keeps serving the local link.
- Phase 2 (only with --delete-local-copies): once a file's effective T2
threshold has passed, flip file.name to the S3 path and delete the
local copy.
T1 (default: s3_uploaded_at + 7 months) is when File.get_url() switches
from serving the local link to a virtual S3 link computed on the fly via
S3OverlayStorage.append_s3_prefix(), without mutating file.name. T2
(default: effective T1 + 8 days) is when the command performs the
flip+delete. Both are also settable as an absolute cutover datetime
(AWS_S3_SERVE_AT / AWS_S3_DELETE_LOCAL_AT) that overrides the per-file
relative delta for all uploaded files at once - an ops lever for forcing a
bulk rollout. See common/s3_rollout.py for the threshold logic.
Also adds File.create_time/update_time (matching the Issue model's
existing convention) as general hygiene, kept deliberately separate from
s3_uploaded_at so unrelated future edits can never corrupt the S3 rollout
schedule. Migration 0012 backfills create_time from Event.timestamp and
s3_uploaded_at with a safe synthetic sentinel for files already migrated
under the old algorithm.
Scope: issues.File only. users.UserProfile.avatar / s3migrate_user_avatars
/ the shared S3MigrateCommand base class are untouched (avatar's
ImageField is hardcoded to storage=OverwriteStorage(), a local-only
storage, so avatar URLs don't go through S3OverlayStorage today - fixing
that is separate, out-of-scope work).
Verified end-to-end: full issues/api/common test suite (no new failures
vs. baseline - same pre-existing locale/i18n failures before and after),
plus the S3-dependent tests (upload, get_url() state transitions,
delete+flip after T2, absolute override cutover) run for real against a
live DigitalOcean Spaces bucket.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_s3, fix flake8 - settings_docker.py: AWS_S3_SERVE_DELAY_DAYS, AWS_S3_DELETE_LOCAL_DELAY_DAYS, AWS_S3_SERVE_AT, AWS_S3_DELETE_LOCAL_AT are now overridable via env vars, matching the existing AWS_S3_PRESIGNED_URLS_ENABLED pattern. - issues/models.py: extract File.is_migrated_to_s3 property for readability, no behavior change. - issues/tests.py: wrap a line over 120 chars (flake8 E501). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lready-uploaded path issue.set_field_comment() re-wraps a previously uploaded solution file in a brand new File row when it's attached to a comment event (same storage path, same already-committed FieldFile bytes, no re-upload). That new row started with s3_uploaded_at=NULL and was wrongly treated as never-uploaded by get_url()'s T1 check, even though the underlying bytes had long since been pushed to S3 - breaking test_attached_file_in_s3 in CI (local run had skipped it, since minio wasn't reachable in the sandbox). File.save() now inherits s3_uploaded_at from any sibling row already pointing at the same path, for freshly created rows. Verified by reproducing the exact CI failure locally against a moto-backed S3 mock (matching CI's minio setup) with the fix reverted, then confirming it passes with the fix and the full common/issues/api suite shows the same 12 pre-existing, unrelated i18n failures and no new ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…flag-driven
Previously, migrating a File attachment to S3 was a single atomic step: upload bytes and immediately rewrite File.file.name to the S3 path in the same command invocation. There was no record of when a file was uploaded, and - contrary to the informal "day 1 upload, day 2 delete" mental model - there was actually no code path that ever deleted a local copy on a run after the one that uploaded it.
Add File.s3_uploaded_at (nullable). The management command's new two-phase flow:
T1 (default: s3_uploaded_at + 7 months) is when File.get_url() switches from serving the local link to a virtual S3 link computed on the fly via S3OverlayStorage.append_s3_prefix(), without mutating file.name. T2 (default: effective T1 + 8 days) is when the command performs the flip+delete. Both are also settable as an absolute cutover datetime (AWS_S3_SERVE_AT / AWS_S3_DELETE_LOCAL_AT) that overrides the per-file relative delta for all uploaded files at once - an ops lever for forcing a bulk rollout. See common/s3_rollout.py for the threshold logic.
Also adds File.create_time/update_time (matching the Issue model's existing convention) as general hygiene, kept deliberately separate from s3_uploaded_at so unrelated future edits can never corrupt the S3 rollout schedule. Migration 0012 backfills create_time from Event.timestamp and s3_uploaded_at with a safe synthetic sentinel for files already migrated under the old algorithm.
Scope: issues.File only. users.UserProfile.avatar / s3migrate_user_avatars / the shared S3MigrateCommand base class are untouched (avatar's ImageField is hardcoded to storage=OverwriteStorage(), a local-only storage, so avatar URLs don't go through S3OverlayStorage today - fixing that is separate, out-of-scope work).
Verified end-to-end: full issues/api/common test suite (no new failures vs. baseline - same pre-existing locale/i18n failures before and after), plus the S3-dependent tests (upload, get_url() state transitions, delete+flip after T2, absolute override cutover) run for real against a live DigitalOcean Spaces bucket.