PveHttpClientLockRetryTests set GuestLockRetry's retry budget via reflection
on a private field with no production writer, and scripted two lock
failures to land inside a 400ms window. A cold or loaded CI runner's
first-attempt JIT and scheduling could burn past 400ms before the second
attempt started, failing the test though the retry itself was correct
(#134).
GuestLockRetry.ExecuteAsync gains an internal overload that takes the
inter-attempt delay as a Func<TimeSpan, Task>; the public overload keeps
defaulting to Task.Delay, so production behaviour (45s window, budget/4
capped at 2s) is unchanged. PveHttpClient gains a matching internal
constructor seam (window, handler, delay), replacing the reflection the
tests used for both the private HttpClient and the retry window. Tests
now pass a no-op delay, so the retry loop's real elapsed time drops to
microseconds and the production 45s window can never be exhausted by
runner speed.
Two tests pin that production still waits for real: one records the
delay invocations through the internal seam and asserts the computed
interval, the other drives the public overload with a small window and
asserts wall-clock time actually advances. Both were mutation-tested
against a no-op-default regression and fail without the fix.
The give-up test was renamed (PutAsync_DoesNotReissueWhenTheRetryWindowIsAlreadySpent)
to describe what TimeSpan.Zero actually proves: the client never attempts
a reissue once the budget reads spent, not a multi-attempt exhaustion
sequence — a review finding on the original name.
Out of scope, noted for follow-up: GuestLockRetryTests.cs's synchronous
Execute() tests still use a 400ms ShortWindow with real Thread.Sleep,
which is the same flake shape on the sync path; Execute() has no delay
seam. PveHttpClientTimeoutTests.cs and PveHttpClientFormEncodingTests.cs
still reflect on the private _httpClient field, which the new handler
seam could also retire.
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
Two non-blocking review observations.
A 45s retry is indistinguishable from a hang with nothing on the wire, so
GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports
each reissue through WriteVerbose.
The gap between attempts now scales with the budget, capped at the 2s
production value. A caller passing a short window wants a fast answer
rather than one long sleep, which also takes the retrying unit tests off
a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's
window becomes a field so those tests can shorten it too.
WaitForStatusTransition refused to return while snapshot.Locked, and its
comment quoted the exact error it was meant to prevent. Locked reads the
guest config's lock: property; the failure is the flock on
/var/lock/qemu-server/lock-<vmid>.conf, which PVE exposes nowhere.
The flock cannot be observed, so it is retried. GuestLockRetry reissues an
operation for a bounded 45s while PVE reports failing to enter lock_config
for a guest, which it raises before doing any work.
Two seams, because the failure has two surfaces. PveHttpClient.SendAsync
retries the request for operations PVE serialises in the API handler; it
takes a request factory because an HttpRequestMessage cannot be resent.
PveCmdletBase.InvokeGuestTask reissues the call and re-waits its task for
operations serialised in the forked worker, where the POST returns 200 and
only the task fails. WaitForStatusTransition routes through the latter,
hence Func<PveTask>.
The predicate is path-specific and anchored at the start of what PVE said:
lock_file uses identical wording for storage, LVM and HA locks, and a
qmclone that fails after allocating disks must not be reissued into
"VM already exists". That requires the raw text, so it reads
PveTaskFailedException.ExitStatus and PveApiException.ApiMessage.
The Locked check stays — it is correct for the config lock — with a comment
that says so.
Closes#113
goodolclint-claude[bot]