Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#22557 closed Bug (fixed)

staticfiles.json keeps deleted entries when collectstatic is run

Reported by: tedtieken Owned by: syphar
Component: contrib.staticfiles Version: 1.7-beta-2
Severity: Release blocker Keywords: hashed_files, collectstatic, manifest
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is some surprising behavior when the new HashedFilesMixin is used with the new ManifestFilesMixin.

When you run manage.py collectstatic --clear:

1) a copy of the old manifest.json is loaded from disk into memory at ManifestFilesMixin.hashed_files. This happens at the very beginning of collectstatic (when ManifestFilesMixin.__init__ is called, which happens when the storage is initialized, which happens during Command.__init__)
2) the old manifest.json is deleted (which would lead most people to believe the old manifest information is deleted as well)
3) new files are added to the ManifestFilesMixin.hashed_files dict, and updated files get their records updated. But, this is building on top of the last version of manifest.json. Keys for deleted files are never removed and the deleted file mappings persist in the new manifest.json which gets written back to disk at the end of collectstatic's post_process phase.

There are at least a few problems caused by the current setup:
1) It can lead to hard-to-find asset bugs. If you rename the mypage.css file to mynewpage.css, but forget to update all of the templates, the {% static %} template tag will happily serve you the stale cache record. This could lead to submarine bugs that aren't caught until production as few sites have full enough test suites to catch missing css. And the issue could get buried even deeper if you concatenate your CSS.

2) staticfiles.json has a memory leak. New files get added, but deleted files never get deleted unless you physically delete the staticfiles.json record.

3) If you write code that does anything with ManifestFilesMixin.hashed_files, you can't trust the cache and you have no way of cleaning it up when you find cache misses. (how I found the bug) I'm writing a css concatenator and because staticfiles.json never gets cleared, I have to keep a separate manifest for my mappings.

This problem may also exist when you use the CachedFilesMixin. I haven't tested that, but a quick read over the code suggests similar behavior exists there. 2 is less of an issue because caches are built to drop data over time. 3 is less of an issue because you can easily delete stale cache keys from anywhere in your application.

I see a few ways to fix the problem, and I'm sure there are others:

Option 1: clear the hashed files cache when collectstatic is run with --clear.

If the code in django.contrib.staticfiles.management.commands.collectstatic on lines 88-89 is changed from:

        if self.clear:
            self.clear_dir('')

to

        if self.clear:
            self.clear_dir('')
            if hasattr(self.storage, "hashed_files"):
                self.storage.hashed_files.clear()

This is pretty simple when you're using a manifest but, when a cache backend is used without a separate cache defined for staticfiles, this looks like it will clear _THE ENTIRE_ default cache.

Option 2: Move the cleanup work into the StaticFilesStorage, add a call from collectstatic.

If we added something like an on_collectstatic method to staticfiles storages and call it during collectstatic, we could have better encapsulation of this kind of cleanup that is also more futureproof.

in django.contrib.staticfiles.management.commands.collectstatic add the following somewhere before post_process (~ line 107):

    if hasattr(self.storage, "on_collectstatic"):
        self.storage.on_collectstatic(self)

Which would allow us to add to the ManifestFilesMixin:

    def on_collectstatic(command_object, *args, **kwargs):
        if command_object.clear:
            self.hashed_files = OrderedDict()

This lets us to do manifest files related cleanup work in the ManifestFilesMixin, and anyone who needs to can do custom cleanup of their own storage.

Option 3: add a warning to the documentation,

Of the options, I like some version of 2 the best.

I don't think this is necessarily a release blocker, but as soon as people start using the ManifestFilesMixin they're going to start bumping into this whether they realize it or not.

Attachments (2)

22557.diff (3.0 KB) - added by tedtieken 15 months ago.
proposed patch in diff form
22557.2.diff (2.7 KB) - added by tedtieken 15 months ago.
Updates following timgraham's comments on github

Download all attachments as: .zip

Change History (9)

comment:1 Changed 15 months ago by tedtieken

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Patch is in this pull request:

https://github.com/django/django/pull/2627

Version 0, edited 15 months ago by tedtieken (next)

Changed 15 months ago by tedtieken

proposed patch in diff form

comment:2 Changed 15 months ago by timo

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

The release blocker flag is typically warranted for bugs in new features. I have looked at this a little, but it would be great if Jannis could do a final review.

comment:3 Changed 15 months ago by apollo13

  • Owner changed from nobody to apollo13
  • Status changed from new to assigned

comment:4 Changed 15 months ago by syphar

  • Owner changed from apollo13 to syphar

Changed 15 months ago by tedtieken

Updates following timgraham's comments on github

comment:6 Changed 15 months ago by Florian Apolloner <florian@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 3bec38888f6f4ee9245b004fcb9fe15b35cef469:

Fixed #22557 -- ManifestStaticFilesStorage did not cleanup deleted files.

When using ManifestStaticFilesStorage, deleted static files would be
correctly cleaned up by "collectstatic --clear", but the manifest file
would still contain the stale entries.

Thanks to tedtieken for the report

comment:7 Changed 15 months ago by Florian Apolloner <florian@…>

In 0007a43198670a1716bc5ff2d0a659daa4c7cf63:

[1.7.X] Fixed #22557 -- ManifestStaticFilesStorage did not cleanup deleted files.

When using ManifestStaticFilesStorage, deleted static files would be
correctly cleaned up by "collectstatic --clear", but the manifest file
would still contain the stale entries.

Thanks to tedtieken for the report

Backport of 3bec38888f6f4ee9245b004fcb9fe15b35cef469 from master.

Note: See TracTickets for help on using tickets.
Back to Top