rclone/docs/plans/001-protondrive-tier1-workspace/task-21f-review.md

Task 21f review (a2dc6f71f..af6fdd789)

Spec Compliance

• ❌ Issues found: the requirement "never delete anything when the commit outcome is unknown (ruling 11)" is not met under the lag this task establishes as real. After a commit that failed, one stale read showing the draft (new file) or the base revision (update) still counts as proof, and the draft is deleted (backend/protondrive/upload.go:401-403 stops retrying on such a read because lagging(..., committed=false) is false for it, and then upload.go:425 returns committed=false, which makes upload call deleteDraft). The implementer knows this and logged it in docs/TODO.md:165 instead of fixing it. See Critical 1. Everything else asked for is there: retrying while the node is missing or has no active revision, errUploadSuperseded kept for a genuine other revision, TDD with a lag fault in the fake.
• ⚠️ Cannot verify from the diff: what the real server does with delete_multiple on a link that is now active, and with DELETE .../revisions/{id} on a revision that is now active (the existing docs/TODO.md item says this is unconfirmed). Critical 1 is a data-loss bug if either call succeeds.

Strengths

• The diagnosis is right and the RED evidence reproduces both live messages byte for byte (empty revision gives superseded, and not-found becomes a bare fs.ErrorObjectNotFound through translateError).
• readBack (nodes.go:733) is small: it is bounded (8 reads, 100 ms doubling, capped at 2 s, about 7.1 s), honours ctx, and calls fetchNode once on the normal path, so the request budgets don't change. TestUploadReadBackLagOutlasted pins the bound at exactly readBackAttempts link reads and no deletes.
• The success path cannot delete anything. Every non-ours branch with commitErr == nil returns committed=true (upload.go:418-423). A stale node is never left in the cache: on the ours path cache.add replaces it, and every other path calls forget.
• The targetFolder retry is limited to c.cache == nil (nodes.go:967), so with caching on (the default) a genuine not-found costs nothing extra. Choosing errUploadUnconfirmed over an invented node is sound and recorded in DECISIONS.

Issues

Critical (Must Fix)

1. backend/protondrive/upload.go:401-403, 425 (with upload.go:126-130 and deleteDraft ~upload.go:927): a failed commit plus one stale read sends a delete against a file that is committed. Checked in a scratch worktree at af6fdd789 (since removed) with a commit fault {Status: 422, AfterHandler: true} (the commit lands, the client sees an error) and SetReadLag(1, false):
   • New file: the read shows the pre-commit draft, notCommitted is true, and deleteDraft sends POST delete_multiple for the link ID, which is now the active file. The fake refuses ("only drafts can be deleted"). The real server's behaviour is unconfirmed (docs/TODO.md), and the SDK sends delete_multiple for active links in its conflict handler. If Proton honours it, the whole file is permanently deleted, not trashed.
   • Update: the read shows the base revision, and the client sends DELETE .../revisions/{ourRevision}, the revision that is now active. The fake answers 2501. On the real server this could remove the active revision, leaving the file without content or rolled back. This path is likely to occur, not just possible. shouldRetry resends the commit PUT on 5xx/408/network errors (nodes.go:272-273). So "commit landed, 502 or timeout, resent commit refused with 4xx", or simply a cancelled ctx during the commit, followed by a read in the lag window, reaches the deletion. In both cases the error returned is the plain commit error, which rclone retries at low level. On a new file the source still has the data, but a retry can race the deletion, and the update case has no such fallback. Safe rule (recommended): decide by the commit error, not by a read.
   • Delete only when the server definitively refused the commit on a single, un-retried attempt: an *api.Error with a 4xx status and a code. A refusal means it did not commit whatever any read shows, so no read and no 7 s wait are needed. This answers the report's worry that "retrying there would slow every refused commit by 7 s".
   • For any other failure (transport error, timeout, 5xx/408, a refusal after the pacer resent the PUT, a done ctx): never delete. Return errUploadOutcomeUnknown and leave the draft, which the next upload of the name replaces (the existing comment at upload.go ~429). Tracking "was retried" needs a flag set in the paced fn, or calling c.s.call for the commit with your own single retry.
   • If a read-based proof is still wanted for unknown failures, it has to be readBackAttempts consecutive reads that all show the draft or base revision over the whole window, and even that is a heuristic because the lag length is unmeasured (docs/TODO.md). The rule above is strictly safer. Add a test like the scratch one (AfterHandler 422 or 502 on the commit plus SetReadLag(n, false), assert requireNoDeletes). It fails on af6fdd789 today.

Important (Should Fix)

None beyond Critical 1.

Minor (Nice to Have)

1. upload.go:446: the "active with an empty revision" lag state is not tested. The fake's stale view of a new file is the draft (protontest/drive.go:1710, before captured at commit), so deleting that case leaves every test passing. Either model it (a SetReadLag mode returning the link active with no active revision, which may be what the live "revision is active" was) or say it is speculative.
2. upload.go:399-403: the success-path retry runs on the cancel-detached readCtx, so Ctrl-C after a successful commit can block about 7 s more. This is acceptable and logged. If wanted, return errUploadUnconfirmed as soon as the caller's ctx is done and commitErr == nil (nothing is deleted on that path anyway).
3. errUploadUnconfirmed / NoLowLevelRetryError (protondrive.go:366): operations.Copy won't retry immediately, which is correct. A whole-sync --retries pass relists, and since listing lag isn't modelled (docs/TODO.md) a stale listing re-uploads, creating an extra revision of identical content. That wastes work but loses no data. The DECISIONS entry could mention it.
4. TestUploadReadBackLagOutlasted (upload_internal_test.go:822) and the errUploadUnconfirmed row of TestTranslateError were written after the fix and never seen failing (the report says so). By reasoning they do fail without it: before the fix the stale case gives superseded and the missing case gives a wrapped errNotFound.
5. With caching off, a commit that failed and whose node is genuinely gone (deleted elsewhere mid-upload) now takes about 7 s to give outcome-unknown. This is rare and harmless.

Checks run: one scratch-worktree test (described in Critical 1), reasoning over shouldRetry (nodes.go:256-280), deleteDraft/deleteDraftLink (upload.go ~927-955), newNodeCache(false) == nil (cache.go:29), and the partial-upload removal in fs/operations/copy.go (not reached: protondrive doesn't set PartialUploads). Budget and normal path: readBack makes a single fetchNode when the first read shows the change, so it adds no request or sleep.

Assessment

Task quality: Needs fixes

Reasoning: The lag handling on the success path is correct, bounded, costs nothing on the normal path and is well tested. But the failed-commit path still treats one read, now known to be possibly stale, as proof, and sends a delete against a file that may be committed. That breaks ruling 11, which the task told it to keep.