From e2b073201474185a36ee78b33fa50e85a9bb91eb Mon Sep 17 00:00:00 2001 From: claudecode Date: Wed, 22 Jul 2026 06:37:40 +0000 Subject: [PATCH 1/3] Fix data races, robustness, and cleanup across codebase (#10-#21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .env.example | 6 +- README.md | 2 +- go.mod | 3 +- go.sum | 2 - internal/data/data_test.go | 133 +++++++++++++++++++++ internal/data/ipdb.go | 6 +- internal/data/query.go | 50 ++++++-- internal/services/api/handler_json.go | 23 +++- internal/services/api/handler_metrics.go | 2 +- internal/services/api/main.go | 10 +- internal/services/db_updater/downloader.go | 6 +- internal/services/db_updater/main.go | 21 +++- pkg/osx/file.go | 13 +- 13 files changed, 248 insertions(+), 29 deletions(-) create mode 100644 internal/data/data_test.go diff --git a/.env.example b/.env.example index 0703487..1ad1db2 100644 --- a/.env.example +++ b/.env.example @@ -1,2 +1,6 @@ -API_PORT=28080 +# API_PORT is the port the service listens on INSIDE the container. +# With docker-compose's "28080:8080" mapping, set this to 8080 so the host +# can reach it at http://localhost:28080. Override only if you also change +# the host-side mapping / non-Docker run. +API_PORT=8080 GIN_TRUSTED_PROXY_IP=127.0.0.1 \ No newline at end of file diff --git a/README.md b/README.md index 39cc4eb..c8a39b2 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ The service will be available at `http://localhost:28080` ### Manual Installation -1. Install Go 1.22 or later +1. Install Go 1.25 or later 2. Clone and build: ```bash git clone diff --git a/go.mod b/go.mod index d76664f..33381bf 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/gin-gonic/gin v1.12.0 github.com/go-resty/resty/v2 v2.17.2 github.com/joho/godotenv v1.5.1 - github.com/oschwald/maxminddb-golang v1.13.1 + github.com/oschwald/maxminddb-golang/v2 v2.4.1 github.com/robfig/cron/v3 v3.0.1 github.com/rs/zerolog v1.35.1 github.com/safing/mmdbmeld v0.3.0 @@ -32,7 +32,6 @@ require ( github.com/maxmind/mmdbwriter v1.2.0 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect - github.com/oschwald/maxminddb-golang/v2 v2.4.1 // indirect github.com/pelletier/go-toml/v2 v2.4.3 // indirect github.com/quic-go/qpack v0.6.0 // indirect github.com/quic-go/quic-go v0.60.0 // indirect diff --git a/go.sum b/go.sum index 2be35ab..8ac8db9 100644 --- a/go.sum +++ b/go.sum @@ -55,8 +55,6 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9Gz0M= github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= -github.com/oschwald/maxminddb-golang v1.13.1 h1:G3wwjdN9JmIK2o/ermkHM+98oX5fS+k5MbwsmL4MRQE= -github.com/oschwald/maxminddb-golang v1.13.1/go.mod h1:K4pgV9N/GcK694KSTmVSDTODk4IsCNThNdTmnaBZ/F8= github.com/oschwald/maxminddb-golang/v2 v2.4.1 h1:OffzqSABE3Sw354GdBThqDsKfpA4GWBqOY2P91V8tjI= github.com/oschwald/maxminddb-golang/v2 v2.4.1/go.mod h1:CZK8iQQMKfy6mKOifoyUmrj4vTHnMiGVaS7hDaZZxQ0= github.com/pelletier/go-toml/v2 v2.4.3 h1:GTRvJQutkOSftxIFD5xw9aepkYNuPWmVJpffdDPYVpY= diff --git a/internal/data/data_test.go b/internal/data/data_test.go new file mode 100644 index 0000000..ad74374 --- /dev/null +++ b/internal/data/data_test.go @@ -0,0 +1,133 @@ +package data + +import ( + "errors" + "net" + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" +) + +// nested looks up a nested map value by successive string keys, returning "" if absent. +func nested(m map[string]any, keys ...string) string { + var cur any = m + for _, k := range keys { + mm, ok := cur.(map[string]any) + if !ok { + return "" + } + cur = mm[k] + } + s, _ := cur.(string) + return s +} + +// parseIP is a test helper that panics on invalid input (used only with literals). +func parseIP(s string) net.IP { + ip := net.ParseIP(s) + if ip == nil { + panic("invalid ip in test: " + s) + } + return ip +} + +// The package's data helpers use relative paths ("data/..."), so tests must +// run with the working directory at the repository root. TestMain moves there +// once for the whole package. +func TestMain(m *testing.M) { + if root, err := filepath.Abs("../.."); err == nil { + _ = os.Chdir(root) + } + os.Exit(m.Run()) +} + +// freshDB returns a loaded IpDb, skipping the test when no mmdb is present. +func freshDB(t *testing.T) *IpDb { + t.Helper() + db := Ins() + if !db.IsLoaded() { + t.Skip("no mmdb present, skipping test that needs real data") + } + return db +} + +func TestQueryValidIP(t *testing.T) { + db := freshDB(t) + + data, netRec, err := db.Query("8.8.8.8") + if err != nil { + t.Fatalf("Query(8.8.8.8) unexpected error: %v", err) + } + if data == nil { + t.Fatal("Query(8.8.8.8) returned nil data") + } + // 8.8.8.8 is Google (AS15169), US. + if got := nested(data, "country", "iso_code"); got != "US" { + t.Errorf("country.iso_code = %v, want US", got) + } + if netRec == nil { + t.Error("expected a non-nil network record") + } else if !netRec.Contains(parseIP("8.8.8.8")) { + t.Errorf("network %s should contain 8.8.8.8", netRec) + } +} + +func TestQueryInvalidIP(t *testing.T) { + db := freshDB(t) + + if _, _, err := db.Query("not-an-ip"); err == nil { + t.Fatal("expected error for invalid IP, got nil") + } +} + +func TestQueryNotFound(t *testing.T) { + db := freshDB(t) + + // 240.0.0.0/4 is reserved/unallocated and should not be in a public GeoIP DB. + _, _, err := db.Query("240.0.0.1") + if err == nil { + t.Fatal("expected not-found error, got nil") + } + if !errors.Is(err, ErrNotFound) { + t.Errorf("expected ErrNotFound sentinel, got %v", err) + } +} + +// TestQueryReloadConcurrent is a regression test for the data race between +// Query (reads d.r) and Reload (reassigns d.r / closes the old reader). It +// must pass under `go test -race`. +func TestQueryReloadConcurrent(t *testing.T) { + db := freshDB(t) + + var reloadOK, queries int64 + var wg sync.WaitGroup + + for w := 0; w < 8; w++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 5000; i++ { + _, _, _ = db.Query("8.8.8.8") + atomic.AddInt64(&queries, 1) + } + }() + } + for r := 0; r < 4; r++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 30; i++ { + if db.Reload() == nil { + atomic.AddInt64(&reloadOK, 1) + } + } + }() + } + wg.Wait() + + if reloadOK == 0 { + t.Fatalf("expected reloads to succeed, got reloads=%d queries=%d", reloadOK, queries) + } +} diff --git a/internal/data/ipdb.go b/internal/data/ipdb.go index 0d117be..bf2217b 100644 --- a/internal/data/ipdb.go +++ b/internal/data/ipdb.go @@ -9,7 +9,7 @@ import ( "sync" "thuanle.me/ip-info/configs" ) -import reader "github.com/oschwald/maxminddb-golang" +import reader "github.com/oschwald/maxminddb-golang/v2" type IpDb struct { r *reader.Reader @@ -61,7 +61,7 @@ func CleanupDataDir() error { if err != nil { return fmt.Errorf("failed to delete file %s: %w", filePath, err) } - fmt.Printf("Deleted file: %s\n", filePath) + log.Info().Str("file", filePath).Msg("Deleted temp file") } } @@ -104,6 +104,8 @@ func (d *IpDb) Reload() error { } func (d *IpDb) IsLoaded() bool { + d.mu.RLock() + defer d.mu.RUnlock() return d.r != nil } diff --git a/internal/data/query.go b/internal/data/query.go index e10a868..cbe44cb 100644 --- a/internal/data/query.go +++ b/internal/data/query.go @@ -1,23 +1,57 @@ package data import ( + "errors" "fmt" "net" + "net/netip" ) +// ErrNotFound is returned by Query when the IP parses but is not present in the DB. +var ErrNotFound = errors.New("IP not found") + func (d *IpDb) Query(ipArg string) (map[string]any, *net.IPNet, error) { - ip := net.ParseIP(ipArg) - if ip == nil { + // maxminddb-golang v2 requires a netip.Addr. + addr, err := netip.ParseAddr(ipArg) + if err != nil { return nil, nil, fmt.Errorf("invalid IP: %s", ipArg) } - // Get data of IP. - anyData := make(map[string]any) - recordNet, ok, err := d.r.LookupNetwork(ip, &anyData) - if err != nil { + + // Hold the read lock: Reload() reassigns d.r and closes the old reader, + // so reading d.r / calling Lookup without synchronization is a data race. + d.mu.RLock() + defer d.mu.RUnlock() + + if d.r == nil { + return nil, nil, fmt.Errorf("db not loaded") + } + + // Look up the record for the IP. + result := d.r.Lookup(addr) + if err := result.Err(); err != nil { return nil, nil, err } - if !ok { - return nil, nil, fmt.Errorf("IP not found: %s", ipArg) + if !result.Found() { + return nil, nil, fmt.Errorf("%w: %s", ErrNotFound, ipArg) + } + + // Decode the record into a generic map. + anyData := make(map[string]any) + if err := result.Decode(&anyData); err != nil { + return nil, nil, err + } + + // Convert the matched prefix back to *net.IPNet for callers. + var recordNet *net.IPNet + if prefix := result.Prefix(); prefix.IsValid() { + recordNet = prefixToIPNet(prefix) } return anyData, recordNet, nil } + +// prefixToIPNet converts a netip.Prefix to a *net.IPNet. +func prefixToIPNet(prefix netip.Prefix) *net.IPNet { + addr := prefix.Addr().AsSlice() + mask := net.CIDRMask(prefix.Bits(), len(addr)*8) + return &net.IPNet{IP: addr, Mask: mask} +} diff --git a/internal/services/api/handler_json.go b/internal/services/api/handler_json.go index 39a4c00..061b7d1 100644 --- a/internal/services/api/handler_json.go +++ b/internal/services/api/handler_json.go @@ -1,6 +1,7 @@ package api import ( + "errors" "github.com/gin-gonic/gin" "github.com/rs/zerolog/log" "net" @@ -16,7 +17,7 @@ func HandleJson(c *gin.Context) { func HandleOtherIp(c *gin.Context) { ip := c.Param("ip") if net.ParseIP(ip) == nil { - c.JSON(http.StatusBadRequest, gin.H{"error": "invalid ip"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid ip", "ip": ip}) return } HandleIpInfo(c, ip) @@ -25,19 +26,31 @@ func HandleOtherIp(c *gin.Context) { func HandleIpInfo(c *gin.Context, ip string) { if !data.Ins().IsLoaded() { log.Error().Msg("DB is not loaded") - c.String(http.StatusInternalServerError, "Try again later") + c.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "service unavailable, db still loading", + "ip": ip, + }) return } ipData, _, err := data.Ins().Query(ip) if err != nil { + // Distinguish "not found" (valid IP, absent from DB) from other errors. + if isNotFound(err) { + c.JSON(http.StatusNotFound, gin.H{"error": "not found", "ip": ip}) + return + } log.Err(err).Msg("Failed to query IP") - c.JSON(http.StatusOK, gin.H{ - "ip": ip, - }) + c.JSON(http.StatusInternalServerError, gin.H{"error": "internal error", "ip": ip}) return } ipData["ip"] = ip c.JSON(http.StatusOK, ipData) } + +// isNotFound reports whether the query error means the IP was not present in +// the DB (as opposed to an invalid IP or an internal lookup failure). +func isNotFound(err error) bool { + return errors.Is(err, data.ErrNotFound) +} diff --git a/internal/services/api/handler_metrics.go b/internal/services/api/handler_metrics.go index 904d3d6..e96e333 100644 --- a/internal/services/api/handler_metrics.go +++ b/internal/services/api/handler_metrics.go @@ -8,6 +8,6 @@ import ( func HandleMetrics(c *gin.Context) { c.JSON(http.StatusOK, gin.H{ - "updated_at": db_updater.DbUpdatedAt.UnixMilli(), + "updated_at": db_updater.DbUpdatedAtMillis(), }) } diff --git a/internal/services/api/main.go b/internal/services/api/main.go index 4bc2ee5..7b546fb 100644 --- a/internal/services/api/main.go +++ b/internal/services/api/main.go @@ -1,6 +1,7 @@ package api import ( + "context" "errors" "github.com/gin-gonic/gin" "github.com/rs/zerolog/log" @@ -8,6 +9,7 @@ import ( "os" "thuanle.me/ip-info/configs" "thuanle.me/ip-info/configs/key" + "time" ) var srv *http.Server @@ -48,6 +50,12 @@ func StartApiService() { } func Shutdown() { - _ = srv.Close() + // Graceful shutdown: let in-flight requests finish (bounded by a timeout) + // instead of abruptly closing connections. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := srv.Shutdown(ctx); err != nil { + log.Err(err).Msg("Error shutting down API service") + } log.Info().Msg("API service stopped") } diff --git a/internal/services/db_updater/downloader.go b/internal/services/db_updater/downloader.go index e0a8867..16ea20f 100644 --- a/internal/services/db_updater/downloader.go +++ b/internal/services/db_updater/downloader.go @@ -7,6 +7,7 @@ import ( "os" "path" "thuanle.me/ip-info/configs" + "time" ) func download(url string) bool { @@ -16,7 +17,10 @@ func download(url string) bool { etag, _ := readFile(filenameEtag) - client := resty.New() + client := resty.New(). + SetTimeout(2 * time.Minute). + SetRetryCount(2). + SetRetryWaitTime(5 * time.Second) resp, err := client.R(). SetHeader("If-None-Match", etag). Get(url) diff --git a/internal/services/db_updater/main.go b/internal/services/db_updater/main.go index 0fce4fd..b27ec47 100644 --- a/internal/services/db_updater/main.go +++ b/internal/services/db_updater/main.go @@ -3,12 +3,22 @@ package db_updater import ( "github.com/robfig/cron/v3" "github.com/rs/zerolog/log" + "sync/atomic" "thuanle.me/ip-info/configs" "thuanle.me/ip-info/internal/data" "time" ) -var DbUpdatedAt time.Time +// DbUpdatedAt stores the DB last-updated time as unix milliseconds. +// Accessed concurrently by the cron goroutine (writer) and the /metrics +// handler (reader), so it must be synchronized — hence atomic.Int64. +var DbUpdatedAt atomic.Int64 + +// DbUpdatedAtMillis returns the last DB update time in unix milliseconds, +// or 0 if the DB has never been updated. +func DbUpdatedAtMillis() int64 { + return DbUpdatedAt.Load() +} func StartUpdateDbService() { c := cron.New() @@ -25,7 +35,12 @@ func fetchDbs() { if newFlag { log.Info().Msg("New DB downloaded. Recreating mmdb") - _ = mergeMmdb() + if err := mergeMmdb(); err != nil { + // Keep the current (previous) DB rather than reloading a + // potentially empty/corrupt merge output. + log.Err(err).Msg("Failed to merge mmdb, keeping current DB") + return + } err := data.Ins().Reload() if err != nil { @@ -33,6 +48,6 @@ func fetchDbs() { return } - DbUpdatedAt = time.Now() + DbUpdatedAt.Store(time.Now().UnixMilli()) } } diff --git a/pkg/osx/file.go b/pkg/osx/file.go index 4d34160..d142d1e 100644 --- a/pkg/osx/file.go +++ b/pkg/osx/file.go @@ -6,7 +6,7 @@ import ( ) // Copy copies a file from source to destination -func Copy(from, to string) error { +func Copy(from, to string) (err error) { // Open the source file for reading srcFile, err := os.Open(from) if err != nil { @@ -19,7 +19,16 @@ func Copy(from, to string) error { if err != nil { return err } - defer dstFile.Close() + // Closing may flush buffered writes, so its error must be surfaced — a + // successful copy with a failed close means the destination is incomplete. + // Defer the close and only let it overwrite the return value when the copy + // itself succeeded; otherwise keep the first error. + defer func() { + cerr := dstFile.Close() + if err == nil { + err = cerr + } + }() // Use a buffer to copy the file in chunks buf := make([]byte, 1024*1024) // 1 MB buffer -- 2.54.0 From 80aa464e09aa270758a2695d98cebc507e39e0d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 17:11:58 +0700 Subject: [PATCH 2/3] Address codex review (#22): client IP validation, updated_at, merge robustness 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 --- internal/data/ipdb.go | 8 ++- internal/services/api/handler_json.go | 23 ++++++- internal/services/db_updater/main.go | 92 +++++++++++++++++++++++++-- 3 files changed, 114 insertions(+), 9 deletions(-) diff --git a/internal/data/ipdb.go b/internal/data/ipdb.go index bf2217b..8ff1edb 100644 --- a/internal/data/ipdb.go +++ b/internal/data/ipdb.go @@ -83,7 +83,11 @@ func (d *IpDb) Reload() error { d.dbFile = tmpFile r, err := reader.Open(tmpFile) if err != nil { - log.Err(err).Msg("Failed to open mmdb") + log.Err(err).Str("file", tmpFile).Msg("Failed to open mmdb") + // Open failed: drop the orphaned temp clone and leave d.dbFile/d.r + // pointing at the previously loaded DB. + _ = os.Remove(tmpFile) + d.dbFile = wilDeleteFile return err } log.Info().Str("file", tmpFile).Msg("MMDB reloaded") @@ -127,6 +131,8 @@ func cloneDBFile() (string, error) { // Copy the data from srcFile to dstFile _, err = io.CopyBuffer(tmpFile, srcFile, buf) if err != nil { + // Leave nothing behind on a failed copy. + _ = os.Remove(tmpFile.Name()) return "", err } diff --git a/internal/services/api/handler_json.go b/internal/services/api/handler_json.go index 061b7d1..27681b2 100644 --- a/internal/services/api/handler_json.go +++ b/internal/services/api/handler_json.go @@ -11,7 +11,17 @@ import ( func HandleJson(c *gin.Context) { ip := c.ClientIP() - HandleIpInfo(c, ip) + // ClientIP can yield an unparseable value from malformed proxy headers; + // reject it explicitly instead of letting Query fail into a 500. + if net.ParseIP(ip) == nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid ip", "ip": ip}) + return + } + // selfLookup=true preserves the "default value when lookup internal ip" + // contract (commit 9d07639): a valid but DB-absent client IP (loopback / + // private addresses) returns 200 {"ip": ...} rather than a 404, since the + // caller did not ask for a specific lookup. + HandleIpInfo(c, ip, true) } func HandleOtherIp(c *gin.Context) { @@ -20,10 +30,11 @@ func HandleOtherIp(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid ip", "ip": ip}) return } - HandleIpInfo(c, ip) + // The caller explicitly requested this IP, so a not-found is a real 404. + HandleIpInfo(c, ip, false) } -func HandleIpInfo(c *gin.Context, ip string) { +func HandleIpInfo(c *gin.Context, ip string, selfLookup bool) { if !data.Ins().IsLoaded() { log.Error().Msg("DB is not loaded") c.JSON(http.StatusServiceUnavailable, gin.H{ @@ -37,6 +48,12 @@ func HandleIpInfo(c *gin.Context, ip string) { if err != nil { // Distinguish "not found" (valid IP, absent from DB) from other errors. if isNotFound(err) { + if selfLookup { + // Graceful default for the self-IP (/json) path: the client's + // address is valid but simply absent from the public GeoIP DB. + c.JSON(http.StatusOK, gin.H{"ip": ip}) + return + } c.JSON(http.StatusNotFound, gin.H{"error": "not found", "ip": ip}) return } diff --git a/internal/services/db_updater/main.go b/internal/services/db_updater/main.go index b27ec47..7418a41 100644 --- a/internal/services/db_updater/main.go +++ b/internal/services/db_updater/main.go @@ -3,10 +3,12 @@ package db_updater import ( "github.com/robfig/cron/v3" "github.com/rs/zerolog/log" + "os" + "path" "sync/atomic" "thuanle.me/ip-info/configs" "thuanle.me/ip-info/internal/data" - "time" + "thuanle.me/ip-info/pkg/osx" ) // DbUpdatedAt stores the DB last-updated time as unix milliseconds. @@ -35,19 +37,99 @@ func fetchDbs() { if newFlag { log.Info().Msg("New DB downloaded. Recreating mmdb") + // mmdbmeld.WriteMMDB truncates the canonical mmdb before writing. If + // the merge fails the file would be left empty/corrupt — and because + // download() already persisted the etag, the next daily run would get + // 304 Not Modified and never retry, leaving the service broken until a + // restart (which then fails to open the empty file). Back the file up + // first so we can restore it on failure. + backup, backupErr := backupMmdb() if err := mergeMmdb(); err != nil { - // Keep the current (previous) DB rather than reloading a - // potentially empty/corrupt merge output. - log.Err(err).Msg("Failed to merge mmdb, keeping current DB") + log.Err(err).Msg("Failed to merge mmdb") + rollBackMmdb(backup, backupErr) return } err := data.Ins().Reload() if err != nil { log.Err(err).Msg("Failed to reload mmdb") + // Reload failed on the newly merged file: restore the previous + // canonical mmdb so we keep serving known-good data. + rollBackMmdb(backup, backupErr) return } - DbUpdatedAt.Store(time.Now().UnixMilli()) + // Both merge and reload succeeded: the new canonical file is live, the + // backup is no longer needed. + cleanupMmdbBackup(backup) + + // Stamp the update time from the freshly written canonical file. + stampDbUpdatedAt() + return + } + + // No new download: on startup this is the path that loads an existing DB. + // Ensure the timestamp reflects the file we are actually serving. + if DbUpdatedAt.Load() == 0 { + stampDbUpdatedAt() + } +} + +// stampDbUpdatedAt records the mtime of the canonical mmdb (unix ms) as the +// last DB update time. This covers both initial load (pre-existing files) and +// post-download reloads, so /metrics never reports 0 while serving real data. +func stampDbUpdatedAt() { + info, err := os.Stat(configs.MmdbDbFile) + if err != nil { + log.Err(err).Str("file", configs.MmdbDbFile).Msg("Failed to stat mmdb") + return + } + DbUpdatedAt.Store(info.ModTime().UnixMilli()) +} + +// backupMmdb copies the canonical mmdb to a sibling backup file. It returns +// ("", nil) when there is no canonical file yet (first-ever build), in which +// case there is nothing to restore on failure. +func backupMmdb() (string, error) { + if _, err := os.Stat(configs.MmdbDbFile); os.IsNotExist(err) { + return "", nil + } + backup := configs.MmdbDbFile + ".bak" + if err := osx.Copy(configs.MmdbDbFile, backup); err != nil { + return "", err + } + return backup, nil +} + +// rollBackMmdb restores the canonical mmdb from backup (if any) and deletes +// the etag files so the next cron run re-downloads and retries the merge. +func rollBackMmdb(backup string, backupErr error) { + if backupErr != nil { + log.Err(backupErr).Msg("No mmdb backup available; cannot restore") + } else if backup != "" { + if err := osx.Copy(backup, configs.MmdbDbFile); err != nil { + log.Err(err).Msg("Failed to restore mmdb backup") + } else { + log.Info().Str("file", configs.MmdbDbFile).Msg("Restored mmdb from backup") + } + _ = os.Remove(backup) + } + // Drop etags so a failed merge is retried on the next run instead of being + // masked by a 304 Not Modified. + for _, url := range configs.GeoDbSourcePaths { + etag := configs.GeoDbFolder + path.Base(url) + ".etag" + if err := os.Remove(etag); err != nil && !os.IsNotExist(err) { + log.Err(err).Str("file", etag).Msg("Failed to remove etag") + } + } +} + +// cleanupMmdbBackup removes a successful merge's backup file. +func cleanupMmdbBackup(backup string) { + if backup == "" { + return + } + if err := os.Remove(backup); err != nil && !os.IsNotExist(err) { + log.Err(err).Str("file", backup).Msg("Failed to remove mmdb backup") } } -- 2.54.0 From b9ebe6f3af41ebdf3f9ed4d69c5b097cc4e26c85 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 17:19:34 +0700 Subject: [PATCH 3/3] Abort merge when backupMmdb fails (codex #3, High) 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 --- internal/services/db_updater/main.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/services/db_updater/main.go b/internal/services/db_updater/main.go index 7418a41..66072e2 100644 --- a/internal/services/db_updater/main.go +++ b/internal/services/db_updater/main.go @@ -44,9 +44,18 @@ func fetchDbs() { // restart (which then fails to open the empty file). Back the file up // first so we can restore it on failure. backup, backupErr := backupMmdb() + if backupErr != nil { + // We could not back up the current canonical mmdb. Do NOT proceed: + // mergeMmdb() truncates the canonical file before writing, and with + // no backup a later merge/reload failure would leave it corrupt + // (unrecoverable on restart). Keep serving the current DB and let + // the next run retry. + log.Err(backupErr).Msg("Failed to back up mmdb, skipping rebuild") + return + } if err := mergeMmdb(); err != nil { log.Err(err).Msg("Failed to merge mmdb") - rollBackMmdb(backup, backupErr) + rollBackMmdb(backup) return } @@ -55,7 +64,7 @@ func fetchDbs() { log.Err(err).Msg("Failed to reload mmdb") // Reload failed on the newly merged file: restore the previous // canonical mmdb so we keep serving known-good data. - rollBackMmdb(backup, backupErr) + rollBackMmdb(backup) return } @@ -103,10 +112,10 @@ func backupMmdb() (string, error) { // rollBackMmdb restores the canonical mmdb from backup (if any) and deletes // the etag files so the next cron run re-downloads and retries the merge. -func rollBackMmdb(backup string, backupErr error) { - if backupErr != nil { - log.Err(backupErr).Msg("No mmdb backup available; cannot restore") - } else if backup != "" { +// backup is "" on a first-ever build (no canonical file existed to back up), +// in which case there is nothing to restore. +func rollBackMmdb(backup string) { + if backup != "" { if err := osx.Copy(backup, configs.MmdbDbFile); err != nil { log.Err(err).Msg("Failed to restore mmdb backup") } else { -- 2.54.0