Skip to content

feat: route PUT and TUS through the upload coordinator - #714

Merged
f-firas merged 8 commits into
mainfrom
OCISDEV-900-pr6b-coordinator
Aug 14, 2026
Merged

feat: route PUT and TUS through the upload coordinator#714
f-firas merged 8 commits into
mainfrom
OCISDEV-900-pr6b-coordinator

Conversation

@f-firas

@f-firas f-firas commented Aug 13, 2026

Copy link
Copy Markdown

Adds the driver-agnostic upload coordinator: pkg/upload/coordinator.go (886) and
pkg/upload/tus_adapter.go (148). Specs are in a separate PR against this branch, so this
diff stays readable; they should land together.

Dormant. Nothing outside pkg/upload imports the package yet — PR 7 wires it
into the dataprovider. Merging this cannot change runtime behaviour, which is why
the divergences below are safe to land and settle in review rather than block on.

What it does

Three phases, because the HTTP context is gone by the time the bytes are committed:

  1. InitiateUpload resolves the target, checks quota and locks, and persists a
    session.
  2. Transport: Upload for PUT, tusAdapter.WriteChunk for TUS.
  3. finishUpload touches the node, marks it processing, verifies checksums,
    prepares, then either commits inline or hands off to postprocessing.

The driver seam is 11 storage.FS methods: GetMD, GetLock, GetQuota,
GetPathByID, ListGrants, TouchFile, MarkProcessing, PrepareUpload,
CommitUpload, RollbackUpload, Delete.

Cleanup, and why the orderings differ

Three paths, deliberately not collapsed into one:

path when what it does
deleteTouchedNode the mark itself failed Delete — there is no processing id for RollbackUpload to key off
rollbackMarked failed before PrepareUpload RollbackUpload with sizeDiff 0, then unmark
rollbackPrepared failed after PrepareUpload RollbackUpload with the propagated sizeDiff, then unmark

Both rollbacks call RollbackUpload before the unmark, because it keys off the
processing id (decomposedfs/upload.go:664-670). deleteTouchedNode runs only
after a failed mark, so the node is unflagged and Delete's
processing-refusal (decomposedfs.go:1121) does not bite. The orderings are
opposite on purpose.

tusAdapter.Terminate duplicates ~8 lines rather than reuse rollbackMarked.
Sharing them was the original bug: rollbackMarked hardcodes sizeDiff 0 and
decrements the processing gauge, both wrong for a cancelled transfer. A client
cancelling after PrepareUpload leaked the propagated size, and every cancelled
upload drove reva_upload_processing negative. The two callers have genuinely
different preconditions.

Declared divergences from main

  • checkQuota proceeds when GetQuota errors (coordinator.go:650). Main used
    node.CheckQuota, which propagates. The permissive form is deliberate — drivers
    without quota support must not be blocked — but it is a behaviour change, stated
    here rather than hidden.
  • describeExisting check order and the dropped Persist between checksum
    verification and prepare.
  • uploadRef reuse in uff.
  • Two TODOs marking known gaps: the write-only-share etag fallback
    (coordinator.go:532) and deny-grant NotFound (:606), where GetMD reports a file
    hidden by a deny-grant as missing, so the upload fails late with 409 instead of 403.

Inherited from main, not introduced

  • sizedeferred + uploadLength == 0. A third-party copy sends
    sizedeferred: true and no Upload-Length, so the zero-length branch finishes it
    with no bytes. Main has the identical shape at decomposedfs/upload.go:331-337.
    Faithful parity — flagging it so it does not read as a porting mistake.
  • The zero-length path returns ids for an already-deleted session. Same as main.
  • chunking.GetChunkBLOBInfo's latent panic is pre-existing and unreachable from
    here.

Fixes made during self-review

  • Terminate no longer decrements the processing gauge or passes sizeDiff 0, and
    logs the expected unmark failure at Debug rather than Error — it was Error on every
    cancelled upload.
  • The async gate regained main's second condition: c.async && c.pub != nil. Main
    gates publish and commit on two independent conditions; collapsing them meant a
    deployment with async on but no publisher wired hit a nil-interface panic in the
    request path instead of committing inline.
  • Dropped the Coordinator interface assertion — unneeded until PR 7, and the idiom
    appears nowhere else in the repo.

Fixes from review

  • the placeholder node id is documented rather than removed. An empty OpaqueId resolves to
    the space root (lookup.go:191) and Terminate can run before TouchFile, so rollback
    and unmark would target the root instead of missing. It only has to match nothing.
  • the expected unmark failure after a rollback logs at Debug, not Error. RollbackUpload
    purges a node the upload created (decomposedfs/upload.go:676), so the unmark that follows
    correctly finds no node and returns NotFound (:352) — every failed upload of a new file
    was logging an error for the expected outcome. Demoted, not dropped. The unmark after a
    successful commit stays at Error, where a missing node is genuinely unexpected.
  • dropped a dead nil-metadata guard.

Tests

233 Ginkgo specs, on OCISDEV-900-pr6b-specs and opened against this branch.
coordinator.go and tus_adapter.go at 100% statement coverage, package at 99.5%; the three
uncovered statements are unreachable (NFS ESTALE, a tusd.FileInfo that cannot fail to
marshal, HMAC signing that cannot fail on a []byte key). Verified by mutation testing rather
than coverage alone: ~33 injected bugs, all killed.

The specs cover the async fork, which had none before, and the UploadProcessing gauge, which
had none anywhere.

Re-verified against the three review fixes above: all 233 still pass. Coverage dips to 97.6%,
the whole gap being spaceOwnerOrManager's grant fallback (untestable while fakeFS has no
ListGrants) and unmarkProcessing's new NotFound branch.

Not addressed here

finishSync destroys the session when the commit fails, which is right for a waiting client
but wrong for the async caller, which wants the upload parked for RestartPostprocessing.
It only gets that second caller in the postprocessing PR, which splits the commit path in two.

No changelog — one goes in at the end of the coordinator stack, not per PR.

@f-firas
f-firas requested a review from a team as a code owner August 13, 2026 11:06
@kw-security

kw-security commented Aug 13, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Comment thread pkg/upload/coordinator.go Outdated
Comment thread pkg/upload/coordinator.go Outdated
Comment thread pkg/upload/coordinator.go Outdated
Comment thread pkg/upload/coordinator.go
Comment thread pkg/upload/coordinator.go Outdated
Comment thread pkg/upload/coordinator.go Outdated
Comment thread pkg/upload/coordinator.go
}

// finishSync commits the staged bytes, then unmarks processing and cleans up.
func (c *coordinator) finishSync(ctx context.Context, session Session) (*provider.ResourceInfo, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we should implement a second method finishAsync here. I'm not sure whether the purpose of this method here is only for synchronous flow, or also after postprocessing.

In case this method is only meant for sync flow:
scanResult, scanDate := session.ScanData() would not make sense here, we only have a scan result, if it's async. And then we would need another method for async flow. I added one in this commit, we could add this here as well.

In case this method is meant for sync & async flow:
In the previous decomposedfs implementation, if session.Finalize(ctx) fails, we don't delete the session & rollback. From what I see the reason is that an admin can then decide what to do with it. E.g. trigger the RestartPostprocessing or CleanUpload event. If we want to keep that behavior, we need to change the implementation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now finishSync only has the sync caller, the async one lands with the postprocessing consumer in a follow-up PR.
We can create a separate FinishAsync function later

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we then remove scanResult, scanDate := session.ScanData()?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we ca nremove it later once we implement the async function

@f-firas
f-firas merged commit 2489bba into main Aug 14, 2026
16 checks passed
@f-firas
f-firas deleted the OCISDEV-900-pr6b-coordinator branch August 14, 2026 08:34
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