Pull request: stats: delete units with bad names

Updates #3506.

Squashed commit of the following:

commit 8e5a2435958ca799693a25292d41cdeb4ba5e774
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Aug 27 14:34:37 2021 +0300

    stats: delete units with bad names

CHANGELOG.md

@@ -10,7 +10,7 @@ and this project adheres to
 ## [Unreleased]
 
 <!--
-## [v0.107.0] - 2021-08-03 (APPROX.)
+## [v0.107.0] - 2021-09-14 (APPROX.)
 -->
 
 ### Added
@@ -44,7 +44,8 @@ and this project adheres to
 
 ### Changed
 
-- Don't show the private key in API responses if it was saved as a string ([#1898]).
+- Don't show the private key in API responses if it was saved as a string
+  ([#1898]).
 - Better OpenWrt detection ([#3435]).
 - DNS-over-HTTPS queries that come from HTTP proxies in the `trusted_proxies`
   list now use the real IP address of the client instead of the address of the
@@ -67,6 +68,7 @@ and this project adheres to
 
 ### Fixed
 
+- Occasional panics when reading old statistics databases ([#3506]).
 - `reload` service action on macOS and FreeBSD ([#3457]).
 - Inaccurate using of service actions in the installation script ([#3450]).
 - Client ID checking ([#3437]).
@@ -134,6 +136,7 @@ and this project adheres to
 [#3437]: https://github.com/AdguardTeam/AdGuardHome/issues/3437
 [#3450]: https://github.com/AdguardTeam/AdGuardHome/issues/3450
 [#3457]: https://github.com/AdguardTeam/AdGuardHome/issues/3457
+[#3506]: https://github.com/AdguardTeam/AdGuardHome/issues/3506
 
 
 

internal/stats/unit.go

@@ -90,29 +90,7 @@ func createObject(conf Config) (s *statsCtx, err error) {
 		firstID := id - s.conf.limit - 1
 		unitDel := 0
 
-		// TODO(a.garipov): See if this is actually necessary.  Looks
-		// like a rather bizarre solution.
-		errStop := errors.Error("stop iteration")
-		forEachBkt := func(name []byte, _ *bolt.Bucket) (cberr error) {
-			nameID := uint32(btoi(name))
-			if nameID < firstID {
-				cberr = tx.DeleteBucket(name)
-				if cberr != nil {
-					log.Debug("stats: tx.DeleteBucket: %s", cberr)
-
-					return nil
-				}
-
-				log.Debug("stats: deleted unit %d", nameID)
-				unitDel++
-
-				return nil
-			}
-
-			return errStop
-		}
-
-		err = tx.ForEach(forEachBkt)
+		err = tx.ForEach(newBucketWalker(tx, &unitDel, firstID))
 		if err != nil && !errors.Is(err, errStop) {
 			log.Debug("stats: deleting units: %s", err)
 		}
@@ -141,6 +119,39 @@ func createObject(conf Config) (s *statsCtx, err error) {
 	return s, nil
 }
 
+// TODO(a.garipov): See if this is actually necessary.  Looks like a rather
+// bizarre solution.
+const errStop errors.Error = "stop iteration"
+
+// newBucketWalker returns a new bucket walker that deletes old units.  The
+// integer that unitDelPtr points to is incremented for every successful
+// deletion.  If the bucket isn't deleted, f returns errStop.
+func newBucketWalker(
+	tx *bolt.Tx,
+	unitDelPtr *int,
+	firstID uint32,
+) (f func(name []byte, b *bolt.Bucket) (err error)) {
+	return func(name []byte, _ *bolt.Bucket) (err error) {
+		nameID, ok := unitNameToID(name)
+		if !ok || nameID < firstID {
+			err = tx.DeleteBucket(name)
+			if err != nil {
+				log.Debug("stats: tx.DeleteBucket: %s", err)
+
+				return nil
+			}
+
+			log.Debug("stats: deleted unit %d (name %x)", nameID, name)
+
+			*unitDelPtr++
+
+			return nil
+		}
+
+		return errStop
+	}
+}
+
 func (s *statsCtx) Start() {
 	s.initWeb()
 	go s.periodicFlush()
@@ -215,21 +226,28 @@ func (s *statsCtx) commitTxn(tx *bolt.Tx) {
 	log.Tracef("tx.Commit")
 }
 
-// Get unit name
-func unitName(id uint32) []byte {
-	return itob(uint64(id))
+// bucketNameLen is the length of a bucket, a 64-bit unsigned integer.
+//
+// TODO(a.garipov): Find out why a 64-bit integer is used when IDs seem to
+// always be 32 bits.
+const bucketNameLen = 8
+
+// idToUnitName converts a numerical ID into a database unit name.
+func idToUnitName(id uint32) (name []byte) {
+	name = make([]byte, bucketNameLen)
+	binary.BigEndian.PutUint64(name, uint64(id))
+
+	return name
 }
 
-// Convert integer to 8-byte array (big endian)
-func itob(v uint64) []byte {
-	b := make([]byte, 8)
-	binary.BigEndian.PutUint64(b, v)
-	return b
-}
+// unitNameToID converts a database unit name into a numerical ID.  ok is false
+// if name is not a valid database unit name.
+func unitNameToID(name []byte) (id uint32, ok bool) {
+	if len(name) < bucketNameLen {
+		return 0, false
+	}
 
-// Convert 8-byte array (big endian) to integer
-func btoi(b []byte) uint64 {
-	return binary.BigEndian.Uint64(b)
+	return uint32(binary.BigEndian.Uint64(name)), true
 }
 
 // Flush the current unit to DB and delete an old unit when a new hour is started
@@ -282,7 +300,7 @@ func (s *statsCtx) periodicFlush() {
 
 // Delete unit's data from file
 func (s *statsCtx) deleteUnit(tx *bolt.Tx, id uint32) bool {
-	err := tx.DeleteBucket(unitName(id))
+	err := tx.DeleteBucket(idToUnitName(id))
 	if err != nil {
 		log.Tracef("stats: bolt DeleteBucket: %s", err)
 
@@ -357,7 +375,7 @@ func deserialize(u *unit, udb *unitDB) {
 func (s *statsCtx) flushUnitToDB(tx *bolt.Tx, id uint32, udb *unitDB) bool {
 	log.Tracef("Flushing unit %d", id)
 
-	bkt, err := tx.CreateBucketIfNotExists(unitName(id))
+	bkt, err := tx.CreateBucketIfNotExists(idToUnitName(id))
 	if err != nil {
 		log.Error("tx.CreateBucketIfNotExists: %s", err)
 		return false
@@ -381,7 +399,7 @@ func (s *statsCtx) flushUnitToDB(tx *bolt.Tx, id uint32, udb *unitDB) bool {
 }
 
 func (s *statsCtx) loadUnitFromDB(tx *bolt.Tx, id uint32) *unitDB {
-	bkt := tx.Bucket(unitName(id))
+	bkt := tx.Bucket(idToUnitName(id))
 	if bkt == nil {
 		return nil
 	}

scripts/make/go-lint.sh

@@ -201,12 +201,12 @@ golint --set_exit_status ./...
 
 # Apply more lax standards to the code we haven't properly refactored yet.
 gocyclo --over 17 ./internal/dhcpd/ ./internal/dnsforward/\
-	./internal/filtering/ ./internal/home/ ./internal/querylog/\
-	./internal/stats/ ./internal/updater/
+	./internal/filtering/ ./internal/home/ ./internal/querylog/
 
-# Apply stricter standards to new or vetted code
+# Apply stricter standards to new or somewhat refactored code.
 gocyclo --over 10 ./internal/aghio/ ./internal/aghnet/ ./internal/aghos/\
-	./internal/aghtest/ ./internal/tools/ ./internal/version/ ./main.go
+	./internal/aghtest/ ./internal/stats/ ./internal/tools/\
+	./internal/updater/ ./internal/version/ ./main.go
 
 gosec --quiet $go_files