fix(lfs): failed upload deletes a concurrent upload's meta object

[hsdfat]

UploadHandler creates the LFS meta object only as the last step of uploadOrVerify, after the content is already in the store, so a request that errors has never created a row of its own. The removal on the error path was a real compensating action when it was added in #14726, where the meta object was created before contentStore.Put, but #16865 moved creation after the Put and left the removal behind.

Since then it can only ever delete a row created by a different request: a stalled git-lfs PUT that fails after its own retry has already succeeded wipes the winner's meta object, leaving the content in the store unreachable and eligible for orphan cleanup.

Drop the removal and add a regression test asserting that a failing upload keeps a pre-existing meta object.

Fixes #38424.

Assisted-by: Claude Code:claude-opus-5

[wxiaoguang]

What if the store content is corrupted (incomplete)? How to recover it by the next upload?

[hsdfat]

Good question, and checking it showed my own premise was wrong, so let me give the accurate picture.

ContentStore.Put does not delete on every error. If Save returns an error it returns at modules/lfs/content_store.go:57, above the Delete block, which is only reached when Save returned nil but the wrapper recorded an error or the written size differs. What actually keeps a partial object from becoming visible is backend atomicity: local Save copies to a temp file and only renames it into place at the end, removing the temp on any error (modules/storage/local.go:80-100), and MinIO/S3 PutObject never publishes a partial object. The one case where the reader error is swallowed — bytes written equal the expected size — is exactly the case Put's explicit Delete covers.

On the removal itself: it cannot be a self-cleanup, because NewLFSMetaObject is the last statement of uploadOrVerify. A request that errors has no row of its own, so RemoveLFSMetaObjectByOid(repo, oid) either deletes nothing or deletes a row another request created after its content was safely stored.

And for missing content with a meta row present, recovery does not depend on deleting the row: BatchHandler keys the upload action on !exists, so the object is offered for upload again regardless of the meta row, and Put rewrites it.

Where recovery genuinely does not work today, independently of this PR:

• Both handlers use contentStore.Exists (Stat only) rather than Verify (Stat + size), so a wrong-sized stored object counts as present. Batch then offers no upload action at all, and if there is no meta row, UploadHandler takes the proof-of-possession branch, hashes the request body, creates the row, and never rewrites the stored bytes. I confirmed the same short-circuit from a test: once content and a meta row both exist, a PUT for that oid returns 200 without reading the body, so a client cannot repair a bad object by re-uploading it.
• Deleting the meta row would not have repaired any of that either — it only changes which branch runs, never the stored bytes.

There is one narrow state where the old unconditional delete did incidentally unblock a repository, and I think it points at a separate bug worth fixing:

LFSAutoAssociate inserts the size submitted in the form rather than the size of the object it validated. It looks up the accessible rows with Cols("oid") (models/git/lfs.go:227), so the real size is never fetched, and then assigns newMetas[i].Size = oidMap[newMetas[i].Oid].Size (models/git/lfs.go:236) — the caller's value, parsed straight from the form at routers/web/repo/setting/lfs.go:521. A row whose size does not match the object is therefore reachable from the repo settings page, and BatchHandler rejects it with 422 at services/lfs/server.go:247 before ever offering an upload, so the repository stays stuck. The old delete could clear such a row if a client still held an upload URL minted before the row appeared — a coincidence rather than a mechanism, since a normal client gets the 422 and never receives an upload URL, but it is fair to say the broad delete happened to paper over it.

Selecting the size alongside the oid and using the stored value looks like the right fix there. I'm happy to send that as a separate PR, and to switch Exists to Verify in both handlers (allowing the upload when the content is genuinely missing even if an existing row has a different size) either here or as a follow-up — I'd rather keep this PR to the concurrent-delete bug unless you prefer it all together.

[wxiaoguang]

Unable to understand, don't reply by AI. Just tell me what's the right thing to do in one sentence.

[hsdfat]

Sorry mb, request only insert meta row after the content is stored, so on failure it has no row of its own and 'RemoveLFSMetaObjectByOid(repo, oid)' can only delete a row that a concurrent successful upload created