From a26ba8db5d1cd80db96fbae7e0f8914937c6f080 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Fri, 17 Jul 2026 15:02:08 +0200 Subject: [PATCH 01/15] add empty upload coordinator skeleton --- .../storageprovider/storageprovider.go | 34 +- .../services/dataprovider/dataprovider.go | 20 +- internal/http/services/owncloud/ocdav/tus.go | 6 +- pkg/rhttp/datatx/datatx.go | 6 +- pkg/rhttp/datatx/manager/simple/simple.go | 7 +- pkg/rhttp/datatx/manager/spaces/spaces.go | 7 +- pkg/rhttp/datatx/manager/tus/tus.go | 87 ++- pkg/upload/coordinated_upload.go | 195 ++++++ pkg/upload/coordinator.go | 655 ++++++++++++++++++ pkg/upload/filestore.go | 196 ++++++ pkg/upload/filestore_test.go | 312 +++++++++ pkg/upload/session.go | 454 ++++++++++++ pkg/upload/session_test.go | 456 ++++++++++++ 13 files changed, 2371 insertions(+), 64 deletions(-) create mode 100644 pkg/upload/coordinated_upload.go create mode 100644 pkg/upload/coordinator.go create mode 100644 pkg/upload/filestore.go create mode 100644 pkg/upload/filestore_test.go create mode 100644 pkg/upload/session.go create mode 100644 pkg/upload/session_test.go diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index b23604a275f..01ffd5e7d3f 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -25,6 +25,7 @@ import ( "net/url" "os" "path" + "path/filepath" "sort" "strconv" "strings" @@ -47,6 +48,7 @@ import ( "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storage/fs/registry" "github.com/owncloud/reva/v2/pkg/storagespace" + "github.com/owncloud/reva/v2/pkg/upload" "github.com/owncloud/reva/v2/pkg/utils" "github.com/pkg/errors" "github.com/rs/zerolog" @@ -69,6 +71,7 @@ type config struct { AvailableXS map[string]uint32 `mapstructure:"available_checksums" docs:"nil;List of available checksums."` CustomMimeTypesJSON string `mapstructure:"custom_mimetypes_json" docs:"nil;An optional mapping file with the list of supported custom file extensions and corresponding mime types."` MountID string `mapstructure:"mount_id"` + UploadDirectory string `mapstructure:"upload_directory" docs:";Local directory for staging upload sessions. Overrides the driver's root. Required for drivers that have no local filesystem root."` UploadExpiration int64 `mapstructure:"upload_expiration" docs:"0;Duration for how long uploads will be valid."` Events eventconfig `mapstructure:"events" docs:"0;Event stream configuration"` } @@ -106,6 +109,7 @@ func (c *config) init() { type Service struct { conf *config Storage storage.FS + Coordinator upload.Coordinator dataServerURL *url.URL availableXS []*provider.ResourceChecksumPriority } @@ -175,11 +179,29 @@ func New(m map[string]interface{}, ss *grpc.Server, log *zerolog.Logger) (rgrpc. c.init() - fs, err := getFS(c, log) + evstream, err := estreamFromConfig(c.Events) if err != nil { return nil, err } + fs, err := getFS(c, evstream, log) + if err != nil { + return nil, err + } + + // Build the coordinator-owned session store. UploadDirectory (service level) + // takes precedence over the driver root, so rootless drivers can still get a + // coordinator. The store points at the same root as the driver's data path so + // it can read the sessions the coordinator writes (decomposedfs on-disk format). + store := upload.NewFileStoreFromConfig(c.UploadDirectory, c.Drivers[c.Driver], log) + if store == nil { + return nil, fmt.Errorf("storageprovider: cannot determine upload directory, set upload_directory in config or driver root") + } + if err := store.Setup(); err != nil { + return nil, fmt.Errorf("storageprovider: upload directory setup failed: %w", err) + } + coordinator := upload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) + // parse data server url u, err := url.Parse(c.DataServerURL) if err != nil { @@ -205,6 +227,7 @@ func New(m map[string]interface{}, ss *grpc.Server, log *zerolog.Logger) (rgrpc. service := &Service{ conf: c, Storage: fs, + Coordinator: coordinator, dataServerURL: u, availableXS: xsTypes, } @@ -427,7 +450,7 @@ func (s *Service) InitiateFileUpload(ctx context.Context, req *provider.Initiate metadata["expires"] = strconv.Itoa(int(expirationTimestamp.Seconds)) } - uploadIDs, err := s.Storage.InitiateUpload(ctx, req.Ref, uploadLength, metadata) + uploadIDs, err := s.Coordinator.InitiateUpload(ctx, req.Ref, uploadLength, metadata) if err != nil { var st *rpc.Status switch err.(type) { @@ -1266,12 +1289,7 @@ func (s *Service) addMissingStorageProviderID(resourceID *provider.ResourceId, s } } -func getFS(c *config, log *zerolog.Logger) (storage.FS, error) { - evstream, err := estreamFromConfig(c.Events) - if err != nil { - return nil, err - } - +func getFS(c *config, evstream events.Stream, log *zerolog.Logger) (storage.FS, error) { if f, ok := registry.NewFuncs[c.Driver]; ok { driverConf := c.Drivers[c.Driver] driverConf["mount_id"] = c.MountID // pass the mount id to the driver diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index bffe73bebe1..b17f04e0e68 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -21,6 +21,7 @@ package dataprovider import ( "fmt" "net/http" + "path/filepath" "github.com/mitchellh/mapstructure" "github.com/rs/zerolog" @@ -33,6 +34,7 @@ import ( "github.com/owncloud/reva/v2/pkg/rhttp/router" "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storage/fs/registry" + pkgupload "github.com/owncloud/reva/v2/pkg/upload" ) func init() { @@ -51,6 +53,7 @@ type config struct { NatsEnableTLS bool `mapstructure:"nats_enable_tls"` NatsUsername string `mapstructure:"nats_username"` NatsPassword string `mapstructure:"nats_password"` + UploadDirectory string `mapstructure:"upload_directory" docs:";Local directory for staging upload sessions. Overrides the driver root. Required for drivers without a local root."` } func (c *config) init() { @@ -104,7 +107,18 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) return nil, err } - dataTXs, err := getDataTXs(conf, fs, evstream, log) + // The data provider finishes uploads that the storage provider initiated, so + // its store must resolve to the same upload directory. + store := pkgupload.NewFileStoreFromConfig(conf.UploadDirectory, conf.Drivers[conf.Driver], log) + if store == nil { + return nil, fmt.Errorf("dataprovider: cannot determine upload directory, set upload_directory in config or driver root") + } + if err := store.Setup(); err != nil { + return nil, fmt.Errorf("dataprovider: upload directory setup failed: %w", err) + } + coord := pkgupload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) + + dataTXs, err := getDataTXs(conf, coord, fs, evstream, log) if err != nil { return nil, err } @@ -126,7 +140,7 @@ func getFS(c *config, stream events.Stream, log *zerolog.Logger) (storage.FS, er return nil, fmt.Errorf("driver not found: %s", c.Driver) } -func getDataTXs(c *config, fs storage.FS, publisher events.Publisher, log *zerolog.Logger) (map[string]http.Handler, error) { +func getDataTXs(c *config, coord pkgupload.Coordinator, driver storage.FS, publisher events.Publisher, log *zerolog.Logger) (map[string]http.Handler, error) { if c.DataTXs == nil { c.DataTXs = make(map[string]map[string]interface{}) } @@ -146,7 +160,7 @@ func getDataTXs(c *config, fs storage.FS, publisher events.Publisher, log *zerol for t := range c.DataTXs { if f, ok := datatxregistry.NewFuncs[t]; ok { if tx, err := f(c.DataTXs[t], publisher, log); err == nil { - if handler, err := tx.Handler(fs); err == nil { + if handler, err := tx.Handler(coord, driver); err == nil { txs[t] = handler } } diff --git a/internal/http/services/owncloud/ocdav/tus.go b/internal/http/services/owncloud/ocdav/tus.go index 06ad4f71d24..2c0af897b00 100644 --- a/internal/http/services/owncloud/ocdav/tus.go +++ b/internal/http/services/owncloud/ocdav/tus.go @@ -319,7 +319,11 @@ func (s *svc) handleTusPost(ctx context.Context, w http.ResponseWriter, r *http. sReq.Ref.Path = uReq.Ref.GetPath() sReq.Ref.ResourceId = nil } else { - if resid, err := storagespace.ParseID(httpRes.Header.Get(net.HeaderOCFileID)); err == nil { + // A new file has no node id until the upload finishes, which is after the + // data server wrote this header, so it may be absent. sReq.Ref then keeps + // the path-based reference it was built with, which already names the + // file and stats just as well. + if resid, err := storagespace.ParseID(httpRes.Header.Get(net.HeaderOCFileID)); err == nil && resid.GetOpaqueId() != "" { sReq.Ref = &provider.Reference{ ResourceId: &resid, } diff --git a/pkg/rhttp/datatx/datatx.go b/pkg/rhttp/datatx/datatx.go index b770f73b890..9bd39d16bd1 100644 --- a/pkg/rhttp/datatx/datatx.go +++ b/pkg/rhttp/datatx/datatx.go @@ -28,12 +28,16 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/owncloud/reva/v2/pkg/events" "github.com/owncloud/reva/v2/pkg/storage" + pkgupload "github.com/owncloud/reva/v2/pkg/upload" "github.com/owncloud/reva/v2/pkg/utils" ) // DataTX provides an abstraction around various data transfer protocols. +// +// Uploads go through the coordinator so the finish path is the same for every +// driver; the driver itself is still needed for downloads. type DataTX interface { - Handler(fs storage.FS) (http.Handler, error) + Handler(coord pkgupload.Coordinator, driver storage.FS) (http.Handler, error) } // EmitFileUploadedEvent is a helper function which publishes a FileUploaded event diff --git a/pkg/rhttp/datatx/manager/simple/simple.go b/pkg/rhttp/datatx/manager/simple/simple.go index 39e60e15662..5a2c8ce8bbd 100644 --- a/pkg/rhttp/datatx/manager/simple/simple.go +++ b/pkg/rhttp/datatx/manager/simple/simple.go @@ -40,6 +40,7 @@ import ( "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storage/cache" "github.com/owncloud/reva/v2/pkg/storagespace" + pkgupload "github.com/owncloud/reva/v2/pkg/upload" "github.com/owncloud/reva/v2/pkg/utils" ) @@ -78,7 +79,7 @@ func New(m map[string]interface{}, publisher events.Publisher, log *zerolog.Logg }, nil } -func (m *manager) Handler(fs storage.FS) (http.Handler, error) { +func (m *manager) Handler(coord pkgupload.Coordinator, driver storage.FS) (http.Handler, error) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { sublog := m.log.With().Str("path", r.URL.Path).Logger() r = r.WithContext(appctx.WithLogger(r.Context(), &sublog)) @@ -92,7 +93,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { metrics.DownloadsActive.Sub(1) }() } - download.GetOrHeadFile(w, r, fs, "") + download.GetOrHeadFile(w, r, driver, "") case "PUT": metrics.UploadsActive.Add(1) defer func() { @@ -114,7 +115,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { ctx = ctxpkg.ContextSetLockID(ctx, lockID) } - info, err := fs.Upload(ctx, storage.UploadRequest{ + info, err := driver.Upload(ctx, storage.UploadRequest{ Ref: ref, Body: r.Body, Length: r.ContentLength, diff --git a/pkg/rhttp/datatx/manager/spaces/spaces.go b/pkg/rhttp/datatx/manager/spaces/spaces.go index b0b2f3b6ad8..285317997c9 100644 --- a/pkg/rhttp/datatx/manager/spaces/spaces.go +++ b/pkg/rhttp/datatx/manager/spaces/spaces.go @@ -42,6 +42,7 @@ import ( "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storage/cache" "github.com/owncloud/reva/v2/pkg/storagespace" + pkgupload "github.com/owncloud/reva/v2/pkg/upload" "github.com/owncloud/reva/v2/pkg/utils" ) @@ -80,7 +81,7 @@ func New(m map[string]interface{}, publisher events.Publisher, log *zerolog.Logg }, nil } -func (m *manager) Handler(fs storage.FS) (http.Handler, error) { +func (m *manager) Handler(coord pkgupload.Coordinator, driver storage.FS) (http.Handler, error) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var spaceID string spaceID, r.URL.Path = router.ShiftPath(r.URL.Path) @@ -97,7 +98,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { metrics.DownloadsActive.Sub(1) }() } - download.GetOrHeadFile(w, r, fs, spaceID) + download.GetOrHeadFile(w, r, driver, spaceID) case "PUT": metrics.UploadsActive.Add(1) defer func() { @@ -117,7 +118,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { Path: fn, } var info *provider.ResourceInfo - info, err = fs.Upload(ctx, storage.UploadRequest{ + info, err = driver.Upload(ctx, storage.UploadRequest{ Ref: ref, Body: r.Body, Length: r.ContentLength, diff --git a/pkg/rhttp/datatx/manager/tus/tus.go b/pkg/rhttp/datatx/manager/tus/tus.go index ac0e037846e..712332c452c 100644 --- a/pkg/rhttp/datatx/manager/tus/tus.go +++ b/pkg/rhttp/datatx/manager/tus/tus.go @@ -33,13 +33,13 @@ import ( "github.com/owncloud/reva/v2/internal/http/services/owncloud/ocdav/net" "github.com/owncloud/reva/v2/pkg/appctx" - "github.com/owncloud/reva/v2/pkg/errtypes" "github.com/owncloud/reva/v2/pkg/events" "github.com/owncloud/reva/v2/pkg/rhttp/datatx" "github.com/owncloud/reva/v2/pkg/rhttp/datatx/manager/registry" "github.com/owncloud/reva/v2/pkg/rhttp/datatx/metrics" "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storagespace" + pkgupload "github.com/owncloud/reva/v2/pkg/upload" ) func init() { @@ -87,20 +87,15 @@ func New(m map[string]interface{}, publisher events.Publisher, log *zerolog.Logg }, nil } -func (m *manager) Handler(fs storage.FS) (http.Handler, error) { - composable, ok := fs.(storage.ComposableFS) - if !ok { - return nil, errtypes.NotSupported("file system does not support the tus protocol") - } - +func (m *manager) Handler(coord pkgupload.Coordinator, driver storage.FS) (http.Handler, error) { // A storage backend for tusd may consist of multiple different parts which // handle upload creation, locking, termination and so on. The composer is a - // place where all those separated pieces are joined together. In this example - // we only use the file store but you may plug in multiple. + // place where all those separated pieces are joined together. composer := tusd.NewStoreComposer() - // let the composable storage tell tus which extensions it supports - composable.UseIn(composer) + // The coordinator is the tus data store for every driver, so finishing an + // upload always ends in driver.CommitUpload. + coord.UseIn(composer) config := tusd.Config{ StoreComposer: composer, @@ -130,33 +125,32 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { return nil, err } - if usl, ok := fs.(storage.UploadSessionLister); ok { - // We can currently only send updates if the fs is decomposedfs as we read very specific keys from the storage map of the tus info - go func() { - for { - ev := <-handler.CompleteUploads - // We should be able to get the upload progress with fs.GetUploadProgress, but currently tus will erase the info files - // so we create a Progress instance here that is used to read the correct properties - ups, err := usl.ListUploadSessions(context.Background(), storage.UploadSessionFilter{ID: &ev.Upload.ID}) - if err != nil { - appctx.GetLogger(context.Background()).Error().Err(err).Str("session", ev.Upload.ID).Msg("failed to list upload session") - } else { - if len(ups) < 1 { - appctx.GetLogger(context.Background()).Error().Str("session", ev.Upload.ID).Msg("upload session not found") - continue - } - up := ups[0] - executant := up.Executant() - ref := up.Reference() - if m.publisher != nil { - if err := datatx.EmitFileUploadedEvent(up.SpaceOwner(), &executant, &ref, m.publisher); err != nil { - appctx.GetLogger(context.Background()).Error().Err(err).Msg("failed to publish FileUploaded event") - } - } + // The coordinator owns the sessions for every driver, so this no longer + // depends on the driver being decomposedfs. + go func() { + for { + ev := <-handler.CompleteUploads + // We should be able to get the upload progress with fs.GetUploadProgress, but currently tus will erase the info files + // so we create a Progress instance here that is used to read the correct properties + ups, err := coord.ListUploadSessions(context.Background(), storage.UploadSessionFilter{ID: &ev.Upload.ID}) + if err != nil { + appctx.GetLogger(context.Background()).Error().Err(err).Str("session", ev.Upload.ID).Msg("failed to list upload session") + continue + } + if len(ups) < 1 { + appctx.GetLogger(context.Background()).Error().Str("session", ev.Upload.ID).Msg("upload session not found") + continue + } + up := ups[0] + executant := up.Executant() + ref := up.Reference() + if m.publisher != nil { + if err := datatx.EmitFileUploadedEvent(up.SpaceOwner(), &executant, &ref, m.publisher); err != nil { + appctx.GetLogger(context.Background()).Error().Err(err).Msg("failed to publish FileUploaded event") } } - }() - } + } + }() h := handler.Middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { sublog := m.log.With().Str("uploadid", r.URL.Path).Logger() @@ -174,7 +168,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { metrics.UploadsActive.Sub(1) }() // set etag, mtime and file id - setHeaders(fs, w, r) + setHeaders(coord, w, r) handler.PostFile(w, r) case "HEAD": handler.HeadFile(w, r) @@ -184,7 +178,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { metrics.UploadsActive.Sub(1) }() // set etag, mtime and file id - setHeaders(fs, w, r) + setHeaders(coord, w, r) handler.PatchFile(w, r) case "DELETE": handler.DelFile(w, r) @@ -205,15 +199,10 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { return h, nil } -func setHeaders(fs storage.FS, w http.ResponseWriter, r *http.Request) { +func setHeaders(coord pkgupload.Coordinator, w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := path.Base(r.URL.Path) - datastore, ok := fs.(tusd.DataStore) - if !ok { - appctx.GetLogger(ctx).Error().Interface("fs", fs).Msg("storage is not a tus datastore") - return - } - upload, err := datastore.GetUpload(ctx, id) + upload, err := coord.GetUpload(ctx, id) if err != nil { appctx.GetLogger(ctx).Error().Err(err).Msg("could not get upload from storage") return @@ -227,6 +216,14 @@ func setHeaders(fs storage.FS, w http.ResponseWriter, r *http.Request) { if expires != "" { w.Header().Set(net.HeaderTusUploadExpires, expires) } + // These headers are written before the upload is finished. For a new file the + // coordinator has only minted a placeholder node id at initiate; the real one + // arrives from TouchFile at commit. Announcing the placeholder makes callers + // stat an id that does not exist, so leave the header off and let them resolve + // the file by path instead. + if info.Storage["NodeExists"] != "true" { + return + } resourceid := &provider.ResourceId{ StorageId: info.MetaData["providerID"], SpaceId: info.Storage["SpaceRoot"], diff --git a/pkg/upload/coordinated_upload.go b/pkg/upload/coordinated_upload.go new file mode 100644 index 00000000000..4765e45e2ca --- /dev/null +++ b/pkg/upload/coordinated_upload.go @@ -0,0 +1,195 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +// This file holds the whole tusd adapter surface: the per-upload +// coordinatedUpload type plus the coordinator methods that exist only to satisfy +// tusd's DataStore interfaces. Upload logic itself lives in coordinator.go. + +package upload + +import ( + "context" + "fmt" + "io" + "net/http" + "os" + + tusd "github.com/tus/tusd/v2/pkg/handler" + + "github.com/owncloud/reva/v2/pkg/errtypes" +) + +var errNotImplemented = tusd.NewError("ERR_NOT_IMPLEMENTED", "use InitiateUpload on the CS3 API to start a new upload", http.StatusNotImplemented) + +// coordinatedUpload adapts a single upload session to the tusd.Upload interface +// family. +// +// It exists because tusd's per-upload methods (WriteChunk, FinishUpload) carry no +// upload id, so the receiver itself must identify the upload. The coordinator is +// shared process-wide and cannot hold that state without racing between +// concurrent uploads. +// +// This type only translates; all upload logic lives in coordinator. +type coordinatedUpload struct { + session Session + coord *coordinator +} + +func (u *coordinatedUpload) GetInfo(ctx context.Context) (tusd.FileInfo, error) { + return u.session.GetInfo(ctx) +} + +func (u *coordinatedUpload) GetReader(ctx context.Context) (io.ReadCloser, error) { + return u.session.GetReader(ctx) +} + +func (u *coordinatedUpload) WriteChunk(ctx context.Context, offset int64, src io.Reader) (int64, error) { + return u.session.WriteChunk(ctx, offset, src) +} + +// FinishUpload runs the coordinator's finish path once all bytes have arrived. +// +// The context tusd provides carries no user, so the session rebuilds the one +// recorded at initiate time. Errors are mapped to tusd errors because tusd turns +// error types it does not recognise into a bare 500. +func (u *coordinatedUpload) FinishUpload(ctx context.Context) error { + // tusd reports the result via its own hooks, so the committed resource is + // only of interest to the PUT path. + _, err := u.coord.finishUpload(u.session.Context(ctx), u.session) + + switch err.(type) { + case nil: + return nil + case errtypes.AlreadyExists: + return tusd.NewError("ERR_ALREADY_EXISTS", err.Error(), http.StatusConflict) + case errtypes.ResourceProcessing, errtypes.TooEarly: + return tusd.NewError("ERR_TOO_EARLY", err.Error(), http.StatusTooEarly) + case errtypes.Aborted: + return tusd.NewError("ERR_PRECONDITION_FAILED", err.Error(), http.StatusPreconditionFailed) + case errtypes.PreconditionFailed: + return tusd.NewError("ERR_PRECONDITION_FAILED", err.Error(), http.StatusMethodNotAllowed) + case errtypes.Locked: + return tusd.NewError("ERR_LOCKED", err.Error(), http.StatusLocked) + case errtypes.BadRequest: + return tusd.NewError("ERR_BAD_REQUEST", err.Error(), http.StatusBadRequest) + case errtypes.ChecksumMismatch: + return tusd.NewError("ERR_CHECKSUM_MISMATCH", err.Error(), errtypes.StatusChecksumMismatch) + default: + return err + } +} + +// Terminate discards an upload: it drops the staged files and, when this upload +// created the node, removes it again so a cancelled upload leaves nothing behind. +func (u *coordinatedUpload) Terminate(ctx context.Context) error { + u.session.Cleanup(true, true) + + // Terminate can run before the node was created, leaving nothing to undo. + ref := u.session.Reference() + if ref.GetResourceId().GetOpaqueId() == "" { + return nil + } + + _ = u.coord.fs.MarkProcessing(ctx, &ref, false, u.session.ID()) + if !u.session.NodeExists() { + _, _ = u.coord.fs.Delete(ctx, &ref) + } + return nil +} + +// DeclareLength records the total size for uploads initiated without one +// (creation-defer-length), so the finish path knows when all bytes have arrived. +func (u *coordinatedUpload) DeclareLength(ctx context.Context, length int64) error { + u.session.SetSize(length) + u.session.SetSizeIsDeferred(false) + return u.session.Persist(ctx) +} + +// ConcatUploads appends the staged bytes of the partial uploads to this upload, +// in the order given, implementing the TUS concatenation extension. +func (u *coordinatedUpload) ConcatUploads(ctx context.Context, partials []tusd.Upload) error { + file, err := os.OpenFile(u.session.BinPath(), os.O_WRONLY|os.O_APPEND, defaultFilePerm) + if err != nil { + return err + } + defer file.Close() + + for _, partial := range partials { + cu, ok := partial.(*coordinatedUpload) + if !ok { + return fmt.Errorf("coordinator: unexpected partial upload type %T", partial) + } + src, err := cu.session.GetReader(ctx) + if err != nil { + return err + } + _, copyErr := io.Copy(file, src) + src.Close() + if copyErr != nil { + return copyErr + } + } + return nil +} + +// UseIn registers the coordinator as tusd's data store, in place of the driver's +// own store. This is what makes the TUS data path run through the coordinator for +// every driver, so finishing an upload ends in driver.CommitUpload. +// +// The opt-in extensions mirror what the drivers registered before, so no TUS +// capability is lost: without them tusd answers those requests with 501. +func (c *coordinator) UseIn(composer *tusd.StoreComposer) { + composer.UseCore(c) + composer.UseTerminater(c) + composer.UseConcater(c) + composer.UseLengthDeferrer(c) +} + +// NewUpload is unsupported: tusd may not create sessions on its own. Uploads are +// always started through the CS3 InitiateUpload call, which performs the +// permission, quota and lock checks that tusd knows nothing about. +func (c *coordinator) NewUpload(_ context.Context, _ tusd.FileInfo) (tusd.Upload, error) { + return nil, errNotImplemented +} + +// GetUpload loads the session with the given id and wraps it so the TUS data +// path runs through the coordinator. This is the only place a coordinatedUpload +// is constructed. +func (c *coordinator) GetUpload(ctx context.Context, id string) (tusd.Upload, error) { + session, err := c.store.Get(ctx, id) + if err != nil { + return nil, err + } + return &coordinatedUpload{session: session, coord: c}, nil +} + +// The As* methods let tusd reach the extension interfaces on an upload it holds as +// a plain tusd.Upload. Every upload we hand out is a *coordinatedUpload, which +// implements all three. + +func (c *coordinator) AsTerminatableUpload(up tusd.Upload) tusd.TerminatableUpload { + return up.(*coordinatedUpload) +} + +func (c *coordinator) AsLengthDeclarableUpload(up tusd.Upload) tusd.LengthDeclarableUpload { + return up.(*coordinatedUpload) +} + +func (c *coordinator) AsConcatableUpload(up tusd.Upload) tusd.ConcatableUpload { + return up.(*coordinatedUpload) +} diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go new file mode 100644 index 00000000000..8e8f2d655b8 --- /dev/null +++ b/pkg/upload/coordinator.go @@ -0,0 +1,655 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +// Package upload provides the driver-agnostic upload coordinator. +package upload + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/google/uuid" + tusd "github.com/tus/tusd/v2/pkg/handler" + + "github.com/owncloud/reva/v2/pkg/appctx" + ctxpkg "github.com/owncloud/reva/v2/pkg/ctx" + "github.com/owncloud/reva/v2/pkg/errtypes" + "github.com/owncloud/reva/v2/pkg/events" + "github.com/owncloud/reva/v2/pkg/rhttp/datatx/metrics" + "github.com/owncloud/reva/v2/pkg/storage" + "github.com/owncloud/reva/v2/pkg/storage/utils/chunking" + "github.com/owncloud/reva/v2/pkg/utils" +) + +// Coordinator owns the upload lifecycle: session initiation, the TUS data +// transfer, and listing sessions. +type Coordinator interface { + // InitiateUpload returns a list of protocols with urls that can be used to append bytes to a new upload session. + InitiateUpload(ctx context.Context, ref *provider.Reference, uploadLength int64, metadata map[string]string) (map[string]string, error) + // GetUpload returns the session with the given id as a tusd upload. + GetUpload(ctx context.Context, id string) (tusd.Upload, error) + // UseIn registers the coordinator as the tusd data store. + UseIn(composer *tusd.StoreComposer) + // ListUploadSessions returns the upload sessions matching the given filter. + ListUploadSessions(ctx context.Context, filter storage.UploadSessionFilter) ([]storage.UploadSession, error) + // Upload writes the whole body of a non-resumable (PUT) upload into the + // session named by req.Ref.Path and finishes it. + Upload(ctx context.Context, req storage.UploadRequest, uff storage.UploadFinishedFunc) (*provider.ResourceInfo, error) +} + +// coordinator is the concrete implementation of Coordinator. +type coordinator struct { + fs storage.FS + store SessionStore + chunkHandler *chunking.ChunkHandler + pub events.Publisher +} + +// NewCoordinator constructs a coordinator backed by the given storage driver +// and session store. The store must use an on-disk session format the driver's +// data path can read (the decomposedfs family: ocis/s3ng/posix). +// +// chunkFolder stages legacy chunking-v1 parts until the final one arrives; pass +// "" to reject chunked uploads. +// +// pub receives the UploadReady event that tells the rest of the system a file is +// available; pass nil to disable publishing. +func NewCoordinator(fs storage.FS, store SessionStore, chunkFolder string, pub events.Publisher) *coordinator { + c := &coordinator{fs: fs, store: store, pub: pub} + if chunkFolder != "" { + c.chunkHandler = chunking.NewChunkHandler(chunkFolder) + } + return c +} + +// InitiateUpload returns a list of protocols with urls that can be used to append bytes to a new upload session. +func (c *coordinator) InitiateUpload(ctx context.Context, ref *provider.Reference, uploadLength int64, metadata map[string]string) (map[string]string, error) { + return c.initiateUpload(ctx, ref, uploadLength, metadata) +} + +// initiateUpload is the driver-agnostic port of decomposedfs.InitiateUpload. +// +// Known open divergences from main, tracked as findings and NOT yet resolved: +// - B2: permission-gated GetMD hides deny-granted files → late 409 instead of 403. +// - B6/B7: spaceOwner manager-fallback and posix scoping (SpaceGid, RunInBaseScope). +func (c *coordinator) initiateUpload(ctx context.Context, ref *provider.Reference, uploadLength int64, metadata map[string]string) (map[string]string, error) { + var chunkName string + if chunking.IsChunked(ref.GetPath()) { // check legacy chunking v1 + var rerr error + ref, chunkName, rerr = rewriteChunkedRef(ref) + if rerr != nil { + return nil, rerr + } + } + + // nodeExists=false is overloaded: genuinely absent, or exists-but-hidden by a deny-grant. + // + // TODO(OCISDEV-900): permission-gated GetMD hides a deny-granted + // file as NotFound, so we take the new-file branch (200) and fail late at + // finish with 409 instead of main's up-front 403 — an existence oracle plus a + // wasted upload. Accepted for now; a clean fix needs a permission-free resolve. + existing, err := c.fs.GetMD(ctx, ref, []string{}, []string{}) + var nodeExists bool + switch err.(type) { + case nil: + nodeExists = true + case errtypes.IsNotFound: + nodeExists = false + default: + return nil, err + } + + var nodeID, spaceID, parentID, dir, nodeName string + var spaceOwner *user.UserId + + // check quota + if uploadLength >= 0 { + spaceRef := &provider.Reference{ResourceId: &provider.ResourceId{ + StorageId: ref.GetResourceId().GetStorageId(), + SpaceId: ref.GetResourceId().GetSpaceId(), + }} + // GetQuota is permission-gated: roles that can upload but lack GetQuota (Uploader, share) error here, so we fail open and let finish enforce. + if _, _, remaining, qErr := c.fs.GetQuota(ctx, spaceRef); qErr == nil { + var existingSize uint64 + if nodeExists { + existingSize = existing.GetSize() + } + netRequired := uint64(uploadLength) + if existingSize < netRequired { + netRequired -= existingSize + } else { + netRequired = 0 + } + if remaining < netRequired { + return nil, errtypes.InsufficientStorage("quota exceeded") + } + } + } + + if nodeExists { + nodeID = existing.GetId().GetOpaqueId() + spaceID = existing.GetId().GetSpaceId() + parentID = existing.GetParentId().GetOpaqueId() + // GetMD returns only the basename for relative (id-based) refs, so + // filepath.Dir would yield "." here. Reconstruct the space-relative + // path via the public FS interface — mirrors main's fs.lu.Path. + // Best-effort: on error keep the basename rather than failing an + // upload main would allow. + relPath := existing.GetPath() + if utils.IsRelativeReference(ref) { + if full, pErr := c.fs.GetPathByID(ctx, existing.GetId()); pErr == nil { + relPath = full + } + } + dir = filepath.Dir(relPath) + nodeName = existing.GetName() + // TODO(OCISDEV-900, finding B6): main uses SpaceOwnerOrManager (falls back to a + // manager when owner is nil/SPACE_OWNER, e.g. project drives). GetOwner() has no + // such fallback, and the new-file branch never sets spaceOwner at all. + spaceOwner = existing.GetOwner() + + // A driver signals "not locked" with an error (NotFound), and one that + // cannot report locks at all with NotSupported. Neither may block the + // upload, so only a lock we actually hold is treated as one. + diskLock, lockErr := c.fs.GetLock(ctx, ref) + if lockErr != nil { + diskLock = nil + } + contextLockID, _ := ctxpkg.ContextGetLockID(ctx) + if diskLock != nil { + switch contextLockID { + case "": + return nil, errtypes.Locked(diskLock.LockId) + case diskLock.LockId: + // ok + default: + return nil, errtypes.Aborted("mismatching lock") + } + } else if contextLockID != "" { + return nil, errtypes.Aborted("not locked") + } + } else { + spaceID = ref.GetResourceId().GetSpaceId() + dir = filepath.Dir(ref.GetPath()) + nodeName = filepath.Base(ref.GetPath()) + } + + if nodeExists { + if !existing.GetPermissionSet().GetInitiateFileUpload() { + return nil, errtypes.PermissionDenied(ref.GetPath()) + } + if existing.GetType() == provider.ResourceType_RESOURCE_TYPE_CONTAINER { + return nil, errtypes.PreconditionFailed("resource is not a file") + } + if metadata["if-none-match"] == "*" { + return nil, errtypes.Aborted(fmt.Sprintf("parent %s already has a child %s, id %s", parentID, nodeName, nodeID)) + } + } else { + parentRef := &provider.Reference{ + ResourceId: ref.GetResourceId(), + Path: dir, + } + parentMD, pErr := c.fs.GetMD(ctx, parentRef, []string{}, []string{}) + switch pErr.(type) { + case nil: + case errtypes.IsNotFound: + // GetMD collapses "dir missing" and "dir hidden (no access)" both into NotFound. + // Walk up: if any ancestor is visible, the dir is genuinely missing → PreconditionFailed. + // If nothing is visible up to the root, the caller has no access → PermissionDenied. + ancestor := dir + permDenied := true + for ancestor != "." && ancestor != "/" { + ancestor = filepath.Dir(ancestor) + ancestorRef := &provider.Reference{ResourceId: ref.GetResourceId(), Path: ancestor} + if _, aErr := c.fs.GetMD(ctx, ancestorRef, []string{}, []string{}); aErr == nil { + permDenied = false + break + } + } + if permDenied { + return nil, errtypes.PermissionDenied(ref.GetPath()) + } + return nil, errtypes.PreconditionFailed(pErr.Error()) + default: + return nil, pErr + } + if !parentMD.GetPermissionSet().GetInitiateFileUpload() { + return nil, errtypes.PermissionDenied(ref.GetPath()) + } + parentID = parentMD.GetId().GetOpaqueId() + spaceID = parentMD.GetId().GetSpaceId() + // id-based refs yield a relative dir ("."); store the parent's full path so + // the UploadReady event carries a space-relative path (main: fs.lu.Path). best-effort. + if utils.IsRelativeReference(ref) { + if parentPath, pErr := c.fs.GetPathByID(ctx, parentMD.GetId()); pErr == nil { + dir = parentPath + } + } + } + + if nodeName == "" { + return nil, errtypes.BadRequest("coordinator: missing filename in ref") + } + if dir == "" { + return nil, errtypes.BadRequest("coordinator: could not determine upload directory") + } + + session := c.store.New(ctx) + session.SetMetadata("filename", nodeName) + session.SetStorageValue("NodeName", nodeName) + session.SetMetadata("dir", dir) + session.SetStorageValue("Dir", dir) + session.SetStorageValue("SpaceRoot", spaceID) + if nodeExists { + session.SetStorageValue("NodeId", nodeID) + session.SetStorageValue("NodeExists", "true") + } else { + //todo not sure if this is correct + // mint the future node id for the new file (main: upload.go:308) + session.SetStorageValue("NodeId", uuid.New().String()) + } + session.SetStorageValue("NodeParentId", parentID) + if spaceOwner != nil { + session.SetStorageValue("SpaceOwnerOrManager", spaceOwner.GetOpaqueId()) + session.SetStorageValue("SpaceOwnerIdp", spaceOwner.GetIdp()) + session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(spaceOwner.GetType())) + } + + // TODO(OCISDEV-900, finding B7): main copies CtxKeySpaceGID into the session + // (upload.go:188) to drive posix uid/gid scoping at commit. That key lives in the + // decomposedfs package; reading it here would make the driver-agnostic coordinator + // depend on a concrete driver. posix-only concern (unset on ocis/s3ng). Deferred. + + usr := ctxpkg.ContextMustGetUser(ctx) + session.SetExecutant(usr) + + lockID, _ := ctxpkg.ContextGetLockID(ctx) + session.SetMetadata("lockid", lockID) + + iid, _ := ctxpkg.ContextGetInitiator(ctx) + session.SetMetadata("initiatorid", iid) + + session.SetSize(uploadLength) + + var mtimeSet bool + if metadata != nil { + session.SetMetadata("providerID", metadata["providerID"]) + if v, ok := metadata["mtime"]; ok && v != "null" { + session.SetMetadata("mtime", v) + mtimeSet = true + } + if v, ok := metadata["expires"]; ok && v != "null" { + session.SetMetadata("expires", v) + } + if _, ok := metadata["sizedeferred"]; ok { + session.SetSizeIsDeferred(true) + } + if checksum, ok := metadata["checksum"]; ok { + parts := strings.SplitN(checksum, " ", 2) + if len(parts) != 2 { + return nil, errtypes.BadRequest("invalid checksum format. must be '[algorithm] [checksum]'") + } + switch parts[0] { + case "sha1", "md5", "adler32": + session.SetMetadata("checksum", checksum) + default: + return nil, errtypes.BadRequest("unsupported checksum algorithm: " + parts[0]) + } + } + if v := metadata["if-match"]; v != "" { + session.SetMetadata("if-match", v) + } + if v := metadata["if-none-match"]; v != "" { + session.SetMetadata("if-none-match", v) + } + if v := metadata["if-unmodified-since"]; v != "" { + session.SetMetadata("if-unmodified-since", v) + } + } + + if !mtimeSet { + session.SetMetadata("mtime", utils.TimeToOCMtime(time.Now())) + } + if chunkName != "" { // check legacy chunking v1 + session.SetStorageValue("Chunk", chunkName) + } + + // TODO(OCISDEV-900, finding B7): main wraps TouchBin+Persist in fs.um.RunInBaseScope + // (upload.go:316) so the .bin/.info files get correct posix ownership. That usermapper + // lives in decomposedfs; the driver-agnostic coordinator can't reach it. posix-only + // (no-op on ocis/s3ng). Same root cause as SpaceGid; deferred. + if err := session.TouchBin(); err != nil { + return nil, fmt.Errorf("coordinator: could not create bin file: %w", err) + } + if err := session.Persist(ctx); err != nil { + session.Cleanup(true, true) + return nil, fmt.Errorf("coordinator: could not persist session: %w", err) + } + + metrics.UploadSessionsInitiated.Inc() + + if uploadLength == 0 { + // zero-length uploads have no bytes to append, so finish immediately (main: upload.go:333) + if _, err := c.finishUpload(ctx, session); err != nil { + return nil, err + } + } + + return map[string]string{ + "simple": session.ID(), + "tus": session.ID(), + }, nil +} + +// Upload writes the entire body of a non-resumable (PUT) upload and finishes it. +// req.Ref.Path carries the session id, as minted by InitiateUpload. +// +// This is the driver-agnostic port of decomposedfs.Upload (upload.go:51). It is +// not yet wired up: the simple and spaces data transfer managers still call +// driver.Upload, because only decomposedfs and ocm implement CommitUpload and +// routing the others here would break their PUT path. +func (c *coordinator) Upload(ctx context.Context, req storage.UploadRequest, uff storage.UploadFinishedFunc) (*provider.ResourceInfo, error) { + // The datatx handler passes the request path straight through, and it arrives + // rooted ("/"), while session ids are stored unrooted. + session, err := c.store.Get(ctx, strings.TrimPrefix(req.Ref.GetPath(), "/")) + if err != nil { + return nil, err + } + + // The session records the user that initiated the upload; the PUT request + // context may be a different one (or none, behind the data gateway). + ctx = session.Context(ctx) + + if chunk := session.Chunk(); chunk != "" { // legacy chunking v1 + if c.chunkHandler == nil { + return nil, errtypes.NotSupported("coordinator: chunked uploads require a chunk folder") + } + assembled, assembledSize, done, aErr := c.chunkHandler.Assemble(chunk, req.Body) + if aErr != nil { + return nil, aErr + } + if !done { + // Not the final chunk. Each chunk arrives as its own PUT with its own + // session, while the bytes accumulate in the chunk folder, so this + // session has nothing left to hold (main: upload.go:69). + session.Cleanup(true, true) + return nil, errtypes.PartialContent(req.Ref.String()) + } + defer assembled.Close() + // The assembled size is authoritative: the declared length covers only + // the final chunk, not the whole file. + req.Body, req.Length = assembled, assembledSize + session.SetSize(assembledSize) + } + + size, err := session.WriteChunk(ctx, 0, req.Body) + if err != nil { + return nil, err + } + if size != req.Length { + return nil, errtypes.PartialContent("coordinator: unexpected end of stream") + } + + ri, err := c.finishUpload(ctx, session) + if err != nil { + return nil, err + } + + if uff != nil { + executant := session.Executant() + uff(session.SpaceOwner(), &executant, &provider.Reference{ + ResourceId: &provider.ResourceId{ + StorageId: session.ProviderID(), + SpaceId: session.SpaceID(), + OpaqueId: session.SpaceID(), + }, + Path: utils.MakeRelativePath(filepath.Join(session.Dir(), session.Filename())), + }) + } + + return ri, nil +} + +// finishUpload lands a fully-received upload: create the node (new files), verify +// checksums, then commit the staged bytes. Zero-length uploads always finish here +// synchronously; the async postprocessing path is not ported yet. +// +// Returns the committed resource as the driver reported it, so PUT callers can +// answer with the new etag/mtime/id without recomputing them. +func (c *coordinator) finishUpload(ctx context.Context, session Session) (*provider.ResourceInfo, error) { + if err := c.touchAndMark(ctx, session); err != nil { + return nil, err + } + if err := verifyAndStoreChecksums(ctx, session); err != nil { + c.rollback(ctx, session) + return nil, err + } + if err := session.Persist(ctx); err != nil { + c.rollback(ctx, session) + return nil, err + } + + metrics.UploadProcessing.Inc() + metrics.UploadSessionsBytesReceived.Inc() + + return c.finishSync(ctx, session) +} + +// touchAndMark creates the node for new files (via the public TouchFile, since +// CommitUpload requires an existing node) and marks it as processing. TouchFile +// mints the real node id, so we overwrite the id minted at initiate. +func (c *coordinator) touchAndMark(ctx context.Context, session Session) error { + if !session.NodeExists() { + pathRef := &provider.Reference{ + ResourceId: &provider.ResourceId{ + SpaceId: session.SpaceID(), + OpaqueId: session.NodeParentID(), + }, + Path: session.Filename(), + } + result, err := c.fs.TouchFile(ctx, pathRef, false, session.Metadata()["mtime"]) + if err != nil { + session.Cleanup(true, true) + if _, ok := err.(errtypes.IsNotFound); ok { + return errtypes.PreconditionFailed(err.Error()) + } + return err + } + session.SetStorageValue("NodeId", result.ResourceID.GetOpaqueId()) + session.SetStorageValue("SpaceRoot", result.SpaceID) + if result.SpaceOwner != nil { + session.SetStorageValue("SpaceOwnerOrManager", result.SpaceOwner.GetOpaqueId()) + session.SetStorageValue("SpaceOwnerIdp", result.SpaceOwner.GetIdp()) + session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(result.SpaceOwner.GetType())) + } + } + nodeRef := session.Reference() + if err := c.fs.MarkProcessing(ctx, &nodeRef, true, session.ID()); err != nil { + session.Cleanup(true, true) + if !session.NodeExists() { + _, _ = c.fs.Delete(ctx, &nodeRef) + } + return err + } + return session.Persist(ctx) +} + +// finishSync commits the staged bytes inline, then unmarks processing and cleans up. +func (c *coordinator) finishSync(ctx context.Context, session Session) (*provider.ResourceInfo, error) { + ref := session.Reference() + f, err := os.Open(session.BinPath()) + if err != nil { + c.rollback(ctx, session) + return nil, err + } + ri, err := c.fs.CommitUpload(ctx, &ref, storage.UploadSource{ + Body: f, + Length: session.Size(), + Metadata: session.Metadata(), + Checksums: session.Checksums(), + }) + if err != nil { + c.rollback(ctx, session) + return nil, err + } + _ = c.fs.MarkProcessing(ctx, &ref, false, session.ID()) + session.Cleanup(true, true) + metrics.UploadSessionsFinalized.Inc() + c.publishUploadReady(ctx, session, ri) + return ri, nil +} + +// publishUploadReady announces that the file is available. Consumers such as the +// search indexer only listen for UploadReady when async uploads are configured, +// and would otherwise never learn about a coordinator upload. The coordinator +// commits inline, so by the time we get here the file really is ready and there +// is no postprocessing round trip to wait for. +func (c *coordinator) publishUploadReady(ctx context.Context, session Session, ri *provider.ResourceInfo) { + if c.pub == nil { + return + } + executant := session.Executant() + if err := events.Publish(ctx, c.pub, events.UploadReady{ + UploadID: session.ID(), + Filename: session.Filename(), + SpaceOwner: session.SpaceOwner(), + ExecutingUser: &user.User{ + Id: &executant, + }, + FileRef: &provider.Reference{ + ResourceId: &provider.ResourceId{ + StorageId: session.ProviderID(), + SpaceId: session.SpaceID(), + OpaqueId: session.SpaceID(), + }, + Path: utils.MakeRelativePath(filepath.Join(session.Dir(), session.Filename())), + }, + ResourceID: ri.GetId(), + Timestamp: utils.TSNow(), + }); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("failed to publish UploadReady event") + } +} + +// rollback unmarks processing, cleans up session files, and deletes the node if +// this upload created it (NodeExists=false at initiation). +func (c *coordinator) rollback(ctx context.Context, session Session) { + ref := session.Reference() + _ = c.fs.MarkProcessing(ctx, &ref, false, session.ID()) + session.Cleanup(true, true) + if !session.NodeExists() { + _, _ = c.fs.Delete(ctx, &ref) + } +} + +// verifyAndStoreChecksums computes checksums over the staged binary, validates any +// client-supplied checksum, and stores the results on the session for CommitUpload. +func verifyAndStoreChecksums(ctx context.Context, session Session) error { + sha1h, md5h, adler32h, err := calculateChecksums(ctx, session.BinPath()) + if err != nil { + return err + } + info, err := session.GetInfo(ctx) + if err != nil { + return err + } + if checksum := info.MetaData["checksum"]; checksum != "" { + parts := strings.SplitN(checksum, " ", 2) + if len(parts) != 2 { + return errtypes.BadRequest("invalid checksum format. must be '[algorithm] [checksum]'") + } + var checkErr error + switch parts[0] { + case "sha1": + checkErr = checkHash(parts[1], sha1h) + case "md5": + checkErr = checkHash(parts[1], md5h) + case "adler32": + checkErr = checkHash(parts[1], adler32h) + default: + checkErr = errtypes.BadRequest("unsupported checksum algorithm: " + parts[0]) + } + if checkErr != nil { + return checkErr + } + } + session.SetChecksums(sha1h.Sum(nil), md5h.Sum(nil), adler32h.Sum(nil)) + return nil +} + +// ListUploadSessions returns the upload sessions matching the given filter. +func (c *coordinator) ListUploadSessions(ctx context.Context, filter storage.UploadSessionFilter) ([]storage.UploadSession, error) { + var sessions []Session + if filter.ID != nil && *filter.ID != "" { + session, err := c.store.Get(ctx, *filter.ID) + if err != nil { + return nil, err + } + sessions = []Session{session} + } else { + var err error + sessions, err = c.store.List(ctx) + if err != nil { + return nil, err + } + } + + filtered := []storage.UploadSession{} + now := time.Now() + for _, session := range sessions { + if filter.Processing != nil && *filter.Processing != session.IsProcessing() { + continue + } + if filter.Expired != nil { + if *filter.Expired { + if now.Before(session.Expires()) { + continue + } + } else { + if now.After(session.Expires()) { + continue + } + } + } + if filter.HasVirus != nil { + sr, _ := session.ScanData() + infected := sr != "" + if *filter.HasVirus != infected { + continue + } + } + filtered = append(filtered, session) + } + return filtered, nil +} + +// rewriteChunkedRef parses a legacy chunking-v1 path, returning a reference to the +// real target file plus the original chunk name. +func rewriteChunkedRef(ref *provider.Reference) (*provider.Reference, string, error) { + ci, err := chunking.GetChunkBLOBInfo(ref.GetPath()) + if err != nil { + return nil, "", errtypes.BadRequest(err.Error()) + } + return &provider.Reference{ResourceId: ref.ResourceId, Path: ci.Path}, filepath.Base(ref.GetPath()), nil +} diff --git a/pkg/upload/filestore.go b/pkg/upload/filestore.go new file mode 100644 index 00000000000..566b7082c76 --- /dev/null +++ b/pkg/upload/filestore.go @@ -0,0 +1,196 @@ +package upload + +import ( + "context" + "encoding/json" + iofs "io/fs" + "os" + "path/filepath" + "strings" + "syscall" + + "github.com/google/uuid" + "github.com/mitchellh/mapstructure" + "github.com/pkg/errors" + "github.com/rs/zerolog" + tusd "github.com/tus/tusd/v2/pkg/handler" +) + +// TokenOptions carries the JWT-signing configuration needed to produce transfer +// URLs for the postprocessing service. +type TokenOptions struct { + DownloadEndpoint string + DataGatewayEndpoint string + TransferSharedSecret string + TransferExpires int64 +} + +// SessionStore abstracts upload-session persistence for the Coordinator. +type SessionStore interface { + New(ctx context.Context) Session + Get(ctx context.Context, id string) (Session, error) + List(ctx context.Context) ([]Session, error) +} + +// FileStore is a filesystem-backed SessionStore. Sessions are stored as a pair +// of files under /uploads/: +// +// - .info — JSON-encoded tusd.FileInfo +// - — staged binary bytes +// +// This is the same on-disk format used by OcisStore so existing sessions +// survive a rolling deploy that switches to FileStore. +type FileStore struct { + root string + opts TokenOptions + log *zerolog.Logger +} + +// FileStoreFromDriverConf builds a FileStore from a reva driver config map. +// Returns nil if the config carries no root path (driver does not support +// coordinated uploads). Each service that mounts the same driver calls this +// independently. +func FileStoreFromDriverConf(driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { + if driverConf == nil { + return nil + } + + // storage_root is the ocm driver's spelling of root; it stages uploads under + // the same /uploads/ layout FileStore uses. + type driverRootConf struct { + Root string `mapstructure:"root"` + UploadDirectory string `mapstructure:"upload_directory"` + StorageRoot string `mapstructure:"storage_root"` + } + var rc driverRootConf + _ = mapstructure.Decode(driverConf, &rc) + + root := rc.UploadDirectory + if root == "" { + root = rc.Root + } + if root == "" { + root = rc.StorageRoot + } + if root == "" { + return nil + } + + return newFileStoreWithTokens(root, driverConf, log) +} + +// NewFileStoreFromConfig builds a FileStore using uploadDir when set, falling +// back to root/upload_directory from the active driver config. This allows +// drivers that have no local root (e.g. KW) to still get a coordinator by +// setting upload_directory at the service level rather than inside the driver. +// Returns nil only when neither source resolves to a non-empty path. +func NewFileStoreFromConfig(uploadDir string, driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { + if uploadDir != "" { + return NewFileStore(uploadDir, TokenOptions{}, log) + } + return FileStoreFromDriverConf(driverConf, log) +} + +func newFileStoreWithTokens(root string, driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { + type tokenConf struct { + DownloadEndpoint string `mapstructure:"download_endpoint"` + DataGatewayEndpoint string `mapstructure:"datagateway_endpoint"` + TransferSharedSecret string `mapstructure:"transfer_shared_secret"` + TransferExpires int64 `mapstructure:"transfer_expires"` + } + var tc tokenConf + if tokens, ok := driverConf["tokens"]; ok { + _ = mapstructure.Decode(tokens, &tc) + } + return NewFileStore(root, TokenOptions{ + DownloadEndpoint: tc.DownloadEndpoint, + DataGatewayEndpoint: tc.DataGatewayEndpoint, + TransferSharedSecret: tc.TransferSharedSecret, + TransferExpires: tc.TransferExpires, + }, log) +} + +// NewFileStore creates a FileStore rooted at root. +// root must be on a shared filesystem when multiple pods handle the same space. +func NewFileStore(root string, opts TokenOptions, log *zerolog.Logger) *FileStore { + return &FileStore{root: root, opts: opts, log: log} +} + +// Root returns the base directory of this FileStore. +func (fs *FileStore) Root() string { + return fs.root +} + +// Setup creates the uploads directory eagerly so permission problems are caught +// at startup rather than on the first upload. +func (fs *FileStore) Setup() error { + return os.MkdirAll(filepath.Join(fs.root, "uploads"), 0700) +} + +// New allocates a fresh session with a new UUID. +func (fs *FileStore) New(_ context.Context) Session { + return &FileSession{ + store: fs, + info: tusd.FileInfo{ + ID: uuid.New().String(), + Storage: map[string]string{ + "Type": "OCISStore", + }, + MetaData: tusd.MetaData{}, + }, + } +} + +// Get loads the session with the given id from disk. +func (fs *FileStore) Get(ctx context.Context, id string) (Session, error) { + infoPath := fileSessionPath(fs.root, id) + + data, err := os.ReadFile(infoPath) + if err != nil { + if pathErr, ok := err.(*os.PathError); ok && pathErr.Err == syscall.ESTALE { + return nil, tusd.ErrNotFound + } + if errors.Is(err, iofs.ErrNotExist) { + return nil, tusd.ErrNotFound + } + return nil, err + } + + var info tusd.FileInfo + if err := json.Unmarshal(data, &info); err != nil { + return nil, err + } + + session := &FileSession{store: fs, info: info} + + stat, err := os.Stat(session.binPath()) + if err != nil { + if os.IsNotExist(err) { + return nil, tusd.ErrNotFound + } + return nil, err + } + session.info.Offset = stat.Size() + + return session, nil +} + +// List returns all sessions found under /uploads/*.info. +func (fs *FileStore) List(ctx context.Context) ([]Session, error) { + infoFiles, err := filepath.Glob(filepath.Join(fs.root, "uploads", "*.info")) + if err != nil { + return nil, err + } + + sessions := make([]Session, 0, len(infoFiles)) + for _, path := range infoFiles { + id := strings.TrimSuffix(filepath.Base(path), ".info") + session, err := fs.Get(ctx, id) + if err != nil { + fs.log.Error().Str("path", path).Err(err).Msg("filestore: could not load session") + continue + } + sessions = append(sessions, session) + } + return sessions, nil +} diff --git a/pkg/upload/filestore_test.go b/pkg/upload/filestore_test.go new file mode 100644 index 00000000000..8ad5c5b9141 --- /dev/null +++ b/pkg/upload/filestore_test.go @@ -0,0 +1,312 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package upload + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + tusd "github.com/tus/tusd/v2/pkg/handler" +) + +func nopLog() *zerolog.Logger { + l := zerolog.Nop() + return &l +} + +// FileStore.Setup + +func TestFileStoreSetup_CreatesUploadsDir(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + + err := fs.Setup() + + require.NoError(t, err) + info, err := os.Stat(filepath.Join(root, "uploads")) + require.NoError(t, err) + assert.True(t, info.IsDir()) +} + +func TestFileStoreSetup_Idempotent(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + + require.NoError(t, fs.Setup()) + assert.NoError(t, fs.Setup()) +} + +// FileStore.New + +func TestFileStoreNew_NonEmptyID(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(context.Background()) + + assert.NotEmpty(t, s.ID()) +} + +func TestFileStoreNew_UniqueIDs(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s1 := fs.New(context.Background()) + s2 := fs.New(context.Background()) + + assert.NotEqual(t, s1.ID(), s2.ID()) +} + +func TestFileStoreNew_StorageTypeIsFileStore(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(context.Background()) + + info, err := s.GetInfo(context.Background()) + require.NoError(t, err) + assert.Equal(t, "FileStore", info.Storage["Type"]) +} + +func TestFileStoreNew_MetaDataIsNotNil(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(context.Background()) + + info, err := s.GetInfo(context.Background()) + require.NoError(t, err) + assert.NotNil(t, info.MetaData) +} + +// FileStore.Get + +func TestFileStoreGet_HappyPath(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(ctx).(*FileSession) + require.NoError(t, s.TouchBin()) + require.NoError(t, s.Persist(ctx)) + + got, err := fs.Get(ctx, s.ID()) + + require.NoError(t, err) + assert.Equal(t, s.ID(), got.ID()) + info, err := got.GetInfo(ctx) + require.NoError(t, err) + assert.Equal(t, "FileStore", info.Storage["Type"]) +} + +func TestFileStoreGet_OffsetFromBinSize(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(ctx).(*FileSession) + require.NoError(t, s.TouchBin()) + require.NoError(t, s.Persist(ctx)) + + payload := []byte("hello world") + require.NoError(t, os.WriteFile(s.binPath(), payload, 0600)) + + got, err := fs.Get(ctx, s.ID()) + + require.NoError(t, err) + assert.Equal(t, int64(len(payload)), got.Offset()) +} + +func TestFileStoreGet_MissingInfoReturnsErrNotFound(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + _, err := fs.Get(context.Background(), "no-such-id") + + assert.ErrorIs(t, err, tusd.ErrNotFound) +} + +func TestFileStoreGet_CorruptInfoReturnsError(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(ctx).(*FileSession) + require.NoError(t, s.TouchBin()) + require.NoError(t, s.Persist(ctx)) + + // Overwrite the .info with garbage JSON. + require.NoError(t, os.WriteFile(s.infoPath(), []byte("{not valid json"), 0600)) + + _, err := fs.Get(ctx, s.ID()) + + require.Error(t, err) + assert.NotErrorIs(t, err, tusd.ErrNotFound) +} + +func TestFileStoreGet_MissingBinReturnsErrNotFound(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s := fs.New(ctx).(*FileSession) + // Write .info but do NOT create the .bin file. + require.NoError(t, s.Persist(ctx)) + + _, err := fs.Get(ctx, s.ID()) + + assert.ErrorIs(t, err, tusd.ErrNotFound) +} + +// FileStore.List + +func TestFileStoreList_EmptyDir(t *testing.T) { + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + sessions, err := fs.List(context.Background()) + + require.NoError(t, err) + assert.Empty(t, sessions) +} + +func TestFileStoreList_ReturnsBothSessions(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + s1 := fs.New(ctx).(*FileSession) + require.NoError(t, s1.TouchBin()) + require.NoError(t, s1.Persist(ctx)) + + s2 := fs.New(ctx).(*FileSession) + require.NoError(t, s2.TouchBin()) + require.NoError(t, s2.Persist(ctx)) + + sessions, err := fs.List(ctx) + + require.NoError(t, err) + ids := make([]string, 0, len(sessions)) + for _, s := range sessions { + ids = append(ids, s.ID()) + } + assert.ElementsMatch(t, []string{s1.ID(), s2.ID()}, ids) +} + +func TestFileStoreList_SkipsSessionWithMissingBin(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, nopLog()) + require.NoError(t, fs.Setup()) + + good := fs.New(ctx).(*FileSession) + require.NoError(t, good.TouchBin()) + require.NoError(t, good.Persist(ctx)) + + bad := fs.New(ctx).(*FileSession) + require.NoError(t, bad.TouchBin()) + require.NoError(t, bad.Persist(ctx)) + require.NoError(t, os.Remove(bad.binPath())) + + sessions, err := fs.List(ctx) + + require.NoError(t, err) + require.Len(t, sessions, 1) + assert.Equal(t, good.ID(), sessions[0].ID()) +} + +// FileStoreFromDriverConf + +func TestFileStoreFromDriverConf_NilConfigReturnsNil(t *testing.T) { + assert.Nil(t, FileStoreFromDriverConf(nil, nopLog())) +} + +func TestFileStoreFromDriverConf_RootKey(t *testing.T) { + root := t.TempDir() + fs := FileStoreFromDriverConf(map[string]interface{}{"root": root}, nopLog()) + + require.NotNil(t, fs) + assert.Equal(t, root, fs.root) +} + +func TestFileStoreFromDriverConf_UploadDirectoryKey(t *testing.T) { + dir := t.TempDir() + fs := FileStoreFromDriverConf(map[string]interface{}{"upload_directory": dir}, nopLog()) + + require.NotNil(t, fs) + assert.Equal(t, dir, fs.root) +} + +func TestFileStoreFromDriverConf_UploadDirectoryWinsOverRoot(t *testing.T) { + root := t.TempDir() + uploadDir := t.TempDir() + fs := FileStoreFromDriverConf(map[string]interface{}{ + "root": root, + "upload_directory": uploadDir, + }, nopLog()) + + require.NotNil(t, fs) + assert.Equal(t, uploadDir, fs.root) +} + +func TestFileStoreFromDriverConf_NeitherKeyReturnsNil(t *testing.T) { + fs := FileStoreFromDriverConf(map[string]interface{}{"some_other_key": "value"}, nopLog()) + + assert.Nil(t, fs) +} + +// NewFileStoreFromConfig + +func TestNewFileStoreFromConfig_UploadDirUsed(t *testing.T) { + uploadDir := t.TempDir() + fs := NewFileStoreFromConfig(uploadDir, map[string]interface{}{"root": "/ignored"}, nopLog()) + + require.NotNil(t, fs) + assert.Equal(t, uploadDir, fs.root) +} + +func TestNewFileStoreFromConfig_FallsBackToDriverConf(t *testing.T) { + root := t.TempDir() + fs := NewFileStoreFromConfig("", map[string]interface{}{"root": root}, nopLog()) + + require.NotNil(t, fs) + assert.Equal(t, root, fs.root) +} + +func TestNewFileStoreFromConfig_BothEmptyReturnsNil(t *testing.T) { + fs := NewFileStoreFromConfig("", nil, nopLog()) + + assert.Nil(t, fs) +} diff --git a/pkg/upload/session.go b/pkg/upload/session.go new file mode 100644 index 00000000000..99fc6f2fd15 --- /dev/null +++ b/pkg/upload/session.go @@ -0,0 +1,454 @@ +package upload + +import ( + "context" + "crypto/md5" //nolint:gosec + "crypto/sha1" //nolint:gosec + "encoding/hex" + "encoding/json" + "fmt" + "hash" + "hash/adler32" + "io" + "os" + "path/filepath" + "strings" + "time" + + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/golang-jwt/jwt/v5" + "github.com/google/renameio/v2" + "github.com/pkg/errors" + tusd "github.com/tus/tusd/v2/pkg/handler" + + "github.com/owncloud/reva/v2/pkg/appctx" + ctxpkg "github.com/owncloud/reva/v2/pkg/ctx" + "github.com/owncloud/reva/v2/pkg/errtypes" + "github.com/owncloud/reva/v2/pkg/storage" + "github.com/owncloud/reva/v2/pkg/utils" +) + +const defaultFilePerm = os.FileMode(0664) + +// FileSession is the Session implementation for disk-backed uploads. While an +// upload is in progress, incoming bytes are staged in a .bin file and upload +// metadata (size, owner, checksums, etc.) is persisted in a .info file. Both +// survive process restarts, allowing TUS resumption. +// +// In scope: read/write the staged .bin file, persist/load upload metadata in the .info file. +// Out of scope: TUS protocol, checksums, event publishing, postprocessing — those live in coordinatedUpload and coordinator. +type FileSession struct { + store *FileStore + info tusd.FileInfo +} + +// Session is the driver-agnostic view of an upload session the Coordinator +// needs. Implementations must be pure state (CRUD): protocol orchestration +// belongs to coordinatedUpload or the coordinator itself. +type Session interface { + storage.UploadSession + + // Data access — delegated to by coordinatedUpload for TUS reads/writes. + GetInfo(ctx context.Context) (tusd.FileInfo, error) + GetReader(ctx context.Context) (io.ReadCloser, error) + WriteChunk(ctx context.Context, offset int64, src io.Reader) (int64, error) + + // Internal coordinator plumbing. + Chunk() string + BinPath() string + ProviderID() string + SpaceID() string + NodeID() string + NodeParentID() string + NodeExists() bool + Dir() string + URL(ctx context.Context) (string, error) + SetScanData(result string, date time.Time) + Checksums() storage.UploadChecksums + SetChecksums(sha1, md5, adler32 []byte) + Metadata() map[string]string + Persist(ctx context.Context) error + Cleanup(cleanBin, cleanInfo bool) + Context(ctx context.Context) context.Context + + // Typed setters used by Coordinator.InitiateUpload to populate a new session + // without knowing internal storage key names. + SetStorageValue(key, value string) + SetMetadata(key, value string) + SetSize(size int64) + SetSizeIsDeferred(value bool) + SetExecutant(u *userpb.User) + TouchBin() error +} + +func (s *FileSession) GetInfo(_ context.Context) (tusd.FileInfo, error) { + return s.info, nil +} + +func (s *FileSession) GetReader(_ context.Context) (io.ReadCloser, error) { + return os.Open(s.binPath()) +} + +func (s *FileSession) WriteChunk(ctx context.Context, offset int64, src io.Reader) (int64, error) { + file, err := os.OpenFile(s.binPath(), os.O_WRONLY|os.O_APPEND, defaultFilePerm) + if err != nil { + return 0, err + } + defer file.Close() + + n, err := io.Copy(file, src) + if err != nil && err != io.ErrUnexpectedEOF { + return n, err + } + s.info.Offset += n + return n, nil +} + +// Purge removes all on-disk state for this session. +func (s *FileSession) Purge(ctx context.Context) { + s.Cleanup(true, true) +} + +// ScanData returns the AV scan result and scan date stored on the session. +func (s *FileSession) ScanData() (string, time.Time) { + date := s.info.MetaData["scanDate"] + if date == "" { + return "", time.Time{} + } + d, _ := time.Parse(time.RFC3339, date) + return s.info.MetaData["scanResult"], d +} + +// ID returns the upload session ID. +func (s *FileSession) ID() string { + return s.info.ID +} + +// Filename returns the filename stored in the session. +func (s *FileSession) Filename() string { + return s.info.Storage["NodeName"] +} + +// Size returns the declared upload size. +func (s *FileSession) Size() int64 { + return s.info.Size +} + +// Offset returns the current upload offset. +func (s *FileSession) Offset() int64 { + return s.info.Offset +} + +// Chunk returns the chunk basename stored in the session, or "" for non-chunked uploads. +func (s *FileSession) Chunk() string { + return s.info.Storage["Chunk"] +} + +// BinPath returns the path to the staged binary file. +func (s *FileSession) BinPath() string { + return s.binPath() +} + +// SpaceGid returns the numeric GID of the space owner, or "" if not set. +func (s *FileSession) SpaceGid() string { + return s.info.Storage["SpaceGid"] +} + +// ProviderID returns the storage provider ID stored in the session. +func (s *FileSession) ProviderID() string { + return s.info.MetaData["providerID"] +} + +// SpaceID returns the space (root) ID. +func (s *FileSession) SpaceID() string { + return s.info.Storage["SpaceRoot"] +} + +// NodeID returns the node ID for this upload. +func (s *FileSession) NodeID() string { + return s.info.Storage["NodeId"] +} + +// NodeParentID returns the parent node ID for this upload. +func (s *FileSession) NodeParentID() string { + return s.info.Storage["NodeParentId"] +} + +// NodeExists returns whether the target node existed when the upload was initiated. +func (s *FileSession) NodeExists() bool { + return s.info.Storage["NodeExists"] == "true" +} + +// Dir returns the directory portion of the upload path. +func (s *FileSession) Dir() string { + return s.info.Storage["Dir"] +} + +// IsProcessing returns true if all bytes are received but postprocessing has not finished. +func (s *FileSession) IsProcessing() bool { + return s.info.Size == s.info.Offset && s.info.MetaData["scanResult"] == "" +} + +// SpaceOwner returns the space owner user ID. +func (s *FileSession) SpaceOwner() *userpb.UserId { + return &userpb.UserId{ + OpaqueId: s.info.Storage["SpaceOwnerOrManager"], + Idp: s.info.Storage["SpaceOwnerIdp"], + Type: userpb.UserType(userpb.UserType_value[s.info.Storage["SpaceOwnerType"]]), + } +} + +// Executant returns the user ID of the user who initiated this upload. +func (s *FileSession) Executant() userpb.UserId { + return userpb.UserId{ + Type: utils.UserTypeMap(s.info.Storage["UserType"]), + Idp: s.info.Storage["Idp"], + OpaqueId: s.info.Storage["UserId"], + } +} + +// Expires returns the upload expiry time. +func (s *FileSession) Expires() time.Time { + var t time.Time + if value, ok := s.info.MetaData["expires"]; ok { + t, _ = utils.MTimeToTime(value) + } + return t +} + +// Reference returns a CS3 reference for the resource being uploaded. +func (s *FileSession) Reference() provider.Reference { + return provider.Reference{ + ResourceId: &provider.ResourceId{ + StorageId: s.info.MetaData["providerID"], + SpaceId: s.info.Storage["SpaceRoot"], + OpaqueId: s.info.Storage["NodeId"], + }, + } +} + +// Checksums returns the pre-computed checksums stored on the session. +func (s *FileSession) Checksums() storage.UploadChecksums { + decode := func(key string) []byte { + b, _ := hex.DecodeString(s.info.MetaData[key]) + return b + } + return storage.UploadChecksums{ + SHA1: decode("checksumSHA1"), + MD5: decode("checksumMD5"), + Adler32: decode("checksumAdler32"), + } +} + +// Metadata returns the upload metadata map passed to CommitUpload. +func (s *FileSession) Metadata() map[string]string { + return map[string]string{ + "providerID": s.info.MetaData["providerID"], + "mtime": s.info.MetaData["mtime"], + "nodeExists": s.info.Storage["NodeExists"], + "versionsPath": s.info.MetaData["versionsPath"], + "sessionID": s.info.ID, + } +} + +// SetScanData stores AV scan results on the session. +func (s *FileSession) SetScanData(result string, date time.Time) { + s.info.MetaData["scanResult"] = result + s.info.MetaData["scanDate"] = date.Format(time.RFC3339) +} + +// SetChecksums stores pre-computed checksums so CommitUpload can use them without +// re-reading the binary file. +func (s *FileSession) SetChecksums(sha1Sum, md5Sum, adler32Sum []byte) { + s.info.MetaData["checksumSHA1"] = hex.EncodeToString(sha1Sum) + s.info.MetaData["checksumMD5"] = hex.EncodeToString(md5Sum) + s.info.MetaData["checksumAdler32"] = hex.EncodeToString(adler32Sum) +} + +// SetMetadata sets a user-visible upload metadata field. +func (s *FileSession) SetMetadata(key, value string) { + s.info.MetaData[key] = value +} + +// SetStorageValue sets an internal storage field on the session. +func (s *FileSession) SetStorageValue(key, value string) { + s.info.Storage[key] = value +} + +// SetSize updates the declared upload size. +func (s *FileSession) SetSize(size int64) { + s.info.Size = size +} + +// SetSizeIsDeferred marks the upload size as not yet known. +func (s *FileSession) SetSizeIsDeferred(value bool) { + s.info.SizeIsDeferred = value +} + +// SetExecutant stores the identity of the user who initiated the upload. +func (s *FileSession) SetExecutant(u *userpb.User) { + s.info.Storage["Idp"] = u.GetId().GetIdp() + s.info.Storage["UserId"] = u.GetId().GetOpaqueId() + s.info.Storage["UserType"] = utils.UserTypeToString(u.GetId().Type) + s.info.Storage["UserName"] = u.GetUsername() + s.info.Storage["UserDisplayName"] = u.GetDisplayName() + b, _ := json.Marshal(u.GetOpaque()) + s.info.Storage["UserOpaque"] = string(b) + g, _ := json.Marshal(u.GetGroups()) + s.info.Storage["UserGroups"] = string(g) +} + +// TouchBin creates the empty staging file. +func (s *FileSession) TouchBin() error { + f, err := os.OpenFile(s.binPath(), os.O_CREATE|os.O_WRONLY, defaultFilePerm) + if err != nil { + return err + } + return f.Close() +} + +// Persist writes the session metadata atomically to disk. +func (s *FileSession) Persist(ctx context.Context) error { + infoPath := s.infoPath() + if err := os.MkdirAll(filepath.Dir(infoPath), 0700); err != nil { + return err + } + d, err := json.Marshal(s.info) + if err != nil { + return err + } + return renameio.WriteFile(infoPath, d, 0600) +} + +// Cleanup removes the staged binary and/or info file. +// Node deletion and processing flag changes are the coordinator's responsibility. +func (s *FileSession) Cleanup(cleanBin, cleanInfo bool) { + log := s.store.log + if cleanBin { + if err := os.Remove(s.binPath()); err != nil && !errors.Is(err, os.ErrNotExist) { + log.Error().Str("path", s.binPath()).Err(err).Msg("filestore: removing staged binary failed") + } + } + if cleanInfo { + if err := os.Remove(s.infoPath()); err != nil && !errors.Is(err, os.ErrNotExist) { + log.Error().Str("path", s.infoPath()).Err(err).Msg("filestore: removing session info failed") + } + } +} + +// Context reconstructs a context carrying the user, logger, lock ID, and +// initiator ID that were recorded when the upload was initiated. +func (s *FileSession) Context(ctx context.Context) context.Context { + sub := s.store.log.With().Int("pid", os.Getpid()).Logger() + ctx = appctx.WithLogger(ctx, &sub) + ctx = ctxpkg.ContextSetLockID(ctx, s.info.MetaData["lockid"]) + ctx = ctxpkg.ContextSetUser(ctx, s.executantUser()) + return ctxpkg.ContextSetInitiator(ctx, s.info.MetaData["initiatorid"]) +} + +// URL returns a signed JWT URL that the postprocessing service can use to +// download the staged binary via the data gateway. +func (s *FileSession) URL(_ context.Context) (string, error) { + type transferClaims struct { + jwt.RegisteredClaims + Target string `json:"target"` + } + + target := joinURLParts(s.store.opts.DownloadEndpoint, "tus/", s.info.ID) + ttl := time.Duration(s.store.opts.TransferExpires) * time.Second + claims := transferClaims{ + RegisteredClaims: jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(time.Now().Add(ttl)), + Audience: jwt.ClaimStrings{"reva"}, + IssuedAt: jwt.NewNumericDate(time.Now()), + }, + Target: target, + } + t := jwt.NewWithClaims(jwt.GetSigningMethod("HS256"), claims) + tkn, err := t.SignedString([]byte(s.store.opts.TransferSharedSecret)) + if err != nil { + return "", errors.Wrapf(err, "filestore: error signing transfer token with claims %+v", claims) + } + return joinURLParts(s.store.opts.DataGatewayEndpoint, tkn), nil +} + +func (s *FileSession) ToFileInfo() tusd.FileInfo { + return s.info +} + +func (s *FileSession) InitiatorID() string { + return s.info.MetaData["initiatorid"] +} + +func (s *FileSession) binPath() string { + return filepath.Join(s.store.root, "uploads", s.info.ID) +} + +func (s *FileSession) infoPath() string { + return fileSessionPath(s.store.root, s.info.ID) +} + +func (s *FileSession) executantUser() *userpb.User { + var o *typespb.Opaque + _ = json.Unmarshal([]byte(s.info.Storage["UserOpaque"]), &o) + var groups []string + _ = json.Unmarshal([]byte(s.info.Storage["UserGroups"]), &groups) + return &userpb.User{ + Id: &userpb.UserId{ + Type: utils.UserTypeMap(s.info.Storage["UserType"]), + Idp: s.info.Storage["Idp"], + OpaqueId: s.info.Storage["UserId"], + }, + Username: s.info.Storage["UserName"], + DisplayName: s.info.Storage["UserDisplayName"], + Opaque: o, + Groups: groups, + } +} + +// fileSessionPath returns the path to the .info file for the given session ID. +func fileSessionPath(root, id string) string { + return filepath.Join(root, "uploads", id+".info") +} + +// calculateChecksums computes sha1, md5, and adler32 in a single pass over path. +func calculateChecksums(_ context.Context, path string) (hash.Hash, hash.Hash, hash.Hash32, error) { + sha1h := sha1.New() //nolint:gosec + md5h := md5.New() //nolint:gosec + adler32h := adler32.New() + + f, err := os.Open(path) + if err != nil { + return nil, nil, nil, err + } + defer f.Close() + + r1 := io.TeeReader(f, sha1h) + r2 := io.TeeReader(r1, md5h) + if _, err = io.Copy(adler32h, r2); err != nil { + return nil, nil, nil, err + } + return sha1h, md5h, adler32h, nil +} + +func checkHash(expected string, h hash.Hash) error { + got := hex.EncodeToString(h.Sum(nil)) + if expected != got { + return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %s", expected, got)) + } + return nil +} + +// joinURLParts concatenates URL path segments, inserting "/" between them if needed. +func joinURLParts(parts ...string) string { + var b strings.Builder + for i, p := range parts { + b.WriteString(p) + if i < len(parts)-1 && !strings.HasSuffix(p, "/") { + b.WriteByte('/') + } + } + return b.String() +} diff --git a/pkg/upload/session_test.go b/pkg/upload/session_test.go new file mode 100644 index 00000000000..8bcf1167a98 --- /dev/null +++ b/pkg/upload/session_test.go @@ -0,0 +1,456 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package upload + +import ( + "context" + "crypto/sha1" //nolint:gosec + "encoding/hex" + "os" + "path/filepath" + "strings" + "testing" + "time" + + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + ctxpkg "github.com/owncloud/reva/v2/pkg/ctx" + "github.com/owncloud/reva/v2/pkg/errtypes" + "github.com/owncloud/reva/v2/pkg/utils" +) + +func newTestSession(t *testing.T) (*FileSession, *FileStore) { + t.Helper() + log := zerolog.Nop() + fs := NewFileStore(t.TempDir(), TokenOptions{}, &log) + sess := fs.New(context.Background()).(*FileSession) + return sess, fs +} + +// SetMetadata / SetStorageValue / SetSize / SetSizeIsDeferred + +func TestSetMetadata(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetMetadata("providerID", "storage-1") + assert.Equal(t, "storage-1", sess.ProviderID()) +} + +func TestSetStorageValue(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetStorageValue("SpaceRoot", "space-abc") + assert.Equal(t, "space-abc", sess.SpaceID()) +} + +func TestSetSize(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetSize(1234) + assert.Equal(t, int64(1234), sess.Size()) +} + +func TestSetSizeIsDeferred(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetSizeIsDeferred(true) + assert.True(t, sess.info.SizeIsDeferred) + sess.SetSizeIsDeferred(false) + assert.False(t, sess.info.SizeIsDeferred) +} + +// SetExecutant / Executant + +func TestSetExecutant_Executant(t *testing.T) { + sess, _ := newTestSession(t) + u := &userpb.User{ + Id: &userpb.UserId{ + Idp: "idp.example.com", + OpaqueId: "user-42", + Type: userpb.UserType_USER_TYPE_PRIMARY, + }, + Username: "alice", + DisplayName: "Alice", + } + sess.SetExecutant(u) + got := sess.Executant() + assert.Equal(t, "idp.example.com", got.Idp) + assert.Equal(t, "user-42", got.OpaqueId) + assert.Equal(t, userpb.UserType_USER_TYPE_PRIMARY, got.Type) +} + +// NodeExists + +func TestNodeExists(t *testing.T) { + sess, _ := newTestSession(t) + + assert.False(t, sess.NodeExists(), "absent key should return false") + + sess.SetStorageValue("NodeExists", "true") + assert.True(t, sess.NodeExists()) + + sess.SetStorageValue("NodeExists", "false") + assert.False(t, sess.NodeExists()) + + sess.SetStorageValue("NodeExists", "yes") + assert.False(t, sess.NodeExists(), "non-'true' value should return false") +} + +// Checksums / SetChecksums + +func TestChecksums_SetChecksums(t *testing.T) { + sess, _ := newTestSession(t) + sha1bytes := []byte{0x01, 0x02, 0x03, 0x04} + md5bytes := []byte{0x05, 0x06, 0x07, 0x08} + adlerBytes := []byte{0x09, 0x0a, 0x0b, 0x0c} + + sess.SetChecksums(sha1bytes, md5bytes, adlerBytes) + got := sess.Checksums() + assert.Equal(t, sha1bytes, got.SHA1) + assert.Equal(t, md5bytes, got.MD5) + assert.Equal(t, adlerBytes, got.Adler32) +} + +// ScanData / SetScanData + +func TestScanData_SetScanData(t *testing.T) { + sess, _ := newTestSession(t) + + result, date := sess.ScanData() + assert.Empty(t, result, "fresh session should have no scan result") + assert.True(t, date.IsZero(), "fresh session should have zero scan date") + + now := time.Now().Truncate(time.Second) + sess.SetScanData("clean", now) + result, date = sess.ScanData() + assert.Equal(t, "clean", result) + assert.WithinDuration(t, now, date, time.Second) +} + +// Expires + +func TestExpires(t *testing.T) { + sess, _ := newTestSession(t) + assert.True(t, sess.Expires().IsZero(), "absent expires should be zero time") + + want := time.Now().Add(time.Hour).Truncate(time.Second) + sess.SetMetadata("expires", utils.TimeToOCMtime(want)) + got := sess.Expires() + assert.WithinDuration(t, want, got, time.Second) +} + +// IsProcessing + +func TestIsProcessing(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetSize(100) + + assert.False(t, sess.IsProcessing(), "size != offset should not be processing") + + sess.info.Offset = 100 + assert.True(t, sess.IsProcessing(), "size == offset with no scan result should be processing") + + sess.SetScanData("clean", time.Now()) + assert.False(t, sess.IsProcessing(), "scan result set means processing finished") +} + +// Reference + +func TestReference(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetMetadata("providerID", "prov-1") + sess.SetStorageValue("SpaceRoot", "space-2") + sess.SetStorageValue("NodeId", "node-3") + + ref := sess.Reference() + require.NotNil(t, ref.ResourceId) + assert.Equal(t, "prov-1", ref.ResourceId.StorageId) + assert.Equal(t, "space-2", ref.ResourceId.SpaceId) + assert.Equal(t, "node-3", ref.ResourceId.OpaqueId) +} + +// Metadata + +func TestMetadata(t *testing.T) { + sess, _ := newTestSession(t) + sess.SetMetadata("providerID", "p1") + sess.SetMetadata("mtime", "12345.0") + sess.SetStorageValue("NodeExists", "true") + sess.SetMetadata("versionsPath", "/v/path") + + m := sess.Metadata() + assert.Equal(t, "p1", m["providerID"]) + assert.Equal(t, "12345.0", m["mtime"]) + assert.Equal(t, "true", m["nodeExists"]) + assert.Equal(t, "/v/path", m["versionsPath"]) + assert.Equal(t, sess.ID(), m["sessionID"]) +} + +// Persist + Get round-trip + +func TestPersist_GetRoundtrip(t *testing.T) { + log := zerolog.Nop() + root := t.TempDir() + fs := NewFileStore(root, TokenOptions{}, &log) + require.NoError(t, fs.Setup()) + + sess := fs.New(context.Background()).(*FileSession) + sess.SetSize(512) + sess.SetMetadata("providerID", "prov-rt") + sess.SetStorageValue("NodeId", "nd-rt") + sess.SetStorageValue("NodeExists", "true") + + require.NoError(t, sess.TouchBin()) + require.NoError(t, sess.Persist(context.Background())) + + loaded, err := fs.Get(context.Background(), sess.ID()) + require.NoError(t, err) + ls := loaded.(*FileSession) + + assert.Equal(t, int64(512), ls.Size()) + assert.Equal(t, "prov-rt", ls.ProviderID()) + assert.Equal(t, "nd-rt", ls.NodeID()) + assert.True(t, ls.NodeExists()) +} + +func TestPersist_CreatesIntermediateDirs(t *testing.T) { + log := zerolog.Nop() + root := filepath.Join(t.TempDir(), "sub", "nested") + fs := NewFileStore(root, TokenOptions{}, &log) + + sess := fs.New(context.Background()).(*FileSession) + sess.SetSize(1) + + err := sess.Persist(context.Background()) + assert.NoError(t, err) + _, statErr := os.Stat(sess.infoPath()) + assert.NoError(t, statErr) +} + +// WriteChunk + +func TestWriteChunk(t *testing.T) { + sess, _ := newTestSession(t) + require.NoError(t, os.MkdirAll(filepath.Dir(sess.binPath()), 0700)) + require.NoError(t, sess.TouchBin()) + + n, err := sess.WriteChunk(context.Background(), 0, strings.NewReader("hello")) + require.NoError(t, err) + assert.Equal(t, int64(5), n) + assert.Equal(t, int64(5), sess.Offset()) + + n2, err := sess.WriteChunk(context.Background(), 5, strings.NewReader(" world")) + require.NoError(t, err) + assert.Equal(t, int64(6), n2) + assert.Equal(t, int64(11), sess.Offset()) + + data, err := os.ReadFile(sess.binPath()) + require.NoError(t, err) + assert.Equal(t, "hello world", string(data)) +} + +func TestWriteChunk_NoBinFile(t *testing.T) { + sess, _ := newTestSession(t) + require.NoError(t, os.MkdirAll(filepath.Dir(sess.binPath()), 0700)) + + _, err := sess.WriteChunk(context.Background(), 0, strings.NewReader("data")) + assert.Error(t, err) +} + +// Cleanup + +func setupCleanupSession(t *testing.T) *FileSession { + t.Helper() + sess, _ := newTestSession(t) + require.NoError(t, os.MkdirAll(filepath.Dir(sess.binPath()), 0700)) + require.NoError(t, sess.TouchBin()) + require.NoError(t, sess.Persist(context.Background())) + return sess +} + +func TestCleanup_BinOnly(t *testing.T) { + sess := setupCleanupSession(t) + sess.Cleanup(true, false) + _, err := os.Stat(sess.binPath()) + assert.True(t, os.IsNotExist(err), "bin should be removed") + _, err = os.Stat(sess.infoPath()) + assert.NoError(t, err, "info should survive") +} + +func TestCleanup_InfoOnly(t *testing.T) { + sess := setupCleanupSession(t) + sess.Cleanup(false, true) + _, err := os.Stat(sess.binPath()) + assert.NoError(t, err, "bin should survive") + _, err = os.Stat(sess.infoPath()) + assert.True(t, os.IsNotExist(err), "info should be removed") +} + +func TestCleanup_Both(t *testing.T) { + sess := setupCleanupSession(t) + sess.Cleanup(true, true) + _, err := os.Stat(sess.binPath()) + assert.True(t, os.IsNotExist(err), "bin should be removed") + _, err = os.Stat(sess.infoPath()) + assert.True(t, os.IsNotExist(err), "info should be removed") +} + +func TestCleanup_Neither(t *testing.T) { + sess := setupCleanupSession(t) + sess.Cleanup(false, false) + _, err := os.Stat(sess.binPath()) + assert.NoError(t, err, "bin should survive") + _, err = os.Stat(sess.infoPath()) + assert.NoError(t, err, "info should survive") +} + +func TestCleanup_MissingFiles_NoError(t *testing.T) { + sess, _ := newTestSession(t) + assert.NotPanics(t, func() { + sess.Cleanup(true, true) + }) +} + +// calculateChecksums + +func TestCalculateChecksums(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "testfile") + require.NoError(t, os.WriteFile(path, []byte("hello"), 0600)) + + sha1h, md5h, adler32h, err := calculateChecksums(context.Background(), path) + require.NoError(t, err) + + assert.Equal(t, "aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d", hex.EncodeToString(sha1h.Sum(nil))) + assert.Equal(t, "5d41402abc4b2a76b9719d911017c592", hex.EncodeToString(md5h.Sum(nil))) + assert.Equal(t, "062c0215", hex.EncodeToString(adler32h.Sum(nil))) +} + +func TestCalculateChecksums_MissingFile(t *testing.T) { + _, _, _, err := calculateChecksums(context.Background(), "/nonexistent/path/file.bin") + assert.Error(t, err) +} + +// checkHash + +func TestCheckHash_Correct(t *testing.T) { + h := sha1.New() //nolint:gosec + h.Write([]byte("hello")) + err := checkHash("aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d", h) + assert.NoError(t, err) +} + +func TestCheckHash_Mismatch(t *testing.T) { + h := sha1.New() //nolint:gosec + h.Write([]byte("hello")) + err := checkHash("0000000000000000000000000000000000000000", h) + require.Error(t, err) + assert.ErrorAs(t, err, new(errtypes.ChecksumMismatch)) +} + +// joinURLParts + +func TestJoinURLParts(t *testing.T) { + tests := []struct { + parts []string + want string + }{ + {[]string{"http://host/", "path"}, "http://host/path"}, + {[]string{"http://host", "path"}, "http://host/path"}, + {[]string{"http://host"}, "http://host"}, + {[]string{"http://host", "a", "b"}, "http://host/a/b"}, + {[]string{"http://host/", "a/", "b"}, "http://host/a/b"}, + } + for _, tc := range tests { + got := joinURLParts(tc.parts...) + assert.Equal(t, tc.want, got, "joinURLParts(%v)", tc.parts) + } +} + +// Context + +func TestContext(t *testing.T) { + log := zerolog.Nop() + fs := NewFileStore(t.TempDir(), TokenOptions{}, &log) + sess := fs.New(context.Background()).(*FileSession) + + u := &userpb.User{ + Id: &userpb.UserId{ + Idp: "idp.test", + OpaqueId: "ctx-user", + Type: userpb.UserType_USER_TYPE_PRIMARY, + }, + Username: "ctxuser", + } + sess.SetExecutant(u) + sess.SetMetadata("lockid", "lock-xyz") + sess.SetMetadata("initiatorid", "initiator-abc") + + ctx := sess.Context(context.Background()) + + gotUser, ok := ctxpkg.ContextGetUser(ctx) + require.True(t, ok) + assert.Equal(t, "ctx-user", gotUser.GetId().GetOpaqueId()) + assert.Equal(t, "idp.test", gotUser.GetId().GetIdp()) + + lockID, ok := ctxpkg.ContextGetLockID(ctx) + require.True(t, ok) + assert.Equal(t, "lock-xyz", lockID) + + initiator, ok := ctxpkg.ContextGetInitiator(ctx) + require.True(t, ok) + assert.Equal(t, "initiator-abc", initiator) +} + +// URL + +func TestURL(t *testing.T) { + log := zerolog.Nop() + opts := TokenOptions{ + DownloadEndpoint: "http://download.example.com", + DataGatewayEndpoint: "http://gateway.example.com", + TransferSharedSecret: "s3cr3t", + TransferExpires: 3600, + } + fs := NewFileStore(t.TempDir(), opts, &log) + sess := fs.New(context.Background()).(*FileSession) + + url, err := sess.URL(context.Background()) + require.NoError(t, err) + assert.NotEmpty(t, url) + assert.True(t, strings.HasPrefix(url, "http://gateway.example.com"), "URL should start with DataGatewayEndpoint") +} + +func TestURL_NonEmpty(t *testing.T) { + log := zerolog.Nop() + opts := TokenOptions{ + DataGatewayEndpoint: "http://gw.example.com", + TransferSharedSecret: "secret", + TransferExpires: 60, + } + fs := NewFileStore(t.TempDir(), opts, &log) + sess := fs.New(context.Background()).(*FileSession) + + url1, err := sess.URL(context.Background()) + require.NoError(t, err) + url2, err := sess.URL(context.Background()) + require.NoError(t, err) + + assert.NotEmpty(t, url1) + assert.NotEmpty(t, url2) +} From a07ac3635ebc0fa0f00f27d9af0f063f4f3611e3 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Thu, 30 Jul 2026 15:34:51 +0200 Subject: [PATCH 02/15] fix: fix rebase and unit tests --- pkg/upload/coordinator.go | 94 ++++++++++++++++++++++++++++++++---- pkg/upload/filestore_test.go | 6 ++- pkg/upload/session.go | 14 +++--- pkg/upload/session_test.go | 9 +++- 4 files changed, 104 insertions(+), 19 deletions(-) diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index 8e8f2d655b8..a2435f3548f 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -495,31 +495,95 @@ func (c *coordinator) touchAndMark(ctx context.Context, session Session) error { return session.Persist(ctx) } -// finishSync commits the staged bytes inline, then unmarks processing and cleans up. +// finishSync writes the node metadata, commits the staged bytes, then unmarks +// processing and cleans up. +// +// The driver seam is split in two: PrepareUpload takes the node lock and writes +// the metadata (checksums, size, mtime, preconditions, a new version), then +// CommitUpload writes the blob the metadata points at. Both must be given the +// same session id, because the driver uses it as the blob id to pair them up. func (c *coordinator) finishSync(ctx context.Context, session Session) (*provider.ResourceInfo, error) { ref := session.Reference() - f, err := os.Open(session.BinPath()) + + info, err := uploadInfo(session) if err != nil { c.rollback(ctx, session) return nil, err } - ri, err := c.fs.CommitUpload(ctx, &ref, storage.UploadSource{ - Body: f, - Length: session.Size(), - Metadata: session.Metadata(), - Checksums: session.Checksums(), - }) + // A failed PrepareUpload has already undone its own writes, so the plain + // rollback is what we want here: asking the driver to roll back again would + // revert the node past the point this upload started. + prepared, err := c.fs.PrepareUpload(ctx, &ref, session.ID(), info) if err != nil { c.rollback(ctx, session) return nil, err } - _ = c.fs.MarkProcessing(ctx, &ref, false, session.ID()) + + f, err := os.Open(session.BinPath()) + if err != nil { + c.rollbackPrepared(ctx, session, prepared.SizeDiff) + return nil, err + } + // CommitUpload does not own the body; we opened it, so we close it. + err = c.fs.CommitUpload(ctx, &ref, session.ID(), storage.UploadSource{ + Body: f, + Length: session.Size(), + }) + f.Close() + if err != nil { + c.rollbackPrepared(ctx, session, prepared.SizeDiff) + return nil, err + } + + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + } session.Cleanup(true, true) metrics.UploadSessionsFinalized.Inc() + + // The driver owns the resulting etag and mtime, so read them back rather than + // assembling a ResourceInfo from what we sent. GetMD is permission-gated and + // the executant may not be allowed to stat what they just uploaded (a + // write-only share), so a failure here must not fail the committed upload. + ri, err := c.fs.GetMD(ctx, &ref, nil, nil) + if err != nil { + appctx.GetLogger(ctx).Debug().Err(err).Str("uploadid", session.ID()).Msg("could not stat committed upload") + ri = &provider.ResourceInfo{Id: ref.GetResourceId(), Size: uint64(session.Size())} + } + c.publishUploadReady(ctx, session, ri) return ri, nil } +// uploadInfo collects what the driver needs to write node metadata. The +// precondition headers are recorded at initiate time and re-checked here, +// because the resource may have changed while the bytes were being uploaded. +func uploadInfo(session Session) (storage.UploadInfo, error) { + md := session.Metadata() + info := storage.UploadInfo{ + NodeExisted: session.NodeExists(), + Size: session.Size(), + Checksums: session.Checksums(), + IfMatch: md["if-match"], + IfNoneMatch: md["if-none-match"], + } + if v := md["mtime"]; v != "" { + mtime, err := utils.MTimeToTime(v) + if err != nil { + return info, errtypes.BadRequest("coordinator: invalid mtime: " + v) + } + info.MTime = mtime + } + if v := md["if-unmodified-since"]; v != "" { + ius, err := time.Parse(time.RFC3339Nano, v) + if err != nil { + return info, errtypes.BadRequest("coordinator: invalid if-unmodified-since: " + v) + } + info.IfUnmodifiedSince = ius + } + return info, nil +} + // publishUploadReady announces that the file is available. Consumers such as the // search indexer only listen for UploadReady when async uploads are configured, // and would otherwise never learn about a coordinator upload. The coordinator @@ -563,6 +627,18 @@ func (c *coordinator) rollback(ctx context.Context, session Session) { } } +// rollbackPrepared undoes a failed finish. It additionally asks the driver to +// revert what PrepareUpload wrote: the previous revision and the optimistic size +// propagation. sizeDiff is 0 when PrepareUpload never ran or itself failed, in +// which case the driver has nothing to undo. +func (c *coordinator) rollbackPrepared(ctx context.Context, session Session, sizeDiff int64) { + ref := session.Reference() + if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), session.NodeExists(), sizeDiff); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") + } + c.rollback(ctx, session) +} + // verifyAndStoreChecksums computes checksums over the staged binary, validates any // client-supplied checksum, and stores the results on the session for CommitUpload. func verifyAndStoreChecksums(ctx context.Context, session Session) error { diff --git a/pkg/upload/filestore_test.go b/pkg/upload/filestore_test.go index 8ad5c5b9141..4cf5ab1692a 100644 --- a/pkg/upload/filestore_test.go +++ b/pkg/upload/filestore_test.go @@ -89,7 +89,9 @@ func TestFileStoreNew_StorageTypeIsFileStore(t *testing.T) { info, err := s.GetInfo(context.Background()) require.NoError(t, err) - assert.Equal(t, "FileStore", info.Storage["Type"]) + // Deliberately OcisStore's value: sessions written by either store must stay + // readable across a rolling deploy. + assert.Equal(t, "OCISStore", info.Storage["Type"]) } func TestFileStoreNew_MetaDataIsNotNil(t *testing.T) { @@ -122,7 +124,7 @@ func TestFileStoreGet_HappyPath(t *testing.T) { assert.Equal(t, s.ID(), got.ID()) info, err := got.GetInfo(ctx) require.NoError(t, err) - assert.Equal(t, "FileStore", info.Storage["Type"]) + assert.Equal(t, "OCISStore", info.Storage["Type"]) } func TestFileStoreGet_OffsetFromBinSize(t *testing.T) { diff --git a/pkg/upload/session.go b/pkg/upload/session.go index 99fc6f2fd15..18f3f57903b 100644 --- a/pkg/upload/session.go +++ b/pkg/upload/session.go @@ -242,14 +242,16 @@ func (s *FileSession) Checksums() storage.UploadChecksums { } } -// Metadata returns the upload metadata map passed to CommitUpload. +// Metadata returns the upload metadata the coordinator passes to the driver. func (s *FileSession) Metadata() map[string]string { return map[string]string{ - "providerID": s.info.MetaData["providerID"], - "mtime": s.info.MetaData["mtime"], - "nodeExists": s.info.Storage["NodeExists"], - "versionsPath": s.info.MetaData["versionsPath"], - "sessionID": s.info.ID, + "providerID": s.info.MetaData["providerID"], + "mtime": s.info.MetaData["mtime"], + "nodeExists": s.info.Storage["NodeExists"], + "sessionID": s.info.ID, + "if-match": s.info.MetaData["if-match"], + "if-none-match": s.info.MetaData["if-none-match"], + "if-unmodified-since": s.info.MetaData["if-unmodified-since"], } } diff --git a/pkg/upload/session_test.go b/pkg/upload/session_test.go index 8bcf1167a98..4bd7179b474 100644 --- a/pkg/upload/session_test.go +++ b/pkg/upload/session_test.go @@ -191,14 +191,19 @@ func TestMetadata(t *testing.T) { sess.SetMetadata("providerID", "p1") sess.SetMetadata("mtime", "12345.0") sess.SetStorageValue("NodeExists", "true") - sess.SetMetadata("versionsPath", "/v/path") + sess.SetMetadata("if-match", "etag-1") + sess.SetMetadata("if-none-match", "*") + sess.SetMetadata("if-unmodified-since", "2026-07-30T10:00:00Z") m := sess.Metadata() assert.Equal(t, "p1", m["providerID"]) assert.Equal(t, "12345.0", m["mtime"]) assert.Equal(t, "true", m["nodeExists"]) - assert.Equal(t, "/v/path", m["versionsPath"]) assert.Equal(t, sess.ID(), m["sessionID"]) + // The driver re-checks these at PrepareUpload, so they must survive the session. + assert.Equal(t, "etag-1", m["if-match"]) + assert.Equal(t, "*", m["if-none-match"]) + assert.Equal(t, "2026-07-30T10:00:00Z", m["if-unmodified-since"]) } // Persist + Get round-trip From 217f8f85b81c1aecb437dbb4b6010264664b7ea0 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Thu, 30 Jul 2026 16:14:07 +0200 Subject: [PATCH 03/15] add coodinator event consumer --- .../storageprovider/storageprovider.go | 3 +- .../services/dataprovider/dataprovider.go | 3 +- pkg/storage/storage.go | 5 +- pkg/upload/coordinator.go | 142 +++++-- pkg/upload/filestore.go | 24 +- pkg/upload/filestore_test.go | 27 ++ pkg/upload/postprocessing.go | 186 +++++++++ pkg/upload/session.go | 17 + pkg/upload/upload_async_test.go | 387 ++++++++++++++++++ 9 files changed, 766 insertions(+), 28 deletions(-) create mode 100644 pkg/upload/postprocessing.go create mode 100644 pkg/upload/upload_async_test.go diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 01ffd5e7d3f..539b2c0d01f 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -200,7 +200,8 @@ func New(m map[string]interface{}, ss *grpc.Server, log *zerolog.Logger) (rgrpc. if err := store.Setup(); err != nil { return nil, fmt.Errorf("storageprovider: upload directory setup failed: %w", err) } - coordinator := upload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) + coordinator := upload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream, + upload.AsyncUploadsFromDriverConf(c.Drivers[c.Driver])) // parse data server url u, err := url.Parse(c.DataServerURL) diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index b17f04e0e68..e6167a056a1 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -116,7 +116,8 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) if err := store.Setup(); err != nil { return nil, fmt.Errorf("dataprovider: upload directory setup failed: %w", err) } - coord := pkgupload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) + coord := pkgupload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream, + pkgupload.AsyncUploadsFromDriverConf(conf.Drivers[conf.Driver])) dataTXs, err := getDataTXs(conf, coord, fs, evstream, log) if err != nil { diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index e28ce4fbc07..1ff6d7aae82 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -149,8 +149,9 @@ type FS interface { // It is the inverse of PrepareUpload: restores previous metadata and reverts the optimistic // size propagation. The caller (coordinator) is responsible for unmarking the processing flag // and deleting the upload session files. Drivers that performed no work in PrepareUpload may return nil. - // nodeExisted indicates whether the target node had a prior version; drivers that have nothing - // to undo for new nodes may no-op when nodeExisted is false. + // nodeExisted indicates whether the target node had a prior version. When it is false the + // upload itself created the node, and the driver must remove it outright rather than move it + // to the trash: the file never became visible, so it must not be recoverable. RollbackUpload(ctx context.Context, ref *provider.Reference, sessionID string, nodeExisted bool, sizeDiff int64) error // Revisions diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index a2435f3548f..2013ad3a46a 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -64,6 +64,7 @@ type coordinator struct { store SessionStore chunkHandler *chunking.ChunkHandler pub events.Publisher + async bool } // NewCoordinator constructs a coordinator backed by the given storage driver @@ -75,8 +76,12 @@ type coordinator struct { // // pub receives the UploadReady event that tells the rest of the system a file is // available; pass nil to disable publishing. -func NewCoordinator(fs storage.FS, store SessionStore, chunkFolder string, pub events.Publisher) *coordinator { - c := &coordinator{fs: fs, store: store, pub: pub} +// +// async makes finished uploads wait for postprocessing (virus scanning) before +// their bytes are committed. It requires pub: without a publisher there is +// nothing to start postprocessing, so the upload would never complete. +func NewCoordinator(fs storage.FS, store SessionStore, chunkFolder string, pub events.Publisher, async bool) *coordinator { + c := &coordinator{fs: fs, store: store, pub: pub, async: async && pub != nil} if chunkFolder != "" { c.chunkHandler = chunking.NewChunkHandler(chunkFolder) } @@ -432,11 +437,18 @@ func (c *coordinator) Upload(ctx context.Context, req storage.UploadRequest, uff } // finishUpload lands a fully-received upload: create the node (new files), verify -// checksums, then commit the staged bytes. Zero-length uploads always finish here -// synchronously; the async postprocessing path is not ported yet. +// checksums, write the node metadata, then commit the staged bytes. +// +// With async uploads enabled the commit is deferred: the bytes stay staged and a +// BytesReceived event hands the upload to postprocessing (virus scanning), which +// reports back with PostprocessingFinished and only then does the blob get +// written. Zero-length uploads always finish inline — there is nothing to scan, +// and no BytesReceived consumer would ever complete them. // // Returns the committed resource as the driver reported it, so PUT callers can -// answer with the new etag/mtime/id without recomputing them. +// answer with the new etag/mtime/id without recomputing them. On the async path +// nothing is committed yet, so the resource is empty rather than nil: callers +// read fields off it directly (e.g. simple.go sets an ETag header from it). func (c *coordinator) finishUpload(ctx context.Context, session Session) (*provider.ResourceInfo, error) { if err := c.touchAndMark(ctx, session); err != nil { return nil, err @@ -453,9 +465,63 @@ func (c *coordinator) finishUpload(ctx context.Context, session Session) (*provi metrics.UploadProcessing.Inc() metrics.UploadSessionsBytesReceived.Inc() + if err := c.prepare(ctx, session); err != nil { + return nil, err + } + + if c.async && session.Size() > 0 { + if err := c.publishBytesReceived(ctx, session); err != nil { + c.rollbackPrepared(ctx, session, session.SizeDiff()) + return nil, err + } + // The node is left flagged as processing and the staged bytes are kept: + // the postprocessing consumer needs both to finish the upload later. + return &provider.ResourceInfo{Id: &provider.ResourceId{ + StorageId: session.ProviderID(), + SpaceId: session.SpaceID(), + OpaqueId: session.NodeID(), + }}, nil + } + return c.finishSync(ctx, session) } +// publishBytesReceived hands the staged upload to postprocessing. The event +// carries a signed URL the postprocessing service downloads the bytes from, so +// it can scan them before they are committed. +func (c *coordinator) publishBytesReceived(ctx context.Context, session Session) error { + url, err := session.URL(ctx) + if err != nil { + return err + } + + executant := session.Executant() + var impersonating *user.User + if u, ok := ctxpkg.ContextGetUser(ctx); ok && utils.ExistsInOpaque(u.GetOpaque(), "impersonating-user") { + impersonating = &user.User{} + if err := utils.ReadJSONFromOpaque(u.GetOpaque(), "impersonating-user", impersonating); err != nil { + return err + } + } + + return events.Publish(ctx, c.pub, events.BytesReceived{ + UploadID: session.ID(), + URL: url, + SpaceOwner: session.SpaceOwner(), + ExecutingUser: &user.User{ + Id: &executant, + }, + ResourceID: &provider.ResourceId{ + StorageId: session.ProviderID(), + SpaceId: session.SpaceID(), + OpaqueId: session.NodeID(), + }, + Filename: session.Filename(), + Filesize: uint64(session.Size()), + ImpersonatingUser: impersonating, + }) +} + // touchAndMark creates the node for new files (via the public TouchFile, since // CommitUpload requires an existing node) and marks it as processing. TouchFile // mints the real node id, so we overwrite the id minted at initiate. @@ -502,13 +568,13 @@ func (c *coordinator) touchAndMark(ctx context.Context, session Session) error { // the metadata (checksums, size, mtime, preconditions, a new version), then // CommitUpload writes the blob the metadata points at. Both must be given the // same session id, because the driver uses it as the blob id to pair them up. -func (c *coordinator) finishSync(ctx context.Context, session Session) (*provider.ResourceInfo, error) { +func (c *coordinator) prepare(ctx context.Context, session Session) error { ref := session.Reference() info, err := uploadInfo(session) if err != nil { c.rollback(ctx, session) - return nil, err + return err } // A failed PrepareUpload has already undone its own writes, so the plain // rollback is what we want here: asking the driver to roll back again would @@ -516,12 +582,25 @@ func (c *coordinator) finishSync(ctx context.Context, session Session) (*provide prepared, err := c.fs.PrepareUpload(ctx, &ref, session.ID(), info) if err != nil { c.rollback(ctx, session) - return nil, err + return err + } + + // Persisted, not just held: on the async path the commit happens in another + // process, which can only learn the size to revert by reading it back. + session.SetSizeDiff(prepared.SizeDiff) + if err := session.Persist(ctx); err != nil { + c.rollbackPrepared(ctx, session, prepared.SizeDiff) + return err } + return nil +} + +func (c *coordinator) finishSync(ctx context.Context, session Session) (*provider.ResourceInfo, error) { + ref := session.Reference() f, err := os.Open(session.BinPath()) if err != nil { - c.rollbackPrepared(ctx, session, prepared.SizeDiff) + c.rollbackPrepared(ctx, session, session.SizeDiff()) return nil, err } // CommitUpload does not own the body; we opened it, so we close it. @@ -531,7 +610,7 @@ func (c *coordinator) finishSync(ctx context.Context, session Session) (*provide }) f.Close() if err != nil { - c.rollbackPrepared(ctx, session, prepared.SizeDiff) + c.rollbackPrepared(ctx, session, session.SizeDiff()) return nil, err } @@ -601,14 +680,7 @@ func (c *coordinator) publishUploadReady(ctx context.Context, session Session, r ExecutingUser: &user.User{ Id: &executant, }, - FileRef: &provider.Reference{ - ResourceId: &provider.ResourceId{ - StorageId: session.ProviderID(), - SpaceId: session.SpaceID(), - OpaqueId: session.SpaceID(), - }, - Path: utils.MakeRelativePath(filepath.Join(session.Dir(), session.Filename())), - }, + FileRef: c.uploadRef(session), ResourceID: ri.GetId(), Timestamp: utils.TSNow(), }); err != nil { @@ -616,6 +688,19 @@ func (c *coordinator) publishUploadReady(ctx context.Context, session Session, r } } +// uploadRef builds the space-relative reference upload events carry. The id +// addresses the space root, with the file identified by the path within it. +func (c *coordinator) uploadRef(session Session) *provider.Reference { + return &provider.Reference{ + ResourceId: &provider.ResourceId{ + StorageId: session.ProviderID(), + SpaceId: session.SpaceID(), + OpaqueId: session.SpaceID(), + }, + Path: utils.MakeRelativePath(filepath.Join(session.Dir(), session.Filename())), + } +} + // rollback unmarks processing, cleans up session files, and deletes the node if // this upload created it (NodeExists=false at initiation). func (c *coordinator) rollback(ctx context.Context, session Session) { @@ -627,16 +712,27 @@ func (c *coordinator) rollback(ctx context.Context, session Session) { } } -// rollbackPrepared undoes a failed finish. It additionally asks the driver to -// revert what PrepareUpload wrote: the previous revision and the optimistic size -// propagation. sizeDiff is 0 when PrepareUpload never ran or itself failed, in -// which case the driver has nothing to undo. +// rollbackPrepared undoes a finish that failed after PrepareUpload succeeded: it +// asks the driver to revert what PrepareUpload wrote, then unmarks processing and +// drops the session files. +// +// Node removal is the driver's job, not ours. RollbackUpload restores the +// previous revision, or removes the node entirely when this upload created it — +// permission-free, which the public Delete is not, and without leaving a +// never-visible file recoverable in the trash. +// +// Only call this once PrepareUpload has returned successfully. A failed +// PrepareUpload already undoes its own writes, so rolling back again would +// revert the node past the state this upload started from. func (c *coordinator) rollbackPrepared(ctx context.Context, session Session, sizeDiff int64) { ref := session.Reference() if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), session.NodeExists(), sizeDiff); err != nil { appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") } - c.rollback(ctx, session) + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + } + session.Cleanup(true, true) } // verifyAndStoreChecksums computes checksums over the staged binary, validates any diff --git a/pkg/upload/filestore.go b/pkg/upload/filestore.go index 566b7082c76..07ab98cfb95 100644 --- a/pkg/upload/filestore.go +++ b/pkg/upload/filestore.go @@ -86,11 +86,33 @@ func FileStoreFromDriverConf(driverConf map[string]interface{}, log *zerolog.Log // Returns nil only when neither source resolves to a non-empty path. func NewFileStoreFromConfig(uploadDir string, driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { if uploadDir != "" { - return NewFileStore(uploadDir, TokenOptions{}, log) + // Still take the tokens from the driver config: they sign the transfer URL + // that postprocessing downloads the staged bytes from, and a service-level + // upload directory says nothing about them. + return newFileStoreWithTokens(uploadDir, driverConf, log) } return FileStoreFromDriverConf(driverConf, log) } +// AsyncUploadsFromDriverConf reports whether the driver is configured to run +// uploads through postprocessing. +// +// The key is decomposedfs's (options.go: `asyncfileuploads`), read straight off +// the driver config map the services already hand us. Reading the driver's own +// key rather than introducing a service-level one keeps a single source of truth: +// if the coordinator and the driver disagreed, uploads would either commit twice +// or never get scanned. +func AsyncUploadsFromDriverConf(driverConf map[string]interface{}) bool { + if driverConf == nil { + return false + } + var ac struct { + AsyncFileUploads bool `mapstructure:"asyncfileuploads"` + } + _ = mapstructure.Decode(driverConf, &ac) + return ac.AsyncFileUploads +} + func newFileStoreWithTokens(root string, driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { type tokenConf struct { DownloadEndpoint string `mapstructure:"download_endpoint"` diff --git a/pkg/upload/filestore_test.go b/pkg/upload/filestore_test.go index 4cf5ab1692a..55ddf925a49 100644 --- a/pkg/upload/filestore_test.go +++ b/pkg/upload/filestore_test.go @@ -312,3 +312,30 @@ func TestNewFileStoreFromConfig_BothEmptyReturnsNil(t *testing.T) { assert.Nil(t, fs) } + +// A service-level upload directory must not drop the driver's tokens: they sign +// the transfer URL postprocessing downloads the staged bytes from. +func TestNewFileStoreFromConfig_UploadDirKeepsDriverTokens(t *testing.T) { + uploadDir := t.TempDir() + fs := NewFileStoreFromConfig(uploadDir, map[string]interface{}{ + "root": "/ignored", + "tokens": map[string]interface{}{ + "transfer_shared_secret": "s3cret", + "download_endpoint": "https://dl.example.com/data/", + }, + }, nopLog()) + + require.NotNil(t, fs) + assert.Equal(t, uploadDir, fs.root) + assert.Equal(t, "s3cret", fs.opts.TransferSharedSecret) + assert.Equal(t, "https://dl.example.com/data/", fs.opts.DownloadEndpoint) +} + +// AsyncUploadsFromDriverConf + +func TestAsyncUploadsFromDriverConf(t *testing.T) { + assert.False(t, AsyncUploadsFromDriverConf(nil)) + assert.False(t, AsyncUploadsFromDriverConf(map[string]interface{}{"root": "/x"})) + assert.False(t, AsyncUploadsFromDriverConf(map[string]interface{}{"asyncfileuploads": false})) + assert.True(t, AsyncUploadsFromDriverConf(map[string]interface{}{"asyncfileuploads": true})) +} diff --git a/pkg/upload/postprocessing.go b/pkg/upload/postprocessing.go new file mode 100644 index 00000000000..b4f4702d289 --- /dev/null +++ b/pkg/upload/postprocessing.go @@ -0,0 +1,186 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +// This file holds the postprocessing result collector: the second half of an +// async upload. finishUpload stages the bytes and publishes BytesReceived; the +// postprocessing service scans them and reports back here, and only then are the +// bytes committed through the driver seam. +// +// It is driver-agnostic on purpose: this logic used to live inside decomposedfs, +// which meant only that driver could offer async uploads. + +package upload + +import ( + "context" + + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/rs/zerolog" + + "github.com/owncloud/reva/v2/pkg/appctx" + "github.com/owncloud/reva/v2/pkg/events" + "github.com/owncloud/reva/v2/pkg/rhttp/datatx/metrics" + "github.com/owncloud/reva/v2/pkg/utils" +) + +// RegisteredEvents are the postprocessing events the coordinator consumes. +var RegisteredEvents = []events.Unmarshaller{ + events.PostprocessingFinished{}, + events.PostprocessingStepFinished{}, + events.RestartPostprocessing{}, + events.CleanUpload{}, +} + +// Postprocessing consumes postprocessing results until ch is closed. Run it in +// its own goroutine, one per configured consumer. +func (c *coordinator) Postprocessing(ch <-chan events.Event) { + for event := range ch { + c.processEvent(context.Background(), event) + } +} + +func (c *coordinator) processEvent(ctx context.Context, event events.Event) { + log := appctx.GetLogger(ctx) + + switch ev := event.Event.(type) { + case events.PostprocessingFinished: + c.onPostprocessingFinished(ctx, ev, log) + case events.RestartPostprocessing: + c.onRestartPostprocessing(ctx, ev, log) + case events.CleanUpload: + session, err := c.store.Get(ctx, ev.UploadID) + if err != nil { + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("CleanUpload: could not load session") + return + } + if !ev.KeepUpload { + c.rollbackPrepared(ctx, session, session.SizeDiff()) + } + case events.PostprocessingStepFinished: + if ev.FinishedStep != events.PPStepAntivirus { + // only the antivirus result is recorded on the session + return + } + res, ok := ev.Result.(events.VirusscanResult) + if !ok || res.ErrorMsg != "" { + // the scan itself failed; PostprocessingFinished decides the outcome + return + } + session, err := c.store.Get(ctx, ev.UploadID) + if err != nil { + // an empty upload id means an on-demand scan, which has no session + if ev.UploadID != "" { + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("PostprocessingStepFinished: could not load session") + } + return + } + session.SetScanData(res.Description, res.Scandate) + if err := session.Persist(ctx); err != nil { + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("could not persist scan result") + } + } +} + +// onPostprocessingFinished completes or discards an upload according to the +// outcome postprocessing reported. +func (c *coordinator) onPostprocessingFinished(ctx context.Context, ev events.PostprocessingFinished, log *zerolog.Logger) { + session, err := c.store.Get(ctx, ev.UploadID) + if err != nil { + // Without the session we cannot reach the staged bytes, so they are leaked + // here. Housekeeping cleans them up later. + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("PostprocessingFinished: could not load session") + return + } + ctx = session.Context(ctx) + log = appctx.GetLogger(ctx) + + switch ev.Outcome { + case events.PPOutcomeContinue: + if _, err := c.finishSync(ctx, session); err != nil { + // finishSync has already rolled back and cleaned up. + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("could not commit upload after postprocessing") + c.publishUploadFailed(ctx, session, ev) + } + return + case events.PPOutcomeAbort: + metrics.UploadSessionsAborted.Inc() + // Keep the staged bytes: an abort is a transient failure and the upload can + // be restarted with RestartPostprocessing. + c.rollbackNode(ctx, session) + case events.PPOutcomeDelete: + metrics.UploadSessionsDeleted.Inc() + c.rollbackPrepared(ctx, session, session.SizeDiff()) + default: + log.Error().Str("outcome", string(ev.Outcome)).Str("uploadid", ev.UploadID).Msg("unknown postprocessing outcome, aborting") + metrics.UploadSessionsAborted.Inc() + c.rollbackNode(ctx, session) + } + + c.publishUploadFailed(ctx, session, ev) +} + +// onRestartPostprocessing re-publishes BytesReceived so a previously aborted +// upload gets another postprocessing run. +func (c *coordinator) onRestartPostprocessing(ctx context.Context, ev events.RestartPostprocessing, log *zerolog.Logger) { + session, err := c.store.Get(ctx, ev.UploadID) + if err != nil { + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("RestartPostprocessing: could not load session") + return + } + metrics.UploadSessionsRestarted.Inc() + if err := c.publishBytesReceived(session.Context(ctx), session); err != nil { + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("could not restart postprocessing") + } +} + +// rollbackNode reverts the node to its pre-upload state but keeps the staged +// bytes and the session, so postprocessing can be restarted. +func (c *coordinator) rollbackNode(ctx context.Context, session Session) { + ref := session.Reference() + if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), session.NodeExists(), session.SizeDiff()); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") + } + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + } +} + +// publishUploadFailed tells consumers the upload will not become available. +// Clients wait on UploadReady, so staying silent would leave them hanging. +func (c *coordinator) publishUploadFailed(ctx context.Context, session Session, ev events.PostprocessingFinished) { + if c.pub == nil { + return + } + if err := events.Publish(ctx, c.pub, events.UploadReady{ + UploadID: session.ID(), + Failed: true, + Filename: session.Filename(), + SpaceOwner: session.SpaceOwner(), + ExecutingUser: ev.ExecutingUser, + FileRef: c.uploadRef(session), + ResourceID: &provider.ResourceId{ + StorageId: session.ProviderID(), + SpaceId: session.SpaceID(), + OpaqueId: session.NodeID(), + }, + Timestamp: utils.TSNow(), + ImpersonatingUser: ev.ImpersonatingUser, + }); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("failed to publish UploadReady event") + } +} diff --git a/pkg/upload/session.go b/pkg/upload/session.go index 18f3f57903b..9249a61a824 100644 --- a/pkg/upload/session.go +++ b/pkg/upload/session.go @@ -12,6 +12,7 @@ import ( "io" "os" "path/filepath" + "strconv" "strings" "time" @@ -68,6 +69,8 @@ type Session interface { SetScanData(result string, date time.Time) Checksums() storage.UploadChecksums SetChecksums(sha1, md5, adler32 []byte) + SizeDiff() int64 + SetSizeDiff(d int64) Metadata() map[string]string Persist(ctx context.Context) error Cleanup(cleanBin, cleanInfo bool) @@ -255,6 +258,20 @@ func (s *FileSession) Metadata() map[string]string { } } +// SizeDiff returns the tree size change PrepareUpload propagated optimistically. +// Rolling an upload back has to undo exactly that amount. +func (s *FileSession) SizeDiff() int64 { + d, _ := strconv.ParseInt(s.info.MetaData["sizeDiff"], 10, 64) + return d +} + +// SetSizeDiff records the size change PrepareUpload reported. It is persisted +// because the async path prepares and commits in different processes, so the +// value cannot be held in memory between the two. +func (s *FileSession) SetSizeDiff(d int64) { + s.info.MetaData["sizeDiff"] = strconv.FormatInt(d, 10) +} + // SetScanData stores AV scan results on the session. func (s *FileSession) SetScanData(result string, date time.Time) { s.info.MetaData["scanResult"] = result diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go new file mode 100644 index 00000000000..6935539c0c1 --- /dev/null +++ b/pkg/upload/upload_async_test.go @@ -0,0 +1,387 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +// Async upload tests at the coordinator level. These are the driver-agnostic +// successor to decomposedfs/upload_async_test.go: the same scenarios, driven +// through pkg/upload instead of the driver's own upload machinery, with +// decomposedfs behind the CommitUpload/PrepareUpload/RollbackUpload seam. + +package upload_test + +import ( + "bytes" + "context" + "io" + "os" + "path/filepath" + + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1" + v1beta11 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/rs/zerolog" + "github.com/stretchr/testify/mock" + "google.golang.org/grpc" + + "github.com/owncloud/reva/v2/pkg/appctx" + ruser "github.com/owncloud/reva/v2/pkg/ctx" + "github.com/owncloud/reva/v2/pkg/events" + "github.com/owncloud/reva/v2/pkg/events/stream" + "github.com/owncloud/reva/v2/pkg/rgrpc/todo/pool" + "github.com/owncloud/reva/v2/pkg/storage" + "github.com/owncloud/reva/v2/pkg/storage/cache" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/aspects" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/lookup" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/options" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/permissions" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/permissions/mocks" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/timemanager" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/tree" + treemocks "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/tree/mocks" + "github.com/owncloud/reva/v2/pkg/storagespace" + "github.com/owncloud/reva/v2/pkg/store" + pkgupload "github.com/owncloud/reva/v2/pkg/upload" + "github.com/owncloud/reva/v2/pkg/utils" + "github.com/owncloud/reva/v2/tests/helpers" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Async uploads via the coordinator", func() { + var ( + ref = &provider.Reference{ + ResourceId: &provider.ResourceId{SpaceId: "u-s-e-r-id"}, + Path: "/foo", + } + rootRef = &provider.Reference{ + ResourceId: &provider.ResourceId{SpaceId: "u-s-e-r-id", OpaqueId: "u-s-e-r-id"}, + Path: "/", + } + + user = &userpb.User{ + Id: &userpb.UserId{ + Idp: "idp", + OpaqueId: "u-s-e-r-id", + Type: userpb.UserType_USER_TYPE_PRIMARY, + }, + Username: "username", + } + + firstContent = []byte("0123456789") + secondContent = []byte("01234567890123456789") + + ctx context.Context + + pub, con chan interface{} + uploadID string + + fs storage.FS + coord pkgupload.Coordinator + o *options.Options + bs *treemocks.Blobstore + + // upload runs a full upload through the coordinator and returns the + // session id, leaving it staged and awaiting postprocessing. + upload = func(content []byte) string { + ids, err := coord.InitiateUpload(ctx, ref, int64(len(content)), map[string]string{}) + Expect(err).ToNot(HaveOccurred()) + Expect(ids["simple"]).ToNot(BeEmpty()) + + _, err = coord.Upload(ctx, storage.UploadRequest{ + Ref: &provider.Reference{Path: "/" + ids["simple"]}, + Body: io.NopCloser(bytes.NewReader(content)), + Length: int64(len(content)), + }, nil) + Expect(err).ToNot(HaveOccurred()) + + ev, ok := (<-pub).(events.BytesReceived) + Expect(ok).To(BeTrue(), "expected BytesReceived: the upload must not commit before postprocessing") + Expect(ev.UploadID).To(Equal(ids["simple"])) + Expect(ev.URL).ToNot(BeEmpty(), "postprocessing needs a URL to fetch the staged bytes from") + + return ids["simple"] + } + + succeedPostprocessing = func(id string) { + con <- events.PostprocessingFinished{UploadID: id, Outcome: events.PPOutcomeContinue} + ev, ok := (<-pub).(events.UploadReady) + Expect(ok).To(BeTrue()) + Expect(ev.Failed).To(BeFalse()) + Expect(ev.ResourceID).ToNot(BeNil()) + Expect(ev.ResourceID.OpaqueId).ToNot(BeEmpty()) + Expect(ev.ResourceID.OpaqueId).ToNot(Equal(ev.ResourceID.SpaceId), + "ResourceID.OpaqueId should be the file node ID, not the space ID") + } + + failPostprocessing = func(id string, outcome events.PostprocessingOutcome) { + con <- events.PostprocessingFinished{UploadID: id, Outcome: outcome} + ev, ok := (<-pub).(events.UploadReady) + Expect(ok).To(BeTrue()) + Expect(ev.Failed).To(BeTrue()) + } + + // fileStatus reports whether the file exists, its processing status and size. + fileStatus = func() (bool, string, int) { + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(resources)).To(BeElementOf([2]int{0, 1}), "should not have more than one child") + if len(resources) == 0 { + return false, "", 0 + } + item := resources[0] + Expect(item.Path).To(Equal(ref.Path)) + return true, utils.ReadPlainFromOpaque(item.Opaque, "status"), int(item.GetSize()) + } + parentSize = func() int { + info, err := fs.GetMD(ctx, rootRef, []string{}, []string{}) + Expect(err).ToNot(HaveOccurred()) + return int(info.Size) + } + revisionCount = func() int { + revisions, err := fs.ListRevisions(ctx, ref) + Expect(err).ToNot(HaveOccurred()) + return len(revisions) + } + stagedBytesExist = func(id string) bool { + _, err := os.Stat(filepath.Join(o.Root, "uploads", id)) + return err == nil + } + ) + + BeforeEach(func() { + zl := zerolog.New(os.Stdout).Level(zerolog.ErrorLevel) + ctx = appctx.WithLogger(ruser.ContextSetUser(context.Background(), user), &zl) + + tmpRoot, err := helpers.TempDir("reva-unit-tests-*-root") + Expect(err).ToNot(HaveOccurred()) + + // asyncfileuploads stays false: the driver must not start its own + // postprocessing consumer, because the coordinator now owns that role. + o, err = options.New(map[string]interface{}{ + "root": tmpRoot, + "treetime_accounting": true, + "treesize_accounting": true, + }) + Expect(err).ToNot(HaveOccurred()) + + lu := lookup.New(metadata.NewXattrsBackend(o.Root, cache.Config{}), o, &timemanager.Manager{}) + pmock := &mocks.PermissionsChecker{} + + cs3permissionsclient := &mocks.CS3PermissionsClient{} + pool.RemoveSelector("PermissionsSelector" + "any") + permissionsSelector := pool.GetSelector[cs3permissions.PermissionsAPIClient]( + "PermissionsSelector", "any", + func(cc grpc.ClientConnInterface) cs3permissions.PermissionsAPIClient { + return cs3permissionsclient + }, + ) + bs = &treemocks.Blobstore{} + + cs3permissionsclient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return( + &cs3permissions.CheckPermissionResponse{Status: &v1beta11.Status{Code: v1beta11.Code_CODE_OK}}, nil).Times(1) + + pmock.On("AssemblePermissions", mock.Anything, mock.Anything). + Return(&provider.ResourcePermissions{ + Stat: true, + GetQuota: true, + InitiateFileUpload: true, + ListContainer: true, + ListFileVersions: true, + }, nil) + + pub, con = make(chan interface{}), make(chan interface{}) + evstream := stream.Chan{pub, con} + t := tree.New(lu, bs, o, store.Create(), &zerolog.Logger{}) + + fs, err = decomposedfs.New(o, aspects.Aspects{ + Lookup: lu, + Tree: t, + Permissions: permissions.NewPermissions(pmock, permissionsSelector), + EventStream: evstream, + Trashbin: &decomposedfs.DecomposedfsTrashbin{}, + }, &zerolog.Logger{}) + Expect(err).ToNot(HaveOccurred()) + + // The coordinator stages sessions under the same root the driver reads. + sessionStore := pkgupload.NewFileStore(o.Root, pkgupload.TokenOptions{ + DownloadEndpoint: "http://localhost:9200/data/", + DataGatewayEndpoint: "http://localhost:9200/data/", + TransferSharedSecret: "changemeplease", + TransferExpires: 86400, + }, &zl) + Expect(sessionStore.Setup()).To(Succeed()) + c := pkgupload.NewCoordinator(fs, sessionStore, filepath.Join(o.Root, "uploads"), evstream, true) + coord = c + + ch, err := events.Consume(evstream, "coordinator-test", pkgupload.RegisteredEvents...) + Expect(err).ToNot(HaveOccurred()) + go c.Postprocessing(ch) + + resp, err := fs.CreateStorageSpace(ctx, &provider.CreateStorageSpaceRequest{Owner: user, Type: "personal"}) + Expect(err).ToNot(HaveOccurred()) + Expect(resp.Status.Code).To(Equal(v1beta11.Code_CODE_OK)) + resID, err := storagespace.ParseID(resp.StorageSpace.Id.OpaqueId) + Expect(err).ToNot(HaveOccurred()) + ref.ResourceId = &resID + + // CommitUpload streams the staged bytes, so the blob arrives as a reader. + bs.On("UploadFromReader", mock.AnythingOfType("*node.Node"), mock.Anything, mock.AnythingOfType("int64")). + Return(nil). + Run(func(args mock.Arguments) { + n := args.Get(0).(*node.Node) + data, err := io.ReadAll(args.Get(1).(io.Reader)) + Expect(err).ToNot(HaveOccurred()) + Expect(len(data)).To(Equal(int(n.Blobsize)), "the committed blob must match the declared size") + }) + + uploadID = upload(firstContent) + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) + }) + + AfterEach(func() { + if o.Root != "" { + os.RemoveAll(o.Root) + } + close(pub) + close(con) + }) + + When("the uploaded file is new", func() { + It("is visible as processing and commits only after postprocessing succeeds", func() { + exists, status, _ := fileStatus() + Expect(exists).To(BeTrue()) + Expect(status).To(Equal("processing")) + + succeedPostprocessing(uploadID) + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 1) + exists, status, size := fileStatus() + Expect(exists).To(BeTrue()) + Expect(status).To(BeEmpty()) + Expect(size).To(Equal(len(firstContent))) + Expect(stagedBytesExist(uploadID)).To(BeFalse(), "staged bytes should be cleaned up") + }) + + It("deletes node and bytes when instructed", func() { + Expect(stagedBytesExist(uploadID)).To(BeTrue()) + + failPostprocessing(uploadID, events.PPOutcomeDelete) + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) + exists, _, _ := fileStatus() + Expect(exists).To(BeFalse(), "node should be gone") + Expect(stagedBytesExist(uploadID)).To(BeFalse(), "bytes should be gone") + }) + + It("removes the node but keeps the bytes when aborted", func() { + failPostprocessing(uploadID, events.PPOutcomeAbort) + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) + exists, _, _ := fileStatus() + Expect(exists).To(BeFalse(), "node should be gone") + Expect(stagedBytesExist(uploadID)).To(BeTrue(), "an abort keeps the bytes so it can be restarted") + }) + }) + + When("the uploaded file creates a new version", func() { + var secondUploadID string + + BeforeEach(func() { + succeedPostprocessing(uploadID) + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 1) + Expect(revisionCount()).To(Equal(0)) + + secondUploadID = upload(secondContent) + }) + + It("succeeds eventually, creating a new version", func() { + succeedPostprocessing(secondUploadID) + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 2) + Expect(revisionCount()).To(Equal(1)) + _, status, size := fileStatus() + Expect(status).To(BeEmpty()) + Expect(size).To(Equal(len(secondContent))) + Expect(parentSize()).To(Equal(len(secondContent))) + }) + + It("removes the new version and restores the old one when instructed", func() { + failPostprocessing(secondUploadID, events.PPOutcomeDelete) + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 1) + Expect(revisionCount()).To(Equal(0)) + _, status, size := fileStatus() + Expect(status).To(BeEmpty()) + Expect(size).To(Equal(len(firstContent)), "the previous content must be restored") + Expect(parentSize()).To(Equal(len(firstContent))) + }) + }) + + When("two uploads to the same file are processed in parallel", func() { + var secondUploadID string + + BeforeEach(func() { + succeedPostprocessing(uploadID) + // Both uploads are staged before either is postprocessed. + uploadID = upload(firstContent) + secondUploadID = upload(secondContent) + }) + + It("keeps the processing status until the last upload finished", func() { + succeedPostprocessing(uploadID) + _, status, _ := fileStatus() + Expect(status).To(Equal("processing"), "the second upload is still processing") + + succeedPostprocessing(secondUploadID) + _, status, _ = fileStatus() + Expect(status).To(BeEmpty()) + }) + + It("ends with the content of the last upload to finish", func() { + succeedPostprocessing(uploadID) + succeedPostprocessing(secondUploadID) + + _, _, size := fileStatus() + Expect(size).To(Equal(len(secondContent))) + Expect(parentSize()).To(Equal(len(secondContent))) + }) + + It("keeps the first upload when the second is deleted", func() { + succeedPostprocessing(uploadID) + failPostprocessing(secondUploadID, events.PPOutcomeDelete) + + exists, _, size := fileStatus() + Expect(exists).To(BeTrue(), "the first upload must survive the second being deleted") + Expect(size).To(Equal(len(firstContent))) + Expect(parentSize()).To(Equal(len(firstContent))) + }) + + It("keeps the second upload when the first is deleted", func() { + failPostprocessing(uploadID, events.PPOutcomeDelete) + succeedPostprocessing(secondUploadID) + + exists, _, size := fileStatus() + Expect(exists).To(BeTrue()) + Expect(size).To(Equal(len(secondContent))) + }) + }) +}) From e930016e44a7569dd3eafe08eeb625ca2364cc18 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Thu, 30 Jul 2026 16:48:46 +0200 Subject: [PATCH 04/15] feat: implement new event consumer --- .../storageprovider/storageprovider.go | 7 +- .../services/dataprovider/dataprovider.go | 4 +- pkg/upload/coordinator.go | 18 ++- pkg/upload/filestore.go | 53 ++++++-- pkg/upload/filestore_test.go | 33 ++++- pkg/upload/postprocessing.go | 52 ++++++++ pkg/upload/upload_async_test.go | 124 +++++++++++++++--- pkg/upload/upload_suite_test.go | 31 +++++ 8 files changed, 274 insertions(+), 48 deletions(-) create mode 100644 pkg/upload/upload_suite_test.go diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 539b2c0d01f..7ee58f5905e 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -200,8 +200,11 @@ func New(m map[string]interface{}, ss *grpc.Server, log *zerolog.Logger) (rgrpc. if err := store.Setup(); err != nil { return nil, fmt.Errorf("storageprovider: upload directory setup failed: %w", err) } - coordinator := upload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream, - upload.AsyncUploadsFromDriverConf(c.Drivers[c.Driver])) + // No StartPostprocessing call yet, so the coordinator commits uploads inline. + // Starting it here would put a second consumer in the driver's event group, + // where the two would steal each other's results; the driver still owns that + // subscription. Switching the pair over is OCISDEV-900's remaining step. + coordinator := upload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) // parse data server url u, err := url.Parse(c.DataServerURL) diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index e6167a056a1..1b849e3dec6 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -116,8 +116,8 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) if err := store.Setup(); err != nil { return nil, fmt.Errorf("dataprovider: upload directory setup failed: %w", err) } - coord := pkgupload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream, - pkgupload.AsyncUploadsFromDriverConf(conf.Drivers[conf.Driver])) + // Commits inline: see the storageprovider for why postprocessing is not started here. + coord := pkgupload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) dataTXs, err := getDataTXs(conf, coord, fs, evstream, log) if err != nil { diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index 2013ad3a46a..1633daf91b8 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -56,6 +56,9 @@ type Coordinator interface { // Upload writes the whole body of a non-resumable (PUT) upload into the // session named by req.Ref.Path and finishes it. Upload(ctx context.Context, req storage.UploadRequest, uff storage.UploadFinishedFunc) (*provider.ResourceInfo, error) + // StartPostprocessing subscribes to postprocessing results and enables async + // uploads. Call once, before serving requests. + StartPostprocessing(stream events.Consumer, group, mountID string, numConsumers int) error } // coordinator is the concrete implementation of Coordinator. @@ -64,7 +67,11 @@ type coordinator struct { store SessionStore chunkHandler *chunking.ChunkHandler pub events.Publisher - async bool + // async and mountID are set by StartPostprocessing and read by the upload + // path, which runs on request goroutines. StartPostprocessing is called once + // during service construction, before any request is served. + async bool + mountID string } // NewCoordinator constructs a coordinator backed by the given storage driver @@ -77,11 +84,10 @@ type coordinator struct { // pub receives the UploadReady event that tells the rest of the system a file is // available; pass nil to disable publishing. // -// async makes finished uploads wait for postprocessing (virus scanning) before -// their bytes are committed. It requires pub: without a publisher there is -// nothing to start postprocessing, so the upload would never complete. -func NewCoordinator(fs storage.FS, store SessionStore, chunkFolder string, pub events.Publisher, async bool) *coordinator { - c := &coordinator{fs: fs, store: store, pub: pub, async: async && pub != nil} +// Uploads commit inline until StartPostprocessing is called: deferring the commit +// is only safe once something is listening for the result. +func NewCoordinator(fs storage.FS, store SessionStore, chunkFolder string, pub events.Publisher) *coordinator { + c := &coordinator{fs: fs, store: store, pub: pub} if chunkFolder != "" { c.chunkHandler = chunking.NewChunkHandler(chunkFolder) } diff --git a/pkg/upload/filestore.go b/pkg/upload/filestore.go index 07ab98cfb95..3d1f5902cb3 100644 --- a/pkg/upload/filestore.go +++ b/pkg/upload/filestore.go @@ -94,23 +94,54 @@ func NewFileStoreFromConfig(uploadDir string, driverConf map[string]interface{}, return FileStoreFromDriverConf(driverConf, log) } -// AsyncUploadsFromDriverConf reports whether the driver is configured to run -// uploads through postprocessing. +// AsyncConf is how a service asks for async uploads: whether they are enabled, +// and the consumer subscription to use if they are. +type AsyncConf struct { + Enabled bool + ConsumerGroup string + NumConsumers int + // MountID is the storage id this provider answers for, used to drop + // postprocessing events belonging to other storages. + MountID string +} + +// AsyncConfFromDriverConf reads the postprocessing settings off the driver config +// map the services already hand us. // -// The key is decomposedfs's (options.go: `asyncfileuploads`), read straight off -// the driver config map the services already hand us. Reading the driver's own -// key rather than introducing a service-level one keeps a single source of truth: -// if the coordinator and the driver disagreed, uploads would either commit twice -// or never get scanned. -func AsyncUploadsFromDriverConf(driverConf map[string]interface{}) bool { +// The keys are decomposedfs's (options.go: `asyncfileuploads`, `events`). Reading +// the driver's own keys rather than introducing service-level ones keeps a single +// source of truth: if the coordinator and the driver disagreed, uploads would +// either commit twice or never get scanned. +// +// The consumer group matters most. It is what makes retiring the driver's +// consumer a move rather than an addition: two consumers in one group take turns +// stealing each other's events, two in different groups both act and commit the +// same upload twice. +func AsyncConfFromDriverConf(driverConf map[string]interface{}) AsyncConf { if driverConf == nil { - return false + return AsyncConf{} } var ac struct { - AsyncFileUploads bool `mapstructure:"asyncfileuploads"` + AsyncFileUploads bool `mapstructure:"asyncfileuploads"` + MountID string `mapstructure:"mount_id"` + Events struct { + NumConsumers int `mapstructure:"numconsumers"` + ConsumerGroup string `mapstructure:"consumer_group"` + } `mapstructure:"events"` } _ = mapstructure.Decode(driverConf, &ac) - return ac.AsyncFileUploads + group := ac.Events.ConsumerGroup + if group == "" { + // decomposedfs's default (options.go:177). The coordinator takes over the + // driver's subscription, so it must land in the same group. + group = "dcfs" + } + return AsyncConf{ + Enabled: ac.AsyncFileUploads, + ConsumerGroup: group, + NumConsumers: ac.Events.NumConsumers, + MountID: ac.MountID, + } } func newFileStoreWithTokens(root string, driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { diff --git a/pkg/upload/filestore_test.go b/pkg/upload/filestore_test.go index 55ddf925a49..0cabcb27f52 100644 --- a/pkg/upload/filestore_test.go +++ b/pkg/upload/filestore_test.go @@ -331,11 +331,32 @@ func TestNewFileStoreFromConfig_UploadDirKeepsDriverTokens(t *testing.T) { assert.Equal(t, "https://dl.example.com/data/", fs.opts.DownloadEndpoint) } -// AsyncUploadsFromDriverConf +// AsyncConfFromDriverConf -func TestAsyncUploadsFromDriverConf(t *testing.T) { - assert.False(t, AsyncUploadsFromDriverConf(nil)) - assert.False(t, AsyncUploadsFromDriverConf(map[string]interface{}{"root": "/x"})) - assert.False(t, AsyncUploadsFromDriverConf(map[string]interface{}{"asyncfileuploads": false})) - assert.True(t, AsyncUploadsFromDriverConf(map[string]interface{}{"asyncfileuploads": true})) +func TestAsyncConfFromDriverConf(t *testing.T) { + assert.False(t, AsyncConfFromDriverConf(nil).Enabled) + assert.False(t, AsyncConfFromDriverConf(map[string]interface{}{"root": "/x"}).Enabled) + assert.False(t, AsyncConfFromDriverConf(map[string]interface{}{"asyncfileuploads": false}).Enabled) + assert.True(t, AsyncConfFromDriverConf(map[string]interface{}{"asyncfileuploads": true}).Enabled) +} + +// The coordinator takes over the driver's subscription, so it has to resolve the +// group to the same value the driver does, default included. +func TestAsyncConfFromDriverConfConsumerGroup(t *testing.T) { + assert.Equal(t, "dcfs", AsyncConfFromDriverConf(map[string]interface{}{}).ConsumerGroup) + assert.Equal(t, "dcfs", AsyncConfFromDriverConf(map[string]interface{}{ + "events": map[string]interface{}{"numconsumers": 3}, + }).ConsumerGroup) + + ac := AsyncConfFromDriverConf(map[string]interface{}{ + "asyncfileuploads": true, + "mount_id": "storage-users-1", + "events": map[string]interface{}{ + "consumer_group": "custom", + "numconsumers": 4, + }, + }) + assert.Equal(t, "custom", ac.ConsumerGroup) + assert.Equal(t, 4, ac.NumConsumers) + assert.Equal(t, "storage-users-1", ac.MountID) } diff --git a/pkg/upload/postprocessing.go b/pkg/upload/postprocessing.go index b4f4702d289..37be0897d27 100644 --- a/pkg/upload/postprocessing.go +++ b/pkg/upload/postprocessing.go @@ -28,6 +28,7 @@ package upload import ( "context" + "errors" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/rs/zerolog" @@ -46,6 +47,40 @@ var RegisteredEvents = []events.Unmarshaller{ events.CleanUpload{}, } +// StartPostprocessing subscribes to postprocessing results and switches the +// coordinator over to async uploads: from here on finished uploads stage their +// bytes and wait for a scan verdict instead of committing inline. +// +// The two go together on purpose. Deferring a commit is only safe if something +// will arrive to finish it, so there is no way to enable async without a running +// consumer, and none to run a consumer that never receives work. +// +// mountID is the storage id this provider serves. Postprocessing events are +// broadcast to every provider, so events for other storages must be dropped; +// pass "" only in tests, where a single provider sees a private stream. +// +// numConsumers goroutines share the subscription. Call once, before serving +// requests. Fails without a publisher: nothing would hand uploads to +// postprocessing, so every one of them would wait for a verdict that never comes. +func (c *coordinator) StartPostprocessing(stream events.Consumer, group, mountID string, numConsumers int) error { + if c.pub == nil { + return errors.New("coordinator: async uploads need an event publisher") + } + ch, err := events.Consume(stream, group, RegisteredEvents...) + if err != nil { + return err + } + if numConsumers <= 0 { + numConsumers = 1 + } + c.mountID = mountID + c.async = true + for i := 0; i < numConsumers; i++ { + go c.Postprocessing(ch) + } + return nil +} + // Postprocessing consumes postprocessing results until ch is closed. Run it in // its own goroutine, one per configured consumer. func (c *coordinator) Postprocessing(ch <-chan events.Event) { @@ -54,11 +89,25 @@ func (c *coordinator) Postprocessing(ch <-chan events.Event) { } } +// servesStorage reports whether an event is for the storage this coordinator +// serves. Postprocessing runs as a separate service and broadcasts its results to +// every storage provider, so each has to recognise its own. Events that name no +// storage predate this and are accepted. +func (c *coordinator) servesStorage(id *provider.ResourceId) bool { + if c.mountID == "" || id.GetStorageId() == "" { + return true + } + return id.GetStorageId() == c.mountID +} + func (c *coordinator) processEvent(ctx context.Context, event events.Event) { log := appctx.GetLogger(ctx) switch ev := event.Event.(type) { case events.PostprocessingFinished: + if !c.servesStorage(ev.ResourceID) { + return + } c.onPostprocessingFinished(ctx, ev, log) case events.RestartPostprocessing: c.onRestartPostprocessing(ctx, ev, log) @@ -72,6 +121,9 @@ func (c *coordinator) processEvent(ctx context.Context, event events.Event) { c.rollbackPrepared(ctx, session, session.SizeDiff()) } case events.PostprocessingStepFinished: + if !c.servesStorage(ev.ResourceID) { + return + } if ev.FinishedStep != events.PPStepAntivirus { // only the antivirus result is recorded on the session return diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 6935539c0c1..9971489e7c0 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -94,14 +94,20 @@ var _ = Describe("Async uploads via the coordinator", func() { pub, con chan interface{} uploadID string - fs storage.FS - coord pkgupload.Coordinator - o *options.Options - bs *treemocks.Blobstore - - // upload runs a full upload through the coordinator and returns the - // session id, leaving it staged and awaiting postprocessing. - upload = func(content []byte) string { + fs storage.FS + coord pkgupload.Coordinator + evstream stream.Chan + o *options.Options + bs *treemocks.Blobstore + + // Set in the outer BeforeEach and overridable by an inner one, then acted on + // in JustBeforeEach, which runs after both. + startAsync bool + mountID string + + // initiateAndUpload runs a full upload through the coordinator and returns + // the session id, without asserting anything about what it published. + initiateAndUpload = func(content []byte) string { ids, err := coord.InitiateUpload(ctx, ref, int64(len(content)), map[string]string{}) Expect(err).ToNot(HaveOccurred()) Expect(ids["simple"]).ToNot(BeEmpty()) @@ -113,12 +119,20 @@ var _ = Describe("Async uploads via the coordinator", func() { }, nil) Expect(err).ToNot(HaveOccurred()) + return ids["simple"] + } + + // upload stages an upload and leaves it awaiting postprocessing. Only valid + // on the async path: inline uploads publish UploadReady, not BytesReceived. + upload = func(content []byte) string { + id := initiateAndUpload(content) + ev, ok := (<-pub).(events.BytesReceived) Expect(ok).To(BeTrue(), "expected BytesReceived: the upload must not commit before postprocessing") - Expect(ev.UploadID).To(Equal(ids["simple"])) + Expect(ev.UploadID).To(Equal(id)) Expect(ev.URL).ToNot(BeEmpty(), "postprocessing needs a URL to fetch the staged bytes from") - return ids["simple"] + return id } succeedPostprocessing = func(id string) { @@ -209,7 +223,7 @@ var _ = Describe("Async uploads via the coordinator", func() { }, nil) pub, con = make(chan interface{}), make(chan interface{}) - evstream := stream.Chan{pub, con} + evstream = stream.Chan{pub, con} t := tree.New(lu, bs, o, store.Create(), &zerolog.Logger{}) fs, err = decomposedfs.New(o, aspects.Aspects{ @@ -229,12 +243,7 @@ var _ = Describe("Async uploads via the coordinator", func() { TransferExpires: 86400, }, &zl) Expect(sessionStore.Setup()).To(Succeed()) - c := pkgupload.NewCoordinator(fs, sessionStore, filepath.Join(o.Root, "uploads"), evstream, true) - coord = c - - ch, err := events.Consume(evstream, "coordinator-test", pkgupload.RegisteredEvents...) - Expect(err).ToNot(HaveOccurred()) - go c.Postprocessing(ch) + coord = pkgupload.NewCoordinator(fs, sessionStore, filepath.Join(o.Root, "uploads"), evstream) resp, err := fs.CreateStorageSpace(ctx, &provider.CreateStorageSpaceRequest{Owner: user, Type: "personal"}) Expect(err).ToNot(HaveOccurred()) @@ -253,8 +262,19 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(len(data)).To(Equal(int(n.Blobsize)), "the committed blob must match the declared size") }) - uploadID = upload(firstContent) - bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) + // Most scenarios test the async path. Inner blocks flip these before + // JustBeforeEach acts on them. + startAsync, mountID = true, "" + }) + + JustBeforeEach(func() { + // Starting the consumer is what switches the coordinator to async uploads; + // the two are inseparable by design, so there is no separate flag to set. + if startAsync { + Expect(coord.StartPostprocessing(evstream, "coordinator-test", mountID, 1)).To(Succeed()) + uploadID = upload(firstContent) + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) + } }) AfterEach(func() { @@ -305,7 +325,9 @@ var _ = Describe("Async uploads via the coordinator", func() { When("the uploaded file creates a new version", func() { var secondUploadID string - BeforeEach(func() { + // JustBeforeEach, not BeforeEach: this builds on the upload the outer + // JustBeforeEach stages, which has not happened yet at BeforeEach time. + JustBeforeEach(func() { succeedPostprocessing(uploadID) bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 1) Expect(revisionCount()).To(Equal(0)) @@ -339,7 +361,7 @@ var _ = Describe("Async uploads via the coordinator", func() { When("two uploads to the same file are processed in parallel", func() { var secondUploadID string - BeforeEach(func() { + JustBeforeEach(func() { succeedPostprocessing(uploadID) // Both uploads are staged before either is postprocessed. uploadID = upload(firstContent) @@ -384,4 +406,64 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(size).To(Equal(len(secondContent))) }) }) + + When("postprocessing was never started", func() { + BeforeEach(func() { + startAsync = false + }) + + // The guarantee that keeps the two switches from drifting apart: with no + // consumer running, nothing would ever arrive to finish a deferred upload, so + // the coordinator must commit inline instead of staging and waiting forever. + It("commits inline instead of waiting for a scan that will never come", func() { + id := initiateAndUpload(firstContent) + + ev, ok := (<-pub).(events.UploadReady) + Expect(ok).To(BeTrue(), "an inline commit announces the file directly, without a scan round trip") + Expect(ev.Failed).To(BeFalse()) + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 1) + exists, status, size := fileStatus() + Expect(exists).To(BeTrue()) + Expect(status).To(BeEmpty(), "the upload must not be left in processing") + Expect(size).To(Equal(len(firstContent))) + Expect(stagedBytesExist(id)).To(BeFalse()) + }) + }) + + When("the coordinator serves a specific storage", func() { + BeforeEach(func() { + mountID = "storage-users-1" + }) + + // Postprocessing broadcasts its results to every storage provider, so each + // has to recognise its own. Acting on another storage's event would commit an + // upload this provider knows nothing about. + It("ignores results belonging to a different storage", func() { + otherStorage := upload(secondContent) + + // One consumer processes the stream in order, so once the second result + // has been acted on the first has already been seen and dropped. + con <- events.PostprocessingFinished{ + UploadID: otherStorage, + Outcome: events.PPOutcomeContinue, + ResourceID: &provider.ResourceId{StorageId: "some-other-storage"}, + } + con <- events.PostprocessingFinished{ + UploadID: uploadID, + Outcome: events.PPOutcomeContinue, + ResourceID: &provider.ResourceId{StorageId: mountID}, + } + + ev, ok := (<-pub).(events.UploadReady) + Expect(ok).To(BeTrue()) + Expect(ev.Failed).To(BeFalse()) + Expect(ev.UploadID).To(Equal(uploadID), "the other storage's upload must not be announced") + + // Only the upload addressed to this storage was committed; the other one is + // left untouched for the provider that actually owns it. + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 1) + Expect(stagedBytesExist(otherStorage)).To(BeTrue(), "a filtered upload must be left alone, not cleaned up") + }) + }) }) diff --git a/pkg/upload/upload_suite_test.go b/pkg/upload/upload_suite_test.go new file mode 100644 index 00000000000..90a9cca1fcb --- /dev/null +++ b/pkg/upload/upload_suite_test.go @@ -0,0 +1,31 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package upload_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestUpload(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Upload Coordinator Suite") +} From 0bd2539825d00c57b139e44bf627d000b8d7dcc0 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Thu, 30 Jul 2026 17:36:40 +0200 Subject: [PATCH 05/15] feat: route PUT through the upload coordinator --- pkg/rhttp/datatx/manager/simple/simple.go | 2 +- pkg/rhttp/datatx/manager/spaces/spaces.go | 2 +- pkg/upload/coordinator.go | 55 +++++++++++++++-------- pkg/upload/upload_async_test.go | 26 ++++++++++- 4 files changed, 64 insertions(+), 21 deletions(-) diff --git a/pkg/rhttp/datatx/manager/simple/simple.go b/pkg/rhttp/datatx/manager/simple/simple.go index 5a2c8ce8bbd..dfad2743a21 100644 --- a/pkg/rhttp/datatx/manager/simple/simple.go +++ b/pkg/rhttp/datatx/manager/simple/simple.go @@ -115,7 +115,7 @@ func (m *manager) Handler(coord pkgupload.Coordinator, driver storage.FS) (http. ctx = ctxpkg.ContextSetLockID(ctx, lockID) } - info, err := driver.Upload(ctx, storage.UploadRequest{ + info, err := coord.Upload(ctx, storage.UploadRequest{ Ref: ref, Body: r.Body, Length: r.ContentLength, diff --git a/pkg/rhttp/datatx/manager/spaces/spaces.go b/pkg/rhttp/datatx/manager/spaces/spaces.go index 285317997c9..6250f3d9a5b 100644 --- a/pkg/rhttp/datatx/manager/spaces/spaces.go +++ b/pkg/rhttp/datatx/manager/spaces/spaces.go @@ -118,7 +118,7 @@ func (m *manager) Handler(coord pkgupload.Coordinator, driver storage.FS) (http. Path: fn, } var info *provider.ResourceInfo - info, err = driver.Upload(ctx, storage.UploadRequest{ + info, err = coord.Upload(ctx, storage.UploadRequest{ Ref: ref, Body: r.Body, Length: r.ContentLength, diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index 1633daf91b8..d459082f439 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -376,10 +376,10 @@ func (c *coordinator) initiateUpload(ctx context.Context, ref *provider.Referenc // Upload writes the entire body of a non-resumable (PUT) upload and finishes it. // req.Ref.Path carries the session id, as minted by InitiateUpload. // -// This is the driver-agnostic port of decomposedfs.Upload (upload.go:51). It is -// not yet wired up: the simple and spaces data transfer managers still call -// driver.Upload, because only decomposedfs and ocm implement CommitUpload and -// routing the others here would break their PUT path. +// This is the driver-agnostic port of decomposedfs.Upload (upload.go:51), and +// the simple and spaces data transfer managers now route PUT through it, so PUT +// and TUS share one finish path. Only the drivers that implement CommitUpload +// are supported here; the rest are being retired with OCISDEV-901. func (c *coordinator) Upload(ctx context.Context, req storage.UploadRequest, uff storage.UploadFinishedFunc) (*provider.ResourceInfo, error) { // The datatx handler passes the request path straight through, and it arrives // rooted ("/"), while session ids are stored unrooted. @@ -482,11 +482,11 @@ func (c *coordinator) finishUpload(ctx context.Context, session Session) (*provi } // The node is left flagged as processing and the staged bytes are kept: // the postprocessing consumer needs both to finish the upload later. - return &provider.ResourceInfo{Id: &provider.ResourceId{ - StorageId: session.ProviderID(), - SpaceId: session.SpaceID(), - OpaqueId: session.NodeID(), - }}, nil + // + // PUT callers turn this into response headers (an etag and mtime the + // desktop client stores to detect later changes), so report the node as + // PrepareUpload left it rather than an id on its own. + return c.uploadedResourceInfo(ctx, session), nil } return c.finishSync(ctx, session) @@ -626,18 +626,37 @@ func (c *coordinator) finishSync(ctx context.Context, session Session) (*provide session.Cleanup(true, true) metrics.UploadSessionsFinalized.Inc() - // The driver owns the resulting etag and mtime, so read them back rather than - // assembling a ResourceInfo from what we sent. GetMD is permission-gated and - // the executant may not be allowed to stat what they just uploaded (a - // write-only share), so a failure here must not fail the committed upload. + ri := c.uploadedResourceInfo(ctx, session) + c.publishUploadReady(ctx, session, ri) + return ri, nil +} + +// uploadedResourceInfo describes the uploaded resource for the caller, which +// turns it into PUT response headers. +// +// The driver owns the resulting etag and mtime, so read them back rather than +// assembling them from what we sent. GetMD is permission-gated and the executant +// may not be allowed to stat what they just uploaded (a write-only share), so a +// failure here must not fail the upload; fall back to what the session knows. +// +// TODO(OCISDEV-900): the fallback carries no etag. main's driver.Upload computed +// one unconditionally from the node id and mtime, so an Uploader on a write-only +// share used to get an ETag header and now does not. Computing it here would mean +// importing decomposedfs's node package into the driver-agnostic coordinator; +// doing it properly means returning the etag through the seam. +func (c *coordinator) uploadedResourceInfo(ctx context.Context, session Session) *provider.ResourceInfo { + ref := session.Reference() ri, err := c.fs.GetMD(ctx, &ref, nil, nil) - if err != nil { - appctx.GetLogger(ctx).Debug().Err(err).Str("uploadid", session.ID()).Msg("could not stat committed upload") - ri = &provider.ResourceInfo{Id: ref.GetResourceId(), Size: uint64(session.Size())} + if err == nil { + return ri } + appctx.GetLogger(ctx).Debug().Err(err).Str("uploadid", session.ID()).Msg("could not stat uploaded resource") - c.publishUploadReady(ctx, session, ri) - return ri, nil + fallback := &provider.ResourceInfo{Id: ref.GetResourceId(), Size: uint64(session.Size())} + if mtime, mErr := utils.MTimeToTime(session.Metadata()["mtime"]); mErr == nil { + fallback.Mtime = utils.TimeToTS(mtime) + } + return fallback } // uploadInfo collects what the driver needs to write node metadata. The diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 9971489e7c0..20ff82e1d59 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -105,6 +105,10 @@ var _ = Describe("Async uploads via the coordinator", func() { startAsync bool mountID string + // uploadedInfo is what the last coord.Upload reported. PUT handlers turn it + // into response headers, so it is part of the contract, not a by-product. + uploadedInfo *provider.ResourceInfo + // initiateAndUpload runs a full upload through the coordinator and returns // the session id, without asserting anything about what it published. initiateAndUpload = func(content []byte) string { @@ -112,7 +116,7 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(err).ToNot(HaveOccurred()) Expect(ids["simple"]).ToNot(BeEmpty()) - _, err = coord.Upload(ctx, storage.UploadRequest{ + uploadedInfo, err = coord.Upload(ctx, storage.UploadRequest{ Ref: &provider.Reference{Path: "/" + ids["simple"]}, Body: io.NopCloser(bytes.NewReader(content)), Length: int64(len(content)), @@ -407,6 +411,26 @@ var _ = Describe("Async uploads via the coordinator", func() { }) }) + // PUT goes through coord.Upload and writes the returned etag/mtime/id straight + // into response headers. The desktop client stores the etag to detect later + // changes, so an empty one makes it re-download the file it just sent. + When("a PUT-style upload reports its result", func() { + It("carries an etag, mtime and id while still processing", func() { + Expect(uploadedInfo).ToNot(BeNil()) + Expect(uploadedInfo.GetEtag()).ToNot(BeEmpty(), "PUT sets an ETag header from this") + Expect(uploadedInfo.GetMtime()).ToNot(BeNil(), "PUT sets Last-Modified from this") + Expect(uploadedInfo.GetId().GetOpaqueId()).ToNot(BeEmpty(), "PUT sets the file id header from this") + }) + + It("carries an etag for a new version too", func() { + succeedPostprocessing(uploadID) + + upload(secondContent) + Expect(uploadedInfo.GetEtag()).ToNot(BeEmpty()) + Expect(uploadedInfo.GetId().GetOpaqueId()).ToNot(BeEmpty()) + }) + }) + When("postprocessing was never started", func() { BeforeEach(func() { startAsync = false From 673bcf90d5bd7e1dd6eab027c1b3058b478af7e0 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Fri, 31 Jul 2026 09:37:33 +0200 Subject: [PATCH 06/15] feat: move upload postprocessing to the coordinator --- .../storageprovider/storageprovider.go | 8 +- .../services/dataprovider/dataprovider.go | 10 +- .../utils/decomposedfs/decomposedfs.go | 341 +------- .../utils/decomposedfs/upload_async_test.go | 729 ------------------ pkg/upload/coordinator.go | 49 +- pkg/upload/postprocessing.go | 1 + pkg/upload/session.go | 14 + pkg/upload/upload_async_test.go | 126 ++- 8 files changed, 188 insertions(+), 1090 deletions(-) delete mode 100644 pkg/storage/utils/decomposedfs/upload_async_test.go diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 7ee58f5905e..4758b82fdb7 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -200,10 +200,10 @@ func New(m map[string]interface{}, ss *grpc.Server, log *zerolog.Logger) (rgrpc. if err := store.Setup(); err != nil { return nil, fmt.Errorf("storageprovider: upload directory setup failed: %w", err) } - // No StartPostprocessing call yet, so the coordinator commits uploads inline. - // Starting it here would put a second consumer in the driver's event group, - // where the two would steal each other's results; the driver still owns that - // subscription. Switching the pair over is OCISDEV-900's remaining step. + // Deliberately no StartPostprocessing here: this service initiates uploads but + // never receives the bytes, so it has nothing to hand to postprocessing or to + // commit afterwards. That belongs to the data provider, which owns the PUT and + // TUS paths. coordinator := upload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) // parse data server url diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index 1b849e3dec6..115b282f604 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -116,9 +116,17 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) if err := store.Setup(); err != nil { return nil, fmt.Errorf("dataprovider: upload directory setup failed: %w", err) } - // Commits inline: see the storageprovider for why postprocessing is not started here. coord := pkgupload.NewCoordinator(fs, store, filepath.Join(store.Root(), "uploads"), evstream) + // This is the service that receives the bytes, so it is the one that hands them + // to postprocessing and commits them once the verdict comes back. Without this + // the coordinator commits inline and uploads are never scanned. + if ac := pkgupload.AsyncConfFromDriverConf(conf.Drivers[conf.Driver]); ac.Enabled { + if err := coord.StartPostprocessing(evstream, ac.ConsumerGroup, ac.MountID, ac.NumConsumers); err != nil { + return nil, fmt.Errorf("dataprovider: could not start postprocessing: %w", err) + } + } + dataTXs, err := getDataTXs(conf, coord, fs, evstream, log) if err != nil { return nil, err diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index a0fb931893b..f649021650e 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -28,9 +28,7 @@ import ( "path/filepath" "strconv" "strings" - "time" - user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/jellydator/ttlcache/v2" @@ -49,7 +47,6 @@ import ( "github.com/owncloud/reva/v2/pkg/events" "github.com/owncloud/reva/v2/pkg/logger" "github.com/owncloud/reva/v2/pkg/rgrpc/todo/pool" - "github.com/owncloud/reva/v2/pkg/rhttp/datatx/metrics" "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storage/utils/chunking" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/aspects" @@ -81,11 +78,11 @@ const ( var ( tracer trace.Tracer + // The upload-related postprocessing events are consumed by the upload + // coordinator (pkg/upload), which owns the upload lifecycle for every driver. + // RevertRevision is not part of that lifecycle: the storage-users CLI emits it + // to undo a revision, and reverting one needs the driver's node internals. _registeredEvents = []events.Unmarshaller{ - events.PostprocessingFinished{}, - events.PostprocessingStepFinished{}, - events.RestartPostprocessing{}, - events.CleanUpload{}, events.RevertRevision{}, } ) @@ -260,35 +257,39 @@ func New(o *options.Options, aspects aspects.Aspects, log *zerolog.Logger) (stor return nil, err } + // Gated on AsyncFileUploads only because that is the flag that has always + // enabled this subscription; reverting a revision is otherwise unrelated to + // async uploads. Widening it would start a consumer in deployments that never + // had one. if o.AsyncFileUploads { if fs.stream == nil { log.Error().Msg("need event stream for async file processing") return nil, errors.New("need nats for async file processing") } - ch, err := events.Consume(fs.stream, o.Events.ConsumerGroup, _registeredEvents...) + // A group name is also the JetStream durable consumer name, and members of one + // group share messages rather than each getting a copy. This must therefore not + // share the coordinator's group: an upload event delivered here would be + // dropped by the type filter above and never reach the coordinator. + ch, err := events.Consume(fs.stream, o.Events.ConsumerGroup+"-revisions", _registeredEvents...) if err != nil { return nil, err } - - if o.Events.NumConsumers <= 0 { - o.Events.NumConsumers = 1 - } - - for i := 0; i < o.Events.NumConsumers; i++ { - go fs.Postprocessing(ch) - } + // One consumer is enough: revisions are reverted by a maintenance command, + // not by upload traffic. + go fs.ConsumeRevisionEvents(ch) } return fs, nil } -// Postprocessing starts the postprocessing result collector -func (fs *Decomposedfs) Postprocessing(ch <-chan events.Event) { +// ConsumeRevisionEvents handles revision events until ch is closed. Upload +// postprocessing lives in the upload coordinator; this only reverts revisions on +// behalf of the storage-users CLI, which needs the driver's node internals. +func (fs *Decomposedfs) ConsumeRevisionEvents(ch <-chan events.Event) { log := logger.New() for event := range ch { - evCtx := context.Background() - fs.processEvent(evCtx, event, log) + fs.processEvent(context.Background(), event, log) } } @@ -298,180 +299,6 @@ func (fs *Decomposedfs) processEvent(evCtx context.Context, event events.Event, defer span.End() switch ev := event.Event.(type) { - case events.PostprocessingFinished: - sublog := log.With().Str("event", "PostprocessingFinished").Str("uploadid", ev.UploadID).Logger() - if ev.ResourceID != nil && ev.ResourceID.GetStorageId() != "" && ev.ResourceID.GetStorageId() != fs.o.MountID { - sublog.Debug().Msg("ignoring event for different storage") - return - } - session, err := fs.sessionStore.Get(ctx, ev.UploadID) - if err != nil { - sublog.Error().Err(err).Msg("Failed to get upload") - return // NOTE: since we can't get the upload, we can't delete the blob - } - - ctx = session.Context(ctx) - - n, err := session.Node(ctx) - if err != nil { - // The node metadata is unreadable, so this upload can never finish: - // the destination cannot be resolved. Clean the session up instead of - // leaving it behind to be retried forever. Cleanup falls back to the - // session metadata to release the quota. - sublog.Error().Err(err).Msg("could not read node, cleaning up orphaned session") - session.Cleanup(true, true, true, false) - return - } - sublog = log.With().Str("spaceid", session.SpaceID()).Str("nodeid", session.NodeID()).Logger() - if !n.Exists { - sublog.Debug().Msg("node no longer exists") - session.Cleanup(false, true, true, false) - return - } - - var ( - failed bool - revertNodeMetadata bool - keepUpload bool - ) - unmarkPostprocessing := true - - switch ev.Outcome { - default: - sublog.Error().Str("outcome", string(ev.Outcome)).Msg("unknown postprocessing outcome - aborting") - fallthrough - case events.PPOutcomeAbort: - failed = true - revertNodeMetadata = true - keepUpload = true - metrics.UploadSessionsAborted.Inc() - case events.PPOutcomeContinue: - if err := session.Finalize(ctx); err != nil { - sublog.Error().Err(err).Msg("could not finalize upload") - failed = true - revertNodeMetadata = false - keepUpload = true - // keep postprocessing status so the upload is not deleted during housekeeping - unmarkPostprocessing = false - } else { - metrics.UploadSessionsFinalized.Inc() - } - case events.PPOutcomeDelete: - failed = true - revertNodeMetadata = true - metrics.UploadSessionsDeleted.Inc() - } - - getParent := func() *node.Node { - p, err := n.Parent(ctx) - if err != nil { - sublog.Error().Err(err).Msg("could not read parent") - return nil - } - return p - } - - now := time.Now() - if failed { - // if no other upload session is in progress (processing id != session id) or has finished (processing id == "") - latestSession, err := n.ProcessingID(ctx) - if err != nil { - sublog.Error().Err(err).Msg("reading node for session failed") - } - if latestSession == session.ID() { - // propagate reverted sizeDiff after failed postprocessing - if err := fs.tp.Propagate(ctx, n, -session.SizeDiff()); err != nil { - sublog.Error().Err(err).Msg("could not propagate tree size change") - } - } - } else if p := getParent(); p != nil { - // update parent tmtime to propagate etag change after successful postprocessing - _ = p.SetTMTime(ctx, &now) - if err := fs.tp.Propagate(ctx, p, 0); err != nil { - sublog.Error().Err(err).Msg("could not propagate etag change") - } - } - - session.Cleanup(revertNodeMetadata, !keepUpload, !keepUpload, unmarkPostprocessing) - - var isVersion bool - if session.NodeExists() { - info, err := session.GetInfo(ctx) - if err == nil && info.MetaData["versionsPath"] != "" { - isVersion = true - } - } - - if err := events.Publish( - ctx, - fs.stream, - events.UploadReady{ - UploadID: ev.UploadID, - Failed: failed, - ExecutingUser: ev.ExecutingUser, - Filename: ev.Filename, - FileRef: &provider.Reference{ - ResourceId: &provider.ResourceId{ - StorageId: session.ProviderID(), - SpaceId: session.SpaceID(), - OpaqueId: session.SpaceID(), - }, - Path: utils.MakeRelativePath(filepath.Join(session.Dir(), session.Filename())), - }, - ResourceID: &provider.ResourceId{ - StorageId: session.ProviderID(), - SpaceId: session.SpaceID(), - OpaqueId: session.NodeID(), - }, - Timestamp: utils.TimeToTS(now), - SpaceOwner: n.SpaceOwnerOrManager(ctx), - IsVersion: isVersion, - ImpersonatingUser: ev.ImpersonatingUser, - }, - ); err != nil { - sublog.Error().Err(err).Msg("Failed to publish UploadReady event") - } - case events.RestartPostprocessing: - sublog := log.With().Str("event", "RestartPostprocessing").Str("uploadid", ev.UploadID).Logger() - session, err := fs.sessionStore.Get(ctx, ev.UploadID) - if err != nil { - sublog.Error().Err(err).Msg("Failed to get upload") - return - } - n, err := session.Node(ctx) - if err != nil { - sublog.Error().Err(err).Msg("could not read node") - return - } - sublog = log.With().Str("spaceid", session.SpaceID()).Str("nodeid", session.NodeID()).Logger() - s, err := session.URL(ctx) - if err != nil { - sublog.Error().Err(err).Msg("could not create url") - return - } - - metrics.UploadSessionsRestarted.Inc() - - // restart postprocessing - if err := events.Publish(ctx, fs.stream, events.BytesReceived{ - UploadID: session.ID(), - URL: s, - SpaceOwner: n.SpaceOwnerOrManager(ctx), - ExecutingUser: &user.User{Id: &user.UserId{OpaqueId: "postprocessing-restart"}}, // send nil instead? - ResourceID: &provider.ResourceId{SpaceId: n.SpaceID, OpaqueId: n.ID}, - Filename: session.Filename(), - Filesize: uint64(session.Size()), - }); err != nil { - sublog.Error().Err(err).Msg("Failed to publish BytesReceived event") - } - case events.CleanUpload: - sublog := log.With().Str("event", "CleanUpload").Str("uploadid", ev.UploadID).Logger() - session, err := fs.sessionStore.Get(ctx, ev.UploadID) - if err != nil { - sublog.Error().Err(err).Msg("Failed to get upload") - return // NOTE: since we can't get the upload, we can't delete the blob - } - session.Cleanup(true, !ev.KeepUpload, !ev.KeepUpload, true) case events.RevertRevision: sublog := log.With().Str("event", "RevertRevision").Interface("nodeid", ev.ResourceID).Logger() if ev.ResourceID != nil && ev.ResourceID.GetStorageId() != "" && ev.ResourceID.GetStorageId() != fs.o.MountID { @@ -488,132 +315,6 @@ func (fs *Decomposedfs) processEvent(evCtx context.Context, event events.Event, sublog.Error().Err(err).Msg("Failed to revert revision") return } - case events.PostprocessingStepFinished: - sublog := log.With().Str("event", "PostprocessingStepFinished").Str("uploadid", ev.UploadID).Logger() - if ev.ResourceID != nil && ev.ResourceID.GetStorageId() != "" && ev.ResourceID.GetStorageId() != fs.o.MountID { - sublog.Debug().Msg("ignoring event for different storage") - return - } - if ev.FinishedStep != events.PPStepAntivirus { - // atm we are only interested in antivirus results - return - } - - res := ev.Result.(events.VirusscanResult) - if res.ErrorMsg != "" { - // scan failed somehow - // Should we handle this here? - return - } - sublog = log.With().Str("scan_description", res.Description).Bool("infected", res.Infected).Logger() - - var n *node.Node - switch ev.UploadID { - case "": - // uploadid is empty -> this was an on-demand scan - /* ON DEMAND SCANNING NOT SUPPORTED ATM - ctx := ctxpkg.ContextSetUser(context.Background(), ev.ExecutingUser) - ref := &provider.Reference{ResourceId: ev.ResourceID} - - no, err := fs.lu.NodeFromResource(ctx, ref) - if err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Msg("Failed to get node after scan") - continue - - } - n = no - if ev.Outcome == events.PPOutcomeDelete { - // antivir wants us to delete the file. We must obey and need to - - // check if there a previous versions existing - revs, err := fs.ListRevisions(ctx, ref) - if len(revs) == 0 { - if err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Msg("Failed to list revisions. Fallback to delete file") - } - - // no versions -> trash file - err := fs.Delete(ctx, ref) - if err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Msg("Failed to delete infected resource") - continue - } - - // now purge it from the recycle bin - if err := fs.PurgeRecycleItem(ctx, &provider.Reference{ResourceId: &provider.ResourceId{SpaceId: n.SpaceID, OpaqueId: n.SpaceID}}, n.ID, "/"); err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Msg("Failed to purge infected resource from trash") - } - - // remove cache entry in gateway - fs.cache.RemoveStatContext(ctx, ev.ExecutingUser.GetId(), &provider.ResourceId{SpaceId: n.SpaceID, OpaqueId: n.ID}) - continue - } - - // we have versions - find the newest - versions := make(map[uint64]string) // remember all versions - we need them later - var nv uint64 - for _, v := range revs { - versions[v.Mtime] = v.Key - if v.Mtime > nv { - nv = v.Mtime - } - } - - // restore newest version - if err := fs.RestoreRevision(ctx, ref, versions[nv]); err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Str("revision", versions[nv]).Msg("Failed to restore revision") - continue - } - - // now find infected version - revs, err = fs.ListRevisions(ctx, ref) - if err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Msg("Error listing revisions after restore") - } - - for _, v := range revs { - // we looking for a version that was previously not there - if _, ok := versions[v.Mtime]; ok { - continue - } - - if err := fs.DeleteRevision(ctx, ref, v.Key); err != nil { - log.Error().Err(err).Interface("resourceID", ev.ResourceID).Str("revision", v.Key).Msg("Failed to delete revision") - } - } - - // remove cache entry in gateway - fs.cache.RemoveStatContext(ctx, ev.ExecutingUser.GetId(), &provider.ResourceId{SpaceId: n.SpaceID, OpaqueId: n.ID}) - continue - } - */ - default: - // uploadid is not empty -> this is an async upload - session, err := fs.sessionStore.Get(ctx, ev.UploadID) - if err != nil { - sublog.Error().Err(err).Msg("Failed to get upload") - return - } - - n, err = session.Node(ctx) - if err != nil { - sublog.Error().Err(err).Msg("Failed to get node after scan") - return - } - sublog = log.With().Str("spaceid", session.SpaceID()).Str("nodeid", session.NodeID()).Logger() - - session.SetScanData(res.Description, res.Scandate) - if err := session.Persist(ctx); err != nil { - sublog.Error().Err(err).Msg("Failed to persist scan results") - } - } - - if err := n.SetScanData(ctx, res.Description, res.Scandate); err != nil { - sublog.Error().Err(err).Msg("Failed to set scan results") - return - } - - metrics.UploadSessionsScanned.Inc() default: log.Error().Interface("event", ev).Msg("Unknown event") } diff --git a/pkg/storage/utils/decomposedfs/upload_async_test.go b/pkg/storage/utils/decomposedfs/upload_async_test.go deleted file mode 100644 index 65a25e78898..00000000000 --- a/pkg/storage/utils/decomposedfs/upload_async_test.go +++ /dev/null @@ -1,729 +0,0 @@ -package decomposedfs - -import ( - "bytes" - "context" - "io" - "os" - "path/filepath" - - userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" - cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1" - v1beta11 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" - provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" - "github.com/owncloud/reva/v2/pkg/appctx" - ruser "github.com/owncloud/reva/v2/pkg/ctx" - "github.com/owncloud/reva/v2/pkg/events" - "github.com/owncloud/reva/v2/pkg/events/stream" - "github.com/owncloud/reva/v2/pkg/rgrpc/todo/pool" - "github.com/owncloud/reva/v2/pkg/storage" - "github.com/owncloud/reva/v2/pkg/storage/cache" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/aspects" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/lookup" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/options" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/permissions" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/permissions/mocks" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/timemanager" - "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/tree" - treemocks "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/tree/mocks" - "github.com/owncloud/reva/v2/pkg/storagespace" - "github.com/owncloud/reva/v2/pkg/store" - "github.com/owncloud/reva/v2/pkg/utils" - "github.com/owncloud/reva/v2/tests/helpers" - "github.com/rs/zerolog" - "github.com/stretchr/testify/mock" - "google.golang.org/grpc" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Async file uploads", Ordered, func() { - var ( - ref = &provider.Reference{ - ResourceId: &provider.ResourceId{ - SpaceId: "u-s-e-r-id", - }, - Path: "/foo", - } - - rootRef = &provider.Reference{ - ResourceId: &provider.ResourceId{ - SpaceId: "u-s-e-r-id", - OpaqueId: "u-s-e-r-id", - }, - Path: "/", - } - - user = &userpb.User{ - Id: &userpb.UserId{ - Idp: "idp", - OpaqueId: "u-s-e-r-id", - Type: userpb.UserType_USER_TYPE_PRIMARY, - }, - Username: "username", - } - - firstContent = []byte("0123456789") - secondContent = []byte("01234567890123456789") - - ctx context.Context - - pub chan interface{} - con chan interface{} - uploadID string - - fs storage.FS - o *options.Options - lu *lookup.Lookup - pmock *mocks.PermissionsChecker - cs3permissionsclient *mocks.CS3PermissionsClient - permissionsSelector pool.Selectable[cs3permissions.PermissionsAPIClient] - bs *treemocks.Blobstore - - succeedPostprocessing = func(uploadID string) { - // finish postprocessing - con <- events.PostprocessingFinished{ - UploadID: uploadID, - Outcome: events.PPOutcomeContinue, - } - // wait for upload to be ready - ev, ok := (<-pub).(events.UploadReady) - Expect(ok).To(BeTrue()) - Expect(ev.Failed).To(BeFalse()) - Expect(ev.ResourceID).ToNot(BeNil()) - Expect(ev.ResourceID.OpaqueId).ToNot(BeEmpty()) - Expect(ev.ResourceID.OpaqueId).ToNot(Equal(ev.ResourceID.SpaceId), "ResourceID.OpaqueId should be the file node ID, not the space ID") - } - - failPostprocessing = func(uploadID string, outcome events.PostprocessingOutcome) { - // finish postprocessing - con <- events.PostprocessingFinished{ - UploadID: uploadID, - Outcome: outcome, - } - // wait for upload to be ready - ev, ok := (<-pub).(events.UploadReady) - Expect(ok).To(BeTrue()) - Expect(ev.Failed).To(BeTrue()) - } - - fileStatus = func() (bool, string, int) { - // check processing status - resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(BeElementOf([2]int{0, 1}), "should not have more than one child") - - item := resources[0] - Expect(item.Path).To(Equal(ref.Path)) - return len(resources) == 1, utils.ReadPlainFromOpaque(item.Opaque, "status"), int(item.GetSize()) - } - parentSize = func() int { - parentInfo, err := fs.GetMD(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - return int(parentInfo.Size) - } - revisionCount = func() int { - revisions, err := fs.ListRevisions(ctx, ref) - Expect(err).ToNot(HaveOccurred()) - return len(revisions) - } - ) - - BeforeEach(func() { - zl := zerolog.New(os.Stdout).Level(zerolog.DebugLevel) - ctx = appctx.WithLogger(ruser.ContextSetUser(context.Background(), user), &zl) - - // setup test - tmpRoot, err := helpers.TempDir("reva-unit-tests-*-root") - Expect(err).ToNot(HaveOccurred()) - - o, err = options.New(map[string]interface{}{ - "root": tmpRoot, - "asyncfileuploads": true, - "treetime_accounting": true, - "treesize_accounting": true, - }) - Expect(err).ToNot(HaveOccurred()) - - lu = lookup.New(metadata.NewXattrsBackend(o.Root, cache.Config{}), o, &timemanager.Manager{}) - pmock = &mocks.PermissionsChecker{} - - cs3permissionsclient = &mocks.CS3PermissionsClient{} - pool.RemoveSelector("PermissionsSelector" + "any") - permissionsSelector = pool.GetSelector[cs3permissions.PermissionsAPIClient]( - "PermissionsSelector", - "any", - func(cc grpc.ClientConnInterface) cs3permissions.PermissionsAPIClient { - return cs3permissionsclient - }, - ) - bs = &treemocks.Blobstore{} - - // create space uses CheckPermission endpoint - cs3permissionsclient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&cs3permissions.CheckPermissionResponse{ - Status: &v1beta11.Status{Code: v1beta11.Code_CODE_OK}, - }, nil).Times(1) - - // for this test we don't care about permissions - pmock.On("AssemblePermissions", mock.Anything, mock.Anything). - Return(&provider.ResourcePermissions{ - Stat: true, - GetQuota: true, - InitiateFileUpload: true, - ListContainer: true, - ListFileVersions: true, - }, nil) - - // setup fs - pub, con = make(chan interface{}), make(chan interface{}) - tree := tree.New(lu, bs, o, store.Create(), &zerolog.Logger{}) - - aspects := aspects.Aspects{ - Lookup: lu, - Tree: tree, - Permissions: permissions.NewPermissions(pmock, permissionsSelector), - EventStream: stream.Chan{pub, con}, - Trashbin: &DecomposedfsTrashbin{}, - } - fs, err = New(o, aspects, &zerolog.Logger{}) - Expect(err).ToNot(HaveOccurred()) - - resp, err := fs.CreateStorageSpace(ctx, &provider.CreateStorageSpaceRequest{Owner: user, Type: "personal"}) - Expect(err).ToNot(HaveOccurred()) - Expect(resp.Status.Code).To(Equal(v1beta11.Code_CODE_OK)) - resID, err := storagespace.ParseID(resp.StorageSpace.Id.OpaqueId) - Expect(err).ToNot(HaveOccurred()) - ref.ResourceId = &resID - - bs.On("Upload", mock.AnythingOfType("*node.Node"), mock.AnythingOfType("string"), mock.Anything). - Return(nil). - Run(func(args mock.Arguments) { - n := args.Get(0).(*node.Node) - data, err := os.ReadFile(args.Get(1).(string)) - Expect(err).ToNot(HaveOccurred()) - Expect(len(data)).To(Equal(int(n.Blobsize))) - }) - - // start upload of a file - uploadIds, err := fs.InitiateUpload(ctx, ref, 10, map[string]string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(uploadIds)).To(Equal(2)) - Expect(uploadIds["simple"]).ToNot(BeEmpty()) - Expect(uploadIds["tus"]).ToNot(BeEmpty()) - - uploadRef := &provider.Reference{Path: "/" + uploadIds["simple"]} - - _, err = fs.Upload(ctx, storage.UploadRequest{ - Ref: uploadRef, - Body: io.NopCloser(bytes.NewReader(firstContent)), - Length: int64(len(firstContent)), - }, nil) - Expect(err).ToNot(HaveOccurred()) - - uploadID = uploadIds["simple"] - - // wait for bytes received event - _, ok := (<-pub).(events.BytesReceived) - Expect(ok).To(BeTrue()) - - // blobstore not called yet - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 0) - }) - - AfterEach(func() { - if o.Root != "" { - os.RemoveAll(o.Root) - } - close(pub) - close(con) - }) - - When("the uploaded file is new", func() { - It("succeeds eventually", func() { - // node is created - resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(1)) - - item := resources[0] - Expect(item.Path).To(Equal(ref.Path)) - Expect(utils.ReadPlainFromOpaque(item.Opaque, "status")).To(Equal("processing")) - - succeedPostprocessing(uploadID) - - // blobstore called now - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 1) - - // node ready - resources, err = fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(1)) - - item = resources[0] - Expect(item.Path).To(Equal(ref.Path)) - Expect(utils.ReadPlainFromOpaque(item.Opaque, "status")).To(BeEmpty()) - - }) - - It("deletes node and bytes when instructed", func() { - // node is created - resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(1)) - - item := resources[0] - Expect(item.Path).To(Equal(ref.Path)) - Expect(utils.ReadPlainFromOpaque(item.Opaque, "status")).To(Equal("processing")) - - // bytes are in dedicated path - _, err = os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - Expect(err).To(BeNil()) - - failPostprocessing(uploadID, events.PPOutcomeDelete) - - // blobstore still not called now - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 0) - - // node gone - resources, err = fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(0)) - - // bytes gone - _, err = os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - Expect(err).ToNot(BeNil()) - }) - - It("releases the quota and removes the node when the node metadata is unreadable", func() { - // node is created and the optimistic size has been propagated - resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(1)) - Expect(parentSize()).To(Equal(len(firstContent))) - - // simulate an orphaned node: the node file is still there but its - // metadata is gone, e.g. because an ancestor was trashed while the - // upload was in flight. Reading the node now fails. Purge instead of - // removing the file directly, so the cached attributes go as well. - nodePath := lu.InternalPath(ref.GetResourceId().GetSpaceId(), resources[0].GetId().GetOpaqueId()) - Expect(lu.MetadataBackend().Purge(ctx, nodePath)).To(Succeed()) - _, err = node.ReadNode(ctx, lu, ref.GetResourceId().GetSpaceId(), resources[0].GetId().GetOpaqueId(), false, nil, true) - Expect(err).To(HaveOccurred(), "node should be unreadable after purging its metadata") - - // No UploadReady event is published for an orphaned session: there is - // no node left to report on. Wait for the bytes to be cleaned up - // instead of for an event that will never arrive. - con <- events.PostprocessingFinished{ - UploadID: uploadID, - Outcome: events.PPOutcomeContinue, - } - Eventually(func() bool { - _, err := os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - return err != nil - }).Should(BeTrue(), "the upload bytes should be cleaned up") - - // the blob was never written - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 0) - - // the orphaned node is gone ... - _, err = os.Stat(nodePath) - Expect(err).ToNot(BeNil()) - - // ... and most importantly the quota has been released - Eventually(parentSize).Should(Equal(0)) - }) - - It("deletes node and keeps the bytes when instructed", func() { - // node is created - resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(1)) - - item := resources[0] - Expect(item.Path).To(Equal(ref.Path)) - Expect(utils.ReadPlainFromOpaque(item.Opaque, "status")).To(Equal("processing")) - - // bytes are in dedicated path - _, err = os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - Expect(err).To(BeNil()) - - failPostprocessing(uploadID, events.PPOutcomeAbort) - - // blobstore still not called now - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 0) - - // node gone - resources, err = fs.ListFolder(ctx, rootRef, []string{}, []string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(resources)).To(Equal(0)) - - // bytes are still here - _, err = os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - Expect(err).To(BeNil()) - }) - }) - - When("the uploaded file creates a new version", func() { - JustBeforeEach(func() { - succeedPostprocessing(uploadID) - - // make sure there is no version yet - revs, err := fs.ListRevisions(ctx, ref) - Expect(err).To(BeNil()) - Expect(len(revs)).To(Equal(0)) - - // upload again - uploadIds, err := fs.InitiateUpload(ctx, ref, 10, map[string]string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(uploadIds)).To(Equal(2)) - Expect(uploadIds["simple"]).ToNot(BeEmpty()) - Expect(uploadIds["tus"]).ToNot(BeEmpty()) - - uploadRef := &provider.Reference{Path: "/" + uploadIds["simple"]} - - _, err = fs.Upload(ctx, storage.UploadRequest{ - Ref: uploadRef, - Body: io.NopCloser(bytes.NewReader(firstContent)), - Length: int64(len(firstContent)), - }, nil) - Expect(err).ToNot(HaveOccurred()) - - uploadID = uploadIds["simple"] - - // wait for bytes received event - _, ok := (<-pub).(events.BytesReceived) - Expect(ok).To(BeTrue()) - - // version already created - revs, err = fs.ListRevisions(ctx, ref) - Expect(err).To(BeNil()) - Expect(len(revs)).To(Equal(1)) - - // at this stage: blobstore called once for the original file - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 1) - - }) - - It("succeeds eventually, creating a new version", func() { - succeedPostprocessing(uploadID) - - // version still existing - revs, err := fs.ListRevisions(ctx, ref) - Expect(err).To(BeNil()) - Expect(len(revs)).To(Equal(1)) - - // blobstore now called twice - for original file and new version - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 2) - - // bytes are gone from upload path - _, err = os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - Expect(err).ToNot(BeNil()) - }) - - It("removes new version and restores old one when instructed", func() { - _, status, _ := fileStatus() - Expect(status).To(Equal("processing")) - - failPostprocessing(uploadID, events.PPOutcomeDelete) - - _, status, _ = fileStatus() - Expect(status).To(Equal("")) - - // version gone now - revs, err := fs.ListRevisions(ctx, ref) - Expect(err).To(BeNil()) - Expect(len(revs)).To(Equal(0)) - - // bytes are removed from upload path - _, err = os.Stat(filepath.Join(o.Root, "uploads", uploadID)) - Expect(err).ToNot(BeNil()) - - // blobstore still called only once for the original file - bs.AssertNumberOfCalls(GinkgoT(), "Upload", 1) - }) - - }) - When("Two uploads are processed in parallel", func() { - var secondUploadID string - - JustBeforeEach(func() { - // upload again - uploadIds, err := fs.InitiateUpload(ctx, ref, 20, map[string]string{}) - Expect(err).ToNot(HaveOccurred()) - Expect(len(uploadIds)).To(Equal(2)) - Expect(uploadIds["simple"]).ToNot(BeEmpty()) - Expect(uploadIds["tus"]).ToNot(BeEmpty()) - - uploadRef := &provider.Reference{Path: "/" + uploadIds["simple"]} - - _, err = fs.Upload(ctx, storage.UploadRequest{ - Ref: uploadRef, - Body: io.NopCloser(bytes.NewReader(secondContent)), - Length: int64(len(secondContent)), - }, nil) - Expect(err).ToNot(HaveOccurred()) - - secondUploadID = uploadIds["simple"] - - // wait for bytes received event - _, ok := (<-pub).(events.BytesReceived) - Expect(ok).To(BeTrue()) - }) - - It("doesn't remove processing status when first upload is finished", func() { - succeedPostprocessing(uploadID) - - _, status, _ := fileStatus() - // check processing status - Expect(status).To(Equal("processing")) - }) - - It("removes processing status when second upload is finished, even if first isn't", func() { - succeedPostprocessing(secondUploadID) - - _, status, _ := fileStatus() - Expect(status).To(Equal("")) - }) - - It("correctly calculates the size when the second upload is finished, even if first is deleted", func() { - succeedPostprocessing(secondUploadID) - - _, status, size := fileStatus() - Expect(status).To(Equal("")) - // size should match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should match second upload as well - Expect(parentSize()).To(Equal(len(secondContent))) - - failPostprocessing(uploadID, events.PPOutcomeDelete) - - // check processing status - _, _, size = fileStatus() - // size should still match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should still match second upload as well - Expect(parentSize()).To(Equal(len(secondContent))) - }) - - It("the first can succeed before the second succeeds", func() { - succeedPostprocessing(uploadID) - - _, status, size := fileStatus() - // check processing status - Expect(status).To(Equal("processing")) - // size should match the second upload - Expect(size).To(Equal((len(secondContent)))) - - // parent size should match the second upload - Expect(parentSize()).To(Equal(len(secondContent))) - - succeedPostprocessing(secondUploadID) - - // check processing status has been removed - _, status, size = fileStatus() - Expect(status).To(Equal("")) - - // size should still match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should still match second upload - Expect(parentSize()).To(Equal(len(secondContent))) - - // file should have one revision - Expect(revisionCount()).To(Equal(1)) - }) - - It("the first can succeed after the second succeeds", func() { - succeedPostprocessing(secondUploadID) - - _, status, size := fileStatus() - // check processing status has been removed because the most recent upload finished and can be downloaded - Expect(status).To(Equal("")) - // size should match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should match second upload as well - Expect(parentSize()).To(Equal(len(secondContent))) - - succeedPostprocessing(uploadID) - - _, status, size = fileStatus() - // check processing status is still unset - Expect(status).To(Equal("")) - // size should still match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should still match second upload - Expect(parentSize()).To(Equal(len(secondContent))) - - // file should have one revision - Expect(revisionCount()).To(Equal(1)) - }) - - It("the first can succeed before the second fails", func() { - succeedPostprocessing(uploadID) - - _, status, size := fileStatus() - // check processing status - Expect(status).To(Equal("processing")) - // size should match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should match the second upload - Expect(parentSize()).To(Equal(len(secondContent))) - - failPostprocessing(secondUploadID, events.PPOutcomeDelete) - - _, status, size = fileStatus() - // check processing status has been removed - Expect(status).To(Equal("")) - // size should match the first upload - Expect(size).To(Equal(len(firstContent))) - - // parent size should match first upload - Expect(parentSize()).To(Equal(len(firstContent))) - - // file should not have any revisions - Expect(revisionCount()).To(Equal(0)) - }) - - It("the first can succeed after the second fails", func() { - failPostprocessing(secondUploadID, events.PPOutcomeDelete) - - _, _, size := fileStatus() - // check processing status has not been unset - // FIXME we need to fall back to the previous processing id - // Expect(status).To(Equal("processing")) - // size should match the first upload - Expect(size).To(Equal(len(firstContent))) - - // parent size should match first upload as well - Expect(parentSize()).To(Equal(len(firstContent))) - - succeedPostprocessing(uploadID) - - _, status, size := fileStatus() - // check processing status is now unset - Expect(status).To(Equal("")) - // size should still match the first upload - Expect(size).To(Equal(len(firstContent))) - - // parent size should still match first upload - Expect(parentSize()).To(Equal(len(firstContent))) - - // file should not have any revisions - Expect(revisionCount()).To(Equal(0)) - }) - - It("the first can fail before the second succeeds", func() { - failPostprocessing(uploadID, events.PPOutcomeDelete) - - _, status, size := fileStatus() - // check processing status - Expect(status).To(Equal("processing")) - // size should match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should match second upload as well - Expect(parentSize()).To(Equal(len(secondContent))) - - succeedPostprocessing(secondUploadID) - - _, status, size = fileStatus() - // check processing status has been removed - Expect(status).To(Equal("")) - // size should still match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should still match second upload - Expect(parentSize()).To(Equal(len(secondContent))) - - // file should not have any revisions - // FIXME we need to delete the revision - // Expect(revisionCount()).To(Equal(0)) - }) - - It("the first can fail after the second succeeds", func() { - succeedPostprocessing(secondUploadID) - - _, status, size := fileStatus() - // check processing status has been removed because the most recent upload finished and can be downloaded - Expect(status).To(Equal("")) - // size should match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should match second upload as well - Expect(parentSize()).To(Equal(len(secondContent))) - - failPostprocessing(uploadID, events.PPOutcomeDelete) - - _, status, size = fileStatus() - // check processing status is still unset - Expect(status).To(Equal("")) - // size should still match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should still match second upload - Expect(parentSize()).To(Equal(len(secondContent))) - - // file should not have any revisions - // FIXME we need to delete the revision - // Expect(revisionCount()).To(Equal(0)) - }) - - It("the first can fail before the second fails", func() { - failPostprocessing(uploadID, events.PPOutcomeDelete) - - _, status, size := fileStatus() - // check processing status - Expect(status).To(Equal("processing")) - // size should match the second upload - Expect(size).To(Equal(len(secondContent))) - - // parent size should match second upload as well - Expect(parentSize()).To(Equal(len(secondContent))) - - failPostprocessing(secondUploadID, events.PPOutcomeDelete) - - // check file has been removed - // if all uploads have been processed with outcome delete -> delete the file - // exists, _, _ := fileStatus() - // FIXME this should be false, but we are not deleting the resource - // Expect(exists).To(BeFalse()) - - // parent size should be 0 - // FIXME we are not correctly reverting the sizediff - // Expect(parentSize()).To(Equal(0)) - }) - - It("the first can fail after the second fails", func() { - failPostprocessing(secondUploadID, events.PPOutcomeDelete) - - _, status, size := fileStatus() - // check processing status has been removed because the most recent upload finished and can be downloaded - Expect(status).To(Equal("")) - // size should match the first upload - Expect(size).To(Equal(len(firstContent))) - - // parent size should match second first as well - Expect(parentSize()).To(Equal(len(firstContent))) - - failPostprocessing(uploadID, events.PPOutcomeDelete) - - // check file has been removed - // if all uploads have been processed with outcome delete -> delete the file - // exists, _, _ := fileStatus() - // FIXME this should be false, but we are not deleting the resource - // Expect(exists).To(BeFalse()) - - // parent size should be 0 - // FIXME we are not correctly reverting the sizediff - // Expect(parentSize()).To(Equal(0)) - }) - }) -}) diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index d459082f439..f6f236137b0 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -502,13 +502,6 @@ func (c *coordinator) publishBytesReceived(ctx context.Context, session Session) } executant := session.Executant() - var impersonating *user.User - if u, ok := ctxpkg.ContextGetUser(ctx); ok && utils.ExistsInOpaque(u.GetOpaque(), "impersonating-user") { - impersonating = &user.User{} - if err := utils.ReadJSONFromOpaque(u.GetOpaque(), "impersonating-user", impersonating); err != nil { - return err - } - } return events.Publish(ctx, c.pub, events.BytesReceived{ UploadID: session.ID(), @@ -524,10 +517,27 @@ func (c *coordinator) publishBytesReceived(ctx context.Context, session Session) }, Filename: session.Filename(), Filesize: uint64(session.Size()), - ImpersonatingUser: impersonating, + ImpersonatingUser: impersonatingUser(ctx), }) } +// impersonatingUser returns the user being acted for, when the request runs on a +// borrowed identity: public link and OCM tokens authenticate as the share owner +// and record the real actor in the user's opaque. Upload events carry it so the +// activity feed can attribute the upload to whoever actually performed it. +func impersonatingUser(ctx context.Context) *user.User { + u, ok := ctxpkg.ContextGetUser(ctx) + if !ok || !utils.ExistsInOpaque(u.GetOpaque(), "impersonating-user") { + return nil + } + impersonating := &user.User{} + if err := utils.ReadJSONFromOpaque(u.GetOpaque(), "impersonating-user", impersonating); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Msg("could not read impersonating user") + return nil + } + return impersonating +} + // touchAndMark creates the node for new files (via the public TouchFile, since // CommitUpload requires an existing node) and marks it as processing. TouchFile // mints the real node id, so we overwrite the id minted at initiate. @@ -592,8 +602,9 @@ func (c *coordinator) prepare(ctx context.Context, session Session) error { } // Persisted, not just held: on the async path the commit happens in another - // process, which can only learn the size to revert by reading it back. + // process, which can only learn these by reading them back. session.SetSizeDiff(prepared.SizeDiff) + session.SetVersionCreated(prepared.VersionCreated) if err := session.Persist(ctx); err != nil { c.rollbackPrepared(ctx, session, prepared.SizeDiff) return err @@ -609,10 +620,18 @@ func (c *coordinator) finishSync(ctx context.Context, session Session) (*provide c.rollbackPrepared(ctx, session, session.SizeDiff()) return nil, err } + // The scan verdict rides along with the bytes so the driver records it in the + // same operation that commits them: a committed blob then always carries the + // verdict it was cleared under, with no window where one exists without the + // other. It is empty on the inline path, which never scans. + scanResult, scanDate := session.ScanData() + // CommitUpload does not own the body; we opened it, so we close it. err = c.fs.CommitUpload(ctx, &ref, session.ID(), storage.UploadSource{ - Body: f, - Length: session.Size(), + Body: f, + Length: session.Size(), + ScanResult: scanResult, + ScanDate: scanDate, }) f.Close() if err != nil { @@ -705,9 +724,11 @@ func (c *coordinator) publishUploadReady(ctx context.Context, session Session, r ExecutingUser: &user.User{ Id: &executant, }, - FileRef: c.uploadRef(session), - ResourceID: ri.GetId(), - Timestamp: utils.TSNow(), + FileRef: c.uploadRef(session), + ResourceID: ri.GetId(), + Timestamp: utils.TSNow(), + IsVersion: session.VersionCreated(), + ImpersonatingUser: impersonatingUser(ctx), }); err != nil { appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("failed to publish UploadReady event") } diff --git a/pkg/upload/postprocessing.go b/pkg/upload/postprocessing.go index 37be0897d27..16d768c30a5 100644 --- a/pkg/upload/postprocessing.go +++ b/pkg/upload/postprocessing.go @@ -231,6 +231,7 @@ func (c *coordinator) publishUploadFailed(ctx context.Context, session Session, OpaqueId: session.NodeID(), }, Timestamp: utils.TSNow(), + IsVersion: session.VersionCreated(), ImpersonatingUser: ev.ImpersonatingUser, }); err != nil { appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("failed to publish UploadReady event") diff --git a/pkg/upload/session.go b/pkg/upload/session.go index 9249a61a824..d712554af63 100644 --- a/pkg/upload/session.go +++ b/pkg/upload/session.go @@ -71,6 +71,8 @@ type Session interface { SetChecksums(sha1, md5, adler32 []byte) SizeDiff() int64 SetSizeDiff(d int64) + VersionCreated() bool + SetVersionCreated(v bool) Metadata() map[string]string Persist(ctx context.Context) error Cleanup(cleanBin, cleanInfo bool) @@ -272,6 +274,18 @@ func (s *FileSession) SetSizeDiff(d int64) { s.info.MetaData["sizeDiff"] = strconv.FormatInt(d, 10) } +// VersionCreated reports whether this upload superseded existing content, which +// UploadReady consumers use to tell an overwrite from a new file. +func (s *FileSession) VersionCreated() bool { + return s.info.MetaData["versionCreated"] == "true" +} + +// SetVersionCreated records what PrepareUpload reported. Persisted for the same +// reason as the size diff: on the async path the commit runs in another process. +func (s *FileSession) SetVersionCreated(v bool) { + s.info.MetaData["versionCreated"] = strconv.FormatBool(v) +} + // SetScanData stores AV scan results on the session. func (s *FileSession) SetScanData(result string, date time.Time) { s.info.MetaData["scanResult"] = result diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 20ff82e1d59..89aae8a6564 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -29,6 +29,7 @@ import ( "io" "os" "path/filepath" + "time" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1" @@ -66,6 +67,13 @@ import ( . "github.com/onsi/gomega" ) +// parallelVerdict is one postprocessing result in an ordering table: which of the +// two staged uploads it is for, and whether the scan passed. +type parallelVerdict struct { + second bool + ok bool +} + var _ = Describe("Async uploads via the coordinator", func() { var ( ref = &provider.Reference{ @@ -139,10 +147,14 @@ var _ = Describe("Async uploads via the coordinator", func() { return id } + // uploadReady is what the last succeedPostprocessing announced. + uploadReady events.UploadReady + succeedPostprocessing = func(id string) { con <- events.PostprocessingFinished{UploadID: id, Outcome: events.PPOutcomeContinue} ev, ok := (<-pub).(events.UploadReady) Expect(ok).To(BeTrue()) + uploadReady = ev Expect(ev.Failed).To(BeFalse()) Expect(ev.ResourceID).ToNot(BeNil()) Expect(ev.ResourceID.OpaqueId).ToNot(BeEmpty()) @@ -339,6 +351,16 @@ var _ = Describe("Async uploads via the coordinator", func() { secondUploadID = upload(secondContent) }) + // The activity feed reads IsVersion to say "updated" rather than "created", + // so an overwrite reported as a new file is logged as the wrong action. + It("announces the overwrite as a version", func() { + Expect(uploadReady.IsVersion).To(BeFalse(), "the first upload created the file") + + succeedPostprocessing(secondUploadID) + + Expect(uploadReady.IsVersion).To(BeTrue(), "the second upload overwrote it") + }) + It("succeeds eventually, creating a new version", func() { succeedPostprocessing(secondUploadID) @@ -382,32 +404,73 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(status).To(BeEmpty()) }) - It("ends with the content of the last upload to finish", func() { - succeedPostprocessing(uploadID) - succeedPostprocessing(secondUploadID) + // Scans run concurrently, so the two verdicts can come back in either order. + // What the file ends up containing must depend on which uploads succeeded, + // never on which verdict happened to arrive first. Each entry runs the same + // two uploads and varies only the arrival order and the outcomes. + // + // revisions is -1 where the resulting revision count is a known gap rather + // than a defined result: a deleted upload leaves its revision behind. + DescribeTable("converges on the same state whichever verdict arrives first", + func(verdicts []parallelVerdict, expected []byte, revisions int) { + for _, v := range verdicts { + id := uploadID + if v.second { + id = secondUploadID + } + if v.ok { + succeedPostprocessing(id) + } else { + failPostprocessing(id, events.PPOutcomeDelete) + } + } + + exists, _, size := fileStatus() + Expect(exists).To(BeTrue()) + Expect(size).To(Equal(len(expected))) + Expect(parentSize()).To(Equal(len(expected))) + if revisions >= 0 { + Expect(revisionCount()).To(Equal(revisions)) + } + }, + // Revision counts start from 1, not 0: the setup above already committed a + // version before staging these two uploads. + Entry("both succeed, first verdict first", + []parallelVerdict{{ok: true}, {second: true, ok: true}}, secondContent, 2), + Entry("both succeed, second verdict first", + []parallelVerdict{{second: true, ok: true}, {ok: true}}, secondContent, 2), + Entry("second is deleted after the first succeeded", + []parallelVerdict{{ok: true}, {second: true}}, firstContent, 1), + Entry("second is deleted before the first succeeded", + []parallelVerdict{{second: true}, {ok: true}}, firstContent, 1), + Entry("first is deleted before the second succeeded", + []parallelVerdict{{}, {second: true, ok: true}}, secondContent, -1), + Entry("first is deleted after the second succeeded", + []parallelVerdict{{second: true, ok: true}, {}}, secondContent, -1), + ) + }) - _, _, size := fileStatus() - Expect(size).To(Equal(len(secondContent))) - Expect(parentSize()).To(Equal(len(secondContent))) - }) + // The scan verdict has to end up on the resource, not only on the upload + // session: the session is deleted when the upload finishes, while the resource + // is what every later stat reads. Clients surface it as a scantime. + When("the antivirus step reports a result", func() { + It("records the scan on the committed resource", func() { + scanned := time.Date(2026, 7, 30, 12, 0, 0, 0, time.UTC) + con <- events.PostprocessingStepFinished{ + UploadID: uploadID, + FinishedStep: events.PPStepAntivirus, + Result: events.VirusscanResult{ + Description: "", + Scandate: scanned, + }, + } - It("keeps the first upload when the second is deleted", func() { succeedPostprocessing(uploadID) - failPostprocessing(secondUploadID, events.PPOutcomeDelete) - exists, _, size := fileStatus() - Expect(exists).To(BeTrue(), "the first upload must survive the second being deleted") - Expect(size).To(Equal(len(firstContent))) - Expect(parentSize()).To(Equal(len(firstContent))) - }) - - It("keeps the second upload when the first is deleted", func() { - failPostprocessing(uploadID, events.PPOutcomeDelete) - succeedPostprocessing(secondUploadID) - - exists, _, size := fileStatus() - Expect(exists).To(BeTrue()) - Expect(size).To(Equal(len(secondContent))) + ri, err := fs.GetMD(ctx, ref, nil, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(utils.ReadPlainFromOpaque(ri.Opaque, "scantime")).To(Equal(scanned.Format(time.RFC3339Nano)), + "a clean scan must still be recorded, and only the node keeps it after the session is gone") }) }) @@ -453,6 +516,25 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(size).To(Equal(len(firstContent))) Expect(stagedBytesExist(id)).To(BeFalse()) }) + + // Public link and OCM uploads authenticate as the share owner and carry the + // real actor in the user's opaque. The activity feed attributes the upload to + // them, so dropping it credits the owner for someone else's upload. + It("reports the impersonated user it was uploaded on behalf of", func() { + impersonated := &userpb.User{ + Id: &userpb.UserId{OpaqueId: "public-link-actor", Idp: "idp"}, + Username: "actor", + } + owner := &userpb.User{Id: user.Id, Username: user.Username} + owner.Opaque = utils.AppendJSONToOpaque(nil, "impersonating-user", impersonated) + ctx = ruser.ContextSetUser(ctx, owner) + + initiateAndUpload(firstContent) + + ev, ok := (<-pub).(events.UploadReady) + Expect(ok).To(BeTrue()) + Expect(ev.ImpersonatingUser.GetId().GetOpaqueId()).To(Equal("public-link-actor")) + }) }) When("the coordinator serves a specific storage", func() { From 5608b802913ef45958a143d4a54fc492634f6c0e Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Fri, 31 Jul 2026 11:08:40 +0200 Subject: [PATCH 07/15] fix acceptance tests --- pkg/storage/fs/nextcloud/nextcloud.go | 53 ++++++++++++++++--- .../fs/nextcloud/nextcloud_server_mock.go | 28 ++++++++-- .../utils/decomposedfs/decomposedfs.go | 7 +++ .../fixtures/storageprovider-nextcloud.toml | 3 ++ 4 files changed, 79 insertions(+), 12 deletions(-) diff --git a/pkg/storage/fs/nextcloud/nextcloud.go b/pkg/storage/fs/nextcloud/nextcloud.go index 94eca439711..c736972986d 100644 --- a/pkg/storage/fs/nextcloud/nextcloud.go +++ b/pkg/storage/fs/nextcloud/nextcloud.go @@ -21,7 +21,6 @@ package nextcloud import ( "context" "encoding/json" - "fmt" "io" "net/http" "net/url" @@ -274,7 +273,33 @@ func (nc *StorageDriver) CreateDir(ctx context.Context, ref *provider.Reference) // TouchFile as defined in the storage.FS interface func (nc *StorageDriver) TouchFile(ctx context.Context, ref *provider.Reference, markprocessing bool, mtime string) (*storage.TouchFileResult, error) { - return nil, fmt.Errorf("unimplemented: TouchFile") + type paramsObj struct { + Ref *provider.Reference `json:"ref"` + MarkProcessing bool `json:"markprocessing"` + MTime string `json:"mtime"` + } + bodyStr, err := json.Marshal(¶msObj{Ref: ref, MarkProcessing: markprocessing, MTime: mtime}) + if err != nil { + return nil, err + } + log := appctx.GetLogger(ctx) + log.Info().Msgf("TouchFile %s", bodyStr) + + if _, _, err := nc.do(ctx, Action{"TouchFile", string(bodyStr)}); err != nil { + return nil, err + } + + // The upload coordinator needs the id of the node that was just created, and + // the touch call itself answers with an empty body, so read it back. + md, err := nc.GetMD(ctx, ref, []string{}, []string{}) + if err != nil { + return nil, err + } + return &storage.TouchFileResult{ + ResourceID: md.GetId(), + SpaceID: md.GetId().GetSpaceId(), + SpaceOwner: md.GetOwner(), + }, nil } // Delete as defined in the storage.FS interface @@ -409,14 +434,26 @@ func (nc *StorageDriver) InitiateUpload(ctx context.Context, ref *provider.Refer return respMap, err } -// MarkProcessing as defined in the storage.FS interface -func (nc *StorageDriver) MarkProcessing(ctx context.Context, ref *provider.Reference, processing bool, sessionID string) error { - return errtypes.NotSupported("nextcloud: mark processing not supported") +// MarkProcessing as defined in the storage.FS interface. +// +// The flag advertises an upload that has been announced but not yet committed, so +// that a stat of the file can say so. Nextcloud owns its own metadata and has +// nowhere to record it, and it does not postprocess uploads, so there is no window +// to report: this is a no-op rather than an error, which would fail the upload. +func (nc *StorageDriver) MarkProcessing(_ context.Context, _ *provider.Reference, _ bool, _ string) error { + return nil } -// CommitUpload as defined in the storage.FS interface -func (nc *StorageDriver) CommitUpload(_ context.Context, _ *provider.Reference, _ string, _ storage.UploadSource) error { - return errtypes.NotSupported("nextcloud: commit upload not supported") +// CommitUpload as defined in the storage.FS interface. +// +// The caller owns source.Body and closes it, so this must not. +func (nc *StorageDriver) CommitUpload(ctx context.Context, ref *provider.Reference, _ string, source storage.UploadSource) error { + if source.Body == nil { + return errtypes.BadRequest("nextcloud: source body is nil") + } + // The remote records the scan verdict itself if it scans at all, so + // source.ScanResult/ScanDate have nowhere to go here. + return nc.doUpload(ctx, ref.GetPath(), source.Body) } func (nc *StorageDriver) PrepareUpload(_ context.Context, _ *provider.Reference, _ string, info storage.UploadInfo) (*storage.PrepareUploadResult, error) { diff --git a/pkg/storage/fs/nextcloud/nextcloud_server_mock.go b/pkg/storage/fs/nextcloud/nextcloud_server_mock.go index b42ec12a6bf..f951390129b 100644 --- a/pkg/storage/fs/nextcloud/nextcloud_server_mock.go +++ b/pkg/storage/fs/nextcloud/nextcloud_server_mock.go @@ -25,6 +25,7 @@ import ( "net" "net/http" "net/http/httptest" + "regexp" "strings" ) @@ -52,6 +53,11 @@ const serverStateMetadata = "METADATA" var serverState = serverStateEmpty +// TouchFile carries the wall-clock mtime the upload was initiated with, so the +// request body differs on every run. Responses are keyed on the exact body, so +// the value is folded to a placeholder before lookup. +var mtimeRe = regexp.MustCompile(`"mtime":"[^"]*"`) + var responses = map[string]Response{ `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/AddGrant {"ref":{"path":"/subdir"},"g":{"grantee":{"type":1,"Id":{"UserId":{"opaque_id":"4c510ada-c86b-4815-8820-42cdf82c3d51"}}},"permissions":{"delete":true,"initiate_file_download":true,"move":true,"stat":true}}} EMPTY`: {200, ``, serverStateGrantAdded}, @@ -111,7 +117,20 @@ var responses = map[string]Response{ `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetPathByID {"storage_id":"00000000-0000-0000-0000-000000000000","opaque_id":"fileid-/some/path"} EMPTY`: {200, "/subdir", serverStateEmpty}, - `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetMD {"ref":{"path":"/file"},"mdKeys":null}`: {404, ``, serverStateEmpty}, + `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetMD {"ref":{"path":"/file"},"mdKeys":null}`: {404, ``, serverStateEmpty}, + // Initiating an upload now runs through the upload coordinator, which stats the + // target, checks the quota and stats the parent before it reaches the driver. + // The driver used to be called straight away, so none of these had an entry. + // They keep whichever state they matched: this mock selects responses by server + // state, and clearing it would strand the requests that follow. + `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetMD {"ref":{"path":"/file"},"mdKeys":[]} EMPTY`: {404, ``, serverStateEmpty}, + `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetQuota EMPTY`: {200, `{"totalBytes":456,"usedBytes":123}`, serverStateEmpty}, + `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetMD {"ref":{"path":"/"},"mdKeys":[]} EMPTY`: {200, `{"opaque":{},"type":2,"id":{"opaque_id":"fileid-/"},"checksum":{},"etag":"deadbeef","mime_type":"httpd/unix-directory","mtime":{"seconds":1234567890},"path":"/","permission_set":{"initiate_file_upload":true,"stat":true},"size":12345,"canonical_metadata":{},"arbitrary_metadata":{"metadata":{}}}`, serverStateEmpty}, + // A zero-length upload has no bytes to append, so it finishes inside + // InitiateUpload: the coordinator creates the node and stats it back for the id + // the touch minted. + `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/TouchFile {"ref":{"resource_id":{"opaque_id":"fileid-/"},"path":"file"},"markprocessing":false,"mtime":"MTIME"} EMPTY`: {200, ``, serverStateEmpty}, + `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/GetMD {"ref":{"resource_id":{"opaque_id":"fileid-/"},"path":"file"},"mdKeys":[]} EMPTY`: {200, `{"opaque":{},"type":1,"id":{"opaque_id":"fileid-/file"},"checksum":{},"etag":"deadbeef","mime_type":"text/plain","mtime":{"seconds":1234567890},"path":"/file","permission_set":{"initiate_file_upload":true,"stat":true},"size":0,"canonical_metadata":{},"arbitrary_metadata":{"metadata":{}}}`, serverStateEmpty}, `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/InitiateUpload {"ref":{"path":"/file"},"uploadLength":0,"metadata":{"providerID":""}}`: {200, `{"simple": "yes","tus": "yes"}`, serverStateEmpty}, `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/InitiateUpload {"ref":{"resource_id":{"storage_id":"f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c"},"path":"/versionedFile"},"uploadLength":1,"metadata":{}}`: {200, `{"simple": "yes","tus": "yes"}`, serverStateEmpty}, `POST /apps/sciencemesh/~f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c/api/storage/InitiateUpload {"ref":{"resource_id":{"storage_id":"f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c"},"path":"/versionedFile"},"uploadLength":2,"metadata":{}}`: {200, `{"simple": "yes","tus": "yes"}`, serverStateEmpty}, @@ -194,15 +213,16 @@ func GetNextcloudServerMock(called *[]string) http.Handler { if err != nil { panic("Error reading response into buffer") } - var key = fmt.Sprintf("%s %s %s", r.Method, r.URL, buf.String()) + body := mtimeRe.ReplaceAllString(buf.String(), `"mtime":"MTIME"`) + var key = fmt.Sprintf("%s %s %s", r.Method, r.URL, body) *called = append(*called, key) response := responses[key] if (response == Response{}) { - key = fmt.Sprintf("%s %s %s %s", r.Method, r.URL, buf.String(), serverState) + key = fmt.Sprintf("%s %s %s %s", r.Method, r.URL, body, serverState) response = responses[key] } if (response == Response{}) { - fmt.Printf("%s %s %s %s\n", r.Method, r.URL, buf.String(), serverState) + fmt.Printf("%s %s %s %s\n", r.Method, r.URL, body, serverState) response = Response{500, fmt.Sprintf("response not defined! %s", key), serverStateEmpty} } serverState = responses[key].newServerState diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index f649021650e..63e4f277e9c 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -237,6 +237,13 @@ func New(o *options.Options, aspects aspects.Aspects, log *zerolog.Logger) (stor o.DisableVersioning = true } + // Callers may disable versioning through either the options or the aspects: the + // posix driver builds its own aspects and only sets it there. Fold it into the + // options so code reading o.DisableVersioning sees it too. + if aspects.DisableVersioning { + o.DisableVersioning = true + } + fs := &Decomposedfs{ tp: aspects.Tree, lu: aspects.Lookup, diff --git a/tests/integration/grpc/fixtures/storageprovider-nextcloud.toml b/tests/integration/grpc/fixtures/storageprovider-nextcloud.toml index ef85bb5d060..7aed8872df2 100644 --- a/tests/integration/grpc/fixtures/storageprovider-nextcloud.toml +++ b/tests/integration/grpc/fixtures/storageprovider-nextcloud.toml @@ -3,6 +3,9 @@ address = "{{grpc_address}}" [grpc.services.storageprovider] driver = "nextcloud" +# The nextcloud driver has no local root to stage upload sessions under, so the +# upload directory has to be given explicitly or the service refuses to start. +upload_directory = "{{root}}/uploads" [grpc.services.storageprovider.drivers.nextcloud] endpoint = "http://localhost:8080/apps/sciencemesh/" From 6fa8e367be16a71674c59e0be561934a0ffdbff2 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Fri, 31 Jul 2026 15:37:34 +0200 Subject: [PATCH 08/15] fix: pin etcd to 3.6.13 to keep the go directive at 1.25.11 --- go.mod | 9 +++++---- go.sum | 13 +++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index d679d2dc405..104e0b038bf 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/owncloud/reva/v2 -go 1.26 +go 1.25.11 require ( bou.ke/monkey v1.0.2 @@ -84,7 +84,7 @@ require ( github.com/tus/tusd/v2 v2.10.0 github.com/wk8/go-ordered-map v1.0.0 go-micro.dev/v4 v4.11.0 - go.etcd.io/etcd/client/v3 v3.7.1 + go.etcd.io/etcd/client/v3 v3.6.13 go.opencensus.io v0.24.0 go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.69.0 go.opentelemetry.io/otel v1.44.0 @@ -148,6 +148,7 @@ require ( github.com/go-openapi/strfmt v0.26.2 // indirect github.com/go-task/slim-sprig/v3 v3.0.0 // indirect github.com/go-viper/mapstructure/v2 v2.5.0 // indirect + github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/go-tpm v0.9.8 // indirect @@ -204,8 +205,8 @@ require ( github.com/xrash/smetrics v0.0.0-20240521201337-686a1a2994c1 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect github.com/zeebo/xxh3 v1.1.0 // indirect - go.etcd.io/etcd/api/v3 v3.7.1 // indirect - go.etcd.io/etcd/client/pkg/v3 v3.7.1 // indirect + go.etcd.io/etcd/api/v3 v3.6.13 // indirect + go.etcd.io/etcd/client/pkg/v3 v3.6.13 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.44.0 // indirect go.opentelemetry.io/otel/metric v1.44.0 // indirect diff --git a/go.sum b/go.sum index 11edc6ab3e6..0822a8b86b5 100644 --- a/go.sum +++ b/go.sum @@ -271,6 +271,7 @@ github.com/gofrs/uuid/v5 v5.4.0 h1:EfbpCTjqMuGyq5ZJwxqzn3Cbr2d0rUZU7v5ycAk/e/0= github.com/gofrs/uuid/v5 v5.4.0/go.mod h1:CDOjlDMVAtN56jqyRUZh58JT31Tiw7/oQyEXZV+9bD8= github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ= github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zVXpSg4= +github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= @@ -700,12 +701,12 @@ github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= github.com/zeebo/xxh3 v1.1.0/go.mod h1:IisAie1LELR4xhVinxWS5+zf1lA4p0MW4T+w+W07F5s= -go.etcd.io/etcd/api/v3 v3.7.1 h1:KJG0/DcWGfe3Y1otDf/fsBf0TSSgpxZ5RO/L8SFt73E= -go.etcd.io/etcd/api/v3 v3.7.1/go.mod h1:8bXIpCMeV7E3/XL0Ix123ATn3dB+0V7d9zklHbB0m78= -go.etcd.io/etcd/client/pkg/v3 v3.7.1 h1:rKYsj3pRkR0eK3yjT3XOgrhqfmIfj9pzNgxjh7mfFv4= -go.etcd.io/etcd/client/pkg/v3 v3.7.1/go.mod h1:cnzZGIUzSfjEwLC6UBVsSXlEK1eepS/JUD7wE6PLRT0= -go.etcd.io/etcd/client/v3 v3.7.1 h1:0PEMMC0KuZmVIN+RAbdqfkZ45pYTgKVtmBEbRCvZFUg= -go.etcd.io/etcd/client/v3 v3.7.1/go.mod h1:ffNqALa8tRCYhYo1F9oR489y23K39Gz+BSR3ApAGYq0= +go.etcd.io/etcd/api/v3 v3.6.13 h1:AvHPZv15LYEe7tZDyFglv7xnbiuF6GMZpZqKpIzXTt0= +go.etcd.io/etcd/api/v3 v3.6.13/go.mod h1:X9+3gaKwzjlOxzo6TZ2u3b7HcHBcAL+Ph7EBPjI/VWk= +go.etcd.io/etcd/client/pkg/v3 v3.6.13 h1:7QeMOisYByx8dBA7/CKcwCaPWfjb5C0xpmrIov/8WyY= +go.etcd.io/etcd/client/pkg/v3 v3.6.13/go.mod h1:Dn2zUBOCu/6xYcd6iAjB7LgoY16OTQjDZfWHLwvuQj4= +go.etcd.io/etcd/client/v3 v3.6.13 h1:0E+9ZYGpMsi9KlOJVoCdONh9PUDawKDTy5mSNY8wOEI= +go.etcd.io/etcd/client/v3 v3.6.13/go.mod h1:rtVI3vwobljb8xlTGcp1Yhz7hBIuBWULXwB848kqJGw= go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU= go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8= go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= From 56e69a349e74602c7e6325d56ab8c60081b914e1 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Mon, 3 Aug 2026 10:59:58 +0200 Subject: [PATCH 09/15] fix: qualify uploaded file ids, name the executing user, and enforce quota for new files --- .../utils/decomposedfs/prepare_upload_test.go | 23 +++++++++ pkg/upload/coordinator.go | 30 +++++------ pkg/upload/session.go | 9 +++- pkg/upload/upload_async_test.go | 50 ++++++++++++++++++- 4 files changed, 93 insertions(+), 19 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/prepare_upload_test.go b/pkg/storage/utils/decomposedfs/prepare_upload_test.go index de4aeb64c59..a7d69026256 100644 --- a/pkg/storage/utils/decomposedfs/prepare_upload_test.go +++ b/pkg/storage/utils/decomposedfs/prepare_upload_test.go @@ -234,6 +234,29 @@ var _ = Describe("PrepareUpload", func() { _, ok := err.(errtypes.IsInsufficientStorage) Expect(ok).To(BeTrue(), "expected errtypes.InsufficientStorage, got %T: %v", err, err) }) + + // The coordinator's up-front check fails open whenever the executant cannot + // read the quota — an Uploader or Editor on someone else's share — and relies + // on this one instead. Skipping new files would let those uploads past both + // gates and answer 201 where the space is already full. + It("returns an error for a new file too, not just an overwrite", func() { + var gotOverwrite bool + var gotOldSize uint64 + original := node.CheckQuota + node.CheckQuota = func(_ context.Context, _ *node.Node, overwrite bool, oldSize, _ uint64) (bool, error) { + gotOverwrite, gotOldSize = overwrite, oldSize + return false, errtypes.InsufficientStorage("quota exceeded") + } + defer func() { node.CheckQuota = original }() + + info := storage.UploadInfo{NodeExisted: false, Size: 20} + _, err := env.Fs.PrepareUpload(env.Ctx, ref, "session-new", info) + Expect(err).To(HaveOccurred(), "a new file must be quota checked") + _, ok := err.(errtypes.IsInsufficientStorage) + Expect(ok).To(BeTrue(), "expected errtypes.InsufficientStorage, got %T: %v", err, err) + Expect(gotOverwrite).To(BeFalse(), "a new file is not an overwrite") + Expect(gotOldSize).To(BeZero(), "there is no previous blob to discount") + }) }) Context("quota exceeded on a new file", func() { diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index f6f236137b0..dd7dc5a43e2 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -501,15 +501,11 @@ func (c *coordinator) publishBytesReceived(ctx context.Context, session Session) return err } - executant := session.Executant() - return events.Publish(ctx, c.pub, events.BytesReceived{ - UploadID: session.ID(), - URL: url, - SpaceOwner: session.SpaceOwner(), - ExecutingUser: &user.User{ - Id: &executant, - }, + UploadID: session.ID(), + URL: url, + SpaceOwner: session.SpaceOwner(), + ExecutingUser: session.ExecutantUser(), ResourceID: &provider.ResourceId{ StorageId: session.ProviderID(), SpaceId: session.SpaceID(), @@ -667,6 +663,13 @@ func (c *coordinator) uploadedResourceInfo(ctx context.Context, session Session) ref := session.Reference() ri, err := c.fs.GetMD(ctx, &ref, nil, nil) if err == nil { + // Drivers do not know their own mount id; the storageprovider stamps it on + // the way out (addMissingStorageProviderID). This path answers from the + // dataprovider, which does not, so an unstamped id would reach the client + // as a two-part storageid-less string that later lookups cannot resolve. + if ri.GetId().GetStorageId() == "" { + ri.Id.StorageId = session.ProviderID() + } return ri } appctx.GetLogger(ctx).Debug().Err(err).Str("uploadid", session.ID()).Msg("could not stat uploaded resource") @@ -716,14 +719,11 @@ func (c *coordinator) publishUploadReady(ctx context.Context, session Session, r if c.pub == nil { return } - executant := session.Executant() if err := events.Publish(ctx, c.pub, events.UploadReady{ - UploadID: session.ID(), - Filename: session.Filename(), - SpaceOwner: session.SpaceOwner(), - ExecutingUser: &user.User{ - Id: &executant, - }, + UploadID: session.ID(), + Filename: session.Filename(), + SpaceOwner: session.SpaceOwner(), + ExecutingUser: session.ExecutantUser(), FileRef: c.uploadRef(session), ResourceID: ri.GetId(), Timestamp: utils.TSNow(), diff --git a/pkg/upload/session.go b/pkg/upload/session.go index d712554af63..597afd86b26 100644 --- a/pkg/upload/session.go +++ b/pkg/upload/session.go @@ -59,6 +59,7 @@ type Session interface { // Internal coordinator plumbing. Chunk() string BinPath() string + ExecutantUser() *userpb.User ProviderID() string SpaceID() string NodeID() string @@ -377,7 +378,7 @@ func (s *FileSession) Context(ctx context.Context) context.Context { sub := s.store.log.With().Int("pid", os.Getpid()).Logger() ctx = appctx.WithLogger(ctx, &sub) ctx = ctxpkg.ContextSetLockID(ctx, s.info.MetaData["lockid"]) - ctx = ctxpkg.ContextSetUser(ctx, s.executantUser()) + ctx = ctxpkg.ContextSetUser(ctx, s.ExecutantUser()) return ctxpkg.ContextSetInitiator(ctx, s.info.MetaData["initiatorid"]) } @@ -423,7 +424,11 @@ func (s *FileSession) infoPath() string { return fileSessionPath(s.store.root, s.info.ID) } -func (s *FileSession) executantUser() *userpb.User { +// ExecutantUser returns the full identity of the user who initiated the upload. +// Upload events must carry it rather than the bare id from Executant(): consumers +// read the display name straight off the event and do not look it up, so an +// id-only user reaches the activity feed with a blank name. +func (s *FileSession) ExecutantUser() *userpb.User { var o *typespb.Opaque _ = json.Unmarshal([]byte(s.info.Storage["UserOpaque"]), &o) var groups []string diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 89aae8a6564..3d568d1870f 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -29,6 +29,7 @@ import ( "io" "os" "path/filepath" + "strings" "time" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -91,7 +92,8 @@ var _ = Describe("Async uploads via the coordinator", func() { OpaqueId: "u-s-e-r-id", Type: userpb.UserType_USER_TYPE_PRIMARY, }, - Username: "username", + Username: "username", + DisplayName: "Display Name", } firstContent = []byte("0123456789") @@ -117,10 +119,16 @@ var _ = Describe("Async uploads via the coordinator", func() { // into response headers, so it is part of the contract, not a by-product. uploadedInfo *provider.ResourceInfo + // providerID stands in for the storageprovider's mount id, which reaches the + // coordinator as initiate metadata and must come back out on the file id. + providerID = "storage-users-mount" + // initiateAndUpload runs a full upload through the coordinator and returns // the session id, without asserting anything about what it published. initiateAndUpload = func(content []byte) string { - ids, err := coord.InitiateUpload(ctx, ref, int64(len(content)), map[string]string{}) + ids, err := coord.InitiateUpload(ctx, ref, int64(len(content)), map[string]string{ + "providerID": providerID, + }) Expect(err).ToNot(HaveOccurred()) Expect(ids["simple"]).ToNot(BeEmpty()) @@ -134,6 +142,9 @@ var _ = Describe("Async uploads via the coordinator", func() { return ids["simple"] } + // bytesReceived is what the last upload handed to postprocessing. + bytesReceived events.BytesReceived + // upload stages an upload and leaves it awaiting postprocessing. Only valid // on the async path: inline uploads publish UploadReady, not BytesReceived. upload = func(content []byte) string { @@ -143,6 +154,7 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(ok).To(BeTrue(), "expected BytesReceived: the upload must not commit before postprocessing") Expect(ev.UploadID).To(Equal(id)) Expect(ev.URL).ToNot(BeEmpty(), "postprocessing needs a URL to fetch the staged bytes from") + bytesReceived = ev return id } @@ -492,6 +504,40 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(uploadedInfo.GetEtag()).ToNot(BeEmpty()) Expect(uploadedInfo.GetId().GetOpaqueId()).ToNot(BeEmpty()) }) + + // Consumers read the display name straight off the event rather than looking + // it up: ocis activitylog takes its `u != nil` branch and skips the gateway + // fallback, so an id-only user renders an activity with a blank actor. + It("names the executing user on both upload events", func() { + Expect(bytesReceived.ExecutingUser.GetUsername()).To(Equal(user.GetUsername())) + Expect(bytesReceived.ExecutingUser.GetDisplayName()).To(Equal(user.GetDisplayName()), + "BytesReceived must carry the display name, not just the id") + Expect(bytesReceived.ExecutingUser.GetId().GetOpaqueId()).To(Equal(user.GetId().GetOpaqueId())) + + succeedPostprocessing(uploadID) + + Expect(uploadReady.ExecutingUser.GetDisplayName()).To(Equal(user.GetDisplayName()), + "UploadReady must carry it too — this is the one activitylog consumes") + Expect(uploadReady.ExecutingUser.GetUsername()).To(Equal(user.GetUsername())) + }) + + // Drivers leave StorageId empty and the storageprovider normally stamps it, + // but this path answers from the dataprovider. Without the mount id the + // formatted id loses a segment, and clients that feed it back in — graph + // item lookups, app-open URLs — no longer resolve it. + It("reports a fully qualified file id", func() { + Expect(uploadedInfo.GetId().GetStorageId()).To(Equal(providerID), + "the mount id must survive the round trip through the session") + + formatted := storagespace.FormatResourceID(uploadedInfo.GetId()) + Expect(strings.Count(formatted, "$")).To(Equal(1), "id must be storageid$spaceid!opaqueid: "+formatted) + Expect(strings.Count(formatted, "!")).To(Equal(1), "id must be storageid$spaceid!opaqueid: "+formatted) + + parsed, err := storagespace.ParseID(formatted) + Expect(err).ToNot(HaveOccurred()) + Expect(parsed.GetStorageId()).To(Equal(providerID)) + Expect(parsed.GetOpaqueId()).To(Equal(uploadedInfo.GetId().GetOpaqueId())) + }) }) When("postprocessing was never started", func() { From cc922a030d27f0782108bb83786177a2980736f1 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Mon, 3 Aug 2026 13:22:53 +0200 Subject: [PATCH 10/15] fix: purge the node through the driver when an upload is rejected --- pkg/upload/coordinator.go | 21 +++++++++++++++------ pkg/upload/upload_async_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index dd7dc5a43e2..ea9b7c9b9d9 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -747,15 +747,24 @@ func (c *coordinator) uploadRef(session Session) *provider.Reference { } } -// rollback unmarks processing, cleans up session files, and deletes the node if -// this upload created it (NodeExists=false at initiation). +// rollback undoes a finish that failed before PrepareUpload: it removes the node +// if this upload created it, unmarks processing, and drops the session files. +// +// Removal goes through the driver rather than the public Delete, which is +// permission-gated: an Uploader on someone else's share has no Delete permission +// on the file they just created, so a rejected upload would survive as an empty +// file. The size diff is zero because nothing was propagated yet. func (c *coordinator) rollback(ctx context.Context, session Session) { ref := session.Reference() - _ = c.fs.MarkProcessing(ctx, &ref, false, session.ID()) - session.Cleanup(true, true) - if !session.NodeExists() { - _, _ = c.fs.Delete(ctx, &ref) + // Before unmarking: RollbackUpload keys off the processing id to confirm the + // node is still this upload's, so unmarking first makes it a no-op. + if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), session.NodeExists(), 0); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") } + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + } + session.Cleanup(true, true) } // rollbackPrepared undoes a finish that failed after PrepareUpload succeeded: it diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 3d568d1870f..89e95555404 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -545,6 +545,31 @@ var _ = Describe("Async uploads via the coordinator", func() { startAsync = false }) + // A rejected upload must not leave the node it created behind. Removal has to + // go through the driver, not the permission-gated public Delete: the executant + // of a write-only share has no Delete permission on the file they just + // created, so an empty file would survive every rejected upload. The mock + // permissions here grant no Delete either, which is what reproduces it. + It("removes the node it created when the upload is rejected", func() { + ids, err := coord.InitiateUpload(ctx, ref, int64(len(firstContent)), map[string]string{ + "providerID": providerID, + "checksum": "md5 00000000000000000000000000000000", + }) + Expect(err).ToNot(HaveOccurred()) + + _, err = coord.Upload(ctx, storage.UploadRequest{ + Ref: &provider.Reference{Path: "/" + ids["simple"]}, + Body: io.NopCloser(bytes.NewReader(firstContent)), + Length: int64(len(firstContent)), + }, nil) + Expect(err).To(HaveOccurred(), "a checksum mismatch must be rejected") + + bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) + exists, _, _ := fileStatus() + Expect(exists).To(BeFalse(), "the rejected upload must not leave a node behind") + Expect(stagedBytesExist(ids["simple"])).To(BeFalse(), "staged bytes should be cleaned up") + }) + // The guarantee that keeps the two switches from drifting apart: with no // consumer running, nothing would ever arrive to finish a deferred upload, so // the coordinator must commit inline instead of staging and waiting forever. From 6730f672bf66609d1ce3427738c3140201372254 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Mon, 3 Aug 2026 14:34:48 +0200 Subject: [PATCH 11/15] fix: keep the target file when a rejected overwrite is rolled back --- pkg/upload/coordinator.go | 16 ++++++++++----- pkg/upload/upload_async_test.go | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index ea9b7c9b9d9..8610606a408 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -750,16 +750,22 @@ func (c *coordinator) uploadRef(session Session) *provider.Reference { // rollback undoes a finish that failed before PrepareUpload: it removes the node // if this upload created it, unmarks processing, and drops the session files. // -// Removal goes through the driver rather than the public Delete, which is +// An overwrite is left alone. At this point only touchAndMark has run, so the +// existing node still holds its own blob and metadata and there is nothing to +// undo; asking the driver to roll back would revert or purge content this upload +// never wrote. Only a node this upload brought into existence gets removed, and +// that goes through the driver rather than the public Delete, which is // permission-gated: an Uploader on someone else's share has no Delete permission // on the file they just created, so a rejected upload would survive as an empty // file. The size diff is zero because nothing was propagated yet. func (c *coordinator) rollback(ctx context.Context, session Session) { ref := session.Reference() - // Before unmarking: RollbackUpload keys off the processing id to confirm the - // node is still this upload's, so unmarking first makes it a no-op. - if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), session.NodeExists(), 0); err != nil { - appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") + if !session.NodeExists() { + // Before unmarking: RollbackUpload keys off the processing id to confirm the + // node is still this upload's, so unmarking first makes it a no-op. + if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), false, 0); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") + } } if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 89e95555404..a6b30bd876c 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -570,6 +570,41 @@ var _ = Describe("Async uploads via the coordinator", func() { Expect(stagedBytesExist(ids["simple"])).To(BeFalse(), "staged bytes should be cleaned up") }) + // The other half of the rule above: removal is only correct for a node this + // upload created. A rejected overwrite must leave the file it failed to + // replace exactly as it was — it never wrote anything to undo, so rolling + // the node back would destroy content belonging to the previous upload. + It("keeps the existing file when an overwrite is rejected", func() { + // No consumer runs here, so this commits inline and publishes UploadReady + // straight away. Drain it: the channel is unbuffered. + initiateAndUpload(firstContent) + _, ok := (<-pub).(events.UploadReady) + Expect(ok).To(BeTrue()) + + exists, _, size := fileStatus() + Expect(exists).To(BeTrue()) + Expect(size).To(Equal(len(firstContent))) + + ids, err := coord.InitiateUpload(ctx, ref, int64(len(secondContent)), map[string]string{ + "providerID": providerID, + "checksum": "md5 00000000000000000000000000000000", + }) + Expect(err).ToNot(HaveOccurred()) + + _, err = coord.Upload(ctx, storage.UploadRequest{ + Ref: &provider.Reference{Path: "/" + ids["simple"]}, + Body: io.NopCloser(bytes.NewReader(secondContent)), + Length: int64(len(secondContent)), + }, nil) + Expect(err).To(HaveOccurred(), "a checksum mismatch must be rejected") + + exists, status, size := fileStatus() + Expect(exists).To(BeTrue(), "a rejected overwrite must not delete the file it targeted") + Expect(size).To(Equal(len(firstContent)), "the original content must survive untouched") + Expect(status).ToNot(Equal("processing"), "the node must not be left flagged processing") + Expect(stagedBytesExist(ids["simple"])).To(BeFalse(), "staged bytes should be cleaned up") + }) + // The guarantee that keeps the two switches from drifting apart: with no // consumer running, nothing would ever arrive to finish a deferred upload, so // the coordinator must commit inline instead of staging and waiting forever. From 2d09232bb339dac4e6cb16b44abf1ab17ad60eab Mon Sep 17 00:00:00 2001 From: "lars.jurgensen" Date: Fri, 7 Aug 2026 16:10:32 +0200 Subject: [PATCH 12/15] fix: add retry for mark processing race condition --- .../utils/decomposedfs/metadata/errors.go | 13 ++++++++++++ .../decomposedfs/metadata/xattrs_backend.go | 21 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/pkg/storage/utils/decomposedfs/metadata/errors.go b/pkg/storage/utils/decomposedfs/metadata/errors.go index 659aff0bb13..02d7f1b4823 100644 --- a/pkg/storage/utils/decomposedfs/metadata/errors.go +++ b/pkg/storage/utils/decomposedfs/metadata/errors.go @@ -45,6 +45,19 @@ func IsNotExist(err error) bool { return false } +// IsErrRange checks for a syscall.ERANGE buried inside an xattr error. listxattr +// returns ERANGE when the set of attribute names grows between the size-probe call +// and the read call (a concurrent setxattr landed in between). This is transient and +// self-clearing once the competing writer finishes its batch. +func IsErrRange(err error) bool { + if xerr, ok := errors.Cause(err).(*xattr.Error); ok { + if serr, ok2 := xerr.Err.(syscall.Errno); ok2 { + return serr == syscall.ERANGE + } + } + return false +} + // IsAttrUnset checks the xattr.ENOATTR from the xattr package which redifines it as ENODATA on platforms that do not natively support it (eg. linux) // see https://github.com/pkg/xattr/blob/8725d4ccc0fcef59c8d9f0eaf606b3c6f962467a/xattr_linux.go#L19-L22 func IsAttrUnset(err error) bool { diff --git a/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go b/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go index d460e0a3af9..aca4da04e7b 100644 --- a/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go @@ -26,6 +26,7 @@ import ( "strconv" "strings" + "github.com/owncloud/reva/v2/pkg/appctx" "github.com/owncloud/reva/v2/pkg/storage/cache" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes" "github.com/owncloud/reva/v2/pkg/storage/utils/filelocks" @@ -82,12 +83,32 @@ func (b XattrsBackend) List(ctx context.Context, filePath string) (attribs []str return b.list(ctx, filePath, true) } +const listMaxRetries = 10 + func (b XattrsBackend) list(ctx context.Context, filePath string, acquireLock bool) (attribs []string, err error) { attrs, err := xattr.List(filePath) if err == nil { return attrs, nil } + // ERANGE means a concurrent writer (e.g. the posix inotify assimilate worker) + // mutated the attribute set mid-read. It is transient and self-clearing: retry + // until the set stabilises. + if IsErrRange(err) { + for attempt := 1; attempt <= listMaxRetries; attempt++ { + appctx.GetLogger(ctx).Debug().Err(err).Str("path", filePath).Int("attempt", attempt).Msg("xattr.List ERANGE, retrying") + attrs, err = xattr.List(filePath) + if err == nil { + appctx.GetLogger(ctx).Debug().Str("path", filePath).Int("attempt", attempt).Msg("xattr.List ERANGE recovered after retry") + return attrs, nil + } + if !IsErrRange(err) { + break + } + } + appctx.GetLogger(ctx).Error().Err(err).Str("path", filePath).Msg("xattr.List still failing after ERANGE retries") + } + // listing xattrs failed, try again, either with lock or without if acquireLock { f, err := lockedfile.OpenFile(filePath+filelocks.LockFileSuffix, os.O_CREATE|os.O_WRONLY, 0600) From efc120c809e44b5648a01a3ac3da636692c2fcac Mon Sep 17 00:00:00 2001 From: "lars.jurgensen" Date: Mon, 10 Aug 2026 15:54:47 +0200 Subject: [PATCH 13/15] fix: review comments --- .../services/dataprovider/dataprovider.go | 2 +- pkg/upload/coordinated_upload.go | 17 +---- pkg/upload/coordinator.go | 62 ++++++++++++++----- pkg/upload/filestore.go | 4 +- pkg/upload/postprocessing.go | 9 ++- pkg/upload/upload_async_test.go | 2 +- 6 files changed, 60 insertions(+), 36 deletions(-) diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index 115b282f604..ee325c2149f 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -122,7 +122,7 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) // to postprocessing and commits them once the verdict comes back. Without this // the coordinator commits inline and uploads are never scanned. if ac := pkgupload.AsyncConfFromDriverConf(conf.Drivers[conf.Driver]); ac.Enabled { - if err := coord.StartPostprocessing(evstream, ac.ConsumerGroup, ac.MountID, ac.NumConsumers); err != nil { + if err := coord.RunPostprocessingConsumer(evstream, ac.ConsumerGroup, ac.MountID, ac.NumConsumers); err != nil { return nil, fmt.Errorf("dataprovider: could not start postprocessing: %w", err) } } diff --git a/pkg/upload/coordinated_upload.go b/pkg/upload/coordinated_upload.go index 4765e45e2ca..321f670b831 100644 --- a/pkg/upload/coordinated_upload.go +++ b/pkg/upload/coordinated_upload.go @@ -94,21 +94,10 @@ func (u *coordinatedUpload) FinishUpload(ctx context.Context) error { } } -// Terminate discards an upload: it drops the staged files and, when this upload -// created the node, removes it again so a cancelled upload leaves nothing behind. +// Terminate discards an upload: it drops the staged files and reverts any node +// changes made before the upload completed. func (u *coordinatedUpload) Terminate(ctx context.Context) error { - u.session.Cleanup(true, true) - - // Terminate can run before the node was created, leaving nothing to undo. - ref := u.session.Reference() - if ref.GetResourceId().GetOpaqueId() == "" { - return nil - } - - _ = u.coord.fs.MarkProcessing(ctx, &ref, false, u.session.ID()) - if !u.session.NodeExists() { - _, _ = u.coord.fs.Delete(ctx, &ref) - } + u.coord.rollback(ctx, u.session) return nil } diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index 8610606a408..f4c8d478ff9 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -56,9 +56,9 @@ type Coordinator interface { // Upload writes the whole body of a non-resumable (PUT) upload into the // session named by req.Ref.Path and finishes it. Upload(ctx context.Context, req storage.UploadRequest, uff storage.UploadFinishedFunc) (*provider.ResourceInfo, error) - // StartPostprocessing subscribes to postprocessing results and enables async + // RunPostprocessingConsumer subscribes to postprocessing results and enables async // uploads. Call once, before serving requests. - StartPostprocessing(stream events.Consumer, group, mountID string, numConsumers int) error + RunPostprocessingConsumer(stream events.Consumer, group, mountID string, numConsumers int) error } // coordinator is the concrete implementation of Coordinator. @@ -67,8 +67,8 @@ type coordinator struct { store SessionStore chunkHandler *chunking.ChunkHandler pub events.Publisher - // async and mountID are set by StartPostprocessing and read by the upload - // path, which runs on request goroutines. StartPostprocessing is called once + // async and mountID are set by RunPostprocessingConsumer and read by the upload + // path, which runs on request goroutines. RunPostprocessingConsumer is called once // during service construction, before any request is served. async bool mountID string @@ -84,7 +84,7 @@ type coordinator struct { // pub receives the UploadReady event that tells the rest of the system a file is // available; pass nil to disable publishing. // -// Uploads commit inline until StartPostprocessing is called: deferring the commit +// Uploads commit inline until RunPostprocessingConsumer is called: deferring the commit // is only safe once something is listening for the result. func NewCoordinator(fs storage.FS, store SessionStore, chunkFolder string, pub events.Publisher) *coordinator { c := &coordinator{fs: fs, store: store, pub: pub} @@ -566,7 +566,9 @@ func (c *coordinator) touchAndMark(ctx context.Context, session Session) error { if err := c.fs.MarkProcessing(ctx, &nodeRef, true, session.ID()); err != nil { session.Cleanup(true, true) if !session.NodeExists() { - _, _ = c.fs.Delete(ctx, &nodeRef) + if _, err := c.fs.Delete(ctx, &nodeRef); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not delete node on rollback") + } } return err } @@ -608,6 +610,38 @@ func (c *coordinator) prepare(ctx context.Context, session Session) error { return nil } +// finishAsync commits a staged upload that has passed postprocessing. +// On failure nothing is reverted, leaving the upload restartable. +func (c *coordinator) finishAsync(ctx context.Context, session Session) error { + ref := session.Reference() + + f, err := os.Open(session.BinPath()) + if err != nil { + return err + } + defer f.Close() + scanResult, scanDate := session.ScanData() + + if err := c.fs.CommitUpload(ctx, &ref, session.ID(), storage.UploadSource{ + Body: f, + Length: session.Size(), + ScanResult: scanResult, + ScanDate: scanDate, + }); err != nil { + return err + } + + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + } + session.Cleanup(true, true) + metrics.UploadSessionsFinalized.Inc() + + ri := c.uploadedResourceInfo(ctx, session) + c.publishUploadReady(ctx, session, ri) + return nil +} + func (c *coordinator) finishSync(ctx context.Context, session Session) (*provider.ResourceInfo, error) { ref := session.Reference() @@ -760,15 +794,15 @@ func (c *coordinator) uploadRef(session Session) *provider.Reference { // file. The size diff is zero because nothing was propagated yet. func (c *coordinator) rollback(ctx context.Context, session Session) { ref := session.Reference() - if !session.NodeExists() { - // Before unmarking: RollbackUpload keys off the processing id to confirm the - // node is still this upload's, so unmarking first makes it a no-op. - if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), false, 0); err != nil { - appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") + if ref.GetResourceId().GetOpaqueId() != "" { + if !session.NodeExists() { + if err := c.fs.RollbackUpload(ctx, &ref, session.ID(), false, 0); err != nil { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") + } + } + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil && !errtypes.IsNotFound(err) { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") } - } - if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { - appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") } session.Cleanup(true, true) } diff --git a/pkg/upload/filestore.go b/pkg/upload/filestore.go index 3d1f5902cb3..61931aadab6 100644 --- a/pkg/upload/filestore.go +++ b/pkg/upload/filestore.go @@ -14,6 +14,8 @@ import ( "github.com/pkg/errors" "github.com/rs/zerolog" tusd "github.com/tus/tusd/v2/pkg/handler" + + "github.com/owncloud/reva/v2/pkg/appctx" ) // TokenOptions carries the JWT-signing configuration needed to produce transfer @@ -240,7 +242,7 @@ func (fs *FileStore) List(ctx context.Context) ([]Session, error) { id := strings.TrimSuffix(filepath.Base(path), ".info") session, err := fs.Get(ctx, id) if err != nil { - fs.log.Error().Str("path", path).Err(err).Msg("filestore: could not load session") + appctx.GetLogger(ctx).Error().Str("path", path).Err(err).Msg("filestore: could not load session") continue } sessions = append(sessions, session) diff --git a/pkg/upload/postprocessing.go b/pkg/upload/postprocessing.go index 16d768c30a5..a4b28d8b7da 100644 --- a/pkg/upload/postprocessing.go +++ b/pkg/upload/postprocessing.go @@ -47,7 +47,7 @@ var RegisteredEvents = []events.Unmarshaller{ events.CleanUpload{}, } -// StartPostprocessing subscribes to postprocessing results and switches the +// RunPostprocessingConsumer subscribes to postprocessing results and switches the // coordinator over to async uploads: from here on finished uploads stage their // bytes and wait for a scan verdict instead of committing inline. // @@ -62,7 +62,7 @@ var RegisteredEvents = []events.Unmarshaller{ // numConsumers goroutines share the subscription. Call once, before serving // requests. Fails without a publisher: nothing would hand uploads to // postprocessing, so every one of them would wait for a verdict that never comes. -func (c *coordinator) StartPostprocessing(stream events.Consumer, group, mountID string, numConsumers int) error { +func (c *coordinator) RunPostprocessingConsumer(stream events.Consumer, group, mountID string, numConsumers int) error { if c.pub == nil { return errors.New("coordinator: async uploads need an event publisher") } @@ -163,9 +163,8 @@ func (c *coordinator) onPostprocessingFinished(ctx context.Context, ev events.Po switch ev.Outcome { case events.PPOutcomeContinue: - if _, err := c.finishSync(ctx, session); err != nil { - // finishSync has already rolled back and cleaned up. - log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("could not commit upload after postprocessing") + if err := c.finishAsync(ctx, session); err != nil { + log.Error().Err(err).Str("uploadid", ev.UploadID).Msg("could not commit upload after postprocessing. Upload preserved for restart.") c.publishUploadFailed(ctx, session, ev) } return diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index a6b30bd876c..0404a1276c7 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -299,7 +299,7 @@ var _ = Describe("Async uploads via the coordinator", func() { // Starting the consumer is what switches the coordinator to async uploads; // the two are inseparable by design, so there is no separate flag to set. if startAsync { - Expect(coord.StartPostprocessing(evstream, "coordinator-test", mountID, 1)).To(Succeed()) + Expect(coord.RunPostprocessingConsumer(evstream, "coordinator-test", mountID, 1)).To(Succeed()) uploadID = upload(firstContent) bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) } From e2de2827f9cb02bfe457a097ae77b45f7b1dc5ff Mon Sep 17 00:00:00 2001 From: "lars.jurgensen" Date: Mon, 10 Aug 2026 16:08:55 +0200 Subject: [PATCH 14/15] fix: move AsyncConfFromDriverConf to postprocessing --- .../services/dataprovider/dataprovider.go | 2 +- pkg/upload/coordinator.go | 8 ++- pkg/upload/filestore.go | 50 ---------------- pkg/upload/postprocessing.go | 58 ++++++++++++++++++- pkg/upload/upload_async_test.go | 2 +- 5 files changed, 62 insertions(+), 58 deletions(-) diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index ee325c2149f..fc7b5d62b19 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -122,7 +122,7 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) // to postprocessing and commits them once the verdict comes back. Without this // the coordinator commits inline and uploads are never scanned. if ac := pkgupload.AsyncConfFromDriverConf(conf.Drivers[conf.Driver]); ac.Enabled { - if err := coord.RunPostprocessingConsumer(evstream, ac.ConsumerGroup, ac.MountID, ac.NumConsumers); err != nil { + if err := coord.RunPostprocessingConsumer(evstream, ac); err != nil { return nil, fmt.Errorf("dataprovider: could not start postprocessing: %w", err) } } diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index f4c8d478ff9..e5c2ffd0501 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -58,7 +58,7 @@ type Coordinator interface { Upload(ctx context.Context, req storage.UploadRequest, uff storage.UploadFinishedFunc) (*provider.ResourceInfo, error) // RunPostprocessingConsumer subscribes to postprocessing results and enables async // uploads. Call once, before serving requests. - RunPostprocessingConsumer(stream events.Consumer, group, mountID string, numConsumers int) error + RunPostprocessingConsumer(stream events.Consumer, conf AsyncConf) error } // coordinator is the concrete implementation of Coordinator. @@ -800,8 +800,10 @@ func (c *coordinator) rollback(ctx context.Context, session Session) { appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not roll back upload") } } - if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil && !errtypes.IsNotFound(err) { - appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + if err := c.fs.MarkProcessing(ctx, &ref, false, session.ID()); err != nil { + if _, ok := err.(errtypes.IsNotFound); !ok { + appctx.GetLogger(ctx).Error().Err(err).Str("uploadid", session.ID()).Msg("could not unmark processing") + } } } session.Cleanup(true, true) diff --git a/pkg/upload/filestore.go b/pkg/upload/filestore.go index 61931aadab6..a3a1f3fbd96 100644 --- a/pkg/upload/filestore.go +++ b/pkg/upload/filestore.go @@ -96,56 +96,6 @@ func NewFileStoreFromConfig(uploadDir string, driverConf map[string]interface{}, return FileStoreFromDriverConf(driverConf, log) } -// AsyncConf is how a service asks for async uploads: whether they are enabled, -// and the consumer subscription to use if they are. -type AsyncConf struct { - Enabled bool - ConsumerGroup string - NumConsumers int - // MountID is the storage id this provider answers for, used to drop - // postprocessing events belonging to other storages. - MountID string -} - -// AsyncConfFromDriverConf reads the postprocessing settings off the driver config -// map the services already hand us. -// -// The keys are decomposedfs's (options.go: `asyncfileuploads`, `events`). Reading -// the driver's own keys rather than introducing service-level ones keeps a single -// source of truth: if the coordinator and the driver disagreed, uploads would -// either commit twice or never get scanned. -// -// The consumer group matters most. It is what makes retiring the driver's -// consumer a move rather than an addition: two consumers in one group take turns -// stealing each other's events, two in different groups both act and commit the -// same upload twice. -func AsyncConfFromDriverConf(driverConf map[string]interface{}) AsyncConf { - if driverConf == nil { - return AsyncConf{} - } - var ac struct { - AsyncFileUploads bool `mapstructure:"asyncfileuploads"` - MountID string `mapstructure:"mount_id"` - Events struct { - NumConsumers int `mapstructure:"numconsumers"` - ConsumerGroup string `mapstructure:"consumer_group"` - } `mapstructure:"events"` - } - _ = mapstructure.Decode(driverConf, &ac) - group := ac.Events.ConsumerGroup - if group == "" { - // decomposedfs's default (options.go:177). The coordinator takes over the - // driver's subscription, so it must land in the same group. - group = "dcfs" - } - return AsyncConf{ - Enabled: ac.AsyncFileUploads, - ConsumerGroup: group, - NumConsumers: ac.Events.NumConsumers, - MountID: ac.MountID, - } -} - func newFileStoreWithTokens(root string, driverConf map[string]interface{}, log *zerolog.Logger) *FileStore { type tokenConf struct { DownloadEndpoint string `mapstructure:"download_endpoint"` diff --git a/pkg/upload/postprocessing.go b/pkg/upload/postprocessing.go index a4b28d8b7da..63137a033cf 100644 --- a/pkg/upload/postprocessing.go +++ b/pkg/upload/postprocessing.go @@ -31,6 +31,7 @@ import ( "errors" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/mitchellh/mapstructure" "github.com/rs/zerolog" "github.com/owncloud/reva/v2/pkg/appctx" @@ -39,6 +40,56 @@ import ( "github.com/owncloud/reva/v2/pkg/utils" ) +// AsyncConf is how a service asks for async uploads: whether they are enabled, +// and the consumer subscription to use if they are. +type AsyncConf struct { + Enabled bool + ConsumerGroup string + NumConsumers int + // MountID is the storage id this provider answers for, used to drop + // postprocessing events belonging to other storages. + MountID string +} + +// AsyncConfFromDriverConf reads the postprocessing settings off the driver config +// map the services already hand us. +// +// The keys are decomposedfs's (options.go: `asyncfileuploads`, `events`). Reading +// the driver's own keys rather than introducing service-level ones keeps a single +// source of truth: if the coordinator and the driver disagreed, uploads would +// either commit twice or never get scanned. +// +// The consumer group matters most. It is what makes retiring the driver's +// consumer a move rather than an addition: two consumers in one group take turns +// stealing each other's events, two in different groups both act and commit the +// same upload twice. +func AsyncConfFromDriverConf(driverConf map[string]interface{}) AsyncConf { + if driverConf == nil { + return AsyncConf{} + } + var ac struct { + AsyncFileUploads bool `mapstructure:"asyncfileuploads"` + MountID string `mapstructure:"mount_id"` + Events struct { + NumConsumers int `mapstructure:"numconsumers"` + ConsumerGroup string `mapstructure:"consumer_group"` + } `mapstructure:"events"` + } + _ = mapstructure.Decode(driverConf, &ac) + group := ac.Events.ConsumerGroup + if group == "" { + // decomposedfs's default (options.go:177). The coordinator takes over the + // driver's subscription, so it must land in the same group. + group = "dcfs" + } + return AsyncConf{ + Enabled: ac.AsyncFileUploads, + ConsumerGroup: group, + NumConsumers: ac.Events.NumConsumers, + MountID: ac.MountID, + } +} + // RegisteredEvents are the postprocessing events the coordinator consumes. var RegisteredEvents = []events.Unmarshaller{ events.PostprocessingFinished{}, @@ -62,18 +113,19 @@ var RegisteredEvents = []events.Unmarshaller{ // numConsumers goroutines share the subscription. Call once, before serving // requests. Fails without a publisher: nothing would hand uploads to // postprocessing, so every one of them would wait for a verdict that never comes. -func (c *coordinator) RunPostprocessingConsumer(stream events.Consumer, group, mountID string, numConsumers int) error { +func (c *coordinator) RunPostprocessingConsumer(stream events.Consumer, conf AsyncConf) error { if c.pub == nil { return errors.New("coordinator: async uploads need an event publisher") } - ch, err := events.Consume(stream, group, RegisteredEvents...) + ch, err := events.Consume(stream, conf.ConsumerGroup, RegisteredEvents...) if err != nil { return err } + numConsumers := conf.NumConsumers if numConsumers <= 0 { numConsumers = 1 } - c.mountID = mountID + c.mountID = conf.MountID c.async = true for i := 0; i < numConsumers; i++ { go c.Postprocessing(ch) diff --git a/pkg/upload/upload_async_test.go b/pkg/upload/upload_async_test.go index 0404a1276c7..a3036f8349c 100644 --- a/pkg/upload/upload_async_test.go +++ b/pkg/upload/upload_async_test.go @@ -299,7 +299,7 @@ var _ = Describe("Async uploads via the coordinator", func() { // Starting the consumer is what switches the coordinator to async uploads; // the two are inseparable by design, so there is no separate flag to set. if startAsync { - Expect(coord.RunPostprocessingConsumer(evstream, "coordinator-test", mountID, 1)).To(Succeed()) + Expect(coord.RunPostprocessingConsumer(evstream, pkgupload.AsyncConf{ConsumerGroup: "coordinator-test", MountID: mountID, NumConsumers: 1})).To(Succeed()) uploadID = upload(firstContent) bs.AssertNumberOfCalls(GinkgoT(), "UploadFromReader", 0) } From 22c771153ad7cb8d1ffe00109a7c91518bfec20d Mon Sep 17 00:00:00 2001 From: "lars.jurgensen" Date: Tue, 11 Aug 2026 13:03:49 +0200 Subject: [PATCH 15/15] fix: Remove posixfs related todos --- pkg/upload/coordinator.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/pkg/upload/coordinator.go b/pkg/upload/coordinator.go index e5c2ffd0501..f2a01e95892 100644 --- a/pkg/upload/coordinator.go +++ b/pkg/upload/coordinator.go @@ -287,11 +287,6 @@ func (c *coordinator) initiateUpload(ctx context.Context, ref *provider.Referenc session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(spaceOwner.GetType())) } - // TODO(OCISDEV-900, finding B7): main copies CtxKeySpaceGID into the session - // (upload.go:188) to drive posix uid/gid scoping at commit. That key lives in the - // decomposedfs package; reading it here would make the driver-agnostic coordinator - // depend on a concrete driver. posix-only concern (unset on ocis/s3ng). Deferred. - usr := ctxpkg.ContextMustGetUser(ctx) session.SetExecutant(usr) @@ -346,10 +341,6 @@ func (c *coordinator) initiateUpload(ctx context.Context, ref *provider.Referenc session.SetStorageValue("Chunk", chunkName) } - // TODO(OCISDEV-900, finding B7): main wraps TouchBin+Persist in fs.um.RunInBaseScope - // (upload.go:316) so the .bin/.info files get correct posix ownership. That usermapper - // lives in decomposedfs; the driver-agnostic coordinator can't reach it. posix-only - // (no-op on ocis/s3ng). Same root cause as SpaceGid; deferred. if err := session.TouchBin(); err != nil { return nil, fmt.Errorf("coordinator: could not create bin file: %w", err) }