Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22557 closed Bug (fixed)

staticfiles.json keeps deleted entries when collectstatic is run

Reported by: Ted Owned by: Denis Cornehl
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 Ted 10 years ago.
proposed patch in diff form
22557.2.diff (2.7 KB ) - added by Ted 10 years ago.
Updates following timgraham's comments on github

Download all attachments as: .zip

Change History (9)

comment:1 by Ted, 10 years ago

Has patch: set

Pull request form (though I may have the semantics of requesting it get pulled into 1.7.x stable wrong, please let me know if there is a more appropriate branch)

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

Last edited 10 years ago by Ted (previous) (diff)

by Ted, 10 years ago

Attachment: 22557.diff added

proposed patch in diff form

comment:2 by Tim Graham, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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 by Florian Apolloner, 10 years ago

Owner: changed from nobody to Florian Apolloner
Status: newassigned

comment:4 by Denis Cornehl, 10 years ago

Owner: changed from Florian Apolloner to Denis Cornehl

by Ted, 10 years ago

Attachment: 22557.2.diff added

Updates following timgraham's comments on github

comment:5 by Denis Cornehl, 10 years ago

comment:6 by Florian Apolloner <florian@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

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 by Florian Apolloner <florian@…>, 10 years ago

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