Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions .claude/rules/go.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,55 @@ time.Now().UTC() // yes
time.Now() // no
```

**12. Never return internal pointers from a locked accessor**

If a type owns state behind a mutex, its `Get*`/`List*` methods return
value copies (deep enough — clone nested slices/maps/pointers). Mutation
goes through methods on that owning type; those methods take the lock
and mutate in place. A returned `*T` into concurrently-mutated state is
a bug — the lock protects the map, not the values.

```go
// WRONG — caller holds a pointer into the store's live object,
// mutates it after the lock is dropped, other callers see torn writes
func (s *Store) ListSessions() []*Session {
s.mu.RLock()
defer s.mu.RUnlock()
out := make([]*Session, 0, len(s.sessions))
for _, sess := range s.sessions {
out = append(out, sess) // leaks internal pointer
}
return out
}

// RIGHT — snapshots fully decoupled from internal state
func (s *Store) ListSessions() []Session {
s.mu.RLock()
defer s.mu.RUnlock()
out := make([]Session, 0, len(s.sessions))
for _, sess := range s.sessions {
out = append(out, cloneSession(sess))
}
return out
}

// RIGHT — mutation goes through the owner under the lock
func (s *Store) UpdateSession(key string, fn func(*Session) error) error {
s.mu.Lock()
defer s.mu.Unlock()
sess, ok := s.sessions[key]
if !ok {
return fmt.Errorf("session %s: %w", key, ErrNotFound)
}
return fn(sess)
}
```

The Kubernetes lister/informer pattern is the canonical example: cached
reads give immutable snapshots, writes go through the API. `go test -race`
is the backstop; the type signature is the fence. See orc
finding-032-store-sync-contract-leak.

## Error handling

- Every exported function that can fail returns `error` last
Expand Down
13 changes: 9 additions & 4 deletions internal/api/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,10 +267,15 @@ func (m *Manifest) Apply(store *Store) error {
Generation: 1,
CreatedAt: now,
}
// Update roles if team already exists.
existing, err := store.GetTeam(team.Key())
if err == nil {
existing.Roles = roles
// Update roles if team already exists; route through the store
// lock so the mutation doesn't race concurrent readers.
if _, err := store.GetTeam(team.Key()); err == nil {
if err := store.UpdateTeam(team.Key(), func(live *Team) error {
live.Roles = roles
return nil
}); err != nil {
return fmt.Errorf("apply team %s: %w", mt.Name, err)
}
} else {
if err := store.CreateTeam(team); err != nil {
return fmt.Errorf("apply team %s: %w", mt.Name, err)
Expand Down
Loading
Loading