fix(webdav): replace Google Drive objects without read gap by Youngv · Pull Request #3086 · OpenListTeam/OpenList

Youngv · 2026-09-15T12:16:48Z

Summary

Fix WebDAV MOVE overwrite semantics for same-directory Google Drive objects without exposing a temporary missing canonical path.

Current moveFiles ignores Overwrite; Google Drive rename permits duplicate names, so MOVE staged -> canonical can leave both objects and reads may continue returning the old one. A delete-first implementation fixes duplicates but creates a visible 404 window for concurrent readers.

This change:

honors Overwrite: F with 412 Precondition Failed when the destination exists
returns 204 No Content for a successful replacement and 201 Created for a new destination
adds an optional driver-level Replace capability
implements Google Drive replacement by publishing the source under the destination name first, then deleting the old destination by file ID
updates the directory cache before the backend handoff so concurrent OpenList readers resolve the new source object throughout replacement
preserves existing behavior for drivers without the stronger replacement primitive

Why not delete-first

A delete-then-rename sequence creates a canonical-path visibility gap. In a production-like probe against Google Drive, a delete-first WebDAV implementation produced non-200 reads during concurrent replacement. The implementation here was tested specifically against that failure mode.

Validation

Targeted tests:

go test -vet=off ./drivers/google_drive
go test ./server/webdav ./internal/op

A disposable sidecar built from this branch was tested against a real Google Drive-backed OpenList storage:

PROPFIND depth 0: 207
PROPFIND depth 1: 207
MKCOL: 201
PUT: 201
HEAD: 200
GET content/hash: passed
initial MOVE: 201
Overwrite: F: 412; source and destination preserved
Overwrite: T: 204; staged source disappeared; canonical returned replacement
concurrent replacement probe: 100 reads, 0 non-200, 0 invalid bodies
after restart/cache refresh: canonical GET 200 with latest body
PROPFIND after restart: exactly one canonical entry

The disposable remote objects and sidecar were removed after validation. No credentials or storage tokens are included here.

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-15T12:21:24.678571Z	`75cdaca`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75cdaca78a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-15T12:21:21Z

+		if !overwrite {
+			return http.StatusPreconditionFailed, nil


Treat an omitted Overwrite header as enabled

When a MOVE targets an existing destination and omits Overwrite, webdav.go:562 passes false because it only checks for an explicit T, so this new branch returns 412. RFC 4918 §10.6 requires a missing Overwrite header to be treated as T, and such replacements previously proceeded because moveFiles ignored the argument; default this to true unless the header is explicitly F, as the COPY path already does.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-09-15T12:21:21Z

+	if cachedDir != nil {
+		if srcRawObj.IsDir() {
+			Cache.deleteDirectoryTree(stdpath.Join(dirKey, srcRawObj.GetName()))
+		}
+		cachedDir.RemoveObject(srcRawObj.GetName())


Invalidate the overwritten destination subtree

When both operands are directories and the destination or one of its descendants has a warm directory cache, successful replacement only deletes the source subtree cache. The cache under dstPath still contains objects from the deleted destination directory, so subsequent listings and reads can expose stale entries or links to deleted Google Drive IDs until cache expiry. Delete the destination subtree after the backend handoff as well, while preserving the newly published parent entry.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-09-15T12:21:22Z

+	url = "https://www.googleapis.com/drive/v3/files/" + dstObj.GetID()
+	_, err = d.request(url, http.MethodDelete, nil, nil)
+	return err


Roll back the source rename when destination deletion fails

If the PATCH succeeds but the following DELETE fails, Replace returns an error after already renaming the source to the destination name. The WebDAV client receives a 500 even though its source path has been removed, and Google Drive is left with two objects using the canonical name; the caller's cache rollback cannot repair that backend state. Preserve the original source name and attempt to restore it before returning the deletion error.

Useful? React with 👍 / 👎.

Youngv · 2026-09-15T12:36:59Z

Additional production-like validation found a remaining race: after packaging this branch as a local image and temporarily replacing the production OpenList container, all WebDAV overwrite status/content checks passed, but a concurrent reader observed 2 non-200 responses out of 123 GETs while repeatedly promoting staged objects over the canonical name. The container was rolled back immediately. This means the current cache-first + Drive duplicate-name handoff is not yet sufficient to guarantee continuous readability under the real workload. I am treating this PR as not production-ready until that race is understood.

Youngv · 2026-09-15T13:08:12Z

Root cause of the remaining 404 race is now identified and addressed in commit 4c08593020de. A reader can resolve the old canonical file ID before the cache handoff, then race with immediate deletion of that old ID; its subsequent Google Drive link/download lookup returns 404.

The updated Google Drive Replace sequence is now: publish source as canonical -> rename old destination to .openlist-replaced-<file-id> -> retain old ID for a 30s in-flight reader drain -> asynchronously delete the tombstone. This keeps the canonical cache on the new object while allowing already-resolved readers to finish against the old ID.

Validation against the same real Drive-backed storage after the change: sidecar runs 716/716, 703/703, and final-image 739/739 canonical GETs all returned 200; production cutover then passed 741/741 and a second pass 713/713, with no invalid 200 bodies. The local hotfix is currently healthy in production. Stress testing also confirmed the prior immediate-delete implementation could produce explicit 404s.

Youngv · 2026-09-15T14:43:09Z

Follow-up commit 4c085930 addresses the remaining in-flight-reader race seen in the previous production trial. Root cause was an already-started reader holding the old Google Drive file ID when replacement deleted that ID. The revised Google Drive Replace sequence now publishes the new canonical object, renames the old object to a tombstone (preventing new resolutions), retains the old file ID for a 30s drain window, then deletes it asynchronously.

Clean validation after removing overlapping/stale probe processes:

sidecar against production-data copy: 1,817 concurrent GETs across 3 runs, 90 overwrites, 0 non-200 responses, 0 invalid bodies
production endpoint: 564 concurrent GETs, 30 overwrites, 0 non-200 responses, 0 invalid bodies; final content correct
targeted tests pass for google_drive, server/webdav, and internal/op.

The local production hotfix is currently healthy on this commit.

jyxjjj · 2026-09-19T16:58:27Z

An omitted Overwrite means T for MOVE (RFC 4918 §10.6), but this treats it as false and returns 412 when the destination exists. Could we use r.Header.Get("Overwrite") != "F", as COPY does?

pikachuren

🙏 感谢 @Youngv 提交！
🤖 AI 自动审核声明：本评审报告由 AI 自动生成，当前使用 Claude Opus 5 模型进行分析。
⚠️ AI 分析结果仅供参考，可能存在误判或遗漏。如您发现任何问题或有不同意见，欢迎随时提出讨论和纠正。
⚠️ 重要提醒：即使 AI 评审认为代码质量良好且建议合并，最终是否合并仍需由项目维护者进行人工判定。项目维护者会综合考虑代码质量、项目规划、技术方向、团队资源等多方面因素做出是否合并的决策。

🎯 结论

✅ Approve — 代码质量良好，建议合并

📖 概要

fix(webdav): replace Google Drive objects without read gap

📊 评审结果

改动合理，无重大问题发现。代码逻辑清晰，符合项目规范。

🎯 结论：✅ Approve — 建议合并

jyxjjj · 2026-09-23T17:50:59Z

Please edit the PR description to follow our template

fix(webdav): replace Google Drive objects without read gap

75cdaca

chatgpt-codex-connector Bot reviewed Sep 15, 2026

View reviewed changes

fix(google-drive): drain readers before overwrite cleanup

4c08593

pikachuren reviewed Sep 21, 2026

View reviewed changes

ILoveScratch2 force-pushed the main branch from 90fe446 to ea10624 Compare September 29, 2026 11:07

Navigation Menu

Uh oh!

fix(webdav): replace Google Drive objects without read gap - #3086

fix(webdav): replace Google Drive objects without read gap#3086
Youngv wants to merge 2 commits into
OpenListTeam:mainfrom
Youngv:fix/webdav-move-overwrite-no-gap

Youngv commented Sep 15, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 15, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Sep 15, 2026

Uh oh!

chatgpt-codex-connector Bot Sep 15, 2026

Uh oh!

chatgpt-codex-connector Bot Sep 15, 2026

Uh oh!

Youngv commented Sep 15, 2026

Uh oh!

Youngv commented Sep 15, 2026

Uh oh!

Youngv commented Sep 15, 2026

Uh oh!

jyxjjj commented Sep 19, 2026

Uh oh!

pikachuren left a comment

Uh oh!

jyxjjj commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Youngv commented Sep 15, 2026

Summary

Why not delete-first

Validation

Related

Uh oh!

chatgpt-codex-connector Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Youngv commented Sep 15, 2026

Uh oh!

Youngv commented Sep 15, 2026

Uh oh!

Youngv commented Sep 15, 2026

Uh oh!

jyxjjj commented Sep 19, 2026

Uh oh!

pikachuren left a comment

Choose a reason for hiding this comment

🎯 结论

📖 概要

📊 评审结果

Uh oh!

jyxjjj commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chatgpt-codex-connector Bot commented Sep 15, 2026 •

edited

Loading