Finalise each chunked upload once, under a per-session lock
Resolved a trivial conflict in ChunkedUploadsTest: this branch and
d7e639b both append tests to the end of the file, so both are kept.
A full parallel run failed once and passed on retry while I was doing the
#1703 follow-up. A flake is worse than a steady failure: it trains you to
re-run rather than look, and it quietly weakens every green run reported
beside it.
Upload parts are real files under storage_path('app/uploads-tmp/{session_id}'),
not a faked disk. Every parallel worker gets its own database, so session
ids restart at 1 in each of them, and two workers writing parts land in
the same directory. On top of that ChunkedUploadsTest's afterEach deleted
the whole tree rather than its own share, for everybody. Six test files
write parts, so this was reachable without anything I added.
The same collision exists inside one worker: RefreshDatabase rolls back,
so ids restart at 1 for every test, and a run that died before its
cleanup leaves parts sitting under the id the next test is about to
claim.
LocalPartStore now reads its root from config, defaulting to exactly
where it always was -- an installation with UPLOAD_PARTS_PATH unset
behaves identically. Tests\TestCase points it at a per-worker directory
and empties that directory per test, which closes the cross-worker, the
cross-run and the intra-worker versions together. ChunkedUploadsTest's
cleanup and its two directory assertions read the configured root rather
than the hardcoded path, so they can no longer reach into a neighbour.
Verified with eight consecutive parallel runs, green, and by watching the
per-worker directories appear separately (w1, w2, w4 … w14) rather than
one shared tree. The isolation itself cannot be asserted from inside a
single test; what a test can pin is the mechanism it rests on, so one
does: parts go where the configured root says.
Follow-up to #1703, which made `exists:folders,id` mean what its ten
readers already assumed. Two things it named and deliberately left.
**The chunked upload is two requests.** store()'s rule only ever sees the
first: POST /uploads records the resolved folder on the UploadSession and
complete() reads it back from the session rather than from the caller, so
deleting the folder while the bytes are in flight still files the
assembled file into it -- the same orphan state #1703 removes, reached by
a door a validation rule cannot watch. complete() now re-resolves through
Folder::query() and files at the root when the folder has gone.
Root rather than a refusal, because the two moments cost different
things. At store() nothing has been sent, so refusing is free and honest,
which is the call #1703 made. Here the bytes are already uploaded, and
discarding somebody's finished transfer over a folder that vanished
underneath them is the harsher of the two surprises. The file lands
somewhere they can see it and move it.
**The refusal now explains itself.** "The selected folder id is invalid"
says nothing when the answer is that the folder has been deleted -- and
that is the usual way to meet this rule, since a live id picked from a
list is how anybody gets here. It matters most on the chunked path, the
one place #1703 makes a previously-working request fail. A small
ValidationRule object carries the message, which keeps the single
definition Rules::folderId() exists for: a messages() array would have to
be repeated at all ten call sites, and rules meaning different things in
ten places is what went wrong in the first place.
One note for whoever writes the next test here. Upload parts live in
storage_path('app/uploads-tmp/{session_id}'), which is a real shared
directory rather than a faked disk, and each parallel worker's database
restarts session ids at 1 -- so two files writing parts on two workers
collide, and ChunkedUploadsTest's afterEach deletes the whole tree for
everybody. Six test files write parts today. These two cases live in
ChunkedUploadsTest rather than beside the rest of their subject so this
change does not add a seventh racer; the underlying isolation problem
predates it and is worth its own fix.
complete() assembled the received parts into the one target file and
created the File row with no guard against a second complete() for the
same session running at the same time -- an Uppy retry, a double submit,
a resend after a lost connection. Two of them would interleave writes
into the session's single `assembled` file (the stored bytes then no
longer match the checksum computed from the in-memory buffers) and could
each create a File row.
Take a per-session lock around the finalisation and fail a second caller
fast; the lock's TTL releases the claim if a completion dies mid-flight,
so a genuine retry still works. The body moves to a finalise() helper so
complete() reads as auth + lock + finalise.
store() checks Setting::MaxFileSizeMb against the size the client declares
when it opens the session, and complete() re-checks the storage quota
against the real assembled byte count -- but nothing re-checked the size
limit itself. A client that declared a one-byte upload and then streamed
gigabytes of parts passed store()'s check and was never stopped, so the
configured limit (which store() applies to everyone, staff included) did
not hold for the resumable path that real uploads use.
Re-check the assembled byte count against MaxFileSizeMb in complete(),
cleaning up the assembled bytes and the session exactly as the quota
branch already does.
Two bugs a green suite could not find, both from pointing the
application at a real Google Cloud Storage bucket.
The adapter attaches a legacy per-object ACL to every write, and a
bucket with uniform bucket-level access — which our own setup
instructions require, and which Google recommends — refuses it:
"Cannot insert legacy ACL for an object when uniform bucket-level access
is enabled". So the default configuration could not write to the
recommended bucket. The library ships
UniformBucketLevelAccessVisibility for exactly this, and nothing is
lost by never setting an ACL: every object here is private and every
read is a signed URL.
The second is worse and was never about Google. Both file disks are
configured 'throw' => false, so a refused write returns false rather
than raising, and LocalPartStore ignored the return. The upload reported
success, the File row was written, and the bytes were nowhere — the
listing showed a file whose download could never work. An expired S3
credential did the same thing. It now checks, and the controller already
turns that into a validation error rather than a 500, so the person
uploading is told.
Verified against a live bucket with a key scoped to
roles/storage.objectAdmin: the probe lists, writes land, reads
round-trip byte for byte, and a signed URL comes back 200 carrying
"Informe año.pdf" intact through both the ASCII and RFC 8187 forms of
Content-Disposition.
Client file sharing, rebuilt from the ground up: a private area per
client, resumable uploads, folders, groups and categories, sharing with
expiry dates and download limits, comments, file versions, an activity
log, a REST API, and sixteen languages.
This repository begins here. ProjectSend 2 was developed privately, and
that development history is not published — the previous generation
remains available, with its own history, at projectsend/legacy.
Free software under the GNU General Public License v2, or (at your
option) any later version.
ignacionelson