fix(quark): recover ambiguous delete failures - #9651
Draft
NovumOrbis wants to merge 5 commits into
Draft
NovumOrbis wants to merge 5 commits into
NovumOrbis wants to merge 5 commits into
Conversation
Recover the observed Quark `inner error, requestId ...` response from `/file/delete` with a bounded driver-layer retry loop. After an ambiguous provider error, query the immutable FID before replaying the destructive request. If the FID is already absent, treat the requested delete state as satisfied; otherwise retry only the narrowly matched transient. Keep unrelated provider and transport errors fail-closed, propagate request cancellation to the delete call, and cover retry, ambiguity, cancellation, and exact-FID verification with regression tests.
The delete verifier derived "FID absent" from an empty file list, but request only classifies errors it can read from the HTTP status line. A non-JSON gateway page, a 204, or an application-level error delivered with HTTP 200 therefore reached the verifier as a zero-valued response and was read as proof of absence, turning a failed delete into a reported success. That failure is correlated with the exact provider degradation this path exists to recover from. Require positive evidence instead: reject a non-zero code or an error status, and distinguish an explicit empty list from a missing one by making the list a pointer. Absence is now derived only from a well-formed file query. Also propagate the request context to the verifier query so it responds to cancellation like the delete call already does. Retry budget, backoff, the error classifier, action_type, the successful delete path, and the WebDAV layer are unchanged.
Real-provider testing disproved the presence oracle the verifier relied on: GET /file?fids=<fid> does not answer per-FID existence, so an empty list was never evidence that a delete had completed. Resolve the FID through /file/info instead, which answers existence directly. Presence now requires a successful envelope carrying a data object whose fid equals the requested one. Absence requires the exact signature observed against the provider for a FID that no longer exists: HTTP 404 with provider status 404 and code 21001. Everything else fails closed, including an invalid-FID rejection, an auth failure, a rate limit, a gateway page, malformed JSON, a missing data object, and a mismatched fid. Matching the not-found response needs the provider envelope rather than its message text, so request failures now carry the HTTP status alongside the provider status, code and message. Error returns the message unchanged, so existing callers are unaffected. The envelope fields are pointers because a missing status or code must not collapse into a successful zero value. Retry budget, backoff, the error classifier, action_type, the successful delete path, context propagation, and the WebDAV layer are unchanged.
Make delete attempts fail closed on malformed or contradictory responses, and run destructive /file/delete calls through a no-retry Resty wrapper so transport failures return to the delete state machine before any replay. Treat transport failures as ambiguous, verify the immutable FID before retrying, and stop rather than replay if that verification itself fails. Preserve normal transport/TLS/timeout behavior by reusing the underlying concurrency-safe http.Client without mutating the shared Resty client. Also require the actual /file/info HTTP status to be 200 before accepting a PRESENT result, and add regression coverage for malformed delete responses, HTTP/provider disagreement, and production-like transport retry composition.
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
1 participant
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.
Problem
Quark can return a transient provider error from
/file/deleteeven when thedelete may already have been accepted.
In a Synology Hyper Backup -> AList -> Quark workload this can surface as a
failed WebDAV DELETE after the requested state may already have been reached.
The observed transient form is narrowly matched as:
inner error, requestId ...The result is ambiguous: immediately returning the error can report failure
after success, while blindly replaying a destructive request can repeat an
operation that already completed.
Fix
Add bounded recovery for ambiguous Quark delete failures:
inner error ... requestIdprovider errorreplaying the delete
already-completed delete is not converted back into a failure
Removepreviously did not attach the caller context to the/file/deleterequest. This change makes in-flight delete requests honor cancellation as
well as the verification requests, so cancellation behavior changes on the
normal delete path too.
Presence / absence verification
Real-provider testing showed that:
GET /file?fids=<fid>is not a reliable per-FID existence oracle. A FID independently confirmed
present through
/file/sortwas omitted from the returned list: the requestsucceeded with HTTP 200 / provider status 200 / code 0, returned two list
entries, and contained zero exact matches for that known-present FID.
The recovery path therefore uses:
GET /file/info?fid=<fid>with fail-closed semantics.
PRESENT is accepted only when all of these are explicit:
status == 200code == 0dataexistsdata.fidexactly equals the requested FIDABSENT is accepted only for the exact real-provider signature observed for a
real recycle-bin FID:
404status == 404code == 21001A separately tested synthetic invalid FID returned:
400status == 400code == 14001and is treated as an error, not as absence.
Malformed or incomplete envelopes, authentication errors, rate limits,
gateway/server errors, missing data, and mismatched FIDs all fail closed.
The verifier does not infer absence from
"file not found"message text.Request error handling
The Quark request helper previously reduced provider errors to
errors.New(message), losing the HTTP status and provider status/code.It now preserves those fields in an internal structured error while keeping
Error()equal to the original provider message, so existing callers that usethe error text retain their behavior.
Validation
Regression coverage includes:
/file?fids=oracleValidated on the final candidate with:
gofmtgo vet ./drivers/quark_ucgo test ./drivers/quark_ucgo test -race ./drivers/quark_ucgo test -race -shuffle=on -count=2 ./drivers/quark_ucgo test ./internal/opgo test ./server/webdavFinal Quark driver suite: 68 PASS / 0 FAIL.
Provider evidence / residuals
The PRESENT and recycle-bin ABSENT semantics above were verified directly
against the real provider with GET-only probes.
A permanently-purged FID was not independently probed. If Quark represents
that state differently, the verifier fails closed rather than reporting a
false success.
The shared Resty transport-level retry policy is pre-existing and is not
changed by this PR.
Related fixes from the same Hyper Backup incident:
This PR addresses a separate delete-reliability failure mode.