From 4d4a77738d7822364bfc74e65658a9f3c1761880 Mon Sep 17 00:00:00 2001 From: imneov Date: Tue, 7 Jul 2026 15:48:05 +0000 Subject: [PATCH 1/3] fix(stateio): hold lock across state read-modify-write to stop dropped RecordApplied entries --- cmd/delete.go | 22 ++++++++------ cmd/rename.go | 20 +++++------- cmd/switch.go | 46 ++++++++++++---------------- cmd/switch_test.go | 2 +- internal/adapter/stateio/stateio.go | 39 +++++------------------- internal/config/manager.go | 34 ++++++++------------- internal/storage/filesystem.go | 47 +++++++++++++++++++++++++++++ 7 files changed, 107 insertions(+), 103 deletions(-) diff --git a/cmd/delete.go b/cmd/delete.go index 968ac7b..f41757a 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -28,6 +28,7 @@ import ( "github.com/spf13/cobra" + "github.com/a2d2-dev/claudecm/internal/config" "github.com/a2d2-dev/claudecm/internal/storage" ) @@ -128,16 +129,17 @@ func runDelete(cmd *cobra.Command, args []string) error { // name. Returns (true, nil) when the pointer was cleared, (false, nil) // otherwise. Any I/O error surfaces as-is. func maybeClearActivePointer(store *storage.FileStorage, name string) (bool, error) { - state, err := store.LoadState() + var cleared bool + err := store.UpdateState(func(state *config.State) (bool, error) { + if state.CurrentProfile != name { + return false, nil + } + state.CurrentProfile = "" + cleared = true + return true, nil + }) if err != nil { - return false, fmt.Errorf("load state: %w", err) - } - if state.CurrentProfile != name { - return false, nil - } - state.CurrentProfile = "" - if err := store.SaveState(state); err != nil { - return false, fmt.Errorf("save state after delete: %w", err) + return false, err } - return true, nil + return cleared, nil } diff --git a/cmd/rename.go b/cmd/rename.go index 5f440dc..0cbcb4e 100644 --- a/cmd/rename.go +++ b/cmd/rename.go @@ -26,6 +26,7 @@ import ( "github.com/spf13/cobra" + "github.com/a2d2-dev/claudecm/internal/config" "github.com/a2d2-dev/claudecm/internal/storage" ) @@ -143,16 +144,11 @@ func runRename(cmd *cobra.Command, args []string) error { // active" branch — no state I/O has to fire when the pointer already // pointed elsewhere. func maybeUpdateStateAfterRename(store *storage.FileStorage, oldName, newName string) error { - state, err := store.LoadState() - if err != nil { - return fmt.Errorf("load state: %w", err) - } - if state.CurrentProfile != oldName { - return nil - } - state.SetCurrentProfile(newName) - if err := store.SaveState(state); err != nil { - return fmt.Errorf("save state after rename: %w", err) - } - return nil + return store.UpdateState(func(state *config.State) (bool, error) { + if state.CurrentProfile != oldName { + return false, nil + } + state.SetCurrentProfile(newName) + return true, nil + }) } diff --git a/cmd/switch.go b/cmd/switch.go index 3b2830e..a1006ee 100644 --- a/cmd/switch.go +++ b/cmd/switch.go @@ -216,7 +216,7 @@ func runSwitch(cmd *cobra.Command, args []string) error { // profile pointer moves — a no-op switch is a legitimate outcome // when the profile matches the current on-disk intent. if len(plans) == 0 { - if err := updateStateOnSuccess(resv, store, profileName, nil); err != nil { + if err := updateStateOnSuccess(resv, profileName, nil); err != nil { return fmt.Errorf("no plans to commit but state update failed: %w", err) } return renderNoOp(cmd.OutOrStdout(), format, profileName, planErrors) @@ -308,7 +308,7 @@ func runSwitch(cmd *cobra.Command, args []string) error { return fmt.Errorf("commit: %w", commitErr) } - if err := updateStateOnSuccess(resv, store, profileName, &report); err != nil { + if err := updateStateOnSuccess(resv, profileName, &report); err != nil { return fmt.Errorf("commit succeeded but state update failed: %w", err) } @@ -447,33 +447,25 @@ func isNoConfigErr(err error) bool { // report != nil, records the (path, sha256, appliedAt) tuple for every // committed file so external-drift detection has a fresh anchor. On // the empty-plan path (report == nil) only the pointer moves. -func updateStateOnSuccess(r *storage.Resolver, store *storage.FileStorage, profileName string, report *commit.CommitReport) error { - state, err := store.LoadState() - if err != nil { - return fmt.Errorf("load state: %w", err) - } - state.SetCurrentProfile(profileName) - if err := store.SaveState(state); err != nil { - return fmt.Errorf("save state: %w", err) - } - if report == nil { - return nil - } - for _, pf := range report.PerFile { - if pf.Status != commit.StatusCommitted { - continue +func updateStateOnSuccess(r *storage.Resolver, profileName string, report *commit.CommitReport) error { + return stateio.UpdateState(r, func(state *config.State) (bool, error) { + state.SetCurrentProfile(profileName) + if report == nil { + return true, nil } - if err := stateio.RecordApplied( - r, - config.ToolID(pf.Report.Tool), - pf.Target, - pf.Report.PostFingerprint.SHA256, - pf.Report.AppliedAt, - ); err != nil { - return fmt.Errorf("record applied for %s: %w", pf.Target, err) + for _, pf := range report.PerFile { + if pf.Status != commit.StatusCommitted { + continue + } + state.RecordApplied( + config.ToolID(pf.Report.Tool), + pf.Target, + pf.Report.PostFingerprint.SHA256, + pf.Report.AppliedAt, + ) } - } - return nil + return true, nil + }) } // promptConfirm prints a y/N question and reads a single line from in. diff --git a/cmd/switch_test.go b/cmd/switch_test.go index 73c7c58..492ada3 100644 --- a/cmd/switch_test.go +++ b/cmd/switch_test.go @@ -968,7 +968,7 @@ func TestSwitch_UpdateStateOnSuccessRecordsCommittedOnly(t *testing.T) { }, }, } - if err := updateStateOnSuccess(h.resv, h.store, "prod", &report); err != nil { + if err := updateStateOnSuccess(h.resv, "prod", &report); err != nil { t.Fatalf("updateStateOnSuccess: %v", err) } state, err := h.store.LoadState() diff --git a/internal/adapter/stateio/stateio.go b/internal/adapter/stateio/stateio.go index 8caedf3..178b062 100644 --- a/internal/adapter/stateio/stateio.go +++ b/internal/adapter/stateio/stateio.go @@ -51,29 +51,13 @@ package stateio import ( "crypto/sha256" "encoding/hex" - "errors" - "fmt" "os" - "path/filepath" "time" "github.com/a2d2-dev/claudecm/internal/config" "github.com/a2d2-dev/claudecm/internal/storage" ) -// stateLockTimeout is the flock timeout for the state.yaml -// read-modify-write critical section. Kept short so a stuck adapter -// surfaces as ErrLockTimeout instead of hanging Apply indefinitely. -// The state-file write itself is a few KB of YAML; the practical hold -// time is sub-millisecond, so 5 seconds is generous. -const stateLockTimeout = 5 * time.Second - -// stateLockRelTarget is the HOME-relative path to state.yaml used as -// the flock target. storage.Acquire refuses absolute paths. The literal -// mirrors storage.ConfigDirName / storage.StateFileName; kept as a -// package-level string so any future rename lands in one spot. -var stateLockRelTarget = filepath.Join(storage.ConfigDirName, storage.StateFileName) - // Sha256Hex returns the lowercase hex-encoded SHA-256 digest of data. // Kept in one place so every adapter that hashes a file for drift or // state anchoring uses the same algorithm and the same encoding — @@ -136,25 +120,18 @@ func LoadLastApplied(r *storage.Resolver, tool config.ToolID, filePath string) ( // condition. Silently swallowing would leave the drift detector in a // permanent false-positive state after the next external edit. func RecordApplied(r *storage.Resolver, tool config.ToolID, filePath, sha256 string, appliedAt time.Time) error { - if r == nil { - return errors.New("stateio: RecordApplied: resolver is nil") - } - fs := storage.NewFileStorage(r) - return storage.WithLock(r, stateLockRelTarget, storage.LockOptions{Timeout: stateLockTimeout}, func() error { - state, err := fs.LoadState() - if err != nil { - return fmt.Errorf("stateio: load state: %w", err) - } - // LoadState returns config.NewState() on a missing file, so - // state is never nil when err is nil. No defensive guard here. + return UpdateState(r, func(state *config.State) (bool, error) { state.RecordApplied(tool, filePath, sha256, appliedAt) - if err := fs.SaveState(state); err != nil { - return fmt.Errorf("stateio: save state: %w", err) - } - return nil + return true, nil }) } +// UpdateState delegates to storage.FileStorage.UpdateState so every state.yaml +// writer shares one locked read-modify-write implementation. +func UpdateState(r *storage.Resolver, mutate func(*config.State) (bool, error)) error { + return storage.NewFileStorage(r).UpdateState(mutate) +} + // DriftForFile checks a single owned file for external drift. Returns // true iff (a) state.yaml records a prior Apply for this (tool, path) // AND (b) the file is present on disk AND (c) the current on-disk diff --git a/internal/config/manager.go b/internal/config/manager.go index 29cc646..a791f33 100644 --- a/internal/config/manager.go +++ b/internal/config/manager.go @@ -14,6 +14,7 @@ type Storage interface { ProfileExists(name string) (bool, error) SaveState(state *State) error LoadState() (*State, error) + UpdateState(mutate func(*State) (bool, error)) error } // Manager handles all configuration management operations @@ -138,18 +139,14 @@ func (m *Manager) DeleteProfile(name string) error { m.mu.Lock() defer m.mu.Unlock() - // Check if this is the active profile - state, err := m.storage.LoadState() - if err != nil { - return fmt.Errorf("failed to load state: %w", err) - } - - if state.CurrentProfile == name { - // Clear active profile if deleting it - state.CurrentProfile = "" - if err := m.storage.SaveState(state); err != nil { - return fmt.Errorf("failed to update state: %w", err) + if err := m.storage.UpdateState(func(state *State) (bool, error) { + if state.CurrentProfile != name { + return false, nil } + state.CurrentProfile = "" + return true, nil + }); err != nil { + return fmt.Errorf("failed to update state: %w", err) } // Delete profile @@ -178,17 +175,10 @@ func (m *Manager) SetActive(name string) error { return fmt.Errorf("profile %q not found", name) } - // Load current state - state, err := m.storage.LoadState() - if err != nil { - return fmt.Errorf("failed to load state: %w", err) - } - - // Update active profile - state.SetCurrentProfile(name) - - // Save state - if err := m.storage.SaveState(state); err != nil { + if err := m.storage.UpdateState(func(state *State) (bool, error) { + state.SetCurrentProfile(name) + return true, nil + }); err != nil { return fmt.Errorf("failed to save state: %w", err) } diff --git a/internal/storage/filesystem.go b/internal/storage/filesystem.go index 00064d9..c7fcb14 100644 --- a/internal/storage/filesystem.go +++ b/internal/storage/filesystem.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "strings" + "sync" + "time" "github.com/a2d2-dev/claudecm/internal/config" "gopkg.in/yaml.v3" @@ -47,8 +49,18 @@ type Storage interface { // LoadState reads the state file LoadState() (*config.State, error) + + // UpdateState performs a locked state read-modify-write. + UpdateState(mutate func(*config.State) (bool, error)) error } +const stateLockTimeout = 5 * time.Second + +var ( + stateLockRelTarget = filepath.Join(ConfigDirName, StateFileName) + stateMu sync.Mutex +) + // FileStorage implements Storage using the local filesystem. It routes every // path through the injected *Resolver — the only source of HOME truth. type FileStorage struct { @@ -238,6 +250,41 @@ func (fs *FileStorage) SaveState(state *config.State) error { return nil } +// UpdateState runs mutate against state.yaml and, when mutate reports a change, +// persists the result while holding the state lock across the full +// load → mutate → save cycle. The in-process mutex covers Linux flock's +// per-process semantics so sibling goroutines cannot open competing +// same-process flock descriptors for state.yaml. +func (fs *FileStorage) UpdateState(mutate func(*config.State) (bool, error)) error { + if fs == nil || fs.r == nil { + return errors.New("update state: storage resolver is nil") + } + if mutate == nil { + return errors.New("update state: mutate is nil") + } + + stateMu.Lock() + defer stateMu.Unlock() + + return WithLock(fs.r, stateLockRelTarget, LockOptions{Timeout: stateLockTimeout}, func() error { + state, err := fs.LoadState() + if err != nil { + return fmt.Errorf("load state: %w", err) + } + changed, err := mutate(state) + if err != nil { + return err + } + if !changed { + return nil + } + if err := fs.SaveState(state); err != nil { + return fmt.Errorf("save state: %w", err) + } + return nil + }) +} + // LoadState reads the state file func (fs *FileStorage) LoadState() (*config.State, error) { statePath, err := fs.r.StatePath() From 7e383ca6cc89bf893f6658c0d1b5fd06236c9364 Mon Sep 17 00:00:00 2001 From: imneov Date: Tue, 7 Jul 2026 16:30:10 +0000 Subject: [PATCH 2/3] fix(storage): serialize same-process flock contenders --- internal/storage/filesystem.go | 14 ++------ internal/storage/lock.go | 64 ++++++++++++++++++++++++++++------ 2 files changed, 56 insertions(+), 22 deletions(-) diff --git a/internal/storage/filesystem.go b/internal/storage/filesystem.go index c7fcb14..35d8a07 100644 --- a/internal/storage/filesystem.go +++ b/internal/storage/filesystem.go @@ -6,8 +6,6 @@ import ( "os" "path/filepath" "strings" - "sync" - "time" "github.com/a2d2-dev/claudecm/internal/config" "gopkg.in/yaml.v3" @@ -54,11 +52,8 @@ type Storage interface { UpdateState(mutate func(*config.State) (bool, error)) error } -const stateLockTimeout = 5 * time.Second - var ( stateLockRelTarget = filepath.Join(ConfigDirName, StateFileName) - stateMu sync.Mutex ) // FileStorage implements Storage using the local filesystem. It routes every @@ -252,9 +247,7 @@ func (fs *FileStorage) SaveState(state *config.State) error { // UpdateState runs mutate against state.yaml and, when mutate reports a change, // persists the result while holding the state lock across the full -// load → mutate → save cycle. The in-process mutex covers Linux flock's -// per-process semantics so sibling goroutines cannot open competing -// same-process flock descriptors for state.yaml. +// load → mutate → save cycle. func (fs *FileStorage) UpdateState(mutate func(*config.State) (bool, error)) error { if fs == nil || fs.r == nil { return errors.New("update state: storage resolver is nil") @@ -263,10 +256,7 @@ func (fs *FileStorage) UpdateState(mutate func(*config.State) (bool, error)) err return errors.New("update state: mutate is nil") } - stateMu.Lock() - defer stateMu.Unlock() - - return WithLock(fs.r, stateLockRelTarget, LockOptions{Timeout: stateLockTimeout}, func() error { + return WithLock(fs.r, stateLockRelTarget, LockOptions{}, func() error { state, err := fs.LoadState() if err != nil { return fmt.Errorf("load state: %w", err) diff --git a/internal/storage/lock.go b/internal/storage/lock.go index 8127152..aa431a6 100644 --- a/internal/storage/lock.go +++ b/internal/storage/lock.go @@ -2,9 +2,8 @@ package storage // lock.go is the flock primitive that the FR-5 write-path (writepath.Apply) // and the FR-16 two-phase commit will call. This file is deliberately a pure -// primitive: no SaveProfile/SaveState wiring, no adapter integration, no -// package-level mutable state (coding-standards rule 12). Later stories under -// E7 tie it into the write-path. +// primitive: no SaveProfile/SaveState wiring and no adapter integration. Later +// stories under E7 tie it into the write-path. // // Design choices (per docs/plan/stories/E1-S6.md and architecture §4 step 1): // @@ -25,6 +24,12 @@ package storage // via checkUnderHome on the sidecar path itself after creation. The // second check catches an attacker-planted symlink at the sidecar path. // +// - Same-process callers are serialized through a small process-local gate +// keyed by resolved sidecar path before flock acquisition. Linux flock +// semantics are per-process enough that sibling goroutines can otherwise +// acquire distinct descriptors for the same sidecar and enter the protected +// section together. +// // - The Resolver is required. Passing nil is refused with a clear error — // symmetric with AtomicWrite / EnsureDir in atomic.go. @@ -35,6 +40,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" "github.com/gofrs/flock" @@ -62,6 +68,11 @@ const lockRetryDelay = 25 * time.Millisecond // the resolved sidecar path. var ErrLockTimeout = errors.New("claudecm: lock acquisition timed out") +// processLocks is intentionally process-local and keyed by resolved sidecar +// path. It complements the filesystem flock; it does not replace the +// cross-process lock. +var processLocks sync.Map // map[string]chan struct{} + // LockOptions carries the per-call knobs. Zero-value Timeout maps to // DefaultLockTimeout — see Acquire. type LockOptions struct { @@ -75,9 +86,10 @@ type LockOptions struct { // process exits. Handle carries no package-level state; every field is // unexported so callers cannot manipulate the underlying fd out of band. type Handle struct { - fl *flock.Flock - path string - released bool + fl *flock.Flock + processRelease func() + path string + released bool } // Acquire takes an exclusive advisory lock (flock LOCK_EX) on a sidecar @@ -181,24 +193,50 @@ func Acquire(r *Resolver, target string, opts LockOptions) (*Handle, error) { return nil, fmt.Errorf("lock acquire: sidecar %q: %w", sidecar, err) } + processCtx := context.Background() + if opts.Timeout > 0 { + var cancel context.CancelFunc + processCtx, cancel = context.WithTimeout(context.Background(), opts.Timeout) + defer cancel() + } + releaseProcessLock, err := acquireProcessLock(processCtx, sidecar) + if err != nil { + return nil, err + } + timeout := opts.Timeout if timeout <= 0 { timeout = DefaultLockTimeout } - fl := flock.New(sidecar) - ctx, cancel := context.WithTimeout(context.Background(), timeout) + flockCtx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - locked, lockErr := fl.TryLockContext(ctx, lockRetryDelay) + fl := flock.New(sidecar) + locked, lockErr := fl.TryLockContext(flockCtx, lockRetryDelay) if lockErr != nil { + _ = fl.Close() + releaseProcessLock() if errors.Is(lockErr, context.DeadlineExceeded) { return nil, fmt.Errorf("%w: %s", ErrLockTimeout, sidecar) } return nil, fmt.Errorf("lock acquire: flock %q: %w", sidecar, lockErr) } if !locked { + _ = fl.Close() + releaseProcessLock() + return nil, fmt.Errorf("%w: %s", ErrLockTimeout, sidecar) + } + return &Handle{fl: fl, processRelease: releaseProcessLock, path: sidecar}, nil +} + +func acquireProcessLock(ctx context.Context, sidecar string) (func(), error) { + chAny, _ := processLocks.LoadOrStore(sidecar, make(chan struct{}, 1)) + ch := chAny.(chan struct{}) + select { + case ch <- struct{}{}: + return func() { <-ch }, nil + case <-ctx.Done(): return nil, fmt.Errorf("%w: %s", ErrLockTimeout, sidecar) } - return &Handle{fl: fl, path: sidecar}, nil } // Path returns the resolved sidecar path this Handle owns. Exposed for @@ -223,10 +261,16 @@ func (h *Handle) Release() error { } h.released = true if h.fl == nil { + if h.processRelease != nil { + h.processRelease() + } return nil } unlockErr := h.fl.Unlock() closeErr := h.fl.Close() + if h.processRelease != nil { + h.processRelease() + } // errors.Join is nil-safe: returns nil when both args are nil, and a // single non-nil arg when only one errored. This ensures a closeErr is // never silently dropped just because unlockErr fired first. From a541a8f8744073751870e5cafd7ce19fdff260c8 Mon Sep 17 00:00:00 2001 From: imneov Date: Tue, 7 Jul 2026 16:37:01 +0000 Subject: [PATCH 3/3] docs(standards): declare process-local lock registry as documented rule-12 exception --- docs/architecture/coding-standards.md | 2 +- internal/storage/lock.go | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/architecture/coding-standards.md b/docs/architecture/coding-standards.md index 3397bf2..70179fd 100644 --- a/docs/architecture/coding-standards.md +++ b/docs/architecture/coding-standards.md @@ -48,7 +48,7 @@ These rules encode the locked invariants. Each one is testable. 11. **No `panic` in library code.** `panic` is allowed only in `main()` for unrecoverable startup failures. Every fallible function returns `error`. Wrap with `fmt.Errorf("...: %w", err)` when adding context. -12. **No package-level mutable state.** Pass dependencies explicitly. The single exception is the structured logger configured in `main()`. +12. **No package-level mutable state.** Pass dependencies explicitly. The only documented exceptions are the structured logger configured in `main()` and the process-local lock registry in `internal/storage/lock.go` (`processLocks`). Same-process flock contenders must be serialized process-wide, so the registry's scope must be the process; per-instance state would not serialize goroutines holding different instances. 13. **Two-phase commit on multi-file writes.** When a single command touches more than one owned file, route through `internal/commit`. Direct sequencing of `writepath.Apply` calls across files is a violation. Maps to FR-16. diff --git a/internal/storage/lock.go b/internal/storage/lock.go index aa431a6..c208b73 100644 --- a/internal/storage/lock.go +++ b/internal/storage/lock.go @@ -24,11 +24,13 @@ package storage // via checkUnderHome on the sidecar path itself after creation. The // second check catches an attacker-planted symlink at the sidecar path. // -// - Same-process callers are serialized through a small process-local gate -// keyed by resolved sidecar path before flock acquisition. Linux flock -// semantics are per-process enough that sibling goroutines can otherwise -// acquire distinct descriptors for the same sidecar and enter the protected -// section together. +// - Same-process callers are serialized through processLocks, the documented +// coding-standards rule-12 exception for this package. The registry is +// keyed by resolved sidecar path before flock acquisition because Linux +// flock semantics are per-process enough that sibling goroutines can +// otherwise acquire distinct descriptors for the same sidecar and enter the +// protected section together. The scope must be process-wide; per-instance +// state would not serialize goroutines holding different Resolver instances. // // - The Resolver is required. Passing nil is refused with a clear error — // symmetric with AtomicWrite / EnsureDir in atomic.go.