Skip to content

fix(globals): make package-level lookup state immutable - #27

Merged
hammadmajid merged 1 commit into
fix/17-output-cancellationfrom
fix/18-immutable-globals
Sep 5, 2026
Merged

hammadmajid merged 1 commit into
fix/17-output-cancellationfrom
fix/18-immutable-globals

Conversation

@hammadmajid

@hammadmajid hammadmajid commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

Three small hardening items on mutable package-level state. None is a live bug; each is one edit away from becoming one.

Closes #18

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Performance improvement
  • Refactoring or code cleanup
  • Documentation update
  • CI/CD or build workflow change

Key Changes

filter.BuiltinTypes unexported. An exported map is a writable global, and a FileTypeDef's Extensions/Filenames slices were writable through a read. Nothing writes it today and both readers run single-threaded before the pipeline starts — but one filter.BuiltinTypes[x] = … added later, plausibly for a --type-add flag, becomes a concurrent map write the moment type matching moves off the main goroutine, which is a hard runtime crash rather than something -race warns about first.

Now builtinTypes, with LookupType(name string) (FileTypeDef, bool) returning slices.Cloned slices. NewTypeMatcher routes through LookupType rather than reading the table directly: a couple of tiny clones once per process (type names come from CLI args) buys a single read path that cannot rot into a second, slice-leaking convention.

packMagic / idxV2Magic are now const strings. They were mutable []byte globals used as constants; any function in the package could overwrite the elements. pack.go already compared as a string; pack_index.go's bytes.Equal becomes an allocation-free string(data[:4]) != idxV2Magic.

Deleted the stale unused exclusion in .golangci.yml claiming field zlibPool is unused. Pooling landed — p.zlibPool is used twice in pack.go. nolintlint does not police issues.exclusions, so this would never have self-cleaned.

Verification & Testing

  • Ran go test -v -count=1 ./...
  • Ran go test -race -shuffle=on -count=1 ./...
  • Ran go vet ./...
  • Added or updated unit/integration tests
  • Tested manually against sample Git repository histories
go build ./...                                     clean
go vet ./internal/filter/... ./internal/gitengine/ clean
go test -race -count=1 ./internal/filter/          ok 1.03s
go test -race -count=1 ./internal/gitengine/       ok 1.42s
gofmt -l                                           empty

grep -rn BuiltinTypes over the whole tree returns nothing.

TestLookupTypeReturnsCopy mutates the returned Extensions[0]/Filenames[0], re-looks-up to assert the builtin table is uncorrupted, asserts a matcher built afterwards still matches Dockerfile and my.dockerfile, and asserts an unknown type reports ok=false. Mutation: reverting LookupType to return def, true without the clone fails all four assertions.

One line of pack_test.go (idxBuf.WriteWriteString) is included because bytes.Buffer.Write cannot take a const string; it could not be pre-applied on the layer below, where idxV2Magic is still []byte.

Note

Stacked on #26. Top of the stack at time of writing.

Checklist

  • gofmt clean
  • Every new test mutation-checked: reverting the fix makes it fail
  • This layer builds and passes the full race suite on its own, not just at the top of the stack

@hammadmajid
hammadmajid force-pushed the fix/18-immutable-globals branch from 385d449 to 9327d6e Compare September 5, 2026 14:14
Three hardening items on mutable package-level state. None was a live bug;
each was one edit away from becoming one.

filter.BuiltinTypes was an exported mutable map. Nothing writes it today and
both readers run single-threaded before the pipeline starts, but an exported
map is a writable global and a FileTypeDef's Extensions/Filenames slices were
writable through a read. A single `filter.BuiltinTypes[x] = ...` added later,
plausibly for a --type-add flag, becomes a concurrent map write the moment
type matching moves off the main goroutine -- a hard runtime crash the race
detector does not warn about first. Unexported it to builtinTypes and added
LookupType(name) (FileTypeDef, bool), which returns cloned slices so callers
cannot reach the table. NewTypeMatcher and ListTypes now read through the
accessors, making LookupType the sole read path.

packMagic and idxV2Magic were mutable []byte globals used as constants. Any
function in the package could overwrite their elements, and one stray
copy(idxV2Magic, ...) would silently corrupt validation for every goroutine.
They are now const strings. pack.go's comparison drops its redundant
string(packMagic) conversion, and pack_index.go trades bytes.Equal for an
allocation-free string comparison.

The `unused` exclusion for "field zlibPool is unused" in .golangci.yml was
stale: pooling landed and pack.go:328 and :347 both use p.zlibPool. It only
suppressed hypothetical future findings on that path, and nolintlint does not
police issues.exclusions, so it would never self-clean. Deleted.

Cross-file note: internal/gitengine/pack_test.go:106 needed one line changed
from Write to WriteString, since bytes.Buffer.Write cannot take the now-const
idxV2Magic. That file otherwise belongs to another issue in this stack; Main
assigned me the single line because the fix does not compile on the other
branch, where idxV2Magic is still []byte.

Verified with `go build ./...`, `go vet`, and `go test -race -count=1` on
./internal/filter/... and ./internal/gitengine/.... The new
TestLookupTypeReturnsCopy was mutation-checked: reverting LookupType to return
the table entry directly makes it fail.

Closes #18
@hammadmajid
hammadmajid force-pushed the fix/18-immutable-globals branch from 9327d6e to c1836e6 Compare September 5, 2026 14:16
@hammadmajid
hammadmajid marked this pull request as ready for review September 5, 2026 14:20
@hammadmajid
hammadmajid merged commit 22a2c9f into main Sep 5, 2026
3 of 6 checks passed
@hammadmajid
hammadmajid deleted the fix/18-immutable-globals branch September 5, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make package-level lookup state immutable; drop stale golangci exclusion

1 participant