Repository navigation
Conversation
shelken
force-pushed
the
fix/webdav-same-storage-move
branch
4 times, most recently
from
October 30, 2026 03:09
6990592 to
ece430d
Compare
During WebDAV MOVE operations, `moveWithRollback` previously created a full source copy via `newFileBackup(ctx, src, true)` regardless of whether the move occurred within the same storage. On storage drivers where `Copy` is asynchronous (such as cloud drives that queue batch copy tasks), `copyNamedInStorage` failed immediately with `object not found` when attempting to rename the unfinalized staging object, causing WebDAV MOVE requests to fail with HTTP 500. - Check if source and destination reside within the same storage. - Skip redundant source backup copy for intra-storage moves. - Update rollback compensation to restore moved objects back to the source path if subsequent steps fail. - Add regression coverage for `isSameStorage`. Co-Authored-By: Gemini 3.8 Flash <hakase@ooooo.space> Generated-By: omp 18.13.3
shelken
force-pushed
the
fix/webdav-same-storage-move
branch
from
October 30, 2026 03:23
ece430d to
f062a77
Compare
…n data - Restore moved objects in reverse order during cross-directory rollback: rename back to source name within destination directory before moving to avoid colliding with existing files in the source directory. - Guard destination restoration: only restore dstBackup if source restoration succeeded, preventing data loss of un-rolled-back source. - Track move completion state and prevent forward move failures from overwriting intact source files during rollback. Co-Authored-By: Gemini 3.8 Flash <hakase@ooooo.space> Generated-By: omp 18.13.3
- Exclude Alias and Chunk driver storages before same-directory shortcuts due to asymmetrical multi-backend replica policies and virtual chunking. - Restrict same-storage fast-path to regular files (`!srcInfo.IsDir()`), preserving recursive directory backup safeguards on S3 and object stores. - Add nil checks to prevent dereferencing uninitialized storage pointers. Co-Authored-By: Gemini 3.8 Flash <hakase@ooooo.space> Generated-By: omp 18.13.3
shelken
force-pushed
the
fix/webdav-same-storage-move
branch
from
October 30, 2026 03:35
f062a77 to
ddd093d
Compare
This was referenced Oct 30, 2026
|
补充一个 Quark（夸克网盘）+ FileBot / Mountain Duck 的同根因复现，以及对当前 PR 的回归验证结果。 实际故障环境：官方 OpenList v4.2.9 / 0497ee8，Docker Linux ARM64；macOS 上 FileBot 通过 Mountain Duck WebDAV 挂载重命名。同目录，目标名不存在，文件大小 2,943,551,117 字节。
路径和客户端地址脱敏后的日志摘录： 另一请求读取到 59,998,967 字节时同样被取消。原文件仍在，目标未创建。 本地针对“同目录且目标不存在”改为直接 Rename，并保留事务失败后的反向 Rename，随后经同一个 FileBot / Mountain Duck 链路实测：MOVE 201，1.95 秒。云端与挂载均回读成功，文件大小不变，首尾各 64 KiB 的 SHA-256 一致（抽样，并非全文件校验）。这项实测对应本地窄范围补丁；本 PR 的完整实现尚未部署到真实网盘验证。 对当前 PR 的合成回归验证附带测试驱动支持 Rename，Copy 固定返回
该路径在回滚前写入的是 执行命令（Go 1.27.3，darwin/arm64）： go test ./server/webdav -run TestSameDirectoryMove -count=1 -v可运行的完整回归测试（适配 PR head，保存为 server/webdav/rename_regression_test.go）package webdav
import (
"context"
"errors"
"net/http"
"net/http/httptest"
"os"
"path"
"path/filepath"
"sync/atomic"
"testing"
"time"
"github.com/OpenListTeam/OpenList/v4/drivers/local"
"github.com/OpenListTeam/OpenList/v4/internal/conf"
"github.com/OpenListTeam/OpenList/v4/internal/db"
"github.com/OpenListTeam/OpenList/v4/internal/driver"
"github.com/OpenListTeam/OpenList/v4/internal/errs"
"github.com/OpenListTeam/OpenList/v4/internal/model"
"github.com/OpenListTeam/OpenList/v4/internal/op"
"github.com/OpenListTeam/OpenList/v4/pkg/utils"
"github.com/glebarez/sqlite"
"gorm.io/gorm"
)
// A storage that can rename but cannot copy, like Quark. Reading a transfer
// stream is deliberately unavailable: renaming must not depend on file data.
type renameOnlyDriver struct {
local.Local
copies atomic.Int32
links atomic.Int32
renames atomic.Int32
}
func (d *renameOnlyDriver) Config() driver.Config {
c := d.Local.Config()
c.Name = "WebDAVRenameRegression"
return c
}
func (d *renameOnlyDriver) Copy(context.Context, model.Obj, model.Obj) error {
d.copies.Add(1)
return errs.NotSupport
}
func (d *renameOnlyDriver) Link(context.Context, model.Obj, model.LinkArgs) (*model.Link, error) {
d.links.Add(1)
return nil, errors.New("file data transfer is unavailable")
}
func (d *renameOnlyDriver) Rename(ctx context.Context, obj model.Obj, name string) error {
if err := ctx.Err(); err != nil {
return err
}
d.renames.Add(1)
return d.Local.Rename(ctx, obj, name)
}
type renameFixture struct {
root, mount, src, dst string
d *renameOnlyDriver
ctx context.Context
h *Handler
}
func setupRenameFixture(t *testing.T) *renameFixture {
t.Helper()
root := t.TempDir()
database, err := gorm.Open(sqlite.Open(filepath.Join(t.TempDir(), "test.db")), &gorm.Config{})
if err != nil {
t.Fatal(err)
}
conf.Conf = conf.DefaultConfig(t.TempDir())
db.Init(database)
op.RegisterDriver(func() driver.Driver { return &renameOnlyDriver{} })
addition, err := utils.Json.MarshalToString(map[string]string{"root_folder_path": root})
if err != nil {
t.Fatal(err)
}
mount := "/" + t.Name()
id, err := op.CreateStorage(context.Background(), model.Storage{
Driver: "WebDAVRenameRegression", MountPath: mount, Addition: addition,
})
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() {
if err := op.DeleteStorageById(context.Background(), id); err != nil {
t.Error(err)
}
sqlDB, _ := database.DB()
_ = sqlDB.Close()
})
storage, err := op.GetStorageByMountPath(mount)
if err != nil {
t.Fatal(err)
}
user := &model.User{Username: "rename-only", BasePath: "/", Role: model.GENERAL, Permission: 1 << 4}
ctx := context.WithValue(context.Background(), conf.UserKey, user)
return &renameFixture{
root: root, mount: mount, src: path.Join(mount, "original.mp4"), dst: path.Join(mount, "renamed.mp4"),
d: storage.(*renameOnlyDriver), ctx: ctx, h: &Handler{Prefix: "/dav", LockSystem: NewMemLS()},
}
}
func (f *renameFixture) move() *httptest.ResponseRecorder {
r := httptest.NewRequest("MOVE", "http://example.test/dav"+f.src, nil).WithContext(f.ctx)
r.Header.Set("Destination", "http://example.test/dav"+f.dst)
r.Header.Set("Overwrite", "F")
w := httptest.NewRecorder()
f.h.ServeHTTP(w, r)
return w
}
func (f *renameFixture) assertNoTransfer(t *testing.T) {
t.Helper()
if f.d.copies.Load() != 0 || f.d.links.Load() != 0 {
t.Fatalf("rename accessed file data: copies=%d links=%d", f.d.copies.Load(), f.d.links.Load())
}
}
func TestSameDirectoryMoveAvoidsTransfer(t *testing.T) {
f := setupRenameFixture(t)
src := filepath.Join(f.root, "original.mp4")
file, err := os.Create(src)
if err != nil {
t.Fatal(err)
}
const size int64 = 3_023_209_756
err = file.Truncate(size) // sparse fixture; no multi-GB allocation or transfer
_ = file.Close()
if err != nil {
t.Fatal(err)
}
before, _ := os.Stat(src)
if err := db.GetDb().Create(&model.WebDAVProperty{Path: f.src, Namespace: "urn:test", Name: "label", InnerXML: []byte("keep")}).Error; err != nil {
t.Fatal(err)
}
w := f.move()
if w.Code != http.StatusCreated {
t.Fatalf("MOVE status=%d body=%s", w.Code, w.Body.String())
}
f.assertNoTransfer(t)
after, err := os.Stat(filepath.Join(f.root, "renamed.mp4"))
if err != nil || after.Size() != size || !os.SameFile(before, after) {
t.Fatalf("MOVE did not preserve the file: %v", err)
}
if _, err := os.Stat(src); !os.IsNotExist(err) {
t.Fatalf("original path still exists: %v", err)
}
var props []model.WebDAVProperty
if err := db.GetDb().Find(&props).Error; err != nil || len(props) != 1 || props[0].Path != f.dst || string(props[0].InnerXML) != "keep" {
t.Fatalf("dead properties were not migrated: %+v, %v", props, err)
}
entries, _ := os.ReadDir(f.root)
if len(entries) != 1 || f.d.renames.Load() != 1 {
t.Fatalf("unexpected staging files or operations: entries=%d renames=%d", len(entries), f.d.renames.Load())
}
}
func TestSameDirectoryMovePreservesGuards(t *testing.T) {
for _, guard := range []string{"existing destination", "missing permission", "source lock"} {
t.Run(guard, func(t *testing.T) {
f := setupRenameFixture(t)
if err := os.WriteFile(filepath.Join(f.root, "original.mp4"), []byte("source"), 0o600); err != nil {
t.Fatal(err)
}
want := http.StatusPreconditionFailed
switch guard {
case "existing destination":
if err := os.WriteFile(filepath.Join(f.root, "renamed.mp4"), []byte("keep destination"), 0o600); err != nil {
t.Fatal(err)
}
case "missing permission":
f.ctx = context.WithValue(f.ctx, conf.UserKey, &model.User{BasePath: "/", Role: model.GENERAL})
want = http.StatusForbidden
case "source lock":
if _, err := f.h.LockSystem.Create(time.Now(), LockDetails{Root: f.src, Duration: time.Hour}); err != nil {
t.Fatal(err)
}
want = http.StatusLocked
}
if w := f.move(); w.Code != want {
t.Fatalf("MOVE status=%d want=%d body=%s", w.Code, want, w.Body.String())
}
if b, err := os.ReadFile(filepath.Join(f.root, "original.mp4")); err != nil || string(b) != "source" {
t.Fatalf("source was modified: %v", err)
}
if guard == "existing destination" {
if b, err := os.ReadFile(filepath.Join(f.root, "renamed.mp4")); err != nil || string(b) != "keep destination" {
t.Fatalf("destination was modified: %v", err)
}
}
f.assertNoTransfer(t)
if f.d.renames.Load() != 0 {
t.Fatal("guarded request reached rename")
}
})
}
}
func TestSameDirectoryMoveRollsBackOnTransactionFailure(t *testing.T) {
f := setupRenameFixture(t)
if err := os.WriteFile(filepath.Join(f.root, "original.mp4"), []byte("source"), 0o600); err != nil {
t.Fatal(err)
}
if err := db.GetDb().Create(&model.WebDAVProperty{Path: f.src, Namespace: "urn:test", Name: "label"}).Error; err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(f.ctx)
defer cancel()
injected := errors.New("simulated transaction failure")
err := runDeadPropsFileMutation(ctx,
func(tx *gorm.DB) error { return moveDeadProps(tx, f.src, f.dst) },
func() (*fileMutation, error) {
m, err := moveWithRollback(ctx, f.src, f.dst, f.mount, f.mount, "original.mp4", "renamed.mp4", false, true)
if err != nil {
return m, err
}
cancel() // compensation must not inherit the failed request's cancellation
return m, injected
},
)
if !errors.Is(err, injected) {
t.Fatalf("unexpected error: %v", err)
}
f.assertNoTransfer(t)
if b, err := os.ReadFile(filepath.Join(f.root, "original.mp4")); err != nil || string(b) != "source" {
t.Fatalf("rollback lost source: %v", err)
}
if _, err := os.Stat(filepath.Join(f.root, "renamed.mp4")); !os.IsNotExist(err) {
t.Fatalf("rollback left destination: %v", err)
}
var props []model.WebDAVProperty
if err := db.GetDb().Find(&props).Error; err != nil || len(props) != 1 || props[0].Path != f.src {
t.Fatalf("dead properties did not roll back: %+v, %v", props, err)
}
}
func TestSameDirectoryMoveRollbackPreservesRecreatedSource(t *testing.T) {
f := setupRenameFixture(t)
if err := os.WriteFile(filepath.Join(f.root, "original.mp4"), []byte("original content"), 0o600); err != nil {
t.Fatal(err)
}
m, err := moveWithRollback(f.ctx, f.src, f.dst, f.mount, f.mount, "original.mp4", "renamed.mp4", false, true)
if err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(f.root, "original.mp4"), []byte("new content"), 0o600); err != nil {
t.Fatal(err)
}
if err := m.rollback(); err == nil {
content, _ := os.ReadFile(filepath.Join(f.root, "original.mp4"))
t.Fatalf("rollback should refuse to overwrite a recreated source; source now contains %q", content)
}
for name, want := range map[string]string{"original.mp4": "new content", "renamed.mp4": "original content"} {
if b, err := os.ReadFile(filepath.Join(f.root, name)); err != nil || string(b) != want {
t.Fatalf("rollback changed %s: %v", name, err)
}
}
f.assertNoTransfer(t)
}AI 使用说明： 本条内容由 Codex 协助诊断、编写测试并整理，经受影响用户授权提交。上面的结果来自实际日志、命令执行和云端回读；未声称进行过全量兼容性测试或人工代码审阅。 |
jyxjjj
requested changes
Nov 4, 2026
jyxjjj
left a comment
jyxjjj
left a comment
Member
There was a problem hiding this comment.
[P1] moveWithRollback 在 moveResource 报错且 moved 为 false 时，只要源路径还能查到，就删除 dst。比如最初目标不存在，另一个写入者随后创建了目标，驱动因目标冲突拒绝 Rename，这里会把别人的文件删掉。源仍存在不能证明目标是本次 MOVE 创建的，请移除这段无条件清理。
反向 Rename 也要处理原名被重新占用的情况。评论区已在这个 head 用 Local 复现：事务回滚覆盖了并发创建的源文件。旧备份恢复也有类似风险，请在这条新回滚路径保留冲突双方，并加入对应测试。
审核提交 ddd093d。
Reviewed or refined by GPT-6-Astra High
shelken
force-pushed
the
fix/webdav-same-storage-move
branch
from
November 4, 2026 14:38
cedeeda to
a4049d7
Compare
- Drop the unconditional `removeIfExists(ctx, dst)` cleanup on forward failure. A surviving source proves nothing about who created the object now at the destination: another writer may have claimed it after the existence check, and deleting it destroyed unrelated data. - Require rollback to confirm the target name is free before writing to it, covering the same-directory rename, the cross-directory rename to the source name, and the cross-directory move back into the source root. A source recreated while the dead-property transaction was running is now preserved instead of overwritten, and the rollback reports both paths. - Treat only `ObjectNotFound` as free, so a transient lookup failure cannot turn a refusal into an overwrite. - Add move regression tests backed by a rename-only driver: a destination created by another writer must survive a failed MOVE, a recreated source must survive a rollback in both the same-directory and cross-directory paths, and a failing dead-property transaction must still undo the move. - Document the remaining window honestly: the probe cannot stop a writer that claims the name after the check, which would need an atomic no-replace rename the driver interface does not offer. Co-Authored-By: DeepSeek V4.1 Flash <hakase@ooooo.space> Generated-By: omp 18.14.0
shelken
force-pushed
the
fix/webdav-same-storage-move
branch
from
November 4, 2026 14:57
a4049d7 to
cbc3df0
Compare
Contributor
Author
|
两条都改了，
Co-Authored-By: DeepSeek V4.1 Flash |
jyxjjj
requested changes
Nov 4, 2026
jyxjjj
left a comment
jyxjjj
left a comment
Member
There was a problem hiding this comment.
上一轮意见用了 AI 辅助，我重新看了下，之前没有把修改范围说清楚，这里补充一下。
我认可跳过多余源备份这个方向，但路径检查还不能保证回滚时不覆盖其他写入，所以这版我暂时不批准。
不要求这个 PR 解决所有网盘的限制。我的接受范围是：能保证回滚不覆盖的驱动，可以启用新逻辑并补上测试。暂时做不到的驱动，先保留原逻辑和已知问题，不统一启用。
如果这次不好划清范围，可以拆成能独立验收的改动吗？
Contributor
Author
ok 我后面将这个pr改成仅对 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary / 摘要
Follow-up to #3210: When handling WebDAV MOVE requests,
moveWithRollbackunconditionally creates a rollback copy of the source resource usingnewFileBackup(ctx, src, true).However, on storage backends where
Copyis asynchronous (such as 139 Cloud and certain other cloud drive drivers where/file/batchCopyreturns an asynchronous batch task ID),copyNamedInStoragecreates a temporary.openlist-copy-<uuid>directory and immediately invokesop.Renameon the expected copied object. Because the cloud task is processed asynchronously,op.Getinop.Renamefails withobject not found. This causes the entire MOVE transaction to abort with HTTP 500.In intra-storage moves (including same-directory renames), duplicating regular files prior to moving is redundant—the source is not destroyed across storage boundaries. If subsequent steps (or dead-property database transactions) fail, the object can simply be moved back to the original source path.
This PR:
Adds
isSameStorageto detect when source and destination reside within the same storage mount (with driver-level safety checks excluding Alias from cross-directory shortcuts).Limits same-storage bypass to regular files (
!srcInfo.IsDir()), preserving directory recursive backup safeguards.Implements
restoreSameStorageMovewith reverse-order compensation (renaming back to source name within destination directory before moving back to source directory), preventing destination filename collision with existing files in the source directory.Hardens rollback: if source restoration fails, destination restoration is safely aborted to avoid destroying the only surviving copy of source data.
Adds unit tests for
isSameStorage.This PR has breaking changes.
/ 此 PR 包含破坏性变更。
This PR changes public API, config, storage format, or migration behavior.
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
This PR requires corresponding changes in related repositories.
/ 此 PR 需要关联仓库同步修改。
Testing / 测试
go test ./server/webdav/...MOVE ...: 500during safe atomic temporary file writes.Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Tests / 测试
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。
Co-Authored-By: gemini-3.8-flash