Skip to content

fix partial upload cleanup after write failure - #58

Open
ooiuuii wants to merge 1 commit into
tt-a1i:mainfrom
ooiuuii:fix/upload-partial-write-cleanup
Open

ooiuuii wants to merge 1 commit into
tt-a1i:mainfrom
ooiuuii:fix/upload-partial-write-cleanup

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Oct 7, 2026

Copy link
Copy Markdown

Why

An upload write can create part of its file and then fail before the metadata INSERT. The current catch returns the existing upload error but leaves that unreferenced partial file on disk; normal database close does not remove it.

Acquire file ownership with an explicit exclusive open('wx'), write through the handle and close it in finally. On failure, remove the file only if this call successfully created it. Open failures, including EEXIST, never unlink an existing file. Metadata insertion/rollback, error responses, quotas and successful payload handling are unchanged.

Testing

  • Genuine native failure in a fresh Linux child: only that child's file-size limit is lowered to 65,536 bytes. A synthetic 1 MiB write creates a partial file and fails; no disk filling, mounts, privileges or other-process changes.
  • The native control directly observes EFBIG; the store separately returns its mapped HTTP 500. The test does not pretend to capture the hidden native exception from the store.
  • Baseline leaves two upload files but only one database row after close; fixed code retains only the successful file/row.
  • Product-only revert reproduces the failure; restore passes.
  • Collision coverage injects a deterministic UUID only in the parent unit test, then uses the real store/filesystem to verify existing bytes survive and no row is inserted. Native child imports and I/O are unmocked.
  • 22 focused upload/store/persistence tests passed; pnpm check, pnpm build and git diff --check passed.
  • Native partial-write test skips outside Linux or when prlimit is absent. Full suite, manual HTTP/UI and non-Linux native failure behavior were not tested.

Self-Review

Four independent scoped reviews: architecture A, correctness A-, test quality A-, spec alignment A. No blocking findings; the prior orphan-file defect and exclusive-open ownership were checked.

Retained limitations: cleanup uses the existing best-effort unlink helper, so an unremovable file can remain. Close/unlink failures and a disk-backed database restart are not covered by this regression. The successful-upload control is inside the Linux native test and also skips when that test is unavailable. These limits are disclosed rather than claimed fixed. Aggregate A-. Runtime validation is supported by retained command receipts/logs; source reviewers did not rerun it.

This is independent of #56's filename truncation fix; both changes should be retained when integrated.

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.

1 participant