staticfiles.json keeps deleted entries when collectstatic is run

[Ted]

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.

[Ted]

Patch is in this pull request:

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

[Ted]

proposed patch in diff form

[Tim Graham]

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.

[Ted]

Updates following timgraham's comments on github

[Denis Cornehl]

updated pull-request: ​https://github.com/django/django/pull/2662

[Florian Apolloner <florian@…>]

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

[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.