Postulo/postulo

Watch

Fork 0

Deleting an account or a company: export archives and logos stay on disk, and prune_media deletes live exports #355

New issue

Closed

opened 2026-10-01 10:46:05 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-01 10:46:05 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, comparing every FileField in the models with the lists of files that account deletion and prune_media look after, then deleting a test account that held one of each and running prune_media against a fresh export.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at jobs/models.py:299 (Company.logo) and jobs/models.py:204-206 (logo_upload_to), and at core/server_views.py:288 (PersonDeleteView.post) and :305 (its call to delete_account).

Three hand-written lists say which files a row owns: files_of and media_directories_of for account deletion, and _referenced for prune_media. Two file fields are missing from them: ExportArchive.file (#247) from all three, and Company.logo from the first two. Neither model has a post_delete receiver. So deleting an account leaves the person's export archive, a zip of the whole job search, and their company logos on disk for ever; deleting one company leaves its logo; and prune_media --remove deletes exports that are still waiting to be downloaded.

Related to #33 (closed), which designed account deletion as "export offered first, then everything, including the files on disk", named logos among the files, and set the test that matters as "no file under their prefix remains". This issue adds the two file fields that deletion does not cover, one of which is the export #33 tells the person to take first, and the same omission in prune_media. Also related to #217 (closed), which settled that deleting a row deletes its file and added prune_media, for documents only.

1. Account deletion leaves the export archive and the logos

Where:
- accounts/deletion.py:61-81 (files_of) collects UploadedDocument.file, RenderedDocument.file, CapturedPage.source and .rendering, and the two Profile pictures. ExportArchive.file (core/models.py:811, stored under exports/<owner>/YYYY/MM/ by archive_path, core/models.py:788-790) and Company.logo (jobs/models.py:296, stored under logos/<owner>/ by logo_upload_to, jobs/models.py:201-203) are not there.
- accounts/deletion.py:84-91 (media_directories_of) prunes documents/<pk>, avatars/<pk> and captures/<pk> only.
- core/models.py:830-842 (ExportArchive.delete) is the only code that removes an archive's bytes. delete_account removes the rows through the cascade from user.delete() (accounts/deletion.py:115), and a cascade runs the collector, never a model's own delete(); the docstring of jobs/signals.py (lines 10-15) says so.
- The only post_delete receivers that remove files are documents/signals.py:38-39 (uploads and renders) and jobs/signals.py:57 (CapturedPage). There is none for ExportArchive or Company.
- core/slow.py:70-83 (reap_archives) iterates ExportArchive rows, so once the rows have gone it cannot find the file.
What happens:
1. On Delete my account the person reads "Take your export first" (templates/accounts/delete_account.html:9) and presses the export button (:46). build_an_export (core/slow.py:35) writes an ExportArchive under exports/<pk>/YYYY/MM/, kept for POSTULO_EXPORT_KEEP_HOURS (24 by default).
2. They delete the account. Every row goes. The zip stays under MEDIA_ROOT/exports/<pk>/, and every logo under MEDIA_ROOT/logos/<pk>/ (one logo-<company pk>.* per company, which is a list of where they were applying). The deletion report counts neither.
3. No row names the zip, so the reaper never removes it, and manage.py backup copies it into every later backup with the rest of the media tree.
An administrator deleting somebody from Server settings → People (core/server_views.py:304) and manage.py delete_account take the same path. Checked against the snapshot: with one company logo and one archive, delete_account() reported 1 exportarchive, 1 company and 0 files removed from disk, and after it and a reap_archives() pass both files still existed.
Who can trigger it: anyone deleting their own account in the way the page recommends, and any administrator deleting an account that holds a logo or an unexpired export. No special conditions. Nobody can fetch the files through the application afterwards; the exposure is to whoever holds the disk or a backup.
Why it matters: the page says "Every file is removed from the server's disk, not only its record" (templates/accounts/delete_account.html:35), the wiki says documents "and the files behind them on the disk" go (Accounts-and-invitations.md:206-208), and the module docstring of accounts/deletion.py calls a deletion that leaves files "not a deletion". The file left behind is the most complete copy of the person's data the instance ever writes, and nothing will remove it except prune_media --remove, by accident.

2. Deleting one company leaves its logo

Where: jobs/views.py:573 (CompanyDeleteView); the only code that removes a logo file is replacing or clearing it (jobs/logos.py:169, :180) and the losing side of a merge (jobs/merging.py:505).
What happens: a person deletes a company from its own page. The row goes and logos/<owner>/logo-<pk>.* stays, with nothing pointing at it. Checked with a POST to jobs:company_delete: the logo still existed. A company deleted from the admin (jobs/admin.py:18) behaves the same.
Why it matters: #217 settled that "deleted" means deleted, not hidden; logos were left out of it.

3. `prune_media` treats every live export as an orphan

Where: documents/management/commands/prune_media.py:23-42 (_referenced) lists seven (model, field) pairs, Company.logo among them and ExportArchive.file not; handle() unlinks every file under MEDIA_ROOT outside that set (prune_media.py:84-85). core/views_export.py:72 (export_archive) decides "expired" from archive.file, which is the stored name and not whether the file exists.
What happens: an operator runs manage.py prune_media --remove, as the wiki shows (Files-and-what-you-sent.md:154-155, Backups-and-your-data.md:222) and as restore suggests (core/management/commands/restore.py:83), while somebody's export is inside its keep window. Run against the snapshot with an archive expiring in 24 hours, the listing printed orphan exports/1/2026/09/x.zip and 0 file(s) in use, 1 with no record; with --remove the file was deleted and the row stayed. Your data goes on offering the download, and it answers the bare 404 "File not found." from core/files.py:86-87 rather than "That export has expired. Ask for a new one.", because the row still has its file name.
Why it matters: the command's docstring says it lists by default "because the failure mode of the opposite default is somebody's CV"; this is that failure with somebody's whole-account export, and the listing an operator reads before deciding is wrong. The loss is recoverable, since the person can build the export again. The comment beside CapturedPage in _referenced records what leaving a model out of the list costs; a list kept by hand will miss the next file field too.

4. The files go before the request's transaction commits

Where: accounts/deletion.py:111-127; config/settings/base.py:140 (ATOMIC_REQUESTS); accounts/views.py:394 (DeleteAccountView.post) and core/server_views.py:287, neither marked non_atomic_requests.
What happens: the module docstring promises that "A transaction that fails leaves the files exactly where they were". Called from either view, the transaction.atomic() in delete_account is a savepoint inside the request's transaction, and the loop that removes the files runs before that outer transaction commits. A request that fails after delete_account returns rolls the rows back with their files already gone.
Why it matters: it is the opposite failure, a live row with no file, and the fix below removes it at no extra cost.

Fix

Replace ExportArchive.delete() with a post_delete receiver that removes the file in transaction.on_commit, as jobs/signals.py:57-84 does for kept pages. It then runs for cascades, queryset deletes and the admin as well as for reap_archives.
Add the same receiver for Company.logo, keeping the "only where no other row uses the name" check: a merge hands the other company's logo name to the kept one (jobs/merging.py:489-493) before deleting the other. The explicit removal at jobs/merging.py:505 then becomes redundant.
Add exports/<pk> and logos/<pk> to media_directories_of, and count both kinds of file in the deletion report.
Replace the lists in files_of and prune_media._referenced with one helper that walks apps.get_models() for every FileField and ImageField, so a model that gains a file field is covered on the day it is added. At the least, add (ExportArchive, "file") to _referenced.
Move the file removal in delete_account into transaction.on_commit (or leave it to the receivers, which already wait for the commit), so section 4 cannot happen.
Optionally, have export_archive give the "expired" message when the bytes are gone.

Instances that already hold such files need no migration: once prune_media knows about live exports, its listing shows exactly the archives and logos that deleted accounts and companies left behind.

Done when

tests/test_account_deletion.py::test_deleting_removes_every_owned_row_and_every_file also gives fill() a company with a logo and an unexpired ExportArchive, and asserts after delete_account() that no file remains anywhere under MEDIA_ROOT in a <prefix>/<pk>/ directory of the deleted person, rather than naming the known directories. It fails today on the archive and the logo.
A test in tests/test_logos.py deletes a company with a logo through jobs:company_delete and finds the file gone after commit; another merges two companies and finds the logo the kept company now uses still on disk.
A test asserts that deleting ExportArchive rows through a queryset removes their files.
A test in tests/test_document_record.py, beside test_prune_media_lists_orphans_and_removes_them_when_told, writes an unexpired ExportArchive, runs prune_media --remove, and asserts the file is still there and was not listed as an orphan. It fails today.
A sweep test walks apps.get_models() and fails when a model declares a file field that files_of or _referenced does not read.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, comparing every `FileField` in the models with the lists of files that account deletion and `prune_media` look after, then deleting a test account that held one of each and running `prune_media` against a fresh export. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `jobs/models.py:299` (`Company.logo`) and `jobs/models.py:204-206` (`logo_upload_to`), and at `core/server_views.py:288` (`PersonDeleteView.post`) and `:305` (its call to `delete_account`). Three hand-written lists say which files a row owns: `files_of` and `media_directories_of` for account deletion, and `_referenced` for `prune_media`. Two file fields are missing from them: `ExportArchive.file` (#247) from all three, and `Company.logo` from the first two. Neither model has a `post_delete` receiver. So deleting an account leaves the person's export archive, a zip of the whole job search, and their company logos on disk for ever; deleting one company leaves its logo; and `prune_media --remove` deletes exports that are still waiting to be downloaded. Related to #33 (closed), which designed account deletion as "export offered first, then everything, including the files on disk", named logos among the files, and set the test that matters as "no file under their prefix remains". This issue adds the two file fields that deletion does not cover, one of which is the export #33 tells the person to take first, and the same omission in `prune_media`. Also related to #217 (closed), which settled that deleting a row deletes its file and added `prune_media`, for documents only. ## 1. Account deletion leaves the export archive and the logos - **Where:** - `accounts/deletion.py:61-81` (`files_of`) collects `UploadedDocument.file`, `RenderedDocument.file`, `CapturedPage.source` and `.rendering`, and the two `Profile` pictures. `ExportArchive.file` (`core/models.py:811`, stored under `exports/<owner>/YYYY/MM/` by `archive_path`, `core/models.py:788-790`) and `Company.logo` (`jobs/models.py:296`, stored under `logos/<owner>/` by `logo_upload_to`, `jobs/models.py:201-203`) are not there. - `accounts/deletion.py:84-91` (`media_directories_of`) prunes `documents/<pk>`, `avatars/<pk>` and `captures/<pk>` only. - `core/models.py:830-842` (`ExportArchive.delete`) is the only code that removes an archive's bytes. `delete_account` removes the rows through the cascade from `user.delete()` (`accounts/deletion.py:115`), and a cascade runs the collector, never a model's own `delete()`; the docstring of `jobs/signals.py` (lines 10-15) says so. - The only `post_delete` receivers that remove files are `documents/signals.py:38-39` (uploads and renders) and `jobs/signals.py:57` (`CapturedPage`). There is none for `ExportArchive` or `Company`. - `core/slow.py:70-83` (`reap_archives`) iterates `ExportArchive` rows, so once the rows have gone it cannot find the file. - **What happens:** 1. On *Delete my account* the person reads "Take your export first" (`templates/accounts/delete_account.html:9`) and presses the export button (`:46`). `build_an_export` (`core/slow.py:35`) writes an `ExportArchive` under `exports/<pk>/YYYY/MM/`, kept for `POSTULO_EXPORT_KEEP_HOURS` (24 by default). 2. They delete the account. Every row goes. The zip stays under `MEDIA_ROOT/exports/<pk>/`, and every logo under `MEDIA_ROOT/logos/<pk>/` (one `logo-<company pk>.*` per company, which is a list of where they were applying). The deletion report counts neither. 3. No row names the zip, so the reaper never removes it, and `manage.py backup` copies it into every later backup with the rest of the media tree. An administrator deleting somebody from *Server settings → People* (`core/server_views.py:304`) and `manage.py delete_account` take the same path. Checked against the snapshot: with one company logo and one archive, `delete_account()` reported `1 exportarchive`, `1 company` and `0 files removed from disk`, and after it and a `reap_archives()` pass both files still existed. - **Who can trigger it:** anyone deleting their own account in the way the page recommends, and any administrator deleting an account that holds a logo or an unexpired export. No special conditions. Nobody can fetch the files through the application afterwards; the exposure is to whoever holds the disk or a backup. - **Why it matters:** the page says "Every file is removed from the server's disk, not only its record" (`templates/accounts/delete_account.html:35`), the wiki says documents "*and the files behind them on the disk*" go (`Accounts-and-invitations.md:206-208`), and the module docstring of `accounts/deletion.py` calls a deletion that leaves files "not a deletion". The file left behind is the most complete copy of the person's data the instance ever writes, and nothing will remove it except `prune_media --remove`, by accident. ## 2. Deleting one company leaves its logo - **Where:** `jobs/views.py:573` (`CompanyDeleteView`); the only code that removes a logo file is replacing or clearing it (`jobs/logos.py:169`, `:180`) and the losing side of a merge (`jobs/merging.py:505`). - **What happens:** a person deletes a company from its own page. The row goes and `logos/<owner>/logo-<pk>.*` stays, with nothing pointing at it. Checked with a POST to `jobs:company_delete`: the logo still existed. A company deleted from the admin (`jobs/admin.py:18`) behaves the same. - **Why it matters:** #217 settled that "deleted" means deleted, not hidden; logos were left out of it. ## 3. `prune_media` treats every live export as an orphan - **Where:** `documents/management/commands/prune_media.py:23-42` (`_referenced`) lists seven `(model, field)` pairs, `Company.logo` among them and `ExportArchive.file` not; `handle()` unlinks every file under `MEDIA_ROOT` outside that set (`prune_media.py:84-85`). `core/views_export.py:72` (`export_archive`) decides "expired" from `archive.file`, which is the stored name and not whether the file exists. - **What happens:** an operator runs `manage.py prune_media --remove`, as the wiki shows (`Files-and-what-you-sent.md:154-155`, `Backups-and-your-data.md:222`) and as `restore` suggests (`core/management/commands/restore.py:83`), while somebody's export is inside its keep window. Run against the snapshot with an archive expiring in 24 hours, the listing printed `orphan exports/1/2026/09/x.zip` and `0 file(s) in use, 1 with no record`; with `--remove` the file was deleted and the row stayed. *Your data* goes on offering the download, and it answers the bare 404 "File not found." from `core/files.py:86-87` rather than "That export has expired. Ask for a new one.", because the row still has its file name. - **Why it matters:** the command's docstring says it lists by default "because the failure mode of the opposite default is somebody's CV"; this is that failure with somebody's whole-account export, and the listing an operator reads before deciding is wrong. The loss is recoverable, since the person can build the export again. The comment beside `CapturedPage` in `_referenced` records what leaving a model out of the list costs; a list kept by hand will miss the next file field too. ## 4. The files go before the request's transaction commits - **Where:** `accounts/deletion.py:111-127`; `config/settings/base.py:140` (`ATOMIC_REQUESTS`); `accounts/views.py:394` (`DeleteAccountView.post`) and `core/server_views.py:287`, neither marked `non_atomic_requests`. - **What happens:** the module docstring promises that "A transaction that fails leaves the files exactly where they were". Called from either view, the `transaction.atomic()` in `delete_account` is a savepoint inside the request's transaction, and the loop that removes the files runs before that outer transaction commits. A request that fails after `delete_account` returns rolls the rows back with their files already gone. - **Why it matters:** it is the opposite failure, a live row with no file, and the fix below removes it at no extra cost. ## Fix 1. Replace `ExportArchive.delete()` with a `post_delete` receiver that removes the file in `transaction.on_commit`, as `jobs/signals.py:57-84` does for kept pages. It then runs for cascades, queryset deletes and the admin as well as for `reap_archives`. 2. Add the same receiver for `Company.logo`, keeping the "only where no other row uses the name" check: a merge hands the other company's logo name to the kept one (`jobs/merging.py:489-493`) before deleting the other. The explicit removal at `jobs/merging.py:505` then becomes redundant. 3. Add `exports/<pk>` and `logos/<pk>` to `media_directories_of`, and count both kinds of file in the deletion report. 4. Replace the lists in `files_of` and `prune_media._referenced` with one helper that walks `apps.get_models()` for every `FileField` and `ImageField`, so a model that gains a file field is covered on the day it is added. At the least, add `(ExportArchive, "file")` to `_referenced`. 5. Move the file removal in `delete_account` into `transaction.on_commit` (or leave it to the receivers, which already wait for the commit), so section 4 cannot happen. 6. Optionally, have `export_archive` give the "expired" message when the bytes are gone. Instances that already hold such files need no migration: once `prune_media` knows about live exports, its listing shows exactly the archives and logos that deleted accounts and companies left behind. ## Done when - `tests/test_account_deletion.py::test_deleting_removes_every_owned_row_and_every_file` also gives `fill()` a company with a logo and an unexpired `ExportArchive`, and asserts after `delete_account()` that no file remains anywhere under `MEDIA_ROOT` in a `<prefix>/<pk>/` directory of the deleted person, rather than naming the known directories. It fails today on the archive and the logo. - A test in `tests/test_logos.py` deletes a company with a logo through `jobs:company_delete` and finds the file gone after commit; another merges two companies and finds the logo the kept company now uses still on disk. - A test asserts that deleting `ExportArchive` rows through a queryset removes their files. - A test in `tests/test_document_record.py`, beside `test_prune_media_lists_orphans_and_removes_them_when_told`, writes an unexpired `ExportArchive`, runs `prune_media --remove`, and asserts the file is still there and was not listed as an orphan. It fails today. - A sweep test walks `apps.get_models()` and fails when a model declares a file field that `files_of` or `_referenced` does not read.

tiagoagueda added this to the 0.5.0 milestone

2026-10-01 10:46:05 +00:00

tiagoagueda added the

tier

bug

security

labels

2026-10-01 10:46:05 +00:00

tiagoagueda referenced this issue

2026-10-03 01:58:41 +00:00

Avatar and company logo: database storage that goes with the owner, or files with a guaranteed lifecycle #662

tiagoagueda referenced this issue

2026-10-03 01:58:42 +00:00

Uploaded documents: no orphans, no corrupted files, and one boundary for where the bytes live #663

tiagoagueda referenced this issue

2026-10-03 01:58:43 +00:00

A kept page follows its capture, then the listing it became, and goes when that listing goes #664

tiagoagueda referenced this issue

2026-10-03 01:58:48 +00:00

A CV photo of its own, independent of the avatar, printed only where a CV says so #668

tiagoagueda referenced this issue

2026-10-03 01:58:52 +00:00

Find logo and Refresh logo in a dialog on the company's page, not on a page of their own #674

tiagoagueda commented

2026-10-03 01:59:27 +00:00

Author

Owner

The maintainer's list of 2026-10-02 says an avatar and a company logo must go with their owner. Checking the avatar half found one more path.

By reading main at f09b6010b (not run): deleting a user through Django admin's UserAdmin (src/postulo/accounts/admin.py:14) removes the Profile row by cascade but leaves Profile.avatar and Profile.gravatar_image on disk. files_of is called only from delete_account (accounts/deletion.py:108), and no post_delete receiver exists for Profile. The fix should cover Profile as well as Company.

#662 asks whether both pictures should be database rows. If the maintainer chooses that, it supersedes the logo and avatar half here, so do not close that half early.

From the list of improvements of 2026-10-02.

The maintainer's list of 2026-10-02 says an avatar and a company logo must go with their owner. Checking the avatar half found one more path. By reading `main` at `f09b6010b` (not run): deleting a user through Django admin's `UserAdmin` (`src/postulo/accounts/admin.py:14`) removes the `Profile` row by cascade but leaves `Profile.avatar` and `Profile.gravatar_image` on disk. `files_of` is called only from `delete_account` (`accounts/deletion.py:108`), and no `post_delete` receiver exists for `Profile`. The fix should cover `Profile` as well as `Company`. #662 asks whether both pictures should be database rows. If the maintainer chooses that, it supersedes the logo and avatar half here, so do not close that half early. From the list of improvements of 2026-10-02.

tiagoagueda referenced this issue from a commit

2026-10-04 10:05:00 +00:00

Remove export archives and logos with their rows, and stop pruning live exports

tiagoagueda referenced this issue from a commit

2026-10-04 10:05:01 +00:00

Run the commit callbacks in the tests that now depend on a commit

tiagoagueda referenced this issue from a commit

2026-10-04 13:29:28 +00:00

Remove an account's files only when its deletion commits

tiagoagueda closed this issue

2026-10-04 13:29:28 +00:00