Closed Bug 980916 Opened 10 years ago Closed 10 years ago

Deleting a rejected version and resubmitting with the same version number leaves missing zip

Categories

(Marketplace Graveyard :: Developer Pages, defect, P2)

Product:
Marketplace Graveyard
Component:
Developer Pages
Version:
Avenir
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2014-05-13

People

(Reporter: eviljeff, Assigned: robhudson)

References

Details

(Whiteboard: [incorrect_implementation] [repoman]) 

I've seen this bug a number of times recently, more than I can account for as a random issue or developer error.  The steps are always the following:
1)we reject version1.0 of an app
2)developer deletes version1.0 and submits a new zip, also version1.0 (presumably with no obvious validator errors)
3)when we come to review the app the zip file is missing

One example:
https://marketplace.firefox.com/reviewers/apps/review/ping-quad

I've only ever seen it when the same version number is used, and doesn't appear to happen when we've not first rejected the version.  Its probably not reliably reproducible unfortunately.
Assignee: nobody → mpillard
Priority: -- → P3
This also happened again for:
https://marketplace.firefox.com/reviewers/apps/review/flippy-bird

Bumping priority.
Priority: P3 → P2
Whiteboard: [incorrect_implementation]
Assignee: mpillard → nobody
adding whiteboard tag in the hope of some developer love
Severity: normal → major
Whiteboard: [incorrect_implementation] → [incorrect_implementation] [repoman]
This is now seemingly happening with versions that haven't been rejected also (i.e. the developer just deleted a version and re-uploaded with the same version number)
Assignee: nobody → robhudson.mozbugs
I'm not able to reproduce this locally. I've tried with and without rejecting the app previously.

I did notice that when the developer deletes the app the app on disk isn't actually deleted. But we handle the version in the database correctly. I'll peruse the code with this case in mind and see what I find.
I notice a couple things:

a) we aren't setting the files status to disabled (5) when the version is deleted. We're only doing this when a new version is uploaded and we disable the prior version.
b) we aren't actually removing deleted files from the file system.

I can fix (a) easily enough and perhaps that has something to do with this. We move disabled files to the guarded file path.

I'd like to delete files from the file system when their version is deleted but I don't know if the reviewers need the deleted files for any reason. I don't see any ability currently to compare files to a deleted one.
fwiw, this app is affected:
https://marketplace.firefox.com/reviewers/apps/review/togglestube
and I've not contacted the developer yet to ask them to re-upload yet.  (If you want to poke around to see what broken looks like before its fixed)
(In reply to Andrew Williamson [:eviljeff] from comment #10)
> fwiw, this app is affected:
> https://marketplace.firefox.com/reviewers/apps/review/togglestube
> and I've not contacted the developer yet to ask them to re-upload yet.  (If
> you want to poke around to see what broken looks like before its fixed)

The database tables look fine but there's only the "togglestube-1.0.zip" file in the guarded-addons folder.

I think we should delete files when their version is deleted. (Or if we do need them, rename them to something that won't ever conflict with a new upload.)
(In reply to Rob Hudson [:robhudson] from comment #11)
> (In reply to Andrew Williamson [:eviljeff] from comment #10)
> > fwiw, this app is affected:
> > https://marketplace.firefox.com/reviewers/apps/review/togglestube
> > and I've not contacted the developer yet to ask them to re-upload yet.  (If
> > you want to poke around to see what broken looks like before its fixed)
> 
> The database tables look fine but there's only the "togglestube-1.0.zip"
> file in the guarded-addons folder.
> 
> I think we should delete files when their version is deleted. (Or if we do
> need them, rename them to something that won't ever conflict with a new
> upload.)

Could we [pre|suf]fix all (new) zip filenames on upload with the version ID to fix this?
Here's what I think is happening:

Currently we don't disable files of versions that have been deleted so they hang around with whatever status they were prior. This is probably the PENDING status.

When a new version is uploaded we create the version and files in the database just fine and overwrite the file. After version upload we then call `disable_old_files` which updates older non-public versions to status DISABLED which triggers an "on change" handler that then sweeps the disabled files into the guarded file path. So if the file name just uploaded matches that of one of the ones that just got disabled we end up moving the just uploaded file to the guarded path and disassociated it with the new file.

If we disable the files when their version is deleted, they won't be found when we upload a new file and won't be moved. That's my theory.
https://github.com/mozilla/zamboni/commit/673320a 

STR are kind of difficult. Perhaps just try:

1. Upload a packaged app and set to pending.
2. From the reviewer tools reject it.
3. Delete the file and re-upload it so the version number is the same.
4. Verify that the zip file exists by trying to install it from the reviewer tools.

Assuming all that works we'll keep an eye out if this bug pops up again in the future. If so, Andrew recommended prepending the version ID to the filename which should certainly fix the issue.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2014-05-13
After talking with eviljeff, I figured out that I am able to verify this only on -dev(packaged app installation from reviewers tools) and I am blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1008903
(In reply to Victor Carciu from comment #15)
> After talking with eviljeff, I figured out that I am able to verify this
> only on -dev(packaged app installation from reviewers tools) and I am
> blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1008903

the alternate to the STR 4. in comment#14 is to check the file contents of the zip ('contents' link next to 'validation') - if not empty then we're good.
(In reply to Andrew Williamson [:eviljeff] from comment #16)
> (In reply to Victor Carciu from comment #15)
> > After talking with eviljeff, I figured out that I am able to verify this
> > only on -dev(packaged app installation from reviewers tools) and I am
> > blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1008903
> 
> the alternate to the STR 4. in comment#14 is to check the file contents of
> the zip ('contents' link next to 'validation') - if not empty then we're
> good.

Thank you for the workaround . I verified the bug following last steps and everything appears to be ok. 
Marking as verified...
Status: RESOLVED → VERIFIED
looks like this is still happening :/

The last two uploads were on 2014-05-14
https://marketplace.firefox.com/reviewers/apps/review/notes-5
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
The cronjob was picking up the disabled file and moving it even though it was already moved. This patch tells the cron to ignore files on deleted versions:

https://github.com/mozilla/zamboni/commit/76535d2
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
I uploaded 1.5. But then deleted 1.5, and uploaded 1.5 again (this is wrong).
We might have a re-occurrence of this bug.  Logged as bug 1049681 (separately, in the hope its a more limited problem)
You need to log in before you can comment on or make changes to this bug.