protondrive: never delete an upload's draft unless Proton refused the commit
After a failed commit the client read the file back and deleted the draft if the read showed it uncommitted. Proton's server can take a moment to show a commit, so a stale read could send a delete against a file or revision which had in fact been committed. The draft is now deleted only when the server refused the commit with a 4xx API error on a single attempt which was not resent; any other failure leaves it and reports that the upload may have succeeded. A cancelled upload whose commit succeeded stops waiting for the server to show it at once. The fake server's read lag can also serve a file without an active revision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
1b2fcbd212
commit
be00536078
9 files changed
+261
-76
No files matched your search
@@ -1293,7 +1293,7 @@ func TestCopyReadBackLag(t *testing.T) {
|
||||
fileID := u.SeedFile(u.RootID(), "f.txt", []byte("content"), time.Now())
|
||||
file, err := c.getNode(ctx, fileID)
|
||||
require.NoError(t, err)
|
||||
srv.SetReadLag(2, true)
|
||||
srv.SetReadLag(2, protontest.LagMissing)
|
||||
cp, err := c.copyNode(ctx, file, u.RootID(), "copy.txt")
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, "copy.txt", cp.Name)
|
||||
|
||||
@@ -633,7 +633,7 @@ func TestPutNestedReadBackLag(t *testing.T) {
|
||||
srv := protontest.New(t)
|
||||
u := srv.AddUser("user", "pass")
|
||||
f := newTestFs(t, sessionConfig(srv, u))
|
||||
srv.SetReadLag(2, true)
|
||||
srv.SetReadLag(2, protontest.LagMissing)
|
||||
o := putFile(t, f, "A1/B1/C1/one", []byte("aaa"))
|
||||
assert.Equal(t, int64(3), o.Size())
|
||||
}
|
||||
@@ -1711,31 +1711,49 @@ func (s *Server) handleLinks(w http.ResponseWriter, r *http.Request, u *User, d
|
||||
if v.reads--; v.reads <= 0 {
|
||||
delete(s.lagging, id)
|
||||
}
|
||||
if v.before == nil {
|
||||
switch {
|
||||
case v.view == LagNoRevision && details.File != nil:
|
||||
details.File.ActiveRevision = nil
|
||||
case v.view == LagMissing || v.before == nil:
|
||||
continue
|
||||
default:
|
||||
details = *v.before
|
||||
}
|
||||
details = *v.before
|
||||
}
|
||||
resp.Links = append(resp.Links, details)
|
||||
}
|
||||
writeJSON(w, http.StatusOK, resp)
|
||||
}
|
||||
|
||||
// LagView is what a lagging read of a changed node sees.
|
||||
type LagView int
|
||||
|
||||
const (
|
||||
// LagBefore shows the node as it was before the change, or leaves
|
||||
// it out if it is new.
|
||||
LagBefore LagView = iota
|
||||
// LagMissing leaves the node out.
|
||||
LagMissing
|
||||
// LagNoRevision shows a file as it is but without an active
|
||||
// revision; a new node is left out.
|
||||
LagNoRevision
|
||||
)
|
||||
|
||||
// lagView is what reads of a node see until its change shows.
|
||||
type lagView struct {
|
||||
reads int // reads left which don't see the change
|
||||
before *linkDetailsJSON // the node before the change; nil leaves it out
|
||||
view LagView // what they see
|
||||
before *linkDetailsJSON // the node before the change; nil if it is new
|
||||
}
|
||||
|
||||
// SetReadLag makes the first reads reads of a node through the links
|
||||
// route, after it is created, copied or has a revision committed, not
|
||||
// see the change, as Proton's servers may not for a moment: they leave
|
||||
// the node out if it is new or missing is set, and otherwise show it as
|
||||
// it was before. Zero, the default, turns the lag off.
|
||||
func (s *Server) SetReadLag(reads int, missing bool) {
|
||||
// route, after it is created, copied or has a revision committed, see
|
||||
// it as view says instead of as it is, as Proton's servers may for a
|
||||
// moment. Zero reads, the default, turns the lag off.
|
||||
func (s *Server) SetReadLag(reads int, view LagView) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
s.readLag, s.readLagMissing = reads, missing
|
||||
s.readLag, s.readLagView = reads, view
|
||||
}
|
||||
|
||||
// lagLocked starts the read lag, if any, of node id after a change;
|
||||
@@ -1745,10 +1763,7 @@ func (s *Server) lagLocked(id string, before *linkDetailsJSON) {
|
||||
if s.readLag <= 0 {
|
||||
return
|
||||
}
|
||||
if s.readLagMissing {
|
||||
before = nil
|
||||
}
|
||||
s.lagging[id] = &lagView{reads: s.readLag, before: before}
|
||||
s.lagging[id] = &lagView{reads: s.readLag, view: s.readLagView, before: before}
|
||||
}
|
||||
|
||||
// handleChildren returns a page of the IDs of the active children of a
|
||||
|
||||
@@ -116,7 +116,7 @@ type Server struct {
|
||||
secrets []issuedSecret // every secret issued, in order
|
||||
keysAs map[string]*User // whose keys GET /core/v4/keys/all serves for an email, overriding the owner's
|
||||
readLag int // reads of a node after a change which don't see it; see SetReadLag
|
||||
readLagMissing bool // lagging reads of a changed node leave it out
|
||||
readLagView LagView // what lagging reads of a changed node see
|
||||
lagging map[string]*lagView // nodes whose reads lag, by ID
|
||||
}
|
||||
|
||||
|
||||
@@ -98,12 +98,12 @@ type draft struct {
|
||||
// errUploadMismatch.
|
||||
//
|
||||
// A failed upload deletes its draft only when nothing may have been
|
||||
// committed: before the commit, or when the file read back after a
|
||||
// failed commit proves it didn't take effect (a new file still a draft,
|
||||
// or the revision the upload started from still active). Otherwise
|
||||
// nothing is deleted and a failed commit is errUploadOutcomeUnknown: the
|
||||
// upload may have succeeded. A commit which succeeded but whose file has
|
||||
// another revision by the time it is read back is errUploadSuperseded,
|
||||
// committed: before the commit, or when the server refused the commit
|
||||
// with a 4xx API error on its only attempt. Otherwise nothing is deleted
|
||||
// and a failed commit whose revision the file read back doesn't show is
|
||||
// errUploadOutcomeUnknown: the upload may have succeeded. A commit which
|
||||
// succeeded but whose file has another revision by the time it is read
|
||||
// back is errUploadSuperseded,
|
||||
// and one the server still doesn't show after a few reads is
|
||||
// errUploadUnconfirmed. The returned node is always the upload's own
|
||||
// revision.
|
||||
@@ -360,8 +360,9 @@ func (res *uploadResult) check(r uploadRequest) error {
|
||||
// uploadDraft uploads the content of r to draft d and commits it,
|
||||
// returning the committed file. committed reports whether the revision
|
||||
// may be committed, even if err is set, when the draft must not be
|
||||
// deleted: committed is false only when the file read back proves the
|
||||
// commit didn't take effect.
|
||||
// deleted: committed is false only when the commit wasn't sent or the
|
||||
// server refused it on its only attempt. What a read shows never proves
|
||||
// a commit didn't take effect: reads may lag.
|
||||
func (c *client) uploadDraft(ctx context.Context, d *draft, r uploadRequest, signKR *crypto.KeyRing) (n *node, committed bool, err error) {
|
||||
code, verifySK, err := c.verificationData(ctx, d)
|
||||
if err != nil {
|
||||
@@ -393,14 +394,27 @@ func (c *client) uploadDraft(ctx context.Context, d *draft, r uploadRequest, sig
|
||||
XAttr: encXAttrs,
|
||||
ChecksumVerified: r.SHA1 != "",
|
||||
}
|
||||
commitErr := c.apiCall(ctx, &rest.Opts{Method: "PUT", Path: c.volumePath("/drive/v2/volumes", "files", d.nodeID, "revisions", d.revisionID)}, req, nil)
|
||||
// The outcome must be known even if ctx is done: deleting the draft
|
||||
// of a commit which took effect would delete the file or revision.
|
||||
readCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), draftCleanupTimeout)
|
||||
defer cancel()
|
||||
n, err = c.readBack(readCtx, d.nodeID, func(n *node, err error) bool {
|
||||
return !d.lagging(n, err, commitErr == nil)
|
||||
opts := &rest.Opts{Method: "PUT", Path: c.volumePath("/drive/v2/volumes", "files", d.nodeID, "revisions", d.revisionID)}
|
||||
attempts := 0
|
||||
commitErr := c.pacedCall(ctx, c.apiPacer, func() (*http.Response, error) {
|
||||
attempts++
|
||||
return c.s.call(ctx, opts, req, nil)
|
||||
})
|
||||
if commitErr != nil && attempts == 1 && refusedCommit(commitErr) {
|
||||
c.forget(d.nodeID, d.parentID)
|
||||
return nil, false, fmt.Errorf("commit: %w", commitErr)
|
||||
}
|
||||
// Whether a failed commit took effect is read even if ctx is done,
|
||||
// though only once then, as a commit which did is a success.
|
||||
readCtx, cancel := context.WithTimeout(ctx, draftCleanupTimeout)
|
||||
defer cancel()
|
||||
if commitErr != nil && ctx.Err() != nil {
|
||||
detached, cancelDetached := context.WithTimeout(context.WithoutCancel(ctx), draftCleanupTimeout)
|
||||
defer cancelDetached()
|
||||
n, err = c.fetchNode(detached, d.nodeID)
|
||||
} else {
|
||||
n, err = c.readBack(readCtx, d.nodeID, func(n *node, err error) bool { return !d.lagging(n, err) })
|
||||
}
|
||||
ours := err == nil && n.State == api.LinkStateActive && n.RevisionID == d.revisionID
|
||||
if ours {
|
||||
c.cache.add(n)
|
||||
@@ -415,29 +429,36 @@ func (c *client) uploadDraft(ctx context.Context, d *draft, r uploadRequest, sig
|
||||
return n, true, nil
|
||||
case ours:
|
||||
return n, true, nil
|
||||
case commitErr == nil && d.lagging(n, err, true):
|
||||
case commitErr == nil && (d.lagging(n, err) || ctx.Err() != nil):
|
||||
fs.Debugf(nil, "protondrive: node %s: revision %s committed, but the server doesn't show it yet", d.nodeID, d.revisionID)
|
||||
return nil, true, errUploadUnconfirmed
|
||||
case commitErr == nil && err != nil:
|
||||
return nil, true, fmt.Errorf("read the uploaded file: %w", err)
|
||||
case commitErr == nil:
|
||||
return nil, true, fmt.Errorf("%w (revision %s is active, not %s)", errUploadSuperseded, n.RevisionID, d.revisionID)
|
||||
case err == nil && d.notCommitted(n):
|
||||
return nil, false, fmt.Errorf("commit: %w", commitErr)
|
||||
case err != nil:
|
||||
// Any draft left is replaced by the next upload of the name.
|
||||
return nil, true, fmt.Errorf("commit: %w: %w (reading the file failed: %v)", errUploadOutcomeUnknown, commitErr, err)
|
||||
}
|
||||
// Another client changed the file since, so whether the commit took
|
||||
// effect can't be told: a deletion could remove committed content.
|
||||
return nil, true, fmt.Errorf("commit: %w: %w (the file has changed since)", errUploadOutcomeUnknown, commitErr)
|
||||
// A read may lag, or the file have changed since, so what it shows
|
||||
// doesn't prove the commit didn't take effect.
|
||||
return nil, true, fmt.Errorf("commit: %w: %w (the file doesn't show the revision)", errUploadOutcomeUnknown, commitErr)
|
||||
}
|
||||
|
||||
// refusedCommit reports whether err, from a commit sent only once, is
|
||||
// the server refusing it, so that it didn't take effect: an API error
|
||||
// with a 4xx status other than 408 and a Proton code.
|
||||
func refusedCommit(err error) bool {
|
||||
var apiErr *api.Error
|
||||
return errors.As(err, &apiErr) && apiErr.Code != 0 &&
|
||||
apiErr.Status >= 400 && apiErr.Status < 500 && apiErr.Status != http.StatusRequestTimeout
|
||||
}
|
||||
|
||||
// lagging reports whether n and err, read after the commit of d, may
|
||||
// be from before the server shows the commit: the node isn't found, or
|
||||
// is active without an active revision, or, if the commit is known to
|
||||
// have succeeded, looks as it did before it.
|
||||
func (d *draft) lagging(n *node, err error, committed bool) bool {
|
||||
// be from before the server shows the commit: the node isn't found, is
|
||||
// active without an active revision, or looks as it did before (a new
|
||||
// file still a draft, or the revision d started from still active).
|
||||
func (d *draft) lagging(n *node, err error) bool {
|
||||
switch {
|
||||
case errors.Is(err, errNotFound):
|
||||
return true
|
||||
@@ -445,15 +466,7 @@ func (d *draft) lagging(n *node, err error, committed bool) bool {
|
||||
return false
|
||||
case n.State == api.LinkStateActive && n.RevisionID == "":
|
||||
return true
|
||||
}
|
||||
return committed && d.notCommitted(n)
|
||||
}
|
||||
|
||||
// notCommitted reports whether n, read after a failed commit of d,
|
||||
// proves the commit didn't take effect: a new file still a draft, or a
|
||||
// file whose active revision is still the one d started from.
|
||||
func (d *draft) notCommitted(n *node) bool {
|
||||
if d.newFile {
|
||||
case d.newFile:
|
||||
return n.State == api.LinkStateDraft
|
||||
}
|
||||
return n.State == api.LinkStateActive && n.RevisionID == d.baseRevisionID
|
||||
|
||||
@@ -343,12 +343,13 @@ func TestUploadForeignDraft(t *testing.T) {
|
||||
|
||||
func TestUploadCancel(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
fault protontest.Fault
|
||||
name string
|
||||
fault protontest.Fault
|
||||
drafts int // drafts left: a cancelled commit may have taken effect
|
||||
}{
|
||||
{"draft", protontest.Fault{Method: "POST", Pattern: filesPattern}},
|
||||
{"blocks", protontest.Fault{Method: "POST", Pattern: storageUploadPattern, Skip: 1}},
|
||||
{"commit", protontest.Fault{Method: "PUT", Pattern: commitPattern}},
|
||||
{"draft", protontest.Fault{Method: "POST", Pattern: filesPattern}, 0},
|
||||
{"blocks", protontest.Fault{Method: "POST", Pattern: storageUploadPattern, Skip: 1}, 0},
|
||||
{"commit", protontest.Fault{Method: "PUT", Pattern: commitPattern}, 1},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
c, srv, u := newUploadClient(t, clientOptions{})
|
||||
@@ -361,7 +362,7 @@ func TestUploadCancel(t *testing.T) {
|
||||
_, err := uploadContent(ctx, c, "f", bytes.NewReader(testContent(9<<20)), 9<<20)
|
||||
assert.ErrorIs(t, err, context.DeadlineExceeded)
|
||||
assert.Less(t, time.Since(start), 10*time.Second)
|
||||
assert.Empty(t, u.Drafts())
|
||||
assert.Len(t, u.Drafts(), tc.drafts)
|
||||
assert.Empty(t, files(u))
|
||||
requireNoLeaks(t, srv, goroutines, buffers)
|
||||
})
|
||||
@@ -544,13 +545,14 @@ func TestUploadCancelDuringCommit(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestUploadCommitOutcomeUnknown makes the commit fail and the read
|
||||
// which would tell whether it took effect fail too: nothing may be
|
||||
// deleted, and the error must say the outcome is unknown.
|
||||
// TestUploadCommitOutcomeUnknown makes the commit fail without a
|
||||
// refusal and the read which would tell whether it took effect fail
|
||||
// too: nothing may be deleted, and the error must say the outcome is
|
||||
// unknown.
|
||||
func TestUploadCommitOutcomeUnknown(t *testing.T) {
|
||||
t.Parallel()
|
||||
c, srv, u := newUploadClient(t, clientOptions{})
|
||||
srv.Inject(protontest.Fault{Method: "PUT", Pattern: commitPattern, Status: http.StatusUnprocessableEntity, Code: api.CodeInvalidValue})
|
||||
srv.Inject(protontest.Fault{Method: "PUT", Pattern: commitPattern, Status: http.StatusUnprocessableEntity})
|
||||
srv.Inject(protontest.Fault{Method: "POST", Pattern: linksPattern, Status: http.StatusUnprocessableEntity, Code: api.CodeInvalidValue})
|
||||
_, err := uploadContent(context.Background(), c, "f", strings.NewReader("content"), 7)
|
||||
require.Error(t, err)
|
||||
@@ -763,13 +765,15 @@ func TestUploadReadBackLag(t *testing.T) {
|
||||
content := []byte("content")
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
missing bool
|
||||
view protontest.LagView
|
||||
existing bool
|
||||
}{
|
||||
{name: "NewFileStale"},
|
||||
{name: "NewFileMissing", missing: true},
|
||||
{name: "NewRevisionStale", existing: true},
|
||||
{name: "NewRevisionMissing", existing: true, missing: true},
|
||||
{name: "NewFileStale", view: protontest.LagBefore},
|
||||
{name: "NewFileMissing", view: protontest.LagMissing},
|
||||
{name: "NewFileNoRevision", view: protontest.LagNoRevision},
|
||||
{name: "NewRevisionStale", view: protontest.LagBefore, existing: true},
|
||||
{name: "NewRevisionMissing", view: protontest.LagMissing, existing: true},
|
||||
{name: "NewRevisionNoRevision", view: protontest.LagNoRevision, existing: true},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
@@ -781,7 +785,7 @@ func TestUploadReadBackLag(t *testing.T) {
|
||||
r.Existing = first
|
||||
}
|
||||
srv.ResetRequests()
|
||||
srv.SetReadLag(2, tc.missing)
|
||||
srv.SetReadLag(2, tc.view)
|
||||
n, err := c.upload(context.Background(), r)
|
||||
require.NoError(t, err)
|
||||
assert.NotEmpty(t, n.RevisionID)
|
||||
@@ -801,7 +805,7 @@ func TestUploadReadBackLag(t *testing.T) {
|
||||
func TestUploadIntoNewFolderReadBackLag(t *testing.T) {
|
||||
t.Parallel()
|
||||
c, srv, u := newTestClientOpt(t, clientOptions{DraftClientUID: testDraftClientUID})
|
||||
srv.SetReadLag(2, true)
|
||||
srv.SetReadLag(2, protontest.LagMissing)
|
||||
ctx := context.Background()
|
||||
parentID := c.rootID()
|
||||
for _, name := range []string{"A1", "B1", "C1"} {
|
||||
@@ -821,12 +825,12 @@ func TestUploadIntoNewFolderReadBackLag(t *testing.T) {
|
||||
// fails as unconfirmed and nothing is deleted.
|
||||
func TestUploadReadBackLagOutlasted(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, missing := range []bool{false, true} {
|
||||
t.Run(fmt.Sprintf("missing=%v", missing), func(t *testing.T) {
|
||||
for _, view := range []protontest.LagView{protontest.LagBefore, protontest.LagMissing, protontest.LagNoRevision} {
|
||||
t.Run(fmt.Sprintf("view=%d", view), func(t *testing.T) {
|
||||
t.Parallel()
|
||||
c, srv, u := newUploadClient(t, clientOptions{})
|
||||
c.readBackDelay = time.Millisecond
|
||||
srv.SetReadLag(readBackAttempts, missing)
|
||||
srv.SetReadLag(readBackAttempts, view)
|
||||
n, err := uploadContent(context.Background(), c, "f", strings.NewReader("mine"), 4)
|
||||
assert.ErrorIs(t, err, errUploadUnconfirmed)
|
||||
assert.NotErrorIs(t, err, errUploadSuperseded)
|
||||
@@ -837,3 +841,114 @@ func TestUploadReadBackLagOutlasted(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// commitFailure returns a client of u whose next commit lands on srv
|
||||
// and then fails.
|
||||
type commitFailure func(t *testing.T, srv *protontest.Server, u *protontest.User) *client
|
||||
|
||||
// TestUploadAmbiguousCommitFailureUnderLag makes a commit land on the
|
||||
// server and then fail in a way which doesn't prove it didn't, while
|
||||
// the read back shows the node as before the commit: nothing may be
|
||||
// deleted, as the deletion could hit the committed file or revision.
|
||||
func TestUploadAmbiguousCommitFailureUnderLag(t *testing.T) {
|
||||
t.Parallel()
|
||||
failures := map[string]commitFailure{
|
||||
// The commit lands and its response is lost; the resent commit
|
||||
// is refused, as the draft revision is gone.
|
||||
"ResentThenRefused": func(t *testing.T, srv *protontest.Server, u *protontest.User) *client {
|
||||
srv.Inject(protontest.Fault{Method: "PUT", Pattern: commitPattern, Status: http.StatusBadGateway, AfterHandler: true, Times: 1})
|
||||
srv.Inject(protontest.Fault{Method: "PUT", Pattern: commitPattern, Status: http.StatusUnprocessableEntity, Code: api.CodeInvalidValue, AfterHandler: true, Skip: 1, Times: 1})
|
||||
return newTestClientFor(t, srv, u, clientOptions{DraftClientUID: testDraftClientUID})
|
||||
},
|
||||
// The commit lands and the connection drops before its response.
|
||||
"ConnectionLost": func(t *testing.T, srv *protontest.Server, u *protontest.User) *client {
|
||||
c, armed := interveneAfterCommit(t, srv, u, false, func(string, string) {})
|
||||
armed.Store(true)
|
||||
return c
|
||||
},
|
||||
}
|
||||
for name, fail := range failures {
|
||||
for _, existing := range []bool{false, true} {
|
||||
t.Run(fmt.Sprintf("%s/existing=%v", name, existing), func(t *testing.T) {
|
||||
t.Parallel()
|
||||
srv := protontest.New(t)
|
||||
u := srv.AddUser("user", "pass")
|
||||
var first *node
|
||||
if existing {
|
||||
c := newTestClientFor(t, srv, u, clientOptions{DraftClientUID: testDraftClientUID})
|
||||
first = uploadBytes(t, c, "f", []byte("first"))
|
||||
}
|
||||
c := fail(t, srv, u)
|
||||
c.readBackDelay = time.Millisecond
|
||||
srv.ResetRequests()
|
||||
srv.SetReadLag(readBackAttempts, protontest.LagBefore)
|
||||
r := uploadRequest{ParentID: c.rootID(), Name: "f", Existing: first, In: strings.NewReader("mine"), Size: 4, ModTime: testModTime}
|
||||
_, err := c.upload(context.Background(), r)
|
||||
assert.ErrorIs(t, err, errUploadOutcomeUnknown)
|
||||
requireNoDeletes(t, srv)
|
||||
requireUploaded(t, u, map[string][]byte{"f": []byte("mine")})
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestUploadCommitRefusedNoReadBack makes the server refuse a commit on
|
||||
// its only attempt: the refusal alone proves it didn't land, so the
|
||||
// draft is deleted without reading the file back, as a read may lag.
|
||||
func TestUploadCommitRefusedNoReadBack(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, existing := range []bool{false, true} {
|
||||
t.Run(fmt.Sprintf("existing=%v", existing), func(t *testing.T) {
|
||||
t.Parallel()
|
||||
c, srv, u := newUploadClient(t, clientOptions{})
|
||||
var first *node
|
||||
want := map[string][]byte{}
|
||||
if existing {
|
||||
first = uploadBytes(t, c, "f", []byte("first"))
|
||||
want["f"] = []byte("first")
|
||||
}
|
||||
srv.ResetRequests()
|
||||
srv.Inject(protontest.Fault{Method: "PUT", Pattern: commitPattern, Status: http.StatusUnprocessableEntity, Code: api.CodeInvalidValue, Times: 1})
|
||||
_, err := c.upload(context.Background(), uploadRequest{ParentID: c.rootID(), Name: "f", Existing: first, In: strings.NewReader("mine"), Size: 4, ModTime: testModTime})
|
||||
require.Error(t, err)
|
||||
assert.NotErrorIs(t, err, errUploadOutcomeUnknown)
|
||||
reqs := srv.Requests()
|
||||
assert.Equal(t, 1, reqs["PUT "+commitPattern])
|
||||
assert.Zero(t, reqs["POST "+linksPattern], "file read back")
|
||||
assert.Equal(t, 1, reqs[deleteDraftsRequest]+reqs[deleteRevisionRequest], "draft not deleted")
|
||||
assert.Empty(t, u.Drafts())
|
||||
requireUploaded(t, u, want)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestUploadCommittedThenCancelledUnderLag cancels an upload once its
|
||||
// commit has succeeded and the first read back lagged: it returns at
|
||||
// once as unconfirmed and deletes nothing.
|
||||
func TestUploadCommittedThenCancelledUnderLag(t *testing.T) {
|
||||
t.Parallel()
|
||||
srv := protontest.New(t)
|
||||
u := srv.AddUser("user", "pass")
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
c := newTestClientTransport(t, srv, u, clientOptions{DraftClientUID: testDraftClientUID}, func(next http.RoundTripper) http.RoundTripper {
|
||||
var committed atomic.Bool
|
||||
return roundTripFunc(func(r *http.Request) (*http.Response, error) {
|
||||
resp, err := next.RoundTrip(r)
|
||||
switch {
|
||||
case r.Method == "PUT" && strings.Contains(r.URL.Path, "/revisions/"):
|
||||
committed.Store(true)
|
||||
case committed.Load() && strings.HasSuffix(r.URL.Path, "/links"):
|
||||
cancel()
|
||||
}
|
||||
return resp, err
|
||||
})
|
||||
})
|
||||
srv.SetReadLag(readBackAttempts, protontest.LagBefore)
|
||||
start := time.Now()
|
||||
_, err := uploadContent(ctx, c, "f", strings.NewReader("mine"), 4)
|
||||
assert.ErrorIs(t, err, errUploadUnconfirmed)
|
||||
assert.Less(t, time.Since(start), 2*time.Second, "kept reading after cancel")
|
||||
requireNoDeletes(t, srv)
|
||||
requireUploaded(t, u, map[string][]byte{"f": []byte("mine")})
|
||||
}
|
||||
@@ -5,6 +5,35 @@ it with a new one that links back. One entry per decision a future maintainer
|
||||
would otherwise have to reverse-engineer.
|
||||
No index: `grep -n '^## ' docs/DECISIONS.md` lists every entry.
|
||||
|
||||
## 2026-11-12 - Proton Drive deletes an upload's draft only on an explicit refusal
|
||||
|
||||
**Status:** accepted. Supersedes the read-back half of handoff ruling 11
|
||||
("a draft is deleted only on positive proof it never committed", where a
|
||||
read showing the draft or the old revision counted as proof).
|
||||
**Context:** Proton's server can take a moment to show a commit (entry
|
||||
below). After a failed commit, one stale read showing the draft or the
|
||||
base revision made the client delete the draft, but the commit may
|
||||
have landed: `delete_multiple` against what is now an active file, or
|
||||
`DELETE .../revisions/{id}` against what is now the active revision. What
|
||||
the real server does with those is unconfirmed, so this could lose data.
|
||||
**Decision:** The draft is deleted only when the server explicitly
|
||||
refused the commit, meaning a 4xx `api.Error` (not 408) with a Proton code,
|
||||
on a single attempt that the pacer did not resend. A resend after
|
||||
5xx/408/network/timeout makes the outcome unknown even if the final
|
||||
answer is a refusal, because an earlier attempt may have landed. No
|
||||
read-back decides deletion, and a refusal is not read back at all. Every
|
||||
other failure (5xx, network, timeout, cancelled ctx, resent attempts)
|
||||
returns `errUploadOutcomeUnknown` and deletes nothing. The read after
|
||||
such a failure can still turn it into a success when it shows the
|
||||
upload's revision. It retries through lag while the caller's ctx lives,
|
||||
and reads once, detached, if ctx is done.
|
||||
**Alternatives:** Requiring every read across the whole lag window to
|
||||
show the pre-commit state (a heuristic, as the lag length is unmeasured);
|
||||
the old single-read proof.
|
||||
**Consequences:** More drafts are left behind after ambiguous failures.
|
||||
The next upload of the name replaces them, otherwise they stay (see
|
||||
`docs/TODO.md`). A refused commit fails at once, with no reads.
|
||||
|
||||
## 2026-11-12 - Proton Drive uploads tolerate read-after-write lag, but never invent the result
|
||||
|
||||
**Status:** accepted
|
||||
|
||||
+14
-4
@@ -161,10 +161,20 @@ Small, specific, independently actionable. Remove only when actually done.
|
||||
write into (`targetFolder`) retry, for about 7 s. Not tolerant yet, and the next
|
||||
symptoms to expect: `activeNode` (Update, Move, Copy, DirMove of an
|
||||
object just written) fails "object not found" while the node is
|
||||
missing or still a draft; Move's read of the moved node may be stale;
|
||||
after a commit that *failed*, a stale read showing the draft or the old
|
||||
revision still counts as proof the commit didn't land and the draft is
|
||||
deleted (retrying there would slow every failed commit by 7 s).
|
||||
missing or still a draft; Move's read of the moved node may be stale.
|
||||
- [ ] protondrive: drafts left after an ambiguous commit failure (5xx,
|
||||
network error, timeout, cancel, or a refusal after the commit was
|
||||
resent) are never deleted by the client, by design (see DECISIONS,
|
||||
"a draft is deleted only on an explicit refusal"). The next upload of
|
||||
the same name by this remote replaces its own draft, but a name never
|
||||
uploaded again keeps an orphan draft (a new file's draft node, or a
|
||||
draft revision) taking quota. `CleanUp` only empties the trash. Find
|
||||
out whether Proton expires drafts by itself, and consider having
|
||||
`CleanUp` delete this remote's own drafts (by `DraftClientUID`).
|
||||
- [ ] protondrive: `errUploadUnconfirmed` is not retried at once, but a
|
||||
whole-sync `--retries` pass lists again; if the listing still lags it
|
||||
uploads the file again, adding a revision with identical content
|
||||
(wasted work and version history, no data loss).
|
||||
|
||||
- [ ] protondrive: several `Fs` of one account in one process each have
|
||||
their own node cache; only Move/DirMove sources are invalidated across
|
||||
|
||||
@@ -371,8 +371,11 @@ in summary but has not confirmed them one by one.
|
||||
extended attributes written by the client.
|
||||
10. An honoured `Retry-After` is capped at 60 s.
|
||||
11. Upload commit outcome unknown: delete nothing, return
|
||||
`errUploadOutcomeUnknown`. A draft is deleted only on positive proof it
|
||||
never committed.
|
||||
`errUploadOutcomeUnknown`. A draft is deleted only when the server
|
||||
explicitly refused the commit (a 4xx with a Proton code, not 408) on
|
||||
a single attempt the pacer did not resend; no read-back decides
|
||||
deletion, as reads may lag (Task 21f fix round, superseding "positive
|
||||
proof" from a read).
|
||||
12. The fake reports a draft's owner in conflict details but does not gate
|
||||
routes by `ClientUID`.
|
||||
13. `Proton-API-Bridge` stays in `go.mod` until Task 19 even though no Go
|
||||
|
||||
Reference in new issue
Block a user