diff --git a/Server/CLAUDE.md b/Server/CLAUDE.md index 802586d8..97af742a 100644 --- a/Server/CLAUDE.md +++ b/Server/CLAUDE.md @@ -23,4 +23,5 @@ prometheus. FIFO because clients ack only `max(seq)` — a frame that skips the queue, or a seq allocated for a frame that is then dropped, is silently unrecoverable. - Prefer the standard library. `syncutil` exists so lock usage is uniform and - detectable; do not hand-roll around it. + detectable; do not hand-roll around it. `Server/invariants/` enforces this + at `go test` time; exceptions are greppable via `grep -rn "invariant:allow" Server/`. diff --git a/Server/invariants/invariants.go b/Server/invariants/invariants.go new file mode 100644 index 00000000..04e279f6 --- /dev/null +++ b/Server/invariants/invariants.go @@ -0,0 +1,226 @@ +// Package invariants holds OwnCord-specific structural rules for the server +// tree. Each rule encodes an invariant that is either documented in +// Server/CLAUDE.md or was proven by a real defect in the findings ledger, and +// that the generic linters in .golangci.yml cannot express. +// +// Rules are syntactic. They use go/parser and go/ast only -- no type +// information, no third-party dependency. parser.ParseFile ignores build +// constraints, so files behind -tags otel, -tags wazero and -tags deadlock are +// checked like any other; a rule must not be evadable by moving code behind a +// build tag. +package invariants + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path" + "sort" + "strings" +) + +// Violation is one rule breach at one source location. +type Violation struct { + Rule string // stable rule id, e.g. "syncutil-locks" + File string // slash-separated, relative to the tree root + Line int + Msg string // what is wrong, why it matters, and what to do instead +} + +func (v Violation) String() string { + return fmt.Sprintf("%s:%d: [%s] %s", v.File, v.Line, v.Rule, v.Msg) +} + +// Rule is one structural check. +type Rule struct { + // ID is the stable identifier used in messages and allow comments. + ID string + // Scope lists directories relative to the tree root, e.g. {"ws", "service"}. + // An empty Scope means every directory. + Scope []string + // Check inspects one parsed file. rel is the slash-separated path relative + // to the tree root, and is what the returned Violation.File must carry. + Check func(f *ast.File, fset *token.FileSet, rel string) []Violation +} + +// inScope reports whether dir falls under the rule's Scope. dir is +// slash-separated and relative to the tree root; "" is the root itself. +func (r Rule) inScope(dir string) bool { + if len(r.Scope) == 0 { + return true + } + for _, s := range r.Scope { + if dir == s || strings.HasPrefix(dir, s+"/") { + return true + } + } + return false +} + +// Rules is the registry every gate runs. +var Rules = []Rule{syncutilLocks} + +// allowPrefix introduces a line-scoped suppression: +// +// mu sync.Mutex //invariant:allow syncutil-locks — +// +// The reason is mandatory. An allow comment without one does not suppress +// anything and is itself reported, so the hatch cannot silently disable a +// rule. The comment must be on the same line as the flagged code -- one on +// the line above it is not matched and silently fails to suppress. +const allowPrefix = "//invariant:allow" + +// allowIndex maps line number to the set of rule ids suppressed on that line. +// It also returns a violation for every allow comment that names no rule or +// gives no reason. +func allowIndex(f *ast.File, fset *token.FileSet, rel string) (map[int]map[string]bool, []Violation) { + idx := make(map[int]map[string]bool) + var bad []Violation + + for _, group := range f.Comments { + for _, c := range group.List { + text := strings.TrimSpace(c.Text) + if !strings.HasPrefix(text, allowPrefix) { + continue + } + line := fset.Position(c.Slash).Line + rest := strings.TrimSpace(strings.TrimPrefix(text, allowPrefix)) + id, reason, _ := strings.Cut(rest, " ") + id = strings.TrimSpace(id) + // Strip the separator between the rule id and the prose reason. + reason = strings.TrimSpace(strings.TrimLeft(strings.TrimSpace(reason), "-—:")) + + if id == "" || reason == "" { + bad = append(bad, Violation{ + Rule: "invariant-allow-needs-reason", + File: rel, + Line: line, + Msg: "//invariant:allow must name a rule and give a reason, " + + "e.g. //invariant:allow syncutil-locks — ", + }) + continue + } + if idx[line] == nil { + idx[line] = make(map[string]bool) + } + idx[line][id] = true + } + } + return idx, bad +} + +// CheckSource runs every in-scope rule over one file's source. rel must be the +// slash-separated path relative to the tree root, because Scope matching and +// the reported File both derive from it. +func CheckSource(fset *token.FileSet, rel string, src []byte) []Violation { + return checkSourceWith(Rules, fset, rel, src) +} + +// checkSourceWith is CheckSource against an explicit rule set rather than the +// global registry, so one rule's tests can run it in isolation instead of +// tripping over violations from fixtures written for a sibling rule. +func checkSourceWith(rules []Rule, fset *token.FileSet, rel string, src []byte) []Violation { + f, err := parser.ParseFile(fset, rel, src, parser.ParseComments) + if err != nil { + return []Violation{{Rule: "parse", File: rel, Line: 0, Msg: err.Error()}} + } + + allowed, out := allowIndex(f, fset, rel) + + dir := path.Dir(rel) + if dir == "." { + dir = "" + } + + for _, r := range rules { + if !r.inScope(dir) { + continue + } + for _, v := range r.Check(f, fset, rel) { + // Keyed on v.Rule (what the rule actually emits), not r.ID: a + // rule that reports a sub-id would otherwise require an allow + // comment naming an id nobody prints. + if allowed[v.Line][v.Rule] { + continue + } + out = append(out, v) + } + } + return out +} + +// skipDirs are never descended into, for two different reasons: dbgen and +// vendor hold generated/vendored code governed by their own generator or +// upstream, not by these rules; testdata and data hold non-source content -- +// test fixtures, and (for data, Server/data/) gitignored runtime state such +// as the SQLite db, certs and uploads -- with no Go files to check. +var skipDirs = map[string]bool{ + "dbgen": true, + "vendor": true, + "testdata": true, + "data": true, +} + +// Run parses every non-test .go file under root and returns every violation, +// sorted by file then line so failures are deterministic. +// +// The tree is walked through os.Root, which confines every read to root and +// cannot be escaped by a symlink. That also makes the walk paths slash- +// separated and already relative to root, which is exactly the form +// CheckSource and Violation.File want. +func Run(root string) ([]Violation, error) { + r, err := os.OpenRoot(root) + if err != nil { + return nil, err + } + defer func() { + _ = r.Close() + }() + rfs := r.FS() + + fset := token.NewFileSet() + var out []Violation + + err = fs.WalkDir(rfs, ".", func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + // Never skip the root itself: its name is ".", which would + // otherwise match the dot-prefix test and abort the whole walk. + if p == "." { + return nil + } + if skipDirs[d.Name()] || strings.HasPrefix(d.Name(), ".") { + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + src, err := fs.ReadFile(rfs, p) + if err != nil { + return err + } + out = append(out, CheckSource(fset, p, src)...) + return nil + }) + if err != nil { + return nil, err + } + + // Stable: two violations can share a file:line (an allow comment with no + // reason produces exactly that, alongside the rule it failed to + // suppress), and sort.Slice does not promise to preserve their order. + sort.SliceStable(out, func(i, j int) bool { + if out[i].File != out[j].File { + return out[i].File < out[j].File + } + return out[i].Line < out[j].Line + }) + return out, nil +} diff --git a/Server/invariants/invariants_test.go b/Server/invariants/invariants_test.go new file mode 100644 index 00000000..060792ba --- /dev/null +++ b/Server/invariants/invariants_test.go @@ -0,0 +1,137 @@ +package invariants + +import ( + "go/token" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestServerInvariants is the gate. It runs every registered rule over the +// real server tree and reports every violation at once. +func TestServerInvariants(t *testing.T) { + violations, err := Run("..") + if err != nil { + t.Fatalf("walking the server tree: %v", err) + } + for _, v := range violations { + t.Errorf("%s", v) + } + if len(violations) > 0 { + t.Logf("%d invariant violation(s). Each message names the fix; "+ + "use //invariant:allow — only with a real reason.", + len(violations)) + } + + assertScopesCovered(t, "..") +} + +// assertScopesCovered guards against the gate passing vacuously: zero +// violations is indistinguishable from zero files scanned (a moved package, +// or a Scope entry that no longer exists, would go green while enforcing +// nothing). It fails loudly, naming the offending Scope entry, if any +// registered Rule.Scope directory does not exist under root or holds no +// non-test .go file. +func assertScopesCovered(t *testing.T, root string) { + t.Helper() + for _, r := range Rules { + for _, scope := range r.Scope { + dir := filepath.Join(root, filepath.FromSlash(scope)) + info, err := os.Stat(dir) + if err != nil || !info.IsDir() { + t.Fatalf("rule %q Scope entry %q does not resolve to a directory under %q: %v", + r.ID, scope, root, err) + continue + } + + found := false + walkErr := filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() && strings.HasSuffix(p, ".go") && !strings.HasSuffix(p, "_test.go") { + found = true + } + return nil + }) + if walkErr != nil { + t.Fatalf("walking rule %q Scope entry %q: %v", r.ID, scope, walkErr) + } + if !found { + t.Fatalf("rule %q Scope entry %q contains no non-test .go file under %q; "+ + "the gate would be enforcing nothing there", r.ID, scope, root) + } + } + } +} + +// TestBuildTagGatedFilesAreStillChecked locks in the package doc's central +// anti-evasion guarantee: parser.ParseFile ignores build constraints, so a +// file gated behind e.g. -tags deadlock is checked exactly like any other -- +// a raw mutex cannot be hidden from the rules by moving it behind a tag. +func TestBuildTagGatedFilesAreStillChecked(t *testing.T) { + src := `//go:build deadlock + +package ws + +import "sync" + +type Hub struct{ mu sync.Mutex } +` + got := CheckSource(token.NewFileSet(), "ws/x.go", []byte(src)) + if len(got) != 1 { + t.Fatalf("got %d violation(s), want 1: %v", len(got), got) + } +} + +// TestRunExclusions builds a throwaway tree so the walker's exclusions are +// tested directly, rather than vacuously against a clean real tree. +func TestRunExclusions(t *testing.T) { + root := t.TempDir() + lock := []byte("package ws\nimport \"sync\"\ntype h struct{ mu sync.Mutex }\n") + + write := func(rel string) { + full := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, lock, 0o644); err != nil { + t.Fatal(err) + } + } + + write("ws/real.go") // reported + write("ws/real_test.go") // excluded: _test.go + write("ws/testdata/x.go") // excluded: skipDirs + write("api/other.go") // excluded: out of scope + + got, err := Run(root) + if err != nil { + t.Fatalf("Run: %v", err) + } + if len(got) != 1 { + t.Fatalf("got %d violation(s), want 1:\n%v", len(got), got) + } + if got[0].File != "ws/real.go" { + t.Errorf("File = %q, want %q", got[0].File, "ws/real.go") + } +} + +func TestRuleScopeMatching(t *testing.T) { + r := Rule{ID: "x", Scope: []string{"ws", "service"}} + cases := map[string]bool{ + "ws": true, + "ws/internal": true, + "service": true, + "api": false, + "": false, + "wsx": false, + } + for dir, want := range cases { + if got := r.inScope(dir); got != want { + t.Errorf("inScope(%q) = %v, want %v", dir, got, want) + } + } +} diff --git a/Server/invariants/syncutil_locks.go b/Server/invariants/syncutil_locks.go new file mode 100644 index 00000000..04e0f8f6 --- /dev/null +++ b/Server/invariants/syncutil_locks.go @@ -0,0 +1,105 @@ +package invariants + +import ( + "go/ast" + "go/token" + "strconv" +) + +// syncutilLocksID is the rule's stable id. It is a const, not a field read +// off syncutilLocks, because the Rule var's own initializer (Check: +// checkSyncutilLocks) would otherwise form an initialization cycle with the +// function that emits it. +const syncutilLocksID = "syncutil-locks" + +// syncutilLocks forbids raw sync.Mutex and sync.RWMutex in the packages whose +// lock order the -tags deadlock CI pass exists to observe. +// +// syncutil.Mutex is a build-tag alias: sync.Mutex in production, +// deadlock.Mutex under -tags deadlock. A lock declared as sync.Mutex is +// therefore invisible to that pass. Server/CLAUDE.md states the rule directly: +// "syncutil exists so lock usage is uniform and detectable; do not hand-roll +// around it." +var syncutilLocks = Rule{ + ID: syncutilLocksID, + Scope: []string{"ws", "service"}, + Check: checkSyncutilLocks, +} + +// checkSyncutilLocks flags any sync.Mutex/sync.RWMutex selector, wherever it +// syntactically appears: struct field, embedded field, var spec (typed or +// inferred from a composite literal), short assignment, type alias, or +// composite element/value type ([]sync.Mutex, map[K]sync.Mutex). A single +// selector match subsumes all of these, so no per-construct cases are needed. +// +// A dot-import of "sync" is reported separately: it would let a bare Mutex +// evade the selector match entirely. +func checkSyncutilLocks(f *ast.File, fset *token.FileSet, rel string) []Violation { + var out []Violation + + names, dotImports := syncImportNames(f) + + for _, imp := range dotImports { + out = append(out, Violation{ + Rule: syncutilLocksID, + File: rel, + Line: fset.Position(imp.Pos()).Line, + Msg: `dot-import of "sync" defeats syncutil-locks (a bare Mutex/RWMutex can no longer be matched); import sync normally`, + }) + } + + if len(names) == 0 { + return out + } + + ast.Inspect(f, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok || !names[pkg.Name] { + return true + } + name := sel.Sel.Name + if name != "Mutex" && name != "RWMutex" { + return true + } + out = append(out, Violation{ + Rule: syncutilLocksID, + File: rel, + Line: fset.Position(sel.Pos()).Line, + Msg: "raw sync." + name + " is invisible to the -tags deadlock CI pass; " + + "declare it as syncutil." + name + " (github.com/owncord/server/syncutil), " + + "or add //invariant:allow syncutil-locks — ", + }) + return true + }) + + return out +} + +// syncImportNames returns the local identifiers this file binds to the +// "sync" import path (its name, or an alias), and separately every dot-import +// of "sync" (import . "sync"), which binds no identifier at all. +func syncImportNames(f *ast.File) (names map[string]bool, dotImports []*ast.ImportSpec) { + names = make(map[string]bool) + for _, imp := range f.Imports { + p, err := strconv.Unquote(imp.Path.Value) + if err != nil || p != "sync" { + continue + } + switch { + case imp.Name == nil: + names["sync"] = true + case imp.Name.Name == "_": + // Blank import: no identifier is bound, so sync.Mutex cannot be + // spelled at all. + case imp.Name.Name == ".": + dotImports = append(dotImports, imp) + default: + names[imp.Name.Name] = true + } + } + return names, dotImports +} diff --git a/Server/invariants/syncutil_locks_test.go b/Server/invariants/syncutil_locks_test.go new file mode 100644 index 00000000..15bba93e --- /dev/null +++ b/Server/invariants/syncutil_locks_test.go @@ -0,0 +1,206 @@ +package invariants + +import ( + "go/token" + "strings" + "testing" +) + +func TestSyncutilLocks(t *testing.T) { + tests := []struct { + name string + path string + src string + want int + }{ + { + name: "raw RWMutex field in ws is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct{ mu sync.RWMutex } +`, + want: 1, + }, + { + name: "raw Mutex field in service is flagged", + path: "service/x.go", + src: `package service +import "sync" +type S struct{ mu sync.Mutex } +`, + want: 1, + }, + { + name: "embedded raw Mutex is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct{ sync.Mutex } +`, + want: 1, + }, + { + name: "local variable raw Mutex is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +func f() { var mu sync.Mutex; _ = mu } +`, + want: 1, + }, + { + name: "syncutil alias is clean", + path: "ws/x.go", + src: `package ws +import "github.com/owncord/server/syncutil" +type Hub struct{ mu syncutil.RWMutex } +`, + want: 0, + }, + { + name: "sync.Once and sync.WaitGroup are not locks", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct { + once sync.Once + wg sync.WaitGroup +} +`, + want: 0, + }, + { + name: "package outside scope is ignored", + path: "api/x.go", + src: `package api +import "sync" +type S struct{ mu sync.Mutex } +`, + want: 0, + }, + { + name: "allow comment with a reason suppresses", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct { + mu sync.Mutex //invariant:allow syncutil-locks — guards a cgo callback that needs a std lock +} +`, + want: 0, + }, + { + name: "allow comment without a reason does not suppress and is itself a violation", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct { + mu sync.Mutex //invariant:allow syncutil-locks +} +`, + want: 2, + }, + { + name: "allow comment for a different rule does not suppress", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct { + mu sync.Mutex //invariant:allow some-other-rule — unrelated +} +`, + want: 1, + }, + { + name: "short assignment composite literal is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +func f() { + mu := sync.Mutex{} + _ = mu +} +`, + want: 1, + }, + { + name: "var with inferred type (nil ValueSpec.Type) is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +var mu = sync.Mutex{} +`, + want: 1, + }, + { + name: "type alias to sync.Mutex is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +type m = sync.Mutex +`, + want: 1, + }, + { + name: "aliased sync import is flagged", + path: "ws/x.go", + src: `package ws +import s "sync" +type Hub struct{ mu s.Mutex } +`, + want: 1, + }, + { + name: "dot-import of sync is flagged as its own violation", + path: "ws/x.go", + src: `package ws +import . "sync" +var mu Mutex +`, + want: 1, + }, + { + name: "slice-of-mutex composite type is flagged", + path: "ws/x.go", + src: `package ws +import "sync" +type Hub struct{ mus []sync.Mutex } +`, + want: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := checkSourceWith([]Rule{syncutilLocks}, token.NewFileSet(), tt.path, []byte(tt.src)) + if len(got) != tt.want { + t.Fatalf("got %d violation(s), want %d:\n%v", len(got), tt.want, got) + } + }) + } +} + +func TestSyncutilLocksMessageNamesTheFix(t *testing.T) { + src := `package ws +import "sync" +type Hub struct{ mu sync.RWMutex } +` + got := checkSourceWith([]Rule{syncutilLocks}, token.NewFileSet(), "ws/x.go", []byte(src)) + if len(got) != 1 { + t.Fatalf("got %d violation(s), want 1: %v", len(got), got) + } + v := got[0] + if v.Rule != "syncutil-locks" { + t.Errorf("Rule = %q, want %q", v.Rule, "syncutil-locks") + } + if v.Line != 3 { + t.Errorf("Line = %d, want 3", v.Line) + } + if !strings.Contains(v.Msg, "syncutil.RWMutex") { + t.Errorf("message must name the fix, got %q", v.Msg) + } + if !strings.Contains(v.Msg, "-tags deadlock") { + t.Errorf("message must name the consequence, got %q", v.Msg) + } +} diff --git a/Server/service/permission.go b/Server/service/permission.go index ad3f71ec..a753c4fb 100644 --- a/Server/service/permission.go +++ b/Server/service/permission.go @@ -3,12 +3,12 @@ package service import ( "context" "log/slog" - "sync" "sync/atomic" "time" "github.com/owncord/server/db" "github.com/owncord/server/permissions" + "github.com/owncord/server/syncutil" "github.com/owncord/server/telemetry" ) @@ -31,7 +31,7 @@ type PermissionService struct { st Store checker *permissions.Checker - mu sync.RWMutex + mu syncutil.RWMutex cache map[int64]*cachedPerms // keyed by userID // gen is bumped by every Invalidate* call. getOrPopulate snapshots it before // its DB read and refuses to cache if it changed, so an invalidation that diff --git a/Server/service/role.go b/Server/service/role.go index 7eecb4b3..56344c1d 100644 --- a/Server/service/role.go +++ b/Server/service/role.go @@ -6,10 +6,10 @@ import ( "log/slog" "regexp" "strings" - "sync" "github.com/owncord/server/db" "github.com/owncord/server/permissions" + "github.com/owncord/server/syncutil" ) // RoleService owns create/edit/delete/reorder of roles. It is the only writer @@ -28,7 +28,7 @@ type RoleService struct { // the role cap are enforced against a ListRoles snapshot, not by a DB // constraint, so two interleaved mutations can both see the same free // slot. Single-process server — one lock covers every writer. - mu sync.Mutex + mu syncutil.Mutex } // NewRoleService creates a RoleService. diff --git a/Server/ws/hub.go b/Server/ws/hub.go index d0b5e059..ab2e9076 100644 --- a/Server/ws/hub.go +++ b/Server/ws/hub.go @@ -127,7 +127,7 @@ type Hub struct { // voiceKeyHolders maps channelID → userID of the current key holder. // The key holder is the connected participant with the lowest userID in the channel. // Protected by keyHolderMu. - keyHolderMu sync.RWMutex + keyHolderMu syncutil.RWMutex voiceKeyHolders map[int64]int64 // Presence coalescer (QueuePresence): latest queued presence per user and diff --git a/Server/ws/pubsub.go b/Server/ws/pubsub.go index 3c4078b8..14ee6e0b 100644 --- a/Server/ws/pubsub.go +++ b/Server/ws/pubsub.go @@ -3,7 +3,8 @@ package ws import ( "strconv" "strings" - "sync" + + "github.com/owncord/server/syncutil" ) // Topic is a named pub/sub channel that clients can subscribe to. @@ -61,7 +62,7 @@ func UserTopic(userID int64) Topic { // // Thread-safe: all methods may be called from any goroutine. type PubSub struct { - mu sync.RWMutex + mu syncutil.RWMutex // Forward index: topic → (userID → *Client) topics map[Topic]map[int64]*Client diff --git a/Server/ws/topic_rate_limiter.go b/Server/ws/topic_rate_limiter.go index 2e8e8ff2..59e38474 100644 --- a/Server/ws/topic_rate_limiter.go +++ b/Server/ws/topic_rate_limiter.go @@ -1,14 +1,15 @@ package ws import ( - "sync" "time" + + "github.com/owncord/server/syncutil" ) // TopicRateLimiter enforces per-topic throughput caps to prevent a single // busy channel from saturating the broadcast loop and starving others. type TopicRateLimiter struct { - mu sync.Mutex + mu syncutil.Mutex buckets map[Topic]*tokenBucket defaultRate int // messages per window window time.Duration // window size