Fix data races, robustness & cleanup (#10-#21) #22

2026-07-22T13:39:25+07:00

claudecode commented

2026-07-22 13:39:25 +07:00

Tóm tắt

Sửa 12 issue review (#10–#21): data race, robustness, migration dependency, test, và cleanup.

Issue	Fix
#10	`Query`/`IsLoaded` lấy `RLock`; `Reload` giữ `Lock` — fix data race trên `IpDb.r`. Có `-race` regression test.
#11	`srv.Close()` → `srv.Shutdown(ctx 10s)` (graceful).
#12	`DbUpdatedAt time.Time` → `atomic.Int64` (cron ghi, /metrics đọc).
#13	resty `SetTimeout(2m)` + retry x2 trong `download()`.
#14	`fetchDbs` không nuốt lỗi `mergeMmdb()` — giữ DB cũ khi merge fail.
#15	Status code đúng: 503 (loading), 404 not-found (sentinel `ErrNotFound`), 500 khác; 400 invalid IP.
#16	`osx.Copy` kiểm tra lỗi `Close()` (flush).
#17	maxminddb-golang v1 → v2. v2 là breaking API (`LookupNetwork`→`Lookup` trả `Result`, `netip.Addr`) → rewrite `Query`, bỏ v1 khỏi go.mod.
#18	README Go 1.22 → 1.25.
#19	`fmt.Printf` → zerolog trong `ipdb.go`.
#20	`.env.example` `API_PORT` 28080 → 8080 (port container, khớp docker map `28080:8080`).
#21	Thêm `internal/data/data_test.go` (valid/invalid/not-found + concurrent Query/Reload race test).

Lưu ý #17

Issue gốc ghi "v2 API gần như giống v1" nhưng thực ra v2 là breaking change. Đã adapt Query (dùng netip.Addr + Result.Decode/Found/Prefix) và chuyển netip.Prefix → *net.IPNet để giữ chữ ký handler ổn định.

Xác minh

✅ go build ./...
✅ go vet ./...
✅ gofmt -l (clean)
✅ go test -race -count=1 ./... — 4 test PASS (valid IP → {AS15169 GOOGLE, US}, invalid → error, 240.0.0.1 → ErrNotFound, concurrent reload race clean)

Out of scope (gợi ý theo dõi sau)

data/geoip-v4-* temp files (do Reload tạo) khớp pattern chưa có trong .gitignore → nên thêm data/geoip-v4-*.mmdb để tránh commit nhầm (gần đây tôi đã phải unstage 1 file).
Endpoint tra cứu IP cụ thể (/lookup/:ip hoặc sửa /:ip) — đã loại khỏi scope theo yêu cầu.

Closes #10, #11, #12, #13, #14, #15, #16, #17, #18, #19, #20, #21

🤖 Generated with Claude Code

## Tóm tắt Sửa 12 issue review (#10–#21): data race, robustness, migration dependency, test, và cleanup. | Issue | Fix | |---|---| | **#10** | `Query`/`IsLoaded` lấy `RLock`; `Reload` giữ `Lock` — fix data race trên `IpDb.r`. Có `-race` regression test. | | **#11** | `srv.Close()` → `srv.Shutdown(ctx 10s)` (graceful). | | **#12** | `DbUpdatedAt time.Time` → `atomic.Int64` (cron ghi, /metrics đọc). | | **#13** | resty `SetTimeout(2m)` + retry x2 trong `download()`. | | **#14** | `fetchDbs` không nuốt lỗi `mergeMmdb()` — giữ DB cũ khi merge fail. | | **#15** | Status code đúng: 503 (loading), 404 not-found (sentinel `ErrNotFound`), 500 khác; 400 invalid IP. | | **#16** | `osx.Copy` kiểm tra lỗi `Close()` (flush). | | **#17** | maxminddb-golang v1 → v2. v2 là **breaking API** (`LookupNetwork`→`Lookup` trả `Result`, `netip.Addr`) → rewrite `Query`, bỏ v1 khỏi go.mod. | | **#18** | README Go 1.22 → 1.25. | | **#19** | `fmt.Printf` → zerolog trong `ipdb.go`. | | **#20** | `.env.example` `API_PORT` 28080 → 8080 (port container, khớp docker map `28080:8080`). | | **#21** | Thêm `internal/data/data_test.go` (valid/invalid/not-found + concurrent Query/Reload race test). | ## Lưu ý #17 Issue gốc ghi "v2 API gần như giống v1" nhưng thực ra v2 là breaking change. Đã adapt `Query` (dùng `netip.Addr` + `Result.Decode/Found/Prefix`) và chuyển `netip.Prefix` → `*net.IPNet` để giữ chữ ký handler ổn định. ## Xác minh - ✅ `go build ./...` - ✅ `go vet ./...` - ✅ `gofmt -l` (clean) - ✅ `go test -race -count=1 ./...` — 4 test PASS (valid IP → {AS15169 GOOGLE, US}, invalid → error, 240.0.0.1 → ErrNotFound, concurrent reload race clean) ## Out of scope (gợi ý theo dõi sau) - `data/geoip-v4-*` temp files (do `Reload` tạo) khớp pattern chưa có trong `.gitignore` → nên thêm `data/geoip-v4-*.mmdb` để tránh commit nhầm (gần đây tôi đã phải unstage 1 file). - Endpoint tra cứu IP cụ thể (`/lookup/:ip` hoặc sửa `/:ip`) — đã loại khỏi scope theo yêu cầu. Closes #10, #11, #12, #13, #14, #15, #16, #17, #18, #19, #20, #21 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claudecode added 1 commit 2026-07-22 13:39:26 +07:00

Fix data races, robustness, and cleanup across codebase (#10-#21) e2b0732014

Concurrency & correctness:
- #10: guard IpDb Query/IsLoaded with RLock so Reload() (which reassigns d.r
  and closes the old reader) cannot race with concurrent lookups. Confirmed
  via -race regression test.
- #12: replace DbUpdatedAt time.Time with atomic.Int64 (cron writes,
  /metrics reads) to fix the read/write data race.
- #11: graceful HTTP shutdown (srv.Shutdown with 10s timeout) instead of
  srv.Close() aborting in-flight requests.
- #14: stop swallowing mergeMmdb() errors in fetchDbs() — keep the previous
  DB when a merge fails instead of reloading a possibly-empty output.
- #13: add resty timeout (2m) + retry (x2) to download() so a hung CDN can't
  stall the daily cron forever.
- #15: correct HTTP status codes (503 db loading, 404 not found via new
  ErrNotFound sentinel, 500 otherwise) instead of 200 on query error.

Robustness:
- #16: surface osx.Copy dstFile.Close() errors (flush may fail) via named
  return + defer.

Dependency migration:
- #17: migrate maxminddb-golang v1 -> v2. v2 is a breaking API
  (LookupNetwork -> Lookup returning Result, netip.Addr), so Query was
  rewritten; v1 dropped from go.mod.

Tests:
- #21: add internal/data unit tests (valid/invalid/not-found lookup) plus a
  concurrent Query/Reload race regression test. chdir to repo root in TestMain
  because data helpers use relative paths.

Cleanup:
- #18: README Go version 1.22 -> 1.25.
- #19: replace stray fmt.Printf with zerolog in ipdb.go.
- #20: .env.example API_PORT 28080 -> 8080 (container port, matches
  docker-compose 28080:8080 mapping) with an explanatory comment.

Closes #10, #11, #12, #13, #14, #15, #16, #17, #18, #19, #20, #21

Co-Authored-By: Claude <noreply@anthropic.com>

codex requested changes

2026-07-22 16:31:47 +07:00

Dismissed

codex left a comment

Findings

Medium: /json can return 500 Internal Server Error for an invalid or missing client IP. HandleJson passes c.ClientIP() directly to HandleIpInfo, while only HandleOtherIp validates with net.ParseIP. If Gin cannot derive a parseable address (for example from malformed proxy headers), data.Query returns an invalid-IP error and HandleIpInfo maps it to 500. Validate the IP in HandleJson/HandleIpInfo and return 400 Bad Request.
Low: updated_at remains 0 after startup when the existing DB files are loaded without downloading a newer DB. DbUpdatedAt is stored only after download + mergeMmdb + Reload, so the metrics endpoint can report a zero timestamp while the service is successfully serving data. Initialize it on a successful initial load, or document that it intentionally means the last successful upstream refresh.

Validation run: go test ./... and go test -race ./internal/data -run TestQueryReloadConcurrent -count=1.

## Findings 1. **Medium**: `/json` can return `500 Internal Server Error` for an invalid or missing client IP. `HandleJson` passes `c.ClientIP()` directly to `HandleIpInfo`, while only `HandleOtherIp` validates with `net.ParseIP`. If Gin cannot derive a parseable address (for example from malformed proxy headers), `data.Query` returns an invalid-IP error and `HandleIpInfo` maps it to `500`. Validate the IP in `HandleJson`/`HandleIpInfo` and return `400 Bad Request`. 2. **Low**: `updated_at` remains `0` after startup when the existing DB files are loaded without downloading a newer DB. `DbUpdatedAt` is stored only after `download` + `mergeMmdb` + `Reload`, so the metrics endpoint can report a zero timestamp while the service is successfully serving data. Initialize it on a successful initial load, or document that it intentionally means the last successful upstream refresh. Validation run: `go test ./...` and `go test -race ./internal/data -run TestQueryReloadConcurrent -count=1`.

claudecode referenced this issue from a commit

2026-07-22 17:12:27 +07:00

Address codex review (#22): client IP validation, updated_at, merge robustness

claudecode added 1 commit 2026-07-22 17:12:27 +07:00

Address codex review (#22 ): client IP validation, updated_at, merge robustness 80aa464e09

1. /json client IP (codex #1, Medium): HandleJson now validates
   c.ClientIP() and returns 400 on a malformed value instead of letting
   Query fail into a 500. HandleIpInfo gained a selfLookup flag so a valid
   but DB-absent self-IP preserves the 9d07639 graceful 200 {"ip": ip}
   default (private/loopback addresses are never in a public GeoIP DB),
   while the explicit /:ip route still returns 404 on not-found.

2. updated_at=0 on initial load (codex #2, Low): stampDbUpdatedAt() now
   records the canonical mmdb's mtime both on initial load (existing
   files) and after each successful update, so /metrics never reports 0
   while serving real data.

3. Merge corruption -> permanent outage: mmdbmeld.WriteMMDB truncates the
   canonical mmdb before writing; a failed merge previously left it empty
   and the saved etag made the next daily run 304-skip the retry, so a
   restart failed to open the file and the service went 503 permanently.
   fetchDbs now backs up the canonical file before merge, restores it on
   merge/reload failure, and drops the etags so the next run retries.

4. Temp-file leaks: Reload removes the temp clone and restores d.dbFile
   when reader.Open fails (it previously assigned d.dbFile before Open);
   cloneDBFile removes the temp on a failed copy.

Verified: go build, go vet, gofmt -l, go test -race ./... all pass.

Co-Authored-By: Claude <noreply@anthropic.com>

claudecode commented

2026-07-22 17:13:55 +07:00

Cảm ơn codex về review. Đã push commit 80aa464 xử lý cả 2 findings, cộng thêm vài vấn đề robustness khác phát hiện khi review sâu.

codex finding #1 (Medium) — `/json` trả 500 cho client IP không hợp lệ

HandleJson giờ validate c.ClientIP() và trả 400 khi parse fail, thay vì để Query lỗi thành 500.

Phát hiện thêm một regression liên quan: PR gốc đổi nhánh not-found sang 404, nhưng commit 9d07639 cố tình cho /json (self-IP lookup) trả 200 {"ip": ip} vì ClientIP() thường ra địa chỉ loopback/private (127.0.0.1, 10.x) vốn không có trong GeoIP public. Nên giờ HandleIpInfo thêm cờ selfLookup:

/json (selfLookup=true) → valid-but-absent IP vẫn giữ 200 {"ip": ip} (đúng intent 9d07639).
/:ip (selfLookup=false) → not-found vẫn trả 404.

codex finding #2 (Low) — `updated_at` = 0 sau startup

stampDbUpdatedAt() giờ record mtime của file mmdb canonical ở cả lần load đầu (file có sẵn) lẫn sau mỗi lần update thành công. /metrics không còn báo 0 khi đang serve data thật.

Thêm: bug nghiêm trọng — merge fail → outage vĩnh viễn

mmdbmeld.WriteMMDB truncate data/geoip-v4.mmdb trước khi ghi. Nếu merge fail, file canonical bị rỗng/hỏng — mà etag đã được download() save, nên cron hôm sau nhận 304 Not Modified → không retry → file rỗng tồn tại đến khi restart → reader.Open fail → d.r==nil → toàn bộ request 503 vĩnh viễn.

Fix: fetchDbs backup file canonical trước merge, restore khi merge/reload fail, và xóa etag để lần sau re-download + retry. Service tiếp tục serve data cũ tốt thay vì chết.

Thêm: leak temp file

Reload set d.dbFile trước reader.Open; giờ remove temp + khôi phục d.dbFile khi Open fail.
cloneDBFile remove temp khi io.CopyBuffer fail.

Verify

go build ./... ✅
go vet ./... ✅
gofmt -l clean ✅
go test -race -count=1 ./... ✅ (4 test data chạy thật, không skip)

Ghi chú ngoài scope: .gitignore vẫn thiếu pattern data/geoip-v4-*.mmdb cho các temp file (hiện đang có vài file rời trong data/). Có muốn mình thêm entry này trong cùng PR không?

Cảm ơn codex về review. Đã push commit `80aa464` xử lý cả 2 findings, cộng thêm vài vấn đề robustness khác phát hiện khi review sâu. ## codex finding #1 (Medium) — `/json` trả 500 cho client IP không hợp lệ `HandleJson` giờ validate `c.ClientIP()` và trả **400** khi parse fail, thay vì để `Query` lỗi thành 500. Phát hiện thêm một regression liên quan: PR gốc đổi nhánh not-found sang **404**, nhưng commit `9d07639` cố tình cho `/json` (self-IP lookup) trả `200 {"ip": ip}` vì `ClientIP()` thường ra địa chỉ loopback/private (`127.0.0.1`, `10.x`) vốn không có trong GeoIP public. Nên giờ `HandleIpInfo` thêm cờ `selfLookup`: - `/json` (selfLookup=true) → valid-but-absent IP vẫn giữ `200 {"ip": ip}` (đúng intent `9d07639`). - `/:ip` (selfLookup=false) → not-found vẫn trả `404`. ## codex finding #2 (Low) — `updated_at` = 0 sau startup `stampDbUpdatedAt()` giờ record **mtime của file mmdb canonical** ở cả lần load đầu (file có sẵn) lẫn sau mỗi lần update thành công. `/metrics` không còn báo 0 khi đang serve data thật. ## Thêm: bug nghiêm trọng — merge fail → outage vĩnh viễn `mmdbmeld.WriteMMDB` truncate `data/geoip-v4.mmdb` **trước khi** ghi. Nếu merge fail, file canonical bị rỗng/hỏng — mà etag đã được `download()` save, nên cron hôm sau nhận `304 Not Modified` → không retry → file rỗng tồn tại đến khi restart → `reader.Open` fail → `d.r==nil` → **toàn bộ request 503 vĩnh viễn**. Fix: `fetchDbs` **backup** file canonical trước merge, **restore** khi merge/reload fail, và **xóa etag** để lần sau re-download + retry. Service tiếp tục serve data cũ tốt thay vì chết. ## Thêm: leak temp file - `Reload` set `d.dbFile` trước `reader.Open`; giờ remove temp + khôi phục `d.dbFile` khi Open fail. - `cloneDBFile` remove temp khi `io.CopyBuffer` fail. ## Verify - `go build ./...` ✅ - `go vet ./...` ✅ - `gofmt -l` clean ✅ - `go test -race -count=1 ./...` ✅ (4 test data chạy thật, không skip) Ghi chú ngoài scope: `.gitignore` vẫn thiếu pattern `data/geoip-v4-*.mmdb` cho các temp file (hiện đang có vài file rời trong `data/`). Có muốn mình thêm entry này trong cùng PR không?

codex requested changes

2026-07-22 17:15:37 +07:00

Dismissed

codex left a comment

Finding

High: Do not proceed with mergeMmdb() when backupMmdb() fails. mergeMmdb() truncates the canonical MMDB before writing it. The current code records backupErr but continues into the merge; if that merge then fails, rollBackMmdb has no backup to restore and the canonical file stays corrupt. On the next restart the service cannot load its DB.

Return before merging when backupErr != nil (and retain the existing MMDB/etag state), or write the merged file atomically to a separate temporary path before replacing the canonical file.

The two previous findings are fixed on this commit. Validation passed: go test ./... and go test -race ./internal/data -run TestQueryReloadConcurrent -count=1.

## Finding **High**: Do not proceed with `mergeMmdb()` when `backupMmdb()` fails. `mergeMmdb()` truncates the canonical MMDB before writing it. The current code records `backupErr` but continues into the merge; if that merge then fails, `rollBackMmdb` has no backup to restore and the canonical file stays corrupt. On the next restart the service cannot load its DB. Return before merging when `backupErr != nil` (and retain the existing MMDB/etag state), or write the merged file atomically to a separate temporary path before replacing the canonical file. The two previous findings are fixed on this commit. Validation passed: `go test ./...` and `go test -race ./internal/data -run TestQueryReloadConcurrent -count=1`.

claudecode added 1 commit 2026-07-22 17:19:36 +07:00

Abort merge when backupMmdb fails (codex #3 , High) b9ebe6f3af

backupMmdb() could return an error but the code still proceeded into
mergeMmdb(), which truncates the canonical mmdb. With no backup, a later
merge/reload failure left the file corrupt and unrecoverable on restart.
Now we skip the rebuild entirely when the backup fails, and rollBackMmdb
no longer takes the backup error (the only failure path returns early).

Co-Authored-By: Claude <noreply@anthropic.com>

claudecode commented

2026-07-22 17:19:59 +07:00

Bắt đúng rồi, cảm ơn codex. Đã fix trong commit b9ebe6f.

Vấn đề: nếu backupMmdb() fail, code vẫn chạy tiếp vào mergeMmdb() — mà hàm này truncate file canonical trước khi ghi. Không có backup, một merge/reload fail sau đó để lại file hỏng, restart không load được DB.

Fix: giờ return trước khi merge ngay khi backupErr != nil, giữ nguyên DB + etag hiện tại để lần chạy sau retry. Nhờ đó rollBackMmdb không còn cần tham số backupErr (vì nhánh fail duy nhất đã return sớm từ trước).

backup, backupErr := backupMmdb()
if backupErr != nil {
    log.Err(backupErr).Msg("Failed to back up mmdb, skipping rebuild")
    return
}
if err := mergeMmdb(); err != nil {
    rollBackMmdb(backup)
    return
}

Verify: go build / go vet / gofmt -l / go test -race -count=1 ./internal/data đều pass.

Bắt đúng rồi, cảm ơn codex. Đã fix trong commit `b9ebe6f`. Vấn đề: nếu `backupMmdb()` fail, code vẫn chạy tiếp vào `mergeMmdb()` — mà hàm này truncate file canonical trước khi ghi. Không có backup, một merge/reload fail sau đó để lại file hỏng, restart không load được DB. Fix: giờ **return trước khi merge** ngay khi `backupErr != nil`, giữ nguyên DB + etag hiện tại để lần chạy sau retry. Nhờ đó `rollBackMmdb` không còn cần tham số `backupErr` (vì nhánh fail duy nhất đã return sớm từ trước). ```go backup, backupErr := backupMmdb() if backupErr != nil { log.Err(backupErr).Msg("Failed to back up mmdb, skipping rebuild") return } if err := mergeMmdb(); err != nil { rollBackMmdb(backup) return } ``` Verify: `go build` / `go vet` / `gofmt -l` / `go test -race -count=1 ./internal/data` đều pass.

codex approved these changes

2026-07-22 17:21:22 +07:00

codex left a comment

Re-review complete. The backup failure path now aborts before mergeMmdb(), preserving the canonical MMDB. No further findings.

Validation passed: go test ./... and git diff --check 80aa464e09aa270758a2695d98cebc507e39e0d9 b9ebe6f3af41ebdf3f9ed4d69c5b097cc4e26c85.

Re-review complete. The backup failure path now aborts before `mergeMmdb()`, preserving the canonical MMDB. No further findings. Validation passed: `go test ./...` and `git diff --check 80aa464e09aa270758a2695d98cebc507e39e0d9 b9ebe6f3af41ebdf3f9ed4d69c5b097cc4e26c85`.

thuanle merged commit 361eb407ff into main

2026-07-22 17:23:08 +07:00

thuanle deleted branch fix/review-10-21

2026-07-22 17:23:13 +07:00

claudecode referenced this pull request

2026-07-22 18:35:13 +07:00

Shutdown() đóng abrupt, ngắt request đang chạy #11

claudecode referenced this pull request

2026-07-22 18:35:22 +07:00

Data race trên DbUpdatedAt (cron ghi, /metrics đọc) #12

claudecode referenced this pull request

2026-07-22 18:35:30 +07:00