gantry: address remaining code quality and edge case findings from PR #211
- Dominant language
- Go
- Stars
- 28
- Forks
- 11
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 55
Description
Tracking issue for lower-priority follow-ups from the PR #211 review. Each item is valid but either requires an architectural discussion or is a safe-direction edge case.
**Concurrency:**
- [x] `source_containerd.go:202` - SetMediaTypeRecorder data race - **FIXED in atomic.Pointer cleanup**
- [x] `health.go:275` - stale latency samples not evicted in Score() - **FIXED (evict on entry to latencyScoreLocked)**
- [ ] `advertise.go:293` - Reconcile vs Notify race resurrects withdrawn advertisements; self-corrects on next reconcile (default >= 5 min). Real bug, but for the non-prod merge-in milestone the self-correcting behaviour is acceptable. Hold mu for the apply pass in a follow-up.
- [ ] `health.go:178` - Score() calls callbacks while holding mu (fast/non-blocking in practice)
- [ ] `negcache.go:118` - callbacks invoked under mu (metrics-only, non-blocking)
- [ ] **NEW** `coord.go:212` originally flagged - **FIXED (DefaultMaxConcurrentStreams=512 semaphore added)**
**Security (third-round review):**
- [ ] **NEW** `origin.go:447` - peer-reachable URL injection + credential exposure. Multi-component fix: validate `Repository` against OCI Distribution name grammar in `coord.servePleasePull` and `oci.ParseV2Path`; bind mirror to loopback by default (pair with `hostNetwork: true`) or ship a default NetworkPolicy; don't fall back to Basic auth on 401 unless the registry advertised a Basic challenge. Real attack surface but deferred for the non-prod merge milestone - mitigated today by deploying the agent only inside trusted clusters and not exposing the mirror beyond the node.
- [x] **NEW** `origin.go:599` - token-endpoint JSON OOM via missing size cap - **FIXED (64 KiB io.LimitReader on token body)**
**Architecture:**
- [ ] **NEW** `main.go:2032` - origin pull goroutines detached via context.Background() outlive SIGTERM; coord-server stop ordering allows fresh please_pull during teardown. Lifecycle refactor: thread shutdown-aware ctx into pullerPump; unregister libp2p stream handler at the top of gracefulShutdown; gate pullerPump on a shutdown atomic. Significant scope - kubelet SIGKILL on rolling update is the existing behaviour; deferred to a focused lifecycle PR.
- [ ] **NEW** `source_containerd.go:283` - subscribe-stream errors close `out` indistinguishable from clean ctx cancel; `runOnce` returns nil and outer `Run` resets backoff. Underlying signal-loss is real, but the current code DOES sleep `backoffInitial` on the clean-close path (added in the second-round fix), so we don't hot-loop in practice. Full propagation via `LastErr()` is a follow-up.
- [ ] `mirror.go:993` - MultiWriter couples cache-write failure to client delivery
- [ ] `discovery.go:376` - provider transfer addr inferred vs announced
- [ ] `coord.go:625` - protobuf zero timestamps decode to Unix epoch not time.Time{}
**Validation:**
- [ ] `oci/path.go:27` - no client-side OCI repo name validation (rolls up into the security item above)
- [ ] `config.go:819` - Redacted() shallow-copies slices
- [ ] `config.go:826` - empty env vars treated as unset
**Code quality:**
- [ ] `transfer.go:345` - range parsing rejects valid clampable RFC 7233 ranges
- [ ] `inflight.go:118` - integer division loses sub-second precision
- [ ] `coldstart.go:839` - expansion-pass hits missing expansion prefix in outcome label
- [ ] `prefetch.go:170` - dead branch (hrw.TopK k=1 always returns 1)
- [ ] `members.go:96` - unused selector field on Manager
- [ ] `coord.go:472` - single bad digest rejects entire please_pull batch
- [ ] `coord.go:869` - nopAddr dead code
- [ ] `origin.go:336` - extra origin roundtrip on 404 (partially addressed)
**Fixed in third round (closed here for tracking):**
- [x] `origin.go:347/419` - manifest-fallback transport error misclassified as NotFound - **FIXED (added `else if mErr != nil` arm on both pull and head paths)**
- [x] `origin.go:590` - token endpoint failure classification lost - **FIXED (tokenError carries FailureClass; pull/head use `classOf(err)` via errors.As)**
- [x] `main.go:2170` - lease release direction reverted - **FIXED (back to release only on `*ifaces.ErrNotFound`)**
- [x] `coldstart.go:536` - Health() TOCTOU - **FIXED (health captured once in Resolve and threaded through probe)**
- [x] `deploy/gantry/build.sh` + `Dockerfile` - duplicate of `images/gantry`; broken paths post-merge - **FIXED (deleted both; added `make image-gantry-local`/`image-gantry-push`)**
- [x] `proto/gantry/coord/v1/` - moved to `internal/gantry/proto/coord/v1/` so the wire schema is not an external API surface
Contributor guide
Research direction
Start with the PR #211 review and the unchecked findings in advertise.go, origin.go, main.go, health.go, negcache.go, and the other listed files. First choose a single follow-up whose scope and architectural decision are agreed, then inspect its named entry point and existing behavior. Done means the selected finding is addressed without taking on the separately deferred items.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, distributed-systems, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100