Skip to content

fix(paste, delete): non-silent missing dirent, synchronous dst-writable precheck, complete move src cleanup#329

Merged
lovehunter9 merged 1 commit into
mainfrom
for_cli
May 25, 2026
Merged

fix(paste, delete): non-silent missing dirent, synchronous dst-writable precheck, complete move src cleanup#329
lovehunter9 merged 1 commit into
mainfrom
for_cli

Conversation

@lovehunter9
Copy link
Copy Markdown
Collaborator

Summary

Three independent bugs found while end-to-end testing paste and delete, all
under the same "the request looked like it succeeded but didn't actually do
what the user asked" symptom class:

  1. DELETE /api/resources silently returned 200 OK for a dirent that did
    not exist, leaking a stale entry as if it had been removed.
  2. PATCH /api/paste to a destination the caller has no write permission
    on returned an HTTP 200 with a task id; the failure only surfaced
    asynchronously in the task status, in a backend-dependent shape.
  3. A move from awss3 (or any cloud) to sync, and a move between two
    POSIX-family backends on different nodes, both copied the src to the dst
    but left the src intact. The UI reported the task as Completed.

Each bug is fixed in isolation; the response shape is aligned with sibling
code paths so the frontend has one less per-backend special case.

1. Delete: surface missing dirents instead of silent success

Problem

Backend Dirent missing before this PR
posix (drive / cache / external / smb / usb / hdd / internal) os.RemoveAll returns nil for an absent path -> handler returns 200 OK, no indication the entry was already gone
sync (seafile) seaserv.DelFile is idempotent -> handler returns 200 OK, ditto
cloud (rclone Purge) rclone Purge succeeds against an empty/absent prefix -> handler returns 200 OK, ditto

A user who deletes the same path twice (or whose UI list is stale) has no
way to know the second request did nothing useful.

Fix

  • posix: pre-stat each dirent (Lstat, falling back to Stat) before
    RemoveAll. Lstat is used so dangling symlinks remain deletable.
  • sync: existence-check the dirent against ListDirWithPerm of the
    parent before calling DelFile. Missing entries return a typed
    ErrEntryNotFound{IsDir}. Internal task cleanup (HandleDelete)
    swallows the typed error so task-side idempotent cleanup is preserved;
    the public API path (SyncStorage.Delete) surfaces it.
  • cloud: pre-stat the target before Purge.

Posix response shape aligned with sync/cloud

While we were in PosixStorage.Delete, the response shape was also
brought in line with sync/cloud so the UI has one shape per success/failure
across all backends:

Field Before After
data null for delete-failed (only filled on path-validation errors) array of failed dirents
message "<path>: <reason>;<path>: <reason>" (per-error string, dedup'd by reason) fixed "delete failed paths"

Per-item reasons are still in klog.Errorf for backend debugging. The
parent-missing branch (s.getFiles failure -> "<path>: no such file or directory") is unchanged. The previous error-string dedup meant two
distinct dirents failing for the same reason collapsed to a single entry;
each failed dirent now appears separately, which is a strict improvement.

2. Paste: precheck dst writability before allocating a task

Problem

PasteMethod returned a task id (HTTP 200) the instant request parsing
succeeded. If the destination was not writable, the failure surfaced
asynchronously, and the failure mode depended on the backend:

Backend Symptom for an unwritable dst
posix rsync task transitions Running -> Failed mid-phase
sync (seafile) HandleBatchCopy returns "permission denied" mid-task
cloud (rclone) rclone copy job fails with AccessDenied / 403 forbidden / insufficient_scope

From the UI every variant looked the same: a flaky failure delivered
seconds after the request returned 200.

Fix

A new precheck.DestinationWritable runs immediately after
PasteParam is fully resolved (right after the existing
precheck.SourceExists):

dst.FileType Probe
drive (master node only by routing), local cache/external/internal/smb/usb/hdd WriteTempFile -> create+delete a 1-byte file in the deepest existing ancestor of the dst path
Cross-node POSIX (<extend> does not match the current node) GET /api/resources?probe=write against the owning files-pod -> the receiving side runs the same WriteTempFile. New query param, scoped to this one call.
sync seahub.CheckFolderPermission on the deepest existing parent; must return "rw"
awss3 / tencent / google / dropbox rclone GetFilesList on the bucket root. About (used for free-space) is not supported on S3/Tencent, hence the list probe.

share=1 swaps in DstOwner so the probe runs as the share grantor.

Probe failure now returns a synchronous

HTTP/1.1 403 Forbidden
{"error": "Permission denied."}

-- same shape and message that the rest of the codebase (middleware share
checks, etc.) already uses. No task is allocated, no phantom Completed
entry. The detailed Go error stays in klog.Warningf.

Task-side translation also aligned

Task.formatJobStatusError already collapses rclone's many permission
strings (AccessDenied, 403 forbidden, insufficient_scope,
insufficientFilePermissions, invalid_grant, expired_access_token,
permission denied) into a single user-facing error; it now uses the
same ErrorMessagePermissionDenied constant so the synchronous and the
async paths produce identical UI text.

HandleBatchCopy also requires dstPerm == "rw" (no longer just
non-empty) so sync-to-sync copy targeting a read-only library is
rejected up front instead of half-failing inside seafile.

3. Paste: complete move src cleanup

Bug A: cloud -> sync move was copy-only

task_paste_sync.go::UploadToSync ran os.RemoveAll(srcUri + Path) for
a move. For a cloud src this resolves to a path like /awss3/<bucket>/...
which does not exist on the local filesystem; os.RemoveAll returns
nil and the task completes "successfully" with the cloud object still
in place.

Fix: in UploadToSync, branch on t.param.Src.IsCloud():

  • cloud src -> cmd.Clear(t.param.Src) (the same rclone-aware drop used
    by every other "cloud src move" site in task_paste_cloud.go);
  • everything else -> existing os.RemoveAll(srcUri + Path).

Cloud cmd.Clear failures are log-only -- matches the existing
task_paste_cloud.go convention (the copy already succeeded; a transient
network error during the post-copy delete should not fail the user-visible
task).

Bug B: cross-node POSIX move was copy-only

task_paste_download.go::DownloadFromFiles is the phase used when src
and dst live on different nodes (e.g. moving from an external disk on
node A to a cache on node B). It had no src deletion at all -- for
ActionMove the function returned after the last byte was streamed.

Fix: new helper clearRemoteSrc issues
DELETE /api/resources/<fsType>/<extend><parent> against the src node
(located via IntegrationManager().GetFilesPod). Triggered only after a
successful copy and only when t.param.Action == common.ActionMove. Same
log-only failure handling as cmd.Clear for parity with the cloud path.

The HTTP request runs under context.Background(), not t.ctx, on
purpose: cmd.Clear (the equivalent for cloud src) ignores task ctx so
a user pause issued right after the copy doesn't leave the src
half-dropped. streamHTTPClient.ResponseHeaderTimeout still caps a
stuck peer.

Cross-effect with bug #1: with the missing-dirent fix, a duplicate
trigger (e.g. retry after a flaky network) makes clearRemoteSrc
receive an HTTP 500 with the failed dirents instead of a misleading 200;
the log-only path makes this visible without breaking the task.

Files touched

Path Reason
pkg/drivers/posix/posix/posix.go posix pre-stat + aligned response shape
pkg/drivers/posix/posix/helper.go drop now-unused extractErrMsg
pkg/drivers/sync/seahub/batch.go sync existence check + ErrEntryNotFound; HandleBatchCopy requires dstPerm == "rw"
pkg/drivers/sync/sync.go trailing-slash preservation when calling HandleBatchDelete
pkg/drivers/clouds/rclone/rclone.go cloud pre-stat before Purge
pkg/drivers/precheck/precheck.go new DestinationWritable + per-backend dispatchers
pkg/models/query_param.go new Probe query param
pkg/hertz/biz/handler/api/resources/resources_service.go handleProbeWrite for cross-node POSIX probe
pkg/hertz/biz/handler/api/paste/paste_service.go call DestinationWritable; align error shape
pkg/tasks/task.go formatJobStatusError uses shared ErrorMessagePermissionDenied
pkg/tasks/task_paste_cloud.go unconditional formatJobStatusError on cloud paste failures
pkg/tasks/task_paste_posix.go drop the old in-task external-only probe (precheck covers it)
pkg/tasks/task_paste_sync.go branch UploadToSync on IsCloud() for move src cleanup
pkg/tasks/task_paste_download.go clearRemoteSrc for cross-node POSIX move
pkg/common/constant.go drop ErrorMessageDstNotWritable; reuse ErrorMessagePermissionDenied

Test plan

Delete

  • Posix (drive / external) DELETE /api/resources/<...> for a missing dirent -> 500 with data: ["<dirent>"], message: "delete failed paths". Posix-specific reason still in server log.
  • Sync DELETE /api/resources/sync/<repo>/ for a missing file -> 500 with data: ["<dirent>"], message: "delete failed paths"; same for a missing folder (trailing slash preserved).
  • Cloud DELETE for a missing object -> 500 with the failed path.
  • Internal task cleanup (HandleDelete invoked by share / paste flows) of an already-absent target is still idempotent (no failure surfaced upward).
  • Regression: deleting an existing file/folder on each backend still returns 200 OK.

Paste -- dst-writable precheck

  • Local POSIX dst with no write permission -> synchronous 403 { "error": "Permission denied." }, no task allocated.
  • Cross-node POSIX dst with no write permission -> same 403, with GET /api/resources?probe=write visible in the receiving pod's log.
  • Sync dst with a read-only library -> same 403.
  • Cloud dst (S3 / Tencent COS / Drive / Dropbox) with a scope-limited / expired token -> same 403 (probe failure) or task fails with Permission denied. (async path via formatJobStatusError).
  • share=1 paste to a grantor-owned dst respects the grantor's permission (not the requester's).

Paste -- move src cleanup

  • awss3 -> sync move of a file -> file appears on sync, removed from S3.
  • awss3 -> sync move of a directory -> directory tree appears on sync, prefix removed from S3.
  • tencent / google / dropbox -> sync move -> src removed.
  • Cross-node POSIX move (e.g. external on node A -> cache on node B) -> src removed from node A after copy completes on node B.
  • Existing move flows unaffected: posix -> posix (same node) (atomic mv), cloud -> posix, sync -> posix, sync -> cloud, posix -> cloud, cloud -> cloud.
  • Negative: simulate cmd.Clear / cross-node DELETE failure (e.g. revoke token between copy and clear) -> task completes, klog error visible, dst contains the copy, src still in place (acceptable per existing rclone/seahub convention).

General

  • GOOS=linux GOARCH=amd64 go build clean for all touched packages.
  • gofmt / lint clean for all touched files.

Made with Cursor

- delete: DELETE /api/resources no longer silently succeeds when the
  target dirent is missing (posix pre-stat, sync existence check via
  ListDirWithPerm, cloud pre-stat before Purge). Posix response shape
  aligned with sync/cloud: data carries the failed dirents, message is
  the shared "delete failed paths".
- paste: dst writability is precheck'd in the HTTP handler before a
  task is allocated, so no-write-permission surfaces as a synchronous
  403 + Permission denied instead of a flaky async task status. Cross-
  node posix uses GET /api/resources?probe=write; cloud uses an rclone
  list-root probe; sync uses CheckFolderPermission.
- paste: cloud->sync move and cross-node posix move now actually drop
  the source after the copy completes (UploadToSync routes cloud src
  through cmd.Clear; DownloadFromFiles forwards a DELETE to the src
  files-pod). Clear failures stay log-only, matching the existing
  rclone/seahub clear convention.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lovehunter9 lovehunter9 merged commit 0e99eee into main May 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant