fix: run accounting reconciliation periodically, not just at startup
reconcileAccounting previously ran exactly once, when the evictor goroutine started. Combined with StoreVariant's best-effort accounting insert (warns and continues on failure), a long-running process could accumulate untracked disk usage past cache_max_bytes indefinitely -- the disk-exhaustion failure mode issue #51 exists to close -- with recovery gated on a process restart. evictionLoop now also runs a reconciliation pass on every periodic ticker tick (the same interval eviction itself uses; reconciliation walks the cache directories so it deliberately does not run on every write-pressure wakeup, to stay off the per-store hot path). This bounds unaccounted drift to at most one eviction interval regardless of how long the process has been running. TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup proves it: introduces an untracked variant file only after startup reconciliation has already completed and asserts a later periodic pass adopts it.
This commit is contained in:
@@ -399,10 +399,11 @@ func (c *Cache) notifyWritePressure() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// StartEviction launches the background eviction goroutine, which
|
// StartEviction launches the background eviction goroutine, which
|
||||||
// reconciles the database accounting with the cache directories once
|
// reconciles the database accounting with the cache directories at
|
||||||
// at startup and then evicts to the configured limit on the given
|
// startup and again on every periodic tick thereafter, and evicts to
|
||||||
// periodic interval and on write-pressure notifications. It is a
|
// the configured limit on the given periodic interval and on
|
||||||
// no-op on a disabled cache or when already started.
|
// write-pressure notifications. It is a no-op on a disabled cache or
|
||||||
|
// when already started.
|
||||||
func (c *Cache) StartEviction(interval time.Duration) {
|
func (c *Cache) StartEviction(interval time.Duration) {
|
||||||
if c.disabled || c.evictionStarted {
|
if c.disabled || c.evictionStarted {
|
||||||
return
|
return
|
||||||
@@ -433,10 +434,7 @@ func (c *Cache) evictionLoop(interval time.Duration) {
|
|||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
|
|
||||||
if err := c.reconcileAccounting(ctx); err != nil {
|
c.runReconciliationPass(ctx)
|
||||||
c.log.Warn("cache accounting reconciliation failed", "error", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
c.runEvictionPass(ctx)
|
c.runEvictionPass(ctx)
|
||||||
|
|
||||||
ticker := time.NewTicker(interval)
|
ticker := time.NewTicker(interval)
|
||||||
@@ -447,6 +445,16 @@ func (c *Cache) evictionLoop(interval time.Duration) {
|
|||||||
case <-c.evictionStop:
|
case <-c.evictionStop:
|
||||||
return
|
return
|
||||||
case <-ticker.C:
|
case <-ticker.C:
|
||||||
|
// Reconciliation walks the cache directories, so it only
|
||||||
|
// runs on the periodic ticker rather than on every
|
||||||
|
// write-pressure wakeup, keeping it off the per-store hot
|
||||||
|
// path. Reusing the eviction interval itself (rather than a
|
||||||
|
// separate, longer one) is a deliberate choice: it is the
|
||||||
|
// simplest option that still bounds how long a store's
|
||||||
|
// best-effort accounting insert can stay silently
|
||||||
|
// unaccounted for to one interval, on a process that is
|
||||||
|
// already running this loop regardless.
|
||||||
|
c.runReconciliationPass(ctx)
|
||||||
case <-c.evictionPressure:
|
case <-c.evictionPressure:
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -462,13 +470,25 @@ func (c *Cache) runEvictionPass(ctx context.Context) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// runReconciliationPass runs one reconciliation pass, logging failures
|
||||||
|
// instead of propagating them (the loop must keep running).
|
||||||
|
func (c *Cache) runReconciliationPass(ctx context.Context) {
|
||||||
|
if err := c.reconcileAccounting(ctx); err != nil {
|
||||||
|
c.log.Warn("cache accounting reconciliation failed", "error", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// reconcileAccounting synchronizes the database size accounting with
|
// reconcileAccounting synchronizes the database size accounting with
|
||||||
// the actual contents of the cache directories. It runs once when the
|
// the actual contents of the cache directories. It runs at startup and
|
||||||
// background evictor starts, off the request hot path: it adopts
|
// again on every periodic eviction tick thereafter, off the request
|
||||||
// variant files that predate the accounting table, drops accounting
|
// hot path: it adopts variant files that predate the accounting table
|
||||||
// rows whose files are missing, removes source blob files the database
|
// (or whose accounting insert failed, e.g. StoreVariant's best-effort
|
||||||
// does not know (and rows whose files are gone), and sweeps stale temp
|
// insert under transient DB contention), drops accounting rows whose
|
||||||
// files left behind by crashed writes.
|
// files are missing, removes source blob files the database does not
|
||||||
|
// know (and rows whose files are gone), and sweeps stale temp files
|
||||||
|
// left behind by crashed writes. Running it periodically, not just
|
||||||
|
// once, bounds how long such drift can accumulate unaccounted for on a
|
||||||
|
// long-running process to one eviction interval.
|
||||||
func (c *Cache) reconcileAccounting(ctx context.Context) error {
|
func (c *Cache) reconcileAccounting(ctx context.Context) error {
|
||||||
if c.disabled {
|
if c.disabled {
|
||||||
return nil
|
return nil
|
||||||
|
|||||||
Reference in New Issue
Block a user