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 <noreply@anthropic.com>

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
 	}
 

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
 		}

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")
 	}
 }