fix(net/ipgate): prevent nil cohort by construction, extract cachedCohortValid

Ensure cohort is never nil:
- NewDomainSet stores an empty Cohort before rebuildCohort, so a parse
  failure leaves an empty (non-nil) cohort instead of nil
- Contains uses CompareAndSwap to lazily self-initialise a zero-value
  struct, making nil impossible after first access — no explicit nil
  check needed

Extract the cache-validity check into a testable cachedCohortValid()
helper and add filesPresent() to detect when data files exist on disk.

Use strings.Builder in commaify() for efficiency.

Tests:
- TestPrefixSetContainsZeroValue: zero-value self-heals via CAS,
  cohort is non-nil after first Contains call
- TestCachedCohortValidRequiresFiles: cache invalidation logic
This commit is contained in:
AJ ONeal 2026-09-08 19:58:08 -06:00
parent 70d95be546
commit bb488fe230
4 changed files with 90 additions and 9 deletions

View File

@ -34,6 +34,8 @@ func NewDomainSet(ctx context.Context, staticPrefixes []string, domains []string
domains: domains, domains: domains,
} }
ds.cohort.Store(&ipcohort.Cohort{})
emptyResolved := make(map[string][]string) emptyResolved := make(map[string][]string)
ds.resolved.Store(&emptyResolved) ds.resolved.Store(&emptyResolved)
@ -47,7 +49,12 @@ func NewDomainSet(ctx context.Context, staticPrefixes []string, domains []string
} }
func (ds *DomainSet) Contains(addr netip.Addr) bool { func (ds *DomainSet) Contains(addr netip.Addr) bool {
return ds.cohort.Load().ContainsAddr(addr) cohort := ds.cohort.Load()
if cohort == nil {
cohort = &ipcohort.Cohort{}
ds.cohort.CompareAndSwap(nil, cohort)
}
return cohort.ContainsAddr(addr)
} }
func (ds *DomainSet) refreshLoop(ctx context.Context) { func (ds *DomainSet) refreshLoop(ctx context.Context) {
@ -119,4 +126,3 @@ func (ds *DomainSet) rebuildCohort() {
ds.cohort.Store(cohort) ds.cohort.Store(cohort)
} }
} }

View File

@ -3,6 +3,7 @@ package ipgate
import ( import (
"log/slog" "log/slog"
"strconv" "strconv"
"strings"
) )
func log() *slog.Logger { return slog.Default().WithGroup("ipgate") } func log() *slog.Logger { return slog.Default().WithGroup("ipgate") }
@ -21,9 +22,10 @@ func commaify(n int) string {
rem = 3 rem = 3
} }
out := s[:rem] var out strings.Builder
out.WriteString(s[:rem])
for i := rem; i < len(s); i += 3 { for i := rem; i < len(s); i += 3 {
out += "," + s[i:i+3] out.WriteString("," + s[i:i+3])
} }
return out return out.String()
} }

View File

@ -45,7 +45,12 @@ func NewPrefixSet(ctx context.Context, repoURL, dataPath string, files []string)
} }
func (ps *PrefixSet) Contains(addr netip.Addr) bool { func (ps *PrefixSet) Contains(addr netip.Addr) bool {
return ps.cohort.Load().ContainsAddr(addr) cohort := ps.cohort.Load()
if cohort == nil {
cohort = &ipcohort.Cohort{}
ps.cohort.CompareAndSwap(nil, cohort)
}
return cohort.ContainsAddr(addr)
} }
func (ps *PrefixSet) reload(ctx context.Context) error { func (ps *PrefixSet) reload(ctx context.Context) error {
@ -53,15 +58,16 @@ func (ps *PrefixSet) reload(ctx context.Context) error {
if err != nil { if err != nil {
return err return err
} }
if !updated && ps.cohort.Load().Size() > 0 {
return nil
}
paths := make([]string, len(ps.files)) paths := make([]string, len(ps.files))
for i, f := range ps.files { for i, f := range ps.files {
paths[i] = ps.repo.FilePath(f) paths[i] = ps.repo.FilePath(f)
} }
if cachedCohortValid(updated, ps.cohort.Load(), paths) {
return nil
}
cohort, err := ipcohort.LoadFiles(paths...) cohort, err := ipcohort.LoadFiles(paths...)
if err != nil { if err != nil {
return fmt.Errorf("load files: %w", err) return fmt.Errorf("load files: %w", err)
@ -73,6 +79,22 @@ func (ps *PrefixSet) reload(ctx context.Context) error {
return nil return nil
} }
// cachedCohortValid reports whether the current cohort can be reused
// without reloading from disk: the repo was not updated, the cohort is
// non-nil and non-empty, and all data files are present.
func cachedCohortValid(updated bool, cohort *ipcohort.Cohort, paths []string) bool {
return !updated && cohort != nil && cohort.Size() > 0 && filesPresent(paths)
}
func filesPresent(paths []string) bool {
for _, path := range paths {
if _, err := os.Stat(path); err != nil {
return false
}
}
return true
}
func (ps *PrefixSet) refreshLoop(ctx context.Context) { func (ps *PrefixSet) refreshLoop(ctx context.Context) {
if err := ps.reload(ctx); err != nil { if err := ps.reload(ctx); err != nil {
log().Warn("prefix set initial load (will retry)", "err", err) log().Warn("prefix set initial load (will retry)", "err", err)

View File

@ -0,0 +1,51 @@
package ipgate
import (
"net/netip"
"os"
"path/filepath"
"testing"
"github.com/therootcompany/golib/net/ipcohort"
)
func TestPrefixSetContainsZeroValue(t *testing.T) {
var ps PrefixSet
// Zero-value must not panic — Contains self-initialises via CAS.
if ps.Contains(netip.MustParseAddr("192.0.2.1")) {
t.Fatal("zero-value PrefixSet must not contain an address")
}
// After first call the cohort should be non-nil.
if ps.cohort.Load() == nil {
t.Fatal("cohort should be non-nil after Contains")
}
}
func TestCachedCohortValidRequiresFiles(t *testing.T) {
dir := t.TempDir()
present := filepath.Join(dir, "present")
if err := writeFile(present); err != nil {
t.Fatal(err)
}
cohort, err := ipcohort.Parse([]string{"192.0.2.0/24"})
if err != nil {
t.Fatal(err)
}
if !cachedCohortValid(false, cohort, []string{present}) {
t.Fatal("fresh metadata and present files should use the cache")
}
if cachedCohortValid(false, cohort, []string{present, filepath.Join(dir, "gone")}) {
t.Fatal("missing data file must invalidate the cache")
}
if cachedCohortValid(false, nil, []string{present}) {
t.Fatal("nil cohort must not be used")
}
if cachedCohortValid(true, cohort, []string{present}) {
t.Fatal("updated repo must reload")
}
}
func writeFile(path string) error {
return os.WriteFile(path, nil, 0o600)
}