amadan

carlos / platform Public

host: the held-stage lock removes its file when the stage finishes

withHeldStageLock left <DataDir>/.moves/held-<host>.lock behind. Main's
empty-installation census refuses a file naming the host being installed,
so once a box had held a stage for a host, every later from-empty install
of that host there was refused (region HA M-K25, answered on #236).

The holder now removes the file while it still holds the flock
(unlockRemoving), whatever the stage returned; a failed removal is logged
and leaves the file as a crash would. Acquiring re-checks the name after
the flock (fstat of the fd against an fstatat of the name) and starts
again when the name is gone or names another file, so a waiter that
opened the removed file never runs beside the holder of the new one. The
same removal landing inside the open (the name vanishes, or the file
reopened has no link left) is retried too; a symlink, FIFO or second link
at the name is still refused, as is a .moves that has itself gone. The
lock's name is unchanged.

A holder killed mid-stage still leaves the file: the census refuses that
host (region HA's CensusTree(".moves") fallback refuses only that host),
and the next stage of the host locks the same file and removes it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude b87107271ca7e3a82ed446c5c24a0267f08b3068 parent b04ff85
4 files changed, +438 −20
  • internal/host/movesfloor_linux.go +119 −16
  • internal/host/movesfloor_other.go +2 −0
  • internal/host/movesheldlock.go +23 −4
  • internal/host/movesheldlock_cleanup_test.go +294 −0
diff --git a/internal/host/movesfloor_linux.go b/internal/host/movesfloor_linux.go
index 88deb6e81..5a0d583d3 100644
--- a/internal/host/movesfloor_linux.go
+++ b/internal/host/movesfloor_linux.go
@@ -13,6 +13,7 @@ import (
"time"
"github.com/carlosframework/platform/internal/safefile"
+ "golang.org/x/sys/unix"
)
// movesDir is <DataDir>/.moves, opened with O_NOFOLLOW below a data dir
@@ -20,9 +21,10 @@ import (
// symlink the sandboxed agent planted. Every operation on the floor files
// goes through one: the lock and the writes are all relative to its fd.
type movesDir struct {
- fd int
- path string
- lock *os.File
+ fd int
+ path string
+ lock *os.File
+ lockName string
}
// floorLockFile serialises every writer of the floor files (root's apply,
@@ -95,32 +97,133 @@ func (d *movesDir) Lock() error { return d.lockNamed(floorLockFile, floorLockWai
// read-only fd can be flocked), so the agent can lock a file root created
// and root a file the agent created; O_NOFOLLOW and O_NONBLOCK so a symlink
// or a FIFO planted in its place cannot redirect or park root. The fd is
-// kept until Close, which releases the lock. One lock per movesDir.
+// kept until Close (or unlockRemoving), which releases the lock. One lock
+// per movesDir.
+//
+// A holder may remove the file it holds before releasing it
+// (unlockRemoving, the held-stage lock). A waiter that opened that file
+// would then lock a file no longer at the name, beside whoever has locked
+// the new one there. So once it has the flock it checks that the name still
+// names the file it locked (fstat of the fd against an fstatat of the name)
+// and, when the name is gone or names another file, closes it and starts
+// again on whatever is there now. The same removal can land inside the
+// open itself (the name vanishes, or the file opened has no link left);
+// that is retried too, while a symlink, a FIFO or a second link at the name
+// is still refused. Every retry is within the one wait.
func (d *movesDir) lockNamed(name string, wait time.Duration, busy error) error {
- f, err := safefile.OpenFileAt(d.fd, name, os.O_RDONLY|os.O_CREATE, 0o644)
- if err != nil {
- return fmt.Errorf("open %s: %w", filepath.Join(d.path, name), err)
- }
- fd := int(f.Fd())
deadline := time.Now().Add(wait)
for {
- err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB)
- if err == nil {
- d.lock = f
- return nil
+ f, err := safefile.OpenFileAt(d.fd, name, os.O_RDONLY|os.O_CREATE, 0o644)
+ if err != nil {
+ if time.Now().Before(deadline) && d.removedDuringOpen(name, err) {
+ time.Sleep(floorLockPoll)
+ continue
+ }
+ return fmt.Errorf("open %s: %w", filepath.Join(d.path, name), err)
}
- if !errors.Is(err, syscall.EWOULDBLOCK) {
+ lockNamedOpened(name)
+ if err := flockWithin(f, deadline); err != nil {
f.Close()
+ if errors.Is(err, syscall.EWOULDBLOCK) {
+ err = busy
+ }
return fmt.Errorf("lock %s: %w", filepath.Join(d.path, name), err)
}
- if time.Now().After(deadline) {
+ same, err := d.names(name, f)
+ if err != nil {
f.Close()
- return fmt.Errorf("lock %s: %w", filepath.Join(d.path, name), busy)
+ return fmt.Errorf("lock %s: %w", filepath.Join(d.path, name), err)
+ }
+ if same {
+ d.lock, d.lockName = f, name
+ return nil
+ }
+ // The holder before us removed this file as it released it.
+ f.Close()
+ }
+}
+
+// flockWithin takes an exclusive flock on f, polling until deadline. Still
+// held then is EWOULDBLOCK.
+func flockWithin(f *os.File, deadline time.Time) error {
+ for {
+ err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)
+ if err == nil || !errors.Is(err, syscall.EWOULDBLOCK) || time.Now().After(deadline) {
+ return err
}
time.Sleep(floorLockPoll)
}
}
+// names reports whether name in the directory is still the file f has
+// open: a regular file at the name with f's device and inode. A name that
+// is gone is false, not an error.
+func (d *movesDir) names(name string, f *os.File) (bool, error) {
+ var held, now unix.Stat_t
+ if err := unix.Fstat(int(f.Fd()), &held); err != nil {
+ return false, err
+ }
+ err := unix.Fstatat(d.fd, name, &now, unix.AT_SYMLINK_NOFOLLOW)
+ if errors.Is(err, unix.ENOENT) {
+ return false, nil
+ }
+ if err != nil {
+ return false, err
+ }
+ return now.Dev == held.Dev && now.Ino == held.Ino, nil
+}
+
+// removedDuringOpen reports whether err, from opening name, is the race
+// with a holder removing it: not an errno other than ENOENT (a real refusal,
+// such as EACCES or ELOOP), and the name is now gone or a regular file with
+// one link (a fresh lock file). Anything else at the name keeps the open's
+// refusal, and so does a .moves that has itself been removed (no create
+// can succeed in it, so waiting out the lock's wait would only delay the
+// error).
+func (d *movesDir) removedDuringOpen(name string, err error) bool {
+ var errno syscall.Errno
+ if errors.As(err, &errno) && errno != syscall.ENOENT {
+ return false
+ }
+ var st unix.Stat_t
+ if derr := unix.Fstat(d.fd, &st); derr != nil || st.Nlink == 0 {
+ return false
+ }
+ serr := unix.Fstatat(d.fd, name, &st, unix.AT_SYMLINK_NOFOLLOW)
+ if errors.Is(serr, unix.ENOENT) {
+ return true
+ }
+ return serr == nil && st.Mode&unix.S_IFMT == unix.S_IFREG && st.Nlink == 1
+}
+
+// unlockRemoving removes the lock file while still holding its flock, then
+// releases the lock (closing its fd). The name is removed only while it
+// still names the file held, so another holder's file is never taken. A
+// waiter on the removed file finds the name gone after its flock and starts
+// again (lockNamed), so two holders never exist at once. The lock is
+// released whatever the removal does; an error is the removal's.
+func (d *movesDir) unlockRemoving() error {
+ if d.lock == nil {
+ return nil
+ }
+ f, name := d.lock, d.lockName
+ d.lock, d.lockName = nil, ""
+ defer f.Close()
+ same, err := d.names(name, f)
+ if err != nil || !same {
+ return err
+ }
+ if err := unix.Unlinkat(d.fd, name, 0); err != nil && !errors.Is(err, unix.ENOENT) {
+ return fmt.Errorf("remove %s: %w", filepath.Join(d.path, name), err)
+ }
+ return nil
+}
+
+// lockNamedOpened runs in lockNamed after each open of the lock file and
+// before its flock. A test seam: the window in which the file a waiter has
+// open can be removed by the holder before it and replaced by a new one.
+var lockNamedOpened = func(name string) {}
+
// WriteDurable writes data to .moves/name the way writeDurable does (a
// temp file, fsync, rename, fsync of the directory), all relative to the
// directory's fd. The temp name is random, so two writers never share one.
diff --git a/internal/host/movesfloor_other.go b/internal/host/movesfloor_other.go
index c21c86f92..dbb5ff9c0 100644
--- a/internal/host/movesfloor_other.go
+++ b/internal/host/movesfloor_other.go
@@ -22,6 +22,8 @@ func (d *movesDir) Lock() error { return errSystemUnsupported }
func (d *movesDir) lockNamed(string, time.Duration, error) error { return errSystemUnsupported }
+func (d *movesDir) unlockRemoving() error { return errSystemUnsupported }
+
func (d *movesDir) WriteDurable(name string, data []byte, mode os.FileMode) error {
return errSystemUnsupported
}
diff --git a/internal/host/movesheldlock.go b/internal/host/movesheldlock.go
index 4e12216dd..b60954a86 100644
--- a/internal/host/movesheldlock.go
+++ b/internal/host/movesheldlock.go
@@ -3,6 +3,7 @@ package host
import (
"errors"
"fmt"
+ "log"
"os"
"time"
@@ -28,6 +29,17 @@ import (
// call (D3's admit, which may read the bucket, runs before it on both
// sides), so it never inverts the order and never holds the registry's
// freeze for long.
+//
+// The file is removed when the stage finishes (region HA M-K25, #236), by
+// the holder, while it still holds the flock (unlockRemoving). Main's
+// empty-installation census refuses any file naming the host being
+// installed, so a lock left behind would refuse every later from-empty
+// install of that host on this box. A waiter that opened the removed file
+// re-checks the name after its flock and starts again on the new one
+// (lockNamed), so two holders never exist at once. A holder killed
+// mid-stage never removes it: the file stays, the census refuses that host
+// (region HA's CensusTree(".moves") fallback refuses only that host), and
+// the next stage of the host locks the same file and then removes it.
// heldLockWait bounds the wait for a held-stage lock. The publication waits
// for it inside the registry's freeze, after a claim read bounded by
@@ -45,9 +57,12 @@ var errHeldLockBusy = errors.New("another process holds this host's held-stage l
func heldStageLockFile(host string) string { return "held-" + host + ".lock" }
// withHeldStageLock runs fn holding host's held-stage lock, making .moves
-// when it is missing. An error opening or locking is returned without
-// running fn. Off Linux it always refuses (openMovesDir does), which fails
-// T7 and the publication closed; production boxes are Linux.
+// when it is missing, and removes the lock file before releasing it,
+// whatever fn returns. An error opening or locking is returned without
+// running fn. A removal that fails is logged and leaves the file as a crash
+// would; fn's result stands. Off Linux it always refuses (openMovesDir
+// does), which fails T7 and the publication closed; production boxes are
+// Linux.
func withHeldStageLock(dataDir, host string, fn func() error) error {
if !controlname.ValidHostShape(host) {
return fmt.Errorf("held-stage lock: malformed host %q", host)
@@ -63,5 +78,9 @@ func withHeldStageLock(dataDir, host string, fn func() error) error {
if err := d.lockNamed(heldStageLockFile(host), heldLockWait, errHeldLockBusy); err != nil {
return err
}
- return fn()
+ err = fn()
+ if rerr := d.unlockRemoving(); rerr != nil {
+ log.Printf("moves: %s: held-stage lock: %v", host, rerr)
+ }
+ return err
}
diff --git a/internal/host/movesheldlock_cleanup_test.go b/internal/host/movesheldlock_cleanup_test.go
new file mode 100644
index 000000000..d8bd1cb5d
--- /dev/null
+++ b/internal/host/movesheldlock_cleanup_test.go
@@ -0,0 +1,294 @@
+//go:build linux
+
+package host
+
+import (
+ "errors"
+ "os"
+ "path/filepath"
+ "sync/atomic"
+ "syscall"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/hostbox"
+ "github.com/carlosframework/platform/internal/hostclaim"
+ "github.com/carlosframework/platform/internal/registry"
+ "github.com/carlosframework/platform/internal/release"
+)
+
+// The held-stage lock's file is removed when the stage finishes (region HA
+// M-K25, answered by moves on #236): main's empty-installation census
+// refuses a file that names the host being installed, so a lock left
+// behind would refuse every later from-empty install of that host on the
+// box that held a stage for it.
+
+// heldLockCensus runs main's empty-installation census for a from-empty
+// install of host on a box whose data root is data, with the production
+// classification (claimGlobalSharedPaths) and any extra data entries.
+func heldLockCensus(t *testing.T, data, host string, extra ...string) error {
+ t.Helper()
+ base := t.TempDir()
+ inventory, err := hostbox.OpenStopInventoryDB(filepath.Join(base, "inventory.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer inventory.Close()
+ reg, err := registry.Open(filepath.Join(base, "registry.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer reg.Close()
+ launches, err := hostbox.OpenLaunchJournal(filepath.Join(base, "launches.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer launches.Close()
+ r := ClaimRuntimeResources{DataDir: data, OptDir: filepath.Join(base, "opt"), UnitsDir: filepath.Join(base, "units")}
+ for _, dir := range []string{r.OptDir, r.UnitsDir} {
+ if err := os.Mkdir(dir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ }
+ shared := claimGlobalSharedPaths(r)
+ shared["data"] = append(shared["data"], extra...)
+ c := hostbox.EmptyInstallationCensus{Inventory: inventory, Registry: reg, Launches: launches, Box: "box", Enrollment: "enrollment", DirectoryRevision: "1",
+ Roots: map[string]string{"data": r.DataDir, "opt": r.OptDir, "units": r.UnitsDir}, SharedPaths: shared}
+ obs := hostclaim.Observed{Host: host, Claim: &hostclaim.Claim{Holder: &hostclaim.Holder{Incarnation: "inc", Stamp: 1}}}
+ return c.CheckLocal(t.Context(), obs, hostbox.RecordSelection{Host: host, Incarnation: "inc"})
+}
+
+func heldLockInode(t *testing.T, path string) uint64 {
+ t.Helper()
+ info, err := os.Lstat(path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return info.Sys().(*syscall.Stat_t).Ino
+}
+
+func TestHeldStageLockIsRemovedWhenTheStageFinishes(t *testing.T) {
+ data := t.TempDir()
+ path := filepath.Join(data, ".moves", heldStageLockFile(hsHost))
+ if err := heldLockCensus(t, data, hsHost); err != nil {
+ t.Fatalf("census before any stage: %v", err)
+ }
+ for _, result := range []error{nil, errors.New("the stage failed")} {
+ err := withHeldStageLock(data, hsHost, func() error {
+ if _, err := os.Lstat(path); err != nil {
+ t.Fatalf("no lock file while the lock is held: %v", err)
+ }
+ return result
+ })
+ if err != result {
+ t.Fatalf("stage: %v, want %v", err, result)
+ }
+ if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("after a stage that returned %v, %s remains: %v", result, path, err)
+ }
+ }
+ // The publication path: heldStageAuthority's then, inside the freeze.
+ s := release.NewDirStore(t.TempDir())
+ hsClaim(t, s, movingOp())
+ hsReport(t, s, hsOp, "v7")
+ auth := heldStageAuthority(testRegistry(t), s, hsBox, data)
+ published := false
+ if err := auth(hsRequest(), func() error { published = true; return nil }); err != nil || !published {
+ t.Fatalf("publication: %v (ran %v)", err, published)
+ }
+ if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("after a publication, %s remains: %v", path, err)
+ }
+ // A from-empty install of that host on this box passes main's census.
+ if err := heldLockCensus(t, data, hsHost); err != nil {
+ t.Fatalf("census after held stages of the host: %v", err)
+ }
+}
+
+// A waiter that opened the file a holder then removes must not lock that
+// file and run beside whoever holds the new one at the same name: it
+// re-checks the name after its flock and retries.
+func TestHeldStageLockWaiterOnARemovedFileRetries(t *testing.T) {
+ prevWait, prevHook := heldLockWait, lockNamedOpened
+ t.Cleanup(func() { heldLockWait, lockNamedOpened = prevWait, prevHook })
+ heldLockWait = 10 * time.Second
+ data := t.TempDir()
+ path := filepath.Join(data, ".moves", heldStageLockFile(hsHost))
+ var holders, overlaps atomic.Int32
+ hold := func(entered chan<- struct{}, leave <-chan struct{}) func() error {
+ return func() error {
+ if holders.Add(1) != 1 {
+ overlaps.Add(1)
+ }
+ defer holders.Add(-1)
+ close(entered)
+ if leave != nil {
+ <-leave
+ }
+ return nil
+ }
+ }
+
+ // The first holder holds the lock on the first file.
+ in1, out1, done1 := make(chan struct{}), make(chan struct{}), make(chan error, 1)
+ go func() { done1 <- withHeldStageLock(data, hsHost, hold(in1, out1)) }()
+ <-in1
+
+ // The waiter opens that file and is parked before its flock.
+ opened, resume := make(chan struct{}), make(chan struct{})
+ var opens atomic.Int32
+ lockNamedOpened = func(name string) {
+ if name == heldStageLockFile(hsHost) && opens.Add(1) == 1 {
+ close(opened)
+ <-resume
+ }
+ }
+ inW, doneW := make(chan struct{}), make(chan error, 1)
+ go func() { doneW <- withHeldStageLock(data, hsHost, hold(inW, nil)) }()
+ <-opened
+
+ // The first holder finishes: it removes its file and releases.
+ close(out1)
+ if err := <-done1; err != nil {
+ t.Fatal(err)
+ }
+ if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) {
+ close(resume)
+ t.Fatalf("the first holder left %s: %v", path, err)
+ }
+
+ // A second holder arrives and locks a new file at the same name.
+ in2, out2, done2 := make(chan struct{}), make(chan struct{}), make(chan error, 1)
+ go func() { done2 <- withHeldStageLock(data, hsHost, hold(in2, out2)) }()
+ <-in2
+
+ // The waiter resumes. Its fd names the removed file, which nobody
+ // holds: it must not run until the second holder is done.
+ close(resume)
+ select {
+ case <-inW:
+ close(out2)
+ t.Fatalf("the waiter ran while the second holder held the lock (overlaps %d)", overlaps.Load())
+ case <-time.After(500 * time.Millisecond):
+ }
+ close(out2)
+ if err := <-done2; err != nil {
+ t.Fatal(err)
+ }
+ select {
+ case <-inW:
+ case <-time.After(5 * time.Second):
+ t.Fatal("the waiter never took the lock after the second holder released it")
+ }
+ if err := <-doneW; err != nil {
+ t.Fatal(err)
+ }
+ if n := overlaps.Load(); n != 0 {
+ t.Fatalf("%d holders ran beside another", n)
+ }
+ if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("after every holder finished, %s remains: %v", path, err)
+ }
+}
+
+// A holder killed mid-stage never reaches the removal, so its file stays
+// (the existing behaviour). Main's census then refuses an install of that
+// host; region HA's fallback (CensusTree(".moves")) refuses that host only.
+// The next stage of the host locks the file the crash left and removes it.
+func TestHeldStageLockLeftByACrashIsReusedThenRemoved(t *testing.T) {
+ data := t.TempDir()
+ path := filepath.Join(data, ".moves", heldStageLockFile(hsHost))
+ if err := os.Mkdir(filepath.Dir(path), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(path, nil, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ left := heldLockInode(t, path)
+ if err := heldLockCensus(t, data, hsHost); !errors.Is(err, hostbox.ErrTypedInventory) {
+ t.Fatalf("main's census with a crashed stage's lock: %v, want ErrTypedInventory", err)
+ }
+ if err := heldLockCensus(t, data, hsHost, hostbox.CensusTree(".moves")); !errors.Is(err, hostbox.ErrTypedInventory) {
+ t.Fatalf("the fallback census for the lock's host: %v, want ErrTypedInventory", err)
+ }
+ if err := heldLockCensus(t, data, "other.example.com", hostbox.CensusTree(".moves")); err != nil {
+ t.Fatalf("the fallback census for another host: %v", err)
+ }
+ ran := false
+ if err := withHeldStageLock(data, hsHost, func() error {
+ ran = true
+ if got := heldLockInode(t, path); got != left {
+ t.Fatalf("the next stage locked a new file (inode %d), not the one the crash left (%d)", got, left)
+ }
+ return nil
+ }); err != nil || !ran {
+ t.Fatalf("the next stage: %v (ran %v)", err, ran)
+ }
+ if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("the next stage left %s: %v", path, err)
+ }
+ if err := heldLockCensus(t, data, hsHost); err != nil {
+ t.Fatalf("census after the next stage: %v", err)
+ }
+}
+
+// A holder's removal can land inside a waiter's open: the name vanishes
+// between the create and the reopen, or the file reopened has no link
+// left. lockNamed opens again then, but keeps the refusal of anything
+// else at the name.
+func TestHeldStageLockOpenRetriesOnlyTheRemovalRace(t *testing.T) {
+ data := t.TempDir()
+ d, err := openMovesDir(data, true)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer d.Close()
+ at := func(name string) string { return filepath.Join(data, ".moves", name) }
+ if err := os.WriteFile(at("fresh.lock"), nil, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(at("linked.lock"), nil, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Link(at("linked.lock"), at("second.lock")); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(t.TempDir(), at("planted.lock")); err != nil {
+ t.Fatal(err)
+ }
+ if err := syscall.Mkfifo(at("fifo.lock"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ unlinked := &os.PathError{Op: "open", Path: "x", Err: errors.New("not a regular file with one link")}
+ for _, c := range []struct {
+ name string
+ err error
+ want bool
+ }{
+ {"gone.lock", &os.PathError{Op: "open", Path: "x", Err: syscall.ENOENT}, true},
+ {"gone.lock", unlinked, true},
+ {"fresh.lock", unlinked, true},
+ {"fresh.lock", &os.PathError{Op: "open", Path: "x", Err: syscall.EACCES}, false},
+ {"linked.lock", unlinked, false},
+ {"planted.lock", unlinked, false},
+ {"fifo.lock", unlinked, false},
+ } {
+ if got := d.removedDuringOpen(c.name, c.err); got != c.want {
+ t.Errorf("%s after %v: retry %v, want %v", c.name, c.err, got, c.want)
+ }
+ }
+ // A .moves removed under an open movesDir: nothing can be created in
+ // it, so the open's error stands at once.
+ gone := t.TempDir()
+ g, err := openMovesDir(gone, true)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer g.Close()
+ if err := os.Remove(filepath.Join(gone, ".moves")); err != nil {
+ t.Fatal(err)
+ }
+ if g.removedDuringOpen("gone.lock", &os.PathError{Op: "open", Path: "x", Err: syscall.ENOENT}) {
+ t.Error("a removed .moves was retried")
+ }
+}