Fix import purge on cleaner failure and reclaim leftover local CSV disk - #6547
Open
EmmS21 wants to merge 5 commits into
Open
Fix import purge on cleaner failure and reclaim leftover local CSV disk#6547EmmS21 wants to merge 5 commits into
EmmS21 wants to merge 5 commits into
Conversation
A finished CSV import can still leave a LocalImportAnalyticsCleaner job on analytics_imports with the same import_id. The error reporter used to match on queue + import_id only, so a cleaner crash (for example permission denied deleting leftover files) purged ClickHouse stats and could eventually mark the import failed. Scope import_fail / import_fail_transient to the import worker so cleanup failures cannot undo a completed import. I considered giving the cleaner its own queue. That was never debated when the cleaner was added, and it would not make undeletable files deletable. Matching on worker fixes the misclassification without new queue config or changing forget-import cancellation. Co-authored-by: Cursor <cursoragent@cursor.com>
After import-failure side effects are scoped away from the cleaner, a stuck File.rm still leaves CSVs on disk. On the final Oban attempt, log the path and import_id so operators can see cleanup gave up. I did not add UI or user email for this. Import/export mail in this codebase covers the outcome of loading or producing data; other janitor workers use logs. Leftover file delete is the same class of work. Co-authored-by: Cursor <cursoragent@cursor.com>
Fixing the error reporter stops cleanup crashes from wiping imported stats, but leftover CSVs can still sit on self-hosted disk when the volume is owned by the wrong user and File.rm is denied. Elixir cannot override that OS refusal. When the entrypoint starts as root, chown DATA_DIR to the plausible user, then su-exec down before starting the app so typical Compose upgrades can clear leftovers without teaching Docker UIDs. Non-root starts are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
Author
|
Further discussion: |
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
2 participants
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.
Summary
Fixes #6515: when
LocalImportAnalyticsCleanercrashed (oftenFile.rm!→ permission denied on self-hosted volumes),ObanErrorReportertreated that like anImportAnalyticsfailure and purged already-imported ClickHouse stats.import_fail/import_fail_transienttoPlausible.Workers.ImportAnalyticsonly. Cleanup crashes must not undo a completed import. I considered a separate queue for the cleaner; matching on worker fixes the misclassification with less churn.import_idwhen delete still fails (ops/debug signal, not user email).Leftover local CSV disk / volume ownership on CE images is a separate follow-up and belongs in Discussions as an image/ops change, not this bugfix.
Test plan
mix test test/workers/oban_error_reporter_test.exs test/workers/local_import_analytics_cleaner_test.exs