Skip to content

Fix missing UploadedFile causing silent release failures#50

Draft
Goober5000 wants to merge 2 commits into
ngld:masterfrom
Goober5000:more_fsnebula_fixes
Draft

Fix missing UploadedFile causing silent release failures#50
Goober5000 wants to merge 2 commits into
ngld:masterfrom
Goober5000:more_fsnebula_fixes

Conversation

@Goober5000

Copy link
Copy Markdown
Collaborator

The multiupload/finish idempotency path returned success without verifying the UploadedFile DB record still exists. If the record was lost (e.g. expired) after the upload completed, the release endpoint would reject the mod with "archive missing" — but clients like KNet never saw the error because create_release() returned HTTP 200 for all error responses.

Two fixes:

  • multiupload/finish now checks for the UploadedFile record when ChunkedUpload.done is True, and re-creates it from the file on disk if missing. If the file is also gone, it resets done=False so the client can retry.
  • create_release() error responses now return proper HTTP status codes (400/403/500) instead of 200.

Depends on #49; in draft until that is merged

…n retry

When a client calls multiupload/finish and the reverse proxy times out
(504) before the response reaches the client, the client retries. Each
retry would create a duplicate UploadedFile record (causing NotUniqueError)
or fail reading already-deleted chunks (FileNotFoundError), both surfacing
as 500 errors. This was observed in production as 17 rapid-fire 500s.

Check file.done at the top of finish_chunked_upload and return success
immediately if the upload was already finalized, avoiding redundant
reassembly and the resulting errors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@notimaginative

Copy link
Copy Markdown
Contributor

The OK(200) issue I reported is a bug in KNet. It reports the status code on error rather than the error message it was sent.

The HTTP status code thing in fsnebula is definitely an issue, however it's not one that can be fixed, as KNet and old-knossos depend on the current behavior to work properly. Fixing the status codes on fsnebula would almost certainly break proper error handling on both clients.

The multiupload/finish idempotency path returned success without
verifying the UploadedFile DB record still exists. If the record was
lost (e.g. expired) after the upload completed, the release endpoint
would reject the mod with "archive missing" — but clients like KNet
never saw the error because create_release() returned HTTP 200 for
all error responses.

Two fixes:
- multiupload/finish now checks for the UploadedFile record when
  ChunkedUpload.done is True, and re-creates it from the file on disk
  if missing. If the file is also gone, it resets done=False so the
  client can retry.
- create_release() error responses now return proper HTTP status codes
  (400/403/500) instead of 200.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Goober5000
Goober5000 force-pushed the more_fsnebula_fixes branch from d80f406 to 89182be Compare April 8, 2026 04:09
@chief1983

Copy link
Copy Markdown
Contributor

I followed up with Claude today about the thing I had a question about last night on Discord, about whether there was a risk of it flagging a file still in the temp location as successfully uploaded. Doesn't sound like there is an issue, but it did flag some other things:

However, there is a different subtle problem with the recovery code: it re-creates the UploadedFile with expires=-1 and the correct permanent path, but it doesn't verify the file's checksum. If the file on disk is corrupted or partial (e.g., disk issue), it would happily create a DB record pointing to a bad file. The original path does checksum verification at lines 182-185 before calling make_permanent().

Also, the recovery creates the record with content_checksum and vp_checksum from the retry request's form data, which should be the same as the original — but there's an implicit trust there without verification.

Summary: The temp-stuck-file scenario can't happen due to the ordering of make_permanent() before done=True. But the recovery path does skip checksum verification, which the normal path performs. That's the real gap in PR 50's upload.py changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants