internal/app/state: improve store internals

This fully exposes the store internals for #19 and are final preparations for removing the legacy store interface.

This change also fixes a potential deadlock in the handle initialisation mkdir failure path. This however is never reachable in hakurei as the store is never accessed concurrently.

Signed-off-by: Ophestra <cat@gensokyo.uk>

internal/app/state/segment_test.go

@@ -194,19 +194,7 @@ func TestStoreHandle(t *testing.T) {
 			if err := os.Mkdir(p.String(), 0700); err != nil {
 				t.Fatal(err.Error())
 			}
-
-			for _, s := range tc.ents[0] {
-				if f, err := os.OpenFile(p.Append(s).String(), os.O_CREATE|os.O_EXCL, 0600); err != nil {
-					t.Fatal(err.Error())
-				} else if err = f.Close(); err != nil {
-					t.Fatal(err.Error())
-				}
-			}
-			for _, s := range tc.ents[1] {
-				if err := os.Mkdir(p.Append(s).String(), 0700); err != nil {
-					t.Fatal(err.Error())
-				}
-			}
+			createEntries(t, p, tc.ents)
 
 			var got []*stateEntryHandle
 			if entries, n, err := (&storeHandle{
@@ -253,3 +241,19 @@ func TestStoreHandle(t *testing.T) {
 		}
 	})
 }
+
+// createEntries creates file and directory entries in the specified prefix.
+func createEntries(t *testing.T, prefix *check.Absolute, ents [2][]string) {
+	for _, s := range ents[0] {
+		if f, err := os.OpenFile(prefix.Append(s).String(), os.O_CREATE|os.O_EXCL, 0600); err != nil {
+			t.Fatal(err.Error())
+		} else if err = f.Close(); err != nil {
+			t.Fatal(err.Error())
+		}
+	}
+	for _, s := range ents[1] {
+		if err := os.Mkdir(prefix.Append(s).String(), 0700); err != nil {
+			t.Fatal(err.Error())
+		}
+	}
+}

internal/app/state/state.go

@@ -6,7 +6,6 @@ import (
 
 	"hakurei.app/container/check"
 	"hakurei.app/hst"
-	"hakurei.app/internal/lockedfile"
 	"hakurei.app/message"
 )
 
@@ -23,11 +22,45 @@ type Store interface {
 	List() (identities []int, err error)
 }
 
+func (s *stateStore) Do(identity int, f func(c Cursor)) (bool, error) {
+	if h, err := s.identityHandle(identity); err != nil {
+		return false, err
+	} else {
+		return h.do(f)
+	}
+}
+
+// storeAdapter satisfies [Store] via stateStore.
+type storeAdapter struct {
+	msg message.Msg
+	*stateStore
+}
+
+func (s storeAdapter) List() ([]int, error) {
+	segments, n, err := s.segments()
+	if err != nil {
+		return nil, err
+	}
+
+	identities := make([]int, 0, n)
+	for si := range segments {
+		if si.err != nil {
+			if m, ok := message.GetMessage(err); ok {
+				s.msg.Verbose(m)
+			} else {
+				// unreachable
+				return nil, err
+			}
+			continue
+		}
+		identities = append(identities, si.identity)
+	}
+	return identities, nil
+}
+
 // NewMulti returns an instance of the multi-file store.
 func NewMulti(msg message.Msg, prefix *check.Absolute) Store {
-	store := &stateStore{msg: msg, base: prefix.Append("state")}
-	store.fileMu = lockedfile.MutexAt(store.base.Append(storeMutexName).String())
-	return store
+	return storeAdapter{msg, newStore(prefix.Append("state"))}
 }
 
 // Cursor provides access to the store of an identity.

internal/app/state/state_test.go

@@ -1,17 +1,22 @@
 package state_test
 
 import (
-	"math/rand/v2"
+	"log"
+	"math/rand"
 	"reflect"
 	"slices"
 	"testing"
 	"time"
 
+	"hakurei.app/container/check"
 	"hakurei.app/hst"
 	"hakurei.app/internal/app/state"
+	"hakurei.app/message"
 )
 
-func testStore(t *testing.T, s state.Store) {
+func TestMulti(t *testing.T) {
+	s := state.NewMulti(message.NewMsg(log.New(log.Writer(), "multi: ", 0)), check.MustAbs(t.TempDir()))
+
 	t.Run("list empty store", func(t *testing.T) {
 		if identities, err := s.List(); err != nil {
 			t.Fatalf("List: error = %v", err)
@@ -30,7 +35,12 @@ func testStore(t *testing.T, s state.Store) {
 
 	var tc [tl]hst.State
 	for i := 0; i < tl; i++ {
-		makeState(t, &tc[i])
+		if err := hst.NewInstanceID(&tc[i].ID); err != nil {
+			t.Fatalf("cannot create dummy state: %v", err)
+		}
+		tc[i].PID = rand.Int()
+		tc[i].Config = hst.Template()
+		tc[i].Time = time.Now()
 	}
 
 	do := func(identity int, f func(c state.Cursor)) {
@@ -108,12 +118,3 @@ func testStore(t *testing.T, s state.Store) {
 		}
 	})
 }
-
-func makeState(t *testing.T, s *hst.State) {
-	if err := hst.NewInstanceID(&s.ID); err != nil {
-		t.Fatalf("cannot create dummy state: %v", err)
-	}
-	s.PID = rand.Int()
-	s.Config = hst.Template()
-	s.Time = time.Now()
-}

internal/app/state/store.go

@@ -3,14 +3,15 @@ package state
 import (
 	"errors"
 	"io/fs"
+	"iter"
 	"os"
 	"strconv"
 	"sync"
+	"syscall"
 
 	"hakurei.app/container/check"
 	"hakurei.app/hst"
 	"hakurei.app/internal/lockedfile"
-	"hakurei.app/message"
 )
 
 // storeMutexName is the pathname of the file backing [lockedfile.Mutex] of a stateStore and storeHandle.
@@ -34,11 +35,10 @@ type stateStore struct {
 	mkdirOnce sync.Once
 	// Stored error value via mkdirOnce.
 	mkdirErr error
-
-	msg message.Msg
 }
 
 // bigLock acquires fileMu on stateStore.
+// A non-nil error returned by bigLock is of type [hst.AppError].
 func (s *stateStore) bigLock() (unlock func(), err error) {
 	s.mkdirOnce.Do(func() { s.mkdirErr = os.MkdirAll(s.base.String(), 0700) })
 	if s.mkdirErr != nil {
@@ -52,6 +52,7 @@ func (s *stateStore) bigLock() (unlock func(), err error) {
 }
 
 // identityHandle loads or initialises a storeHandle for identity.
+// A non-nil error returned by identityHandle is of type [hst.AppError].
 func (s *stateStore) identityHandle(identity int) (*storeHandle, error) {
 	h := new(storeHandle)
 	h.mu.Lock()
@@ -70,61 +71,92 @@ func (s *stateStore) identityHandle(identity int) (*storeHandle, error) {
 		h.path = s.base.Append(strconv.Itoa(identity))
 		h.fileMu = lockedfile.MutexAt(h.path.Append(storeMutexName).String())
 
-		if err := os.MkdirAll(h.path.String(), 0700); err != nil && !errors.Is(err, fs.ErrExist) {
+		err := os.MkdirAll(h.path.String(), 0700)
+		h.mu.Unlock()
+		if err != nil && !errors.Is(err, fs.ErrExist) {
+			// handle methods will likely return ENOENT
 			s.handles.CompareAndDelete(identity, h)
 			return nil, &hst.AppError{Step: "create store segment directory", Err: err}
 		}
-		h.mu.Unlock()
 	}
 	return h, nil
 }
 
-func (s *stateStore) Do(identity int, f func(c Cursor)) (bool, error) {
-	if h, err := s.identityHandle(identity); err != nil {
-		return false, err
-	} else {
-		return h.do(f)
-	}
+// segmentIdentity is produced by the iterator returned by stateStore.segments.
+type segmentIdentity struct {
+	// Identity of the current segment.
+	identity int
+	// Error encountered while processing this segment.
+	err error
 }
 
-func (s *stateStore) List() ([]int, error) {
+// segments returns an iterator over all segmentIdentity known to the store.
+// To obtain a storeHandle on a segment, caller must then call identityHandle.
+// A non-nil error returned by segments is of type [hst.AppError].
+func (s *stateStore) segments() (iter.Seq[segmentIdentity], int, error) {
+	// read directory contents, should only contain storeMutexName and identity
 	var entries []os.DirEntry
 
 	// acquire big lock to read store segment list
 	if unlock, err := s.bigLock(); err != nil {
-		return nil, err
+		return nil, -1, err
 	} else {
 		entries, err = os.ReadDir(s.base.String())
 		unlock()
 
 		if err != nil && !errors.Is(err, os.ErrNotExist) {
-			return nil, &hst.AppError{Step: "read store directory", Err: err}
+			return nil, -1, &hst.AppError{Step: "read store segments", Err: err}
 		}
 	}
 
-	identities := make([]int, 0, len(entries))
-	for _, e := range entries {
-		// should only be the big lock
-		if !e.IsDir() {
-			if e.Name() != storeMutexName {
-				s.msg.Verbosef("skipped non-directory entry %q", e.Name())
-			}
-			continue
-		}
-
-		// this either indicates a serious bug or external interference
-		if v, err := strconv.Atoi(e.Name()); err != nil {
-			s.msg.Verbosef("skipped non-identity entry %q", e.Name())
-			continue
-		} else {
-			if v < hst.IdentityMin || v > hst.IdentityMax {
-				s.msg.Verbosef("skipped out of bounds entry %q", e.Name())
-				continue
-			}
-
-			identities = append(identities, v)
-		}
+	// expects lock file
+	l := len(entries)
+	if l > 0 {
+		l--
 	}
 
-	return identities, nil
+	return func(yield func(segmentIdentity) bool) {
+		// for error reporting
+		const step = "process store segment"
+
+		for _, ent := range entries {
+			si := segmentIdentity{identity: -1}
+
+			// should only be the big lock
+			if !ent.IsDir() {
+				if ent.Name() == storeMutexName {
+					continue
+				}
+
+				// this should never happen
+				si.err = &hst.AppError{Step: step, Err: syscall.EISDIR,
+					Msg: "skipped non-directory entry " + strconv.Quote(ent.Name())}
+				goto out
+			}
+
+			// failure paths either indicates a serious bug or external interference
+			if v, err := strconv.Atoi(ent.Name()); err != nil {
+				si.err = &hst.AppError{Step: step, Err: err,
+					Msg: "skipped non-identity entry " + strconv.Quote(ent.Name())}
+				goto out
+			} else if v < hst.IdentityMin || v > hst.IdentityMax {
+				si.err = &hst.AppError{Step: step, Err: syscall.ERANGE,
+					Msg: "skipped out of bounds entry " + strconv.Itoa(v)}
+				goto out
+			} else {
+				si.identity = v
+			}
+
+		out:
+			if !yield(si) {
+				break
+			}
+		}
+	}, l, nil
+}
+
+// newStore returns the address of a new instance of stateStore.
+// Multiple instances of stateStore rooted in the same directory is supported, but discouraged.
+func newStore(base *check.Absolute) *stateStore {
+	return &stateStore{base: base, fileMu: lockedfile.MutexAt(base.Append(storeMutexName).String())}
 }

internal/app/state/store_test.go

@@ -1,14 +1,254 @@
-package state_test
+package state
 
 import (
-	"log"
+	"cmp"
+	"iter"
+	"os"
+	"reflect"
+	"slices"
+	"strconv"
+	"strings"
+	"syscall"
 	"testing"
 
 	"hakurei.app/container/check"
-	"hakurei.app/internal/app/state"
-	"hakurei.app/message"
+	"hakurei.app/hst"
 )
 
-func TestMulti(t *testing.T) {
-	testStore(t, state.NewMulti(message.NewMsg(log.New(log.Writer(), "multi: ", 0)), check.MustAbs(t.TempDir())))
+func TestStateStoreBigLock(t *testing.T) {
+	t.Parallel()
+
+	{
+		s := newStore(check.MustAbs(t.TempDir()).Append("state"))
+		for i := 0; i < 2; i++ { // check once behaviour
+			if unlock, err := s.bigLock(); err != nil {
+				t.Fatalf("bigLock: error = %v", err)
+			} else {
+				unlock()
+			}
+		}
+	}
+
+	t.Run("mkdir", func(t *testing.T) {
+		t.Parallel()
+
+		wantErr := &hst.AppError{Step: "create state store directory",
+			Err: &os.PathError{Op: "mkdir", Path: "/proc/nonexistent", Err: syscall.ENOENT}}
+		for i := 0; i < 2; i++ { // check once behaviour
+			if _, err := newStore(check.MustAbs("/proc/nonexistent")).bigLock(); !reflect.DeepEqual(err, wantErr) {
+				t.Errorf("bigLock: error = %#v, want %#v", err, wantErr)
+			}
+		}
+	})
+
+	t.Run("access", func(t *testing.T) {
+		t.Parallel()
+
+		base := check.MustAbs(t.TempDir()).Append("inaccessible")
+		if err := os.MkdirAll(base.String(), 0); err != nil {
+			t.Fatal(err.Error())
+		}
+
+		wantErr := &hst.AppError{Step: "acquire lock on the state store",
+			Err: &os.PathError{Op: "open", Path: base.Append(storeMutexName).String(), Err: syscall.EACCES}}
+		if _, err := newStore(base).bigLock(); !reflect.DeepEqual(err, wantErr) {
+			t.Errorf("bigLock: error = %#v, want %#v", err, wantErr)
+		}
+	})
+}
+
+func TestStateStoreIdentityHandle(t *testing.T) {
+	t.Parallel()
+
+	t.Run("loadstore", func(t *testing.T) {
+		t.Parallel()
+
+		s := newStore(check.MustAbs(t.TempDir()).Append("store"))
+
+		var handleAddr [8]*storeHandle
+		checkHandle := func(identity int, load bool) {
+			if h, err := s.identityHandle(identity); err != nil {
+				t.Fatalf("identityHandle: error = %v", err)
+			} else if load != (handleAddr[identity] != nil) {
+				t.Fatalf("identityHandle: load = %v, want %v", load, handleAddr[identity] != nil)
+			} else if !load {
+				handleAddr[identity] = h
+
+				if h.identity != identity {
+					t.Errorf("identityHandle: identity = %d, want %d", h.identity, identity)
+				}
+			} else if h != handleAddr[identity] {
+				t.Fatalf("identityHandle: %p, want %p", h, handleAddr[identity])
+			}
+		}
+
+		checkHandle(0, false)
+		checkHandle(1, false)
+		checkHandle(2, false)
+		checkHandle(3, false)
+		checkHandle(7, false)
+		checkHandle(7, true)
+		checkHandle(2, true)
+		checkHandle(1, true)
+		checkHandle(2, true)
+		checkHandle(0, true)
+	})
+
+	t.Run("access", func(t *testing.T) {
+		t.Parallel()
+
+		base := check.MustAbs(t.TempDir()).Append("inaccessible")
+		if err := os.MkdirAll(base.String(), 0); err != nil {
+			t.Fatal(err.Error())
+		}
+
+		wantErr := &hst.AppError{Step: "acquire lock on the state store",
+			Err: &os.PathError{Op: "open", Path: base.Append(storeMutexName).String(), Err: syscall.EACCES}}
+		if _, err := newStore(base).identityHandle(0); !reflect.DeepEqual(err, wantErr) {
+			t.Errorf("identityHandle: error = %#v, want %#v", err, wantErr)
+		}
+	})
+
+	t.Run("access segment", func(t *testing.T) {
+		t.Parallel()
+
+		base := check.MustAbs(t.TempDir()).Append("inaccessible")
+		if err := os.MkdirAll(base.String(), 0700); err != nil {
+			t.Fatal(err.Error())
+		}
+		if f, err := os.Create(base.Append(storeMutexName).String()); err != nil {
+			t.Fatal(err.Error())
+		} else if err = f.Close(); err != nil {
+			t.Fatal(err.Error())
+		}
+		if err := os.Chmod(base.String(), 0100); err != nil {
+			t.Fatal(err.Error())
+		}
+		t.Cleanup(func() {
+			if err := os.Chmod(base.String(), 0700); err != nil {
+				t.Fatal(err.Error())
+			}
+		})
+
+		wantErr := &hst.AppError{Step: "create store segment directory",
+			Err: &os.PathError{Op: "mkdir", Path: base.Append("0").String(), Err: syscall.EACCES}}
+		if _, err := newStore(base).identityHandle(0); !reflect.DeepEqual(err, wantErr) {
+			t.Errorf("identityHandle: error = %#v, want %#v", err, wantErr)
+		}
+	})
+}
+
+func TestStateStoreSegments(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name string
+		ents [2][]string
+		want []segmentIdentity
+		ext  func(t *testing.T, segments iter.Seq[segmentIdentity], n int)
+	}{
+		{"errors", [2][]string{{
+			"f0-invalid-file",
+		}, {
+			"f1-invalid-syntax",
+			"9999",
+			"16384",
+		}}, []segmentIdentity{
+			{-1, &hst.AppError{Step: "process store segment", Err: syscall.EISDIR,
+				Msg: `skipped non-directory entry "f0-invalid-file"`}},
+			{-1, &hst.AppError{Step: "process store segment", Err: syscall.ERANGE,
+				Msg: `skipped out of bounds entry 16384`}},
+			{-1, &hst.AppError{Step: "process store segment",
+				Err: &strconv.NumError{Func: "Atoi", Num: "f1-invalid-syntax", Err: strconv.ErrSyntax},
+				Msg: `skipped non-identity entry "f1-invalid-syntax"`}},
+			{9999, nil},
+		}, nil},
+
+		{"success", [2][]string{{
+			"lock",
+		}, {
+			"0",
+			"1",
+			"2",
+			"3",
+			"4",
+			"5",
+			"6",
+			"7",
+			"9",
+			"13",
+			"20",
+			"31",
+			"197",
+		}}, []segmentIdentity{
+			{0, nil},
+			{1, nil},
+			{2, nil},
+			{3, nil},
+			{4, nil},
+			{5, nil},
+			{6, nil},
+			{7, nil},
+			{9, nil},
+			{13, nil},
+			{20, nil},
+			{31, nil},
+			{197, nil},
+		}, func(t *testing.T, segments iter.Seq[segmentIdentity], n int) {
+			if n != 13 {
+				t.Fatalf("segments: n = %d", n)
+			}
+
+			// check partial drain
+			for range segments {
+				break
+			}
+		}},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			base := check.MustAbs(t.TempDir()).Append("store")
+			if err := os.Mkdir(base.String(), 0700); err != nil {
+				t.Fatal(err.Error())
+			}
+			createEntries(t, base, tc.ents)
+
+			var got []segmentIdentity
+			if segments, n, err := newStore(base).segments(); err != nil {
+				t.Fatalf("segments: error = %v", err)
+			} else {
+				got = slices.AppendSeq(make([]segmentIdentity, 0, n), segments)
+				if tc.ext != nil {
+					tc.ext(t, segments, n)
+				}
+			}
+
+			slices.SortFunc(got, func(a, b segmentIdentity) int {
+				if a.identity == b.identity {
+					return strings.Compare(a.err.Error(), b.err.Error())
+				}
+				return cmp.Compare(a.identity, b.identity)
+			})
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("segments: %#v, want %#v", got, tc.want)
+			}
+		})
+	}
+
+	t.Run("access", func(t *testing.T) {
+		t.Parallel()
+
+		base := check.MustAbs(t.TempDir()).Append("inaccessible")
+		if err := os.MkdirAll(base.String(), 0); err != nil {
+			t.Fatal(err.Error())
+		}
+
+		wantErr := &hst.AppError{Step: "acquire lock on the state store",
+			Err: &os.PathError{Op: "open", Path: base.Append(storeMutexName).String(), Err: syscall.EACCES}}
+		if _, _, err := newStore(base).segments(); !reflect.DeepEqual(err, wantErr) {
+			t.Errorf("segments: error = %#v, want %#v", err, wantErr)
+		}
+	})
 }