mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
feat(invariants): add server invariant rules and close five deadlock blind spots (#1383)
* feat(invariants): add the invariant-rule harness and the syncutil-locks rule * fix(ws,service): route the last five raw mutexes through syncutil The -tags deadlock CI pass only observes locks declared via syncutil, whose Mutex/RWMutex are build-tag aliases. These five were declared as raw sync types and were invisible to it, including the hub voice key-holder lock and the permission and role caches. TestServerInvariants now gates the tree against regressions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(invariants): walk the tree through os.Root to close a symlink TOCTOU gosec G122: reading a filepath.WalkDir-supplied path is race-prone, since a symlink swapped between the walk and the read escapes the intended tree. os.Root confines every read to the root and cannot be traversed out of. Walking the root's fs.FS also yields slash-separated paths already relative to it, so the filepath.Rel and ToSlash conversion is no longer needed. * fix(invariants): close syncutil-locks evasions, isolate per-rule tests, harden the gate - I1: TestServerInvariants now asserts every registered Rule.Scope directory exists and holds at least one non-test .go file, so the gate cannot pass by scanning nothing. - I2: split CheckSource into a thin wrapper over an unexported checkSourceWith(rules, ...), so TestSyncutilLocks tests the syncutil-locks rule in isolation instead of the whole registry. - I3: broaden checkSyncutilLocks to a single SelectorExpr match (any sync.Mutex/sync.RWMutex reference bound via f.Imports, aliases included) instead of only *ast.Field/*ast.ValueSpec. Catches := composite literals, untyped var specs, type aliases, and []sync.Mutex/map[K]sync.Mutex, none of which the old rule saw. A dot-import of "sync" is now its own violation, since it would otherwise let a bare Mutex evade the selector match entirely. - M2: suppression now keys off the violation's own Rule id (allowed[v.Line][v.Rule]) rather than the running rule's ID, so a rule that ever emits a sub-id isn't silently unsuppressible. - M3: Run sorts with sort.SliceStable, since an unreasoned allow comment and the violation it fails to suppress can share a file:line. - M4/M5/M1-partial: add a build-tag-gated fixture test, document that allow comments must be same-line, and correct the skipDirs comment to describe both the generated-code and gitignored-runtime-dir cases it actually covers. All ten original TestSyncutilLocks subtests pass unchanged; six new subtests cover the evasions above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(server): point at the syncutil-locks invariant gate Server/CLAUDE.md told developers not to hand-roll around syncutil but never said it's enforced. Note that Server/invariants/ checks it at go test time and that exceptions are greppable via grep -rn "invariant:allow" Server/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
J3vb
+2
-1
@@ -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/`.
|
||||
|
||||
@@ -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 — <reason>
|
||||
//
|
||||
// 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 — <why this site is exempt>",
|
||||
})
|
||||
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
|
||||
}
|
||||
@@ -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 <rule> — <reason> 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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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 — <reason>",
|
||||
})
|
||||
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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
+1
-1
@@ -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
|
||||
|
||||
+3
-2
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user