Commit Graph

5 Commits

Author SHA1 Message Date
Feng Ruohang 54186040ca test(restore): release the handed-over restore lock before reusing options
Three restore tests record the options of a POST's background restore
with a stub and restore with them later: TestRestoreAttemptGenerationFencing,
TestExpireRestoredUsesQuorumGeneration and TestRestoreHealNeedsRestoreQuorum.
The options carry the restore lock that POST hands to its worker. The
handler releases that lock only after the stub returns, while the test
proceeds as soon as the stub has signaled. When the test's restore
checked the lock before that release, it kept the lock, and the release
then canceled its context: every drive read failed with "context
canceled" and the restore reported insufficient read quorum.

Under -race this failed 11 times in 60 runs of the three tests on
69a4f938a, in the worker and old-reset modes of the fencing test and in
the multi-pool expiry test.

A new helper, restoreCaptureOptions, releases the lock before it records
the options, so a later restore takes it again, as a queued worker whose
lock ended does. TestRestoreSupersededAttempt already does this. With
the helper, the three tests passed 60 runs under -race.

Production code does not change: the handler's goroutine runs the
restore and releases the lock after it, and a lock lost during a
restore stops it as intended.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-06 16:42:08 +08:00
Feng Ruohang 45b1e4bc00 fix(restore): stage native restores outside the object lock
The unreleased native-restore fixes after RELEASE.2026-10-26T00-00-00Z
let the lock decide who owns a restore. Since fa42a0982 a single-pool
worker holds the object lock for the whole tier download, and since
7f38bf628 a busy lock means "a restore runs". 7f38bf628 also keeps the
restore's data in a marked multipart upload. Two regressions followed:

- While an old version restored, every GET, HEAD and write of the same
  key waited for the download. On a single pool this is new; with
  several pools the released code already held the lock that long.
- An ordinary upload whose completion failed after staging its metadata
  became InvalidUploadID for every caller, and could not be aborted for
  an hour. The marker that hid restore uploads could not tell them apart
  once a completion had rewritten the upload's xl.meta.

A restore now runs in two phases:

- The worker erasure-codes the tier copy into the tmp volume of the
  version's set, without the object lock, as heal does. A drive that
  fails any part, or misses a part when CheckParts runs before the
  commit, takes no part in the commit.
- A restore lock per version tells whether a restore runs. POST takes it
  before the object lock and hands it to the worker. It is a liveness
  hint: only the checks under the object lock decide what is written.
- The commit takes the object lock briefly, on one pool or several. It
  reads the version again and checks the attempt's ownership, the
  version identity, the tier copy, the part layout and the erasure
  layout. It then publishes with RenameData drive by drive, never
  through erasure renameData's undo, and needs the write quorum. The
  replaced data directory goes only when every published drive reports
  the same one.
- A publication that reached some drives but not the write quorum
  converges under the same lock hold, in bounded retries after it: back
  to the unrestored version on every drive that holds it, or to a
  committed restore when the copy turns out to be on a write quorum.
  Each pass first checks that every drive holds what the version held
  before the commit, the commit wrote, or a convergence wrote, and stops
  on anything else. It deletes no directory.

Multipart goes back to main: erasure-multipart.go and
bucket-object-lock.go are identical to 69fcf50cd. The commit lock, the
marker and the restore exceptions of multipart completion and of the
null-version Object Lock check are gone, and single-pool completion
again takes the upload lock first and the object lock right before it
commits. With several pools, abandoned-data cleanup now takes the pools
lock that writes take, as heal does, so it cannot remove a directory
that a commit moved in place and has not yet referenced. The restore
capability becomes protocol 2, and a queued worker checks it again.

Recovery after a coordinator crash during publication, and storage RPCs
that complete after their caller returned, stay excluded. Main has the
same exposure, and they are tracked separately.

Tests:
- New regression tests cover both problems. They fail on f85d4a5fa,
  and the multipart one also passes on main.
- Fault-injection tests cover a publication below the write quorum,
  including seven of twelve drives on 8+4; lost answers; convergence
  retries and stops; the old data directory; a stale inline marker; a
  recreated tmp volume; a published revision read across its expiry; a
  second POST; a target deleted during the download; a tier 404; and
  pools-locked abandoned cleanup.
- Thirteen tests of the upload marker from 7f38bf628 are dropped, and
  so is TestNativeRestoreConcurrentCompletion from fa42a0982, which
  completed the restore's upload from a client. Four tests are reframed
  for the new semantics, including the lock-domain restore test, which
  now gates the commit.
- Three self-recursive WriteMetadata test doubles are fixed, and the
  helpers of the dropped tests are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-06 11:30:29 +08:00
Feng Ruohang f85d4a5fa1 test: satisfy acceptance lint and compatibility checks
Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-05 16:04:56 +08:00
Feng Ruohang 67a6532dec test(restore): give native-restore liveness waits one minute
The native-restore tests waited 10 seconds for steps that do real drive
work: metadata writes with synchronous persistence and renames on every
drive. A full suite run on a machine whose SSD was busy with another
decommission run failed TestRestoreUploadStagingFailsClosed/staged once
with "background restore did not finish"; about 430 reruns, under CPU and
fsync stress as well, passed.

The test uses local locks, and after its release the completion takes no
new namespace lock, so the bound is a liveness bound, not a latency
contract. All eight such waits in the native-restore tests now use one
restoreTestWait of one minute, and the background-restore helper prints
the goroutine stacks when it times out. Assertions that something must not
happen keep their own bounds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-05 11:34:34 +08:00
Feng Ruohang 7f38bf6282 fix(restore): give each native restore an owner and a private upload
A native restore (POST ?restore, issue #281) marks the version ongoing,
and a background worker rehydrates it from the tier. The worker, its
upload and every status write could be reached or confused by other
callers. A public client could write into or complete the restore
upload. POST copied a whole metadata snapshot over newer protection
changes. An old or failed attempt could act on a newer one. Restore,
expiry and heal decided on whichever drive's metadata they read.

Each POST that starts a rehydration now writes an attempt ID into the
version's internal metadata, with the identity it resolved: version
ID, ETag, size and ModTime. The worker, its upload, its part writes and
its completion carry the attempt. A worker whose attempt no longer
owns the version stops without writing, completing or resetting. Every
write uses the versioning state and explicit version ID captured by
POST, so a null target stays null.

The restore upload carries an internal marker, decided over all drives.
Public multipart calls answer NoSuchUpload and listings hide it. An
unknown marker fails closed. A completion holds a commit lock from
staging its commit in the upload until the commit ends. Stale cleanup
decides on every upload past the stale age under that lock, and keeps
it until its rename ends, also past the drive deadline. Abort of a
committing upload takes the lock and removes a failed commit's
leftover only when every copy is staged and older than one hour.

POST, the worker, the reset, the ExpireRestored recheck and heal all
decide on the authoritative revision of the exact version. That is the
revision held by max(n/2+1, read quorum) drives, identified by the
version header and the normalized metadata digest. Status writes patch
only the restore fields of that revision. They write only the drives
that hold it, and put the old revision back on failure. Heal copies
restore fields only when a quorum holds the same fields.

A failed attempt resets itself with bounded in-memory retries. A later
POST takes over a stranded marker when no attempt runs. Otherwise it
answers RestoreAlreadyInProgress within a few seconds.

Older nodes do not know the marker. A new grid capability handler lets
the serving node ask every peer. POST answers 503 ServiceUnavailable
and writes nothing until all peers support native restore, and a
queued worker asks again before it creates its upload.

The single-pool lock-domain split between DELETE/CopyObject and restore
is closed by its own fix, which this change depends on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-05 06:47:38 +08:00