Company logos: storing or clearing one deletes the old file before the row is saved #525

New issue

Closed

opened 2026-10-01 10:47:23 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:47:23 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading how files are replaced across the jobs app.

Checked again on 2026-10-01 against main at 0a5cad587: still present. jobs/logos.py and its callers have not moved; pages._discard, cited for comparison, is now at jobs/pages.py:661-682.

logos.store and logos.clear delete the company's current logo file at once, and only then open the transaction that saves the row. If that save fails, the row still names the old file, which is gone; store also leaves the new file behind with no row pointing at it. The rest of the jobs app keeps the rule #217 set: remove a file only after the change commits, and only when no row still uses it (merging.py:505, pages._discard).

Where: jobs/logos.py:155-175 (store: company.logo.delete(save=False) at 168-169, the new file written at 170, company.save(update_fields=LOGO_FIELDS) inside transaction.atomic() at 174-175); jobs/logos.py:178-185 (clear, the same order). Callers outside the request transaction: CompanyLogoActionView.post (jobs/views.py:436-462, Remove) and the logo errand find_a_logo (jobs/slow.py:130-157, Find logo and Refresh). For comparison: jobs/merging.py:502-505 and jobs/pages.py:689-710.
What happens:
1. On SQLite every transaction opens IMMEDIATE with a 20-second busy timeout (config/sqlite.py:48-52). In the action view and the errand, which run outside ATOMIC_REQUESTS (#220), the lock is only asked for after the file is gone. If another writer holds it past the timeout, the save raises OperationalError; after Remove logo the company row still names the deleted file, and every page shows a missing logo (CompanyLogoView answers 404) until a new one is stored.
2. A replacement usually reuses the name logo-<pk>.<ext>, so a failed save then leaves the new picture under the old row's name with the old logo_source and logo_source_url. Where the name changes (a PNG replaced by an SVG since #264, or a logo that came from an import, which keeps the archive's file name at core/importer.py:581), the row names a file that no longer exists.
3. A company deleted while its Find logo errand is fetching makes save(update_fields=...) raise DatabaseError (no rows affected): the errand fails with an unhandled error rather than Refused, and the file it has just written stays on disk with no row.
Why it matters: the conditions are uncommon, but the outcome is a record pointing at nothing, or a file of the person's that nothing will ever remove, and the code that avoids both already exists two modules away.
Fix: in store, remember the old name, write the new file, then save the row inside the atomic block; on failure delete the new file and re-raise (as pages._write does), and on success remove the old name with transaction.on_commit, only if it differs from the new one and no Company row still uses it. In clear, save the row first and remove the file on commit under the same check. Catch the no-rows DatabaseError in find_a_logo and answer Refused(_("That company is no longer here.")).

Done when

Tests in tests/test_logos.py: with Company.save patched to raise, logos.clear(company) leaves the logo file in place, and logos.store(...) with a different extension leaves both the old file in place and no new file behind. Both fail today.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading how files are replaced across the jobs app. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. `jobs/logos.py` and its callers have not moved; `pages._discard`, cited for comparison, is now at `jobs/pages.py:661-682`. `logos.store` and `logos.clear` delete the company's current logo file at once, and only then open the transaction that saves the row. If that save fails, the row still names the old file, which is gone; `store` also leaves the new file behind with no row pointing at it. The rest of the jobs app keeps the rule #217 set: remove a file only after the change commits, and only when no row still uses it (`merging.py:505`, `pages._discard`). - **Where:** `jobs/logos.py:155-175` (`store`: `company.logo.delete(save=False)` at 168-169, the new file written at 170, `company.save(update_fields=LOGO_FIELDS)` inside `transaction.atomic()` at 174-175); `jobs/logos.py:178-185` (`clear`, the same order). Callers outside the request transaction: `CompanyLogoActionView.post` (`jobs/views.py:436-462`, *Remove*) and the `logo` errand `find_a_logo` (`jobs/slow.py:130-157`, *Find logo* and *Refresh*). For comparison: `jobs/merging.py:502-505` and `jobs/pages.py:689-710`. - **What happens:** 1. On SQLite every transaction opens `IMMEDIATE` with a 20-second busy timeout (`config/sqlite.py:48-52`). In the action view and the errand, which run outside `ATOMIC_REQUESTS` (#220), the lock is only asked for after the file is gone. If another writer holds it past the timeout, the save raises `OperationalError`; after *Remove logo* the company row still names the deleted file, and every page shows a missing logo (`CompanyLogoView` answers 404) until a new one is stored. 2. A replacement usually reuses the name `logo-<pk>.<ext>`, so a failed save then leaves the new picture under the old row's name with the old `logo_source` and `logo_source_url`. Where the name changes (a PNG replaced by an SVG since #264, or a logo that came from an import, which keeps the archive's file name at `core/importer.py:581`), the row names a file that no longer exists. 3. A company deleted while its *Find logo* errand is fetching makes `save(update_fields=...)` raise `DatabaseError` (no rows affected): the errand fails with an unhandled error rather than `Refused`, and the file it has just written stays on disk with no row. - **Why it matters:** the conditions are uncommon, but the outcome is a record pointing at nothing, or a file of the person's that nothing will ever remove, and the code that avoids both already exists two modules away. - **Fix:** in `store`, remember the old name, write the new file, then save the row inside the atomic block; on failure delete the new file and re-raise (as `pages._write` does), and on success remove the old name with `transaction.on_commit`, only if it differs from the new one and no `Company` row still uses it. In `clear`, save the row first and remove the file on commit under the same check. Catch the no-rows `DatabaseError` in `find_a_logo` and answer `Refused(_("That company is no longer here."))`. ## Done when - Tests in `tests/test_logos.py`: with `Company.save` patched to raise, `logos.clear(company)` leaves the logo file in place, and `logos.store(...)` with a different extension leaves both the old file in place and no new file behind. Both fail today.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:47:23 +00:00

tiagoagueda added the

tier

bug

labels

2026-10-01 10:47:23 +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:52 +00:00