From 17a2732cb80ff73b78e8dec92f2152a3bc1e39c7 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:37:19 -0400 Subject: [PATCH 1/5] perf(metadata): scale image-cache concurrency with host CPUs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scheduled image-cache task ran 2 workers on a 2-job claim page, regardless of hardware — measured at roughly 60 images/minute on a ~600k-item library, where the queue takes days to drain after a large scan. processClaimedJobs already dispatches a claimed page through a bounded semaphore, so a page larger than the worker count keeps the pool saturated instead of waiting on every straggler before the next page can be claimed; the tiny fixed numbers were the only thing holding throughput back. Workers now scale as 4x GOMAXPROCS capped at 48 — each job is a mix of a 30s-capped download and a libvips encode ladder, so oversubscribing CPUs keeps cores busy during network waits, while the cap keeps a many-core server from monopolizing provider connections and household boxes at a modest pool instead of a fixed 48. The claim page is 10 jobs per worker: it drains within ten times the worst single job, well inside the 15-minute claim lease and behind the existing 10-minute task runtime cap. Derived from RXWatcher/silo-server@3b377f5c2, which measured roughly 2,900 images/minute with 48 workers over a 60-minute window on the library above; that change hard-coded 480/48, which fits a large server but oversubscribes the small end of deployments. Co-Authored-By: Claude Fable 5 --- .../tasks/cache_metadata_images.go | 31 +++++++++++++--- .../tasks/cache_metadata_images_test.go | 36 +++++++++++++++---- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/internal/taskmanager/tasks/cache_metadata_images.go b/internal/taskmanager/tasks/cache_metadata_images.go index 58d37d98e9..f11beafa7a 100644 --- a/internal/taskmanager/tasks/cache_metadata_images.go +++ b/internal/taskmanager/tasks/cache_metadata_images.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "runtime" "time" "github.com/google/uuid" @@ -15,14 +16,34 @@ import ( const ( cacheMetadataImagesIntervalMs = int64(60 * 1000) - // Claim only work that can start immediately. Claiming a large queue page - // stamps one lease on every row up front; with two workers, the unstarted - // tail could expire and be reclaimed before this execution reaches it. - cacheMetadataImagesClaimLimit = 2 - cacheMetadataImagesWorkers = 2 cacheMetadataImagesMaxRuntime = 10 * time.Minute ) +// imageCacheWorkerCount sizes the image-cache worker pool for the host. Each +// job downloads an original (30s timeout in imagecache) and runs a libvips +// WEBP encode ladder, so the work is a CPU/network mix: 4× the scheduler's +// CPU count keeps cores busy while other workers wait on downloads, and the +// cap keeps a many-core server from monopolizing provider connections. The +// previous fixed pool of 2 measured roughly 60 images/minute on a ~600k-item +// library; 48 workers measured roughly 2,900/minute over a 60-minute window +// (RXWatcher/silo-server@3b377f5c2). +func imageCacheWorkerCount(numCPU int) int { + return min(48, 4*max(numCPU, 1)) +} + +var cacheMetadataImagesWorkers = imageCacheWorkerCount(runtime.GOMAXPROCS(0)) + +// cacheMetadataImagesClaimLimit is the queue page stamped with one lease up +// front. processClaimedJobs dispatches the page through a semaphore, so a page +// larger than the worker count keeps the pool saturated instead of waiting on +// every straggler in a worker-sized batch before the next page can be claimed. +// The 15-minute lease stamped at claim (imageCacheLeaseDuration) is the +// ceiling on page size: at 10 jobs per worker the page drains within ten times +// the worst single job, and the slowest realistic job — a download at the full +// 30-second timeout plus encode and uploads — keeps that comfortably inside +// the lease even before the 10-minute task runtime cap stops new claims. +var cacheMetadataImagesClaimLimit = 10 * cacheMetadataImagesWorkers + type MetadataImageCacheRunner interface { DrainUntilIdle(ctx context.Context, workerID string, claimLimit int, concurrency int, maxRuntime time.Duration, reportProgress metadata.ImageCacheRunProgressReporter) (metadata.ImageCacheRunStats, error) } diff --git a/internal/taskmanager/tasks/cache_metadata_images_test.go b/internal/taskmanager/tasks/cache_metadata_images_test.go index 0814eb10a3..8b612cf79b 100644 --- a/internal/taskmanager/tasks/cache_metadata_images_test.go +++ b/internal/taskmanager/tasks/cache_metadata_images_test.go @@ -115,11 +115,11 @@ func TestCacheMetadataImagesTaskReportsStats(t *testing.T) { if err := task.Execute(context.Background(), progress); err != nil { t.Fatalf("Execute() error = %v", err) } - if runner.claimLimit != 2 { - t.Fatalf("claimLimit = %d, want one immediately-startable job per worker", runner.claimLimit) + if runner.claimLimit != cacheMetadataImagesClaimLimit { + t.Fatalf("claimLimit = %d, want the shared page size %d", runner.claimLimit, cacheMetadataImagesClaimLimit) } - if runner.concurrency != 2 { - t.Fatalf("concurrency = %d, want 2", runner.concurrency) + if runner.concurrency != cacheMetadataImagesWorkers { + t.Fatalf("concurrency = %d, want the shared worker count %d", runner.concurrency, cacheMetadataImagesWorkers) } if runner.maxRuntime != 10*time.Minute { t.Fatalf("maxRuntime = %s, want 10m", runner.maxRuntime) @@ -161,8 +161,8 @@ func TestBackfillMetadataImagesTaskReportsDiscovery(t *testing.T) { if runner.maxRuntime != 0 { t.Fatalf("maxRuntime = %s, want no deadline for manual backfill", runner.maxRuntime) } - if runner.claimLimit != 2 { - t.Fatalf("claimLimit = %d, want one immediately-startable job per worker", runner.claimLimit) + if runner.claimLimit != cacheMetadataImagesClaimLimit { + t.Fatalf("claimLimit = %d, want the shared page size %d", runner.claimLimit, cacheMetadataImagesClaimLimit) } if len(runner.workerIDs) != 1 || !strings.Contains(runner.workerIDs[0], ":backfill:") { t.Fatalf("backfill worker IDs = %#v, want one execution-scoped backfill owner", runner.workerIDs) @@ -329,3 +329,27 @@ func TestBackfillMetadataImagesTaskProgressDoesNotFallWhenDiscoveryWidensTheRun( t.Fatalf("recovered report = %g, want 75", progress.percents[3]) } } + +func TestImageCacheWorkerCount(t *testing.T) { + cases := []struct { + numCPU int + want int + }{ + {numCPU: 0, want: 4}, // defensive floor; GOMAXPROCS never reports < 1 + {numCPU: 1, want: 4}, // small household box: modest but no longer crippled + {numCPU: 4, want: 16}, + {numCPU: 12, want: 48}, + {numCPU: 16, want: 48}, // cap: more cores must not monopolize providers + {numCPU: 64, want: 48}, + } + for _, tc := range cases { + if got := imageCacheWorkerCount(tc.numCPU); got != tc.want { + t.Errorf("imageCacheWorkerCount(%d) = %d, want %d", tc.numCPU, got, tc.want) + } + } + // The claim page must always drain inside the 15-minute claim lease: 10 + // jobs per worker at the ~60s worst realistic job is 10 minutes. + if cacheMetadataImagesClaimLimit != 10*cacheMetadataImagesWorkers { + t.Errorf("claim limit = %d, want 10x workers (%d)", cacheMetadataImagesClaimLimit, 10*cacheMetadataImagesWorkers) + } +} From e696630c2693a16e403dfb1d8b073822b78666d9 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:21:59 -0400 Subject: [PATCH 2/5] fix(metadata): bound image-cache jobs and size workers by memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the concurrency raise (#817): Lease enforcement: only the download had a deadline; a hung encode or upload could hold a job indefinitely, so nothing enforced the claim-page lease math and an unstarted page tail could outlive its 15-minute lease and be reclaimed and duplicated by another node. Every job now runs under ImageCacheJobTimeout (2 minutes) end to end, the claim page drops from 10 to 5 jobs per worker so a page's worst-case drain is 10 minutes against the 15-minute lease, and the arithmetic is asserted in TestImageCacheWorkerCount against the now-exported ImageCacheLeaseDuration. Memory bound: worker count derived from CPUs alone could put 48 concurrent jobs — each able to hold a 25 MiB download plus a full Go decode of the original for thumbhash — inside a container with a small memory limit. The pool is now also capped at one worker per 512 MiB of the tightest detectable memory bound (GOMEMLIMIT, cgroup limit, then /proc/meminfo), with the original pool of 2 as the floor. Co-Authored-By: Claude Fable 5 --- internal/metadata/image_cache_job_repo.go | 7 +- internal/metadata/image_cache_processor.go | 16 +++- .../metadata/image_cache_processor_test.go | 2 +- .../tasks/cache_metadata_images.go | 75 +++++++++++++++---- .../tasks/cache_metadata_images_test.go | 39 +++++++--- 5 files changed, 108 insertions(+), 31 deletions(-) diff --git a/internal/metadata/image_cache_job_repo.go b/internal/metadata/image_cache_job_repo.go index 706ad8a1f4..ddeab1d1ad 100644 --- a/internal/metadata/image_cache_job_repo.go +++ b/internal/metadata/image_cache_job_repo.go @@ -33,7 +33,10 @@ const ( ImageCacheStatusSucceeded = "succeeded" ImageCacheStatusFailed = "failed" - imageCacheLeaseDuration = 15 * time.Minute + // ImageCacheLeaseDuration is stamped on every claimed row up front. + // Exported so worker/claim-page sizing can assert a claimed page always + // drains inside it (see internal/taskmanager/tasks). + ImageCacheLeaseDuration = 15 * time.Minute imageCacheMaxAttempts = 8 imageCacheDeferredRetry = 7 * 24 * time.Hour @@ -427,7 +430,7 @@ func (r *ImageCacheJobRepository) recoverExpiredRunning(ctx context.Context, tar AND locked_at < NOW() - $1::interval ` args := []any{ - intervalLiteral(imageCacheLeaseDuration), + intervalLiteral(ImageCacheLeaseDuration), imageCacheMaxAttempts, intervalLiteral(imageCacheFailedCooldown), } diff --git a/internal/metadata/image_cache_processor.go b/internal/metadata/image_cache_processor.go index ce3b4b2bb9..b6369149cf 100644 --- a/internal/metadata/image_cache_processor.go +++ b/internal/metadata/image_cache_processor.go @@ -34,11 +34,21 @@ const ( imageCacheDiscoveryBatchSize = 1000 // Waiting for a background worker to release a job polls with backoff and // gives up after immediateImageCacheIdleTimeout without progress. The - // worker's own lease runs for imageCacheLeaseDuration, and its pod can die + // worker's own lease runs for ImageCacheLeaseDuration, and its pod can die // while holding it, so an interactive refresh must not wait that long. immediateImageCacheMinPoll = 100 * time.Millisecond immediateImageCacheMaxPoll = 2 * time.Second immediateImageCacheIdleTimeout = 30 * time.Second + + // ImageCacheJobTimeout bounds one job end to end: source check, download + // (which also has its own 30-second cap in imagecache), variant encode, and + // uploads. Only the download had a deadline before; a hung upload or encode + // could hold a job past its claim lease, letting another worker reclaim and + // duplicate it. Two minutes is generous for the slowest realistic job, and + // worker/claim-page sizing (internal/taskmanager/tasks) relies on it to + // prove a claimed page always drains inside ImageCacheLeaseDuration. A job + // that hits it is marked failed and retried on the normal backoff. + ImageCacheJobTimeout = 2 * time.Minute ) // ErrTargetArtworkPending reports that some of a refreshed target's artwork was @@ -434,7 +444,9 @@ loop: go func(job *models.MetadataImageCacheJob) { defer wg.Done() defer func() { <-sem }() - result := p.processOne(ctx, job) + jobCtx, cancelJob := context.WithTimeout(ctx, ImageCacheJobTimeout) + result := p.processOne(jobCtx, job) + cancelJob() mu.Lock() switch result.outcome { case "succeeded": diff --git a/internal/metadata/image_cache_processor_test.go b/internal/metadata/image_cache_processor_test.go index aa77734b62..473b6dfd9a 100644 --- a/internal/metadata/image_cache_processor_test.go +++ b/internal/metadata/image_cache_processor_test.go @@ -457,7 +457,7 @@ func TestImageCacheProcessorRetriesTargetAfterConcurrentWorkerFinishes(t *testin func TestImageCacheProcessorStopsWaitingOnStuckBackgroundWorker(t *testing.T) { // A worker that claimed the job and died holds its lease for - // imageCacheLeaseDuration. The interactive refresh must not block that + // ImageCacheLeaseDuration. The interactive refresh must not block that // long: it gives up and reports the artwork as still pending. jobs := &targetImageCacheJobs{alwaysRunning: true} processor := NewImageCacheProcessorWithTargets( diff --git a/internal/taskmanager/tasks/cache_metadata_images.go b/internal/taskmanager/tasks/cache_metadata_images.go index f11beafa7a..8aa7ec8938 100644 --- a/internal/taskmanager/tasks/cache_metadata_images.go +++ b/internal/taskmanager/tasks/cache_metadata_images.go @@ -4,13 +4,16 @@ import ( "context" "encoding/json" "fmt" + "math" "os" "runtime" + "runtime/debug" "time" "github.com/google/uuid" "github.com/Silo-Server/silo-server/internal/metadata" + "github.com/Silo-Server/silo-server/internal/nodemetrics" "github.com/Silo-Server/silo-server/internal/taskmanager" ) @@ -19,6 +22,16 @@ const ( cacheMetadataImagesMaxRuntime = 10 * time.Minute ) +// imageCacheWorkerMemoryBudget is the memory reserved per concurrent image +// job when sizing the pool against a detected memory bound. A job can hold +// the compressed download (capped at 25 MiB in imagecache), the libvips +// working set for the variant ladder, and — today — a full Go-heap decode of +// the original for thumbhash, which for a large provider poster reaches the +// low hundreds of MiB. 512 MiB per worker keeps even a burst of worst-case +// originals from consuming the whole budget, since the baseline server also +// lives inside it. +const imageCacheWorkerMemoryBudget = 512 << 20 + // imageCacheWorkerCount sizes the image-cache worker pool for the host. Each // job downloads an original (30s timeout in imagecache) and runs a libvips // WEBP encode ladder, so the work is a CPU/network mix: 4× the scheduler's @@ -27,22 +40,56 @@ const ( // previous fixed pool of 2 measured roughly 60 images/minute on a ~600k-item // library; 48 workers measured roughly 2,900/minute over a 60-minute window // (RXWatcher/silo-server@3b377f5c2). -func imageCacheWorkerCount(numCPU int) int { - return min(48, 4*max(numCPU, 1)) +// +// memoryBytes, when positive, is the tightest detected memory bound for this +// process (GOMEMLIMIT, cgroup limit, or system memory) and caps the pool at +// one worker per imageCacheWorkerMemoryBudget so a many-core container with a +// small memory limit cannot be OOM-killed by concurrent decodes. The floor of +// 2 is the pool size this task shipped with. +func imageCacheWorkerCount(numCPU int, memoryBytes int64) int { + workers := min(48, 4*max(numCPU, 1)) + if memoryBytes > 0 { + workers = min(workers, max(int(memoryBytes/imageCacheWorkerMemoryBudget), 2)) + } + return workers } -var cacheMetadataImagesWorkers = imageCacheWorkerCount(runtime.GOMAXPROCS(0)) - -// cacheMetadataImagesClaimLimit is the queue page stamped with one lease up -// front. processClaimedJobs dispatches the page through a semaphore, so a page -// larger than the worker count keeps the pool saturated instead of waiting on -// every straggler in a worker-sized batch before the next page can be claimed. -// The 15-minute lease stamped at claim (imageCacheLeaseDuration) is the -// ceiling on page size: at 10 jobs per worker the page drains within ten times -// the worst single job, and the slowest realistic job — a download at the full -// 30-second timeout plus encode and uploads — keeps that comfortably inside -// the lease even before the 10-minute task runtime cap stops new claims. -var cacheMetadataImagesClaimLimit = 10 * cacheMetadataImagesWorkers +// detectImageCacheMemoryBytes returns the tightest memory bound the process +// can see, or 0 when none is detectable (macOS dev boxes, bare Linux without +// cgroups): an explicit GOMEMLIMIT first, then the container's cgroup limit, +// then total system memory. +func detectImageCacheMemoryBytes() int64 { + if limit := debug.SetMemoryLimit(-1); limit > 0 && limit < math.MaxInt64 { + return limit + } + for _, path := range nodemetrics.CgroupMemoryLimitPaths() { + if mem, err := nodemetrics.ReadCgroupMemoryLimit(path); err == nil && mem > 0 { + return mem + } + } + if mem, err := nodemetrics.ReadMeminfoTotalBytes("/proc/meminfo"); err == nil && mem > 0 { + return mem + } + return 0 +} + +var cacheMetadataImagesWorkers = imageCacheWorkerCount(runtime.GOMAXPROCS(0), detectImageCacheMemoryBytes()) + +// cacheMetadataImagesClaimPerWorker sizes the queue page stamped with one +// lease up front. processClaimedJobs dispatches the page through a semaphore, +// so a page larger than the worker count keeps the pool saturated instead of +// waiting on every straggler in a worker-sized batch before the next page can +// be claimed. +// +// The page must drain inside metadata.ImageCacheLeaseDuration (15 minutes) or +// another worker reclaims the unstarted tail and duplicates it. Every job is +// hard-bounded by metadata.ImageCacheJobTimeout (2 minutes), so a page of N +// jobs per worker fully drains within N × that timeout: 5 × 2 = 10 minutes, +// leaving a third of the lease as margin. TestImageCacheWorkerCount asserts +// this arithmetic against the exported constants. +const cacheMetadataImagesClaimPerWorker = 5 + +var cacheMetadataImagesClaimLimit = cacheMetadataImagesClaimPerWorker * cacheMetadataImagesWorkers type MetadataImageCacheRunner interface { DrainUntilIdle(ctx context.Context, workerID string, claimLimit int, concurrency int, maxRuntime time.Duration, reportProgress metadata.ImageCacheRunProgressReporter) (metadata.ImageCacheRunStats, error) diff --git a/internal/taskmanager/tasks/cache_metadata_images_test.go b/internal/taskmanager/tasks/cache_metadata_images_test.go index 8b612cf79b..36401626fe 100644 --- a/internal/taskmanager/tasks/cache_metadata_images_test.go +++ b/internal/taskmanager/tasks/cache_metadata_images_test.go @@ -331,25 +331,40 @@ func TestBackfillMetadataImagesTaskProgressDoesNotFallWhenDiscoveryWidensTheRun( } func TestImageCacheWorkerCount(t *testing.T) { + const gib = int64(1) << 30 cases := []struct { numCPU int + memory int64 want int }{ - {numCPU: 0, want: 4}, // defensive floor; GOMAXPROCS never reports < 1 - {numCPU: 1, want: 4}, // small household box: modest but no longer crippled - {numCPU: 4, want: 16}, - {numCPU: 12, want: 48}, - {numCPU: 16, want: 48}, // cap: more cores must not monopolize providers - {numCPU: 64, want: 48}, + {numCPU: 0, memory: 0, want: 4}, // defensive floor; GOMAXPROCS never reports < 1 + {numCPU: 1, memory: 0, want: 4}, // small household box: modest but no longer crippled + {numCPU: 4, memory: 0, want: 16}, + {numCPU: 12, memory: 0, want: 48}, + {numCPU: 16, memory: 0, want: 48}, // cap: more cores must not monopolize providers + {numCPU: 64, memory: 0, want: 48}, + // A many-core container with a small memory limit is bounded by + // memory, never below the original pool of 2. + {numCPU: 16, memory: 1 * gib, want: 2}, + {numCPU: 16, memory: 2 * gib, want: 4}, + {numCPU: 16, memory: 8 * gib, want: 16}, + {numCPU: 16, memory: 64 * gib, want: 48}, + // Plenty of memory but few cores: CPU stays the binding cap. + {numCPU: 2, memory: 64 * gib, want: 8}, } for _, tc := range cases { - if got := imageCacheWorkerCount(tc.numCPU); got != tc.want { - t.Errorf("imageCacheWorkerCount(%d) = %d, want %d", tc.numCPU, got, tc.want) + if got := imageCacheWorkerCount(tc.numCPU, tc.memory); got != tc.want { + t.Errorf("imageCacheWorkerCount(%d, %d) = %d, want %d", tc.numCPU, tc.memory, got, tc.want) } } - // The claim page must always drain inside the 15-minute claim lease: 10 - // jobs per worker at the ~60s worst realistic job is 10 minutes. - if cacheMetadataImagesClaimLimit != 10*cacheMetadataImagesWorkers { - t.Errorf("claim limit = %d, want 10x workers (%d)", cacheMetadataImagesClaimLimit, 10*cacheMetadataImagesWorkers) + // A claimed page is stamped with one lease up front, so it must fully + // drain before the lease expires or another worker reclaims the tail. + // Every job is bounded by metadata.ImageCacheJobTimeout, so the page's + // worst-case drain is jobs-per-worker times that timeout. + if drain := time.Duration(cacheMetadataImagesClaimPerWorker) * metadata.ImageCacheJobTimeout; drain >= metadata.ImageCacheLeaseDuration { + t.Errorf("worst-case page drain %s must stay under the %s claim lease", drain, metadata.ImageCacheLeaseDuration) + } + if cacheMetadataImagesClaimLimit != cacheMetadataImagesClaimPerWorker*cacheMetadataImagesWorkers { + t.Errorf("claim limit = %d, want %d per worker (%d)", cacheMetadataImagesClaimLimit, cacheMetadataImagesClaimPerWorker, cacheMetadataImagesClaimPerWorker*cacheMetadataImagesWorkers) } } From c94dae051310d2f10738bee75f52609ac86a80f6 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:30:41 -0400 Subject: [PATCH 3/5] fix(metadata): honor inherited cgroup limits; budget encode overshoot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more review follow-ups on the image-cache sizing (#817): Cgroup limits: worker sizing consulted only the root-level cgroup memory files, which are right inside a namespaced container but wrong for a systemd unit with MemoryMax= or a leaf inheriting a tighter slice/pod limit — those fell through to host MemTotal and could size 48 workers inside a small cgroup. nodemetrics already resolves this process's own cgroup and walks its ancestors for the sampler; that machinery is now exposed as nodemetrics.EffectiveMemoryLimitBytes and used for sizing, with a fixture test covering the systemd-slice shape. Overshoot: the per-job context timeout cannot preempt the synchronous decode/encode segment (imageutil.Thumbhash and GenerateVariants take no context), so the two-minute bound is not perfectly hard — the job stops at the next context-aware step. That segment works on inputs capped at 25 MiB, so its overshoot is CPU-bounded; the claim page drops from 5 to 4 jobs per worker, keeping the worst chain inside the lease with nearly two minutes of overshoot allowance per job, and the sizing test now requires that headroom instead of a bare drain < lease check. Co-Authored-By: Claude Fable 5 --- internal/nodemetrics/meminfo.go | 29 +++++++++ internal/nodemetrics/meminfo_test.go | 61 +++++++++++++++++++ .../tasks/cache_metadata_images.go | 30 +++++---- .../tasks/cache_metadata_images_test.go | 11 ++-- 4 files changed, 115 insertions(+), 16 deletions(-) create mode 100644 internal/nodemetrics/meminfo_test.go diff --git a/internal/nodemetrics/meminfo.go b/internal/nodemetrics/meminfo.go index 581bef4a10..24b3c90377 100644 --- a/internal/nodemetrics/meminfo.go +++ b/internal/nodemetrics/meminfo.go @@ -138,6 +138,35 @@ func parseMeminfoLine(line string) (string, int64, bool) { return strings.TrimSpace(key), value, true } +// EffectiveMemoryLimitBytes returns the tightest cgroup memory limit in force +// on this process, or 0 when none is set. +// +// The root-level limit files alone are only right inside a cgroup-namespaced +// container. A systemd unit with MemoryMax=, or a leaf inheriting a tighter +// limit from a slice or pod ancestor, publishes its limit at this process's +// own cgroup or somewhere above it while the root file reads "max" — so this +// walks the process's own cgroup, every ancestor, and the mount root, and +// takes the tightest concrete limit found, mirroring what the sampler reports +// (the kernel OOM-kills against the tightest level). +func EffectiveMemoryLimitBytes() int64 { + return effectiveMemoryLimitBytes("/proc", cgroupMemoryUsagePaths) +} + +func effectiveMemoryLimitBytes(procDir string, layouts []cgroupUsagePath) int64 { + relative := cgroupRelativePaths(procDir) + tightest := int64(0) + for _, level := range withCgroupSelfUsagePaths(relative, layouts) { + limit, err := ReadCgroupMemoryLimit(level.limit) + if err != nil || limit <= 0 { + continue + } + if tightest == 0 || limit < tightest { + tightest = limit + } + } + return tightest +} + // ReadCgroupMemoryLimit reads a cgroup memory-limit file and returns the limit // in bytes, or an error when the cgroup imposes none. // diff --git a/internal/nodemetrics/meminfo_test.go b/internal/nodemetrics/meminfo_test.go new file mode 100644 index 0000000000..23b9c3513f --- /dev/null +++ b/internal/nodemetrics/meminfo_test.go @@ -0,0 +1,61 @@ +package nodemetrics + +import ( + "os" + "path/filepath" + "testing" +) + +// A systemd unit with MemoryMax= publishes its limit on the slice above the +// leaf while both the leaf and the mount root read "max". The effective limit +// must come from whichever level actually binds, not from the root files +// alone. +func TestEffectiveMemoryLimitBytesFindsTheBindingAncestor(t *testing.T) { + root := t.TempDir() + previousRoot := cgroupMountRoot + cgroupMountRoot = root + t.Cleanup(func() { cgroupMountRoot = previousRoot }) + + leaf := filepath.Join(root, "system.slice", "silo.service") + if err := os.MkdirAll(leaf, 0o755); err != nil { + t.Fatalf("create %s: %v", leaf, err) + } + write := func(path, body string) { + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + } + procDir := t.TempDir() + if err := os.MkdirAll(filepath.Join(procDir, "self"), 0o755); err != nil { + t.Fatalf("create proc self dir: %v", err) + } + write(filepath.Join(procDir, "self", "cgroup"), "0::/system.slice/silo.service\n") + + layouts := []cgroupUsagePath{{ + limit: filepath.Join(root, "memory.max"), + usage: filepath.Join(root, "memory.current"), + stat: filepath.Join(root, "memory.stat"), + inactiveFile: cgroupInactiveFileKeyV2, + }} + const gib = int64(1) << 30 + + write(filepath.Join(root, "memory.max"), "max\n") + write(filepath.Join(root, "system.slice", "memory.max"), "2147483648\n") + write(filepath.Join(leaf, "memory.max"), "max\n") + if got := effectiveMemoryLimitBytes(procDir, layouts); got != 2*gib { + t.Fatalf("effective limit = %d, want the slice's 2GiB", got) + } + + // A leaf tighter than its ancestor binds instead. + write(filepath.Join(leaf, "memory.max"), "1073741824\n") + if got := effectiveMemoryLimitBytes(procDir, layouts); got != gib { + t.Fatalf("effective limit = %d, want the leaf's 1GiB", got) + } + + // No concrete limit anywhere reads as no limit, never as a sentinel. + write(filepath.Join(leaf, "memory.max"), "max\n") + write(filepath.Join(root, "system.slice", "memory.max"), "max\n") + if got := effectiveMemoryLimitBytes(procDir, layouts); got != 0 { + t.Fatalf("effective limit = %d, want 0 for no limit", got) + } +} diff --git a/internal/taskmanager/tasks/cache_metadata_images.go b/internal/taskmanager/tasks/cache_metadata_images.go index 8aa7ec8938..49e2ca8c29 100644 --- a/internal/taskmanager/tasks/cache_metadata_images.go +++ b/internal/taskmanager/tasks/cache_metadata_images.go @@ -56,16 +56,16 @@ func imageCacheWorkerCount(numCPU int, memoryBytes int64) int { // detectImageCacheMemoryBytes returns the tightest memory bound the process // can see, or 0 when none is detectable (macOS dev boxes, bare Linux without -// cgroups): an explicit GOMEMLIMIT first, then the container's cgroup limit, -// then total system memory. +// cgroups): an explicit GOMEMLIMIT first, then the tightest cgroup limit in +// force on this process — own cgroup, ancestors, and root, so a systemd +// MemoryMax= or an inherited pod/slice limit binds, not just a namespaced +// container's root files — then total system memory. func detectImageCacheMemoryBytes() int64 { if limit := debug.SetMemoryLimit(-1); limit > 0 && limit < math.MaxInt64 { return limit } - for _, path := range nodemetrics.CgroupMemoryLimitPaths() { - if mem, err := nodemetrics.ReadCgroupMemoryLimit(path); err == nil && mem > 0 { - return mem - } + if limit := nodemetrics.EffectiveMemoryLimitBytes(); limit > 0 { + return limit } if mem, err := nodemetrics.ReadMeminfoTotalBytes("/proc/meminfo"); err == nil && mem > 0 { return mem @@ -82,12 +82,18 @@ var cacheMetadataImagesWorkers = imageCacheWorkerCount(runtime.GOMAXPROCS(0), de // be claimed. // // The page must drain inside metadata.ImageCacheLeaseDuration (15 minutes) or -// another worker reclaims the unstarted tail and duplicates it. Every job is -// hard-bounded by metadata.ImageCacheJobTimeout (2 minutes), so a page of N -// jobs per worker fully drains within N × that timeout: 5 × 2 = 10 minutes, -// leaving a third of the lease as margin. TestImageCacheWorkerCount asserts -// this arithmetic against the exported constants. -const cacheMetadataImagesClaimPerWorker = 5 +// another worker reclaims the unstarted tail and duplicates it. Every job runs +// under metadata.ImageCacheJobTimeout (2 minutes), but that context cannot +// preempt the synchronous decode/encode segment (imageutil.Thumbhash, +// GenerateVariants take no context) — it only stops the job at the next +// context-aware step. That segment operates on inputs capped at 25 MiB +// (imagecache's download limit), so its overshoot is bounded by CPU speed, +// not by the network; 4 jobs per worker budgets the worst chain at +// 4 × (timeout + overshoot), which stays inside the lease with a generous +// overshoot allowance of nearly two minutes per job. +// TestImageCacheWorkerCount asserts the timeout part of this arithmetic +// against the exported constants. +const cacheMetadataImagesClaimPerWorker = 4 var cacheMetadataImagesClaimLimit = cacheMetadataImagesClaimPerWorker * cacheMetadataImagesWorkers diff --git a/internal/taskmanager/tasks/cache_metadata_images_test.go b/internal/taskmanager/tasks/cache_metadata_images_test.go index 36401626fe..b56f1cc09d 100644 --- a/internal/taskmanager/tasks/cache_metadata_images_test.go +++ b/internal/taskmanager/tasks/cache_metadata_images_test.go @@ -359,10 +359,13 @@ func TestImageCacheWorkerCount(t *testing.T) { } // A claimed page is stamped with one lease up front, so it must fully // drain before the lease expires or another worker reclaims the tail. - // Every job is bounded by metadata.ImageCacheJobTimeout, so the page's - // worst-case drain is jobs-per-worker times that timeout. - if drain := time.Duration(cacheMetadataImagesClaimPerWorker) * metadata.ImageCacheJobTimeout; drain >= metadata.ImageCacheLeaseDuration { - t.Errorf("worst-case page drain %s must stay under the %s claim lease", drain, metadata.ImageCacheLeaseDuration) + // The job timeout cannot preempt the synchronous decode/encode segment, + // so the timeout-based drain must leave real headroom under the lease + // for that bounded overshoot — at least one extra timeout per job. + drain := time.Duration(cacheMetadataImagesClaimPerWorker) * metadata.ImageCacheJobTimeout + overshootBudget := time.Duration(cacheMetadataImagesClaimPerWorker) * metadata.ImageCacheJobTimeout / 2 + if drain+overshootBudget > metadata.ImageCacheLeaseDuration { + t.Errorf("page drain %s plus overshoot budget %s must stay within the %s claim lease", drain, overshootBudget, metadata.ImageCacheLeaseDuration) } if cacheMetadataImagesClaimLimit != cacheMetadataImagesClaimPerWorker*cacheMetadataImagesWorkers { t.Errorf("claim limit = %d, want %d per worker (%d)", cacheMetadataImagesClaimLimit, cacheMetadataImagesClaimPerWorker, cacheMetadataImagesClaimPerWorker*cacheMetadataImagesWorkers) From b3a92ec201301e1d9c22569b1afa5cc42a3b7282 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:36:43 -0400 Subject: [PATCH 4/5] fix(metadata): take the tightest of all detected memory bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: detection preferred GOMEMLIMIT outright, so a GOMEMLIMIT set looser than a tight cgroup limit would size workers past what the container can hold. All sources — GOMEMLIMIT, the effective cgroup limit, host memory — are now consulted and the smallest positive one wins, with the min logic extracted and unit-tested alongside new sub-1GiB sizing cases. The floor of 2 workers is kept deliberately: a sub-1GiB deployment already ran 2 workers before this branch, so the floor preserves the shipped baseline rather than regressing below it. Co-Authored-By: Claude Fable 5 --- .../tasks/cache_metadata_images.go | 30 ++++++++++++------- .../tasks/cache_metadata_images_test.go | 26 +++++++++++++++- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/internal/taskmanager/tasks/cache_metadata_images.go b/internal/taskmanager/tasks/cache_metadata_images.go index 49e2ca8c29..f5c2f9ec1f 100644 --- a/internal/taskmanager/tasks/cache_metadata_images.go +++ b/internal/taskmanager/tasks/cache_metadata_images.go @@ -56,21 +56,29 @@ func imageCacheWorkerCount(numCPU int, memoryBytes int64) int { // detectImageCacheMemoryBytes returns the tightest memory bound the process // can see, or 0 when none is detectable (macOS dev boxes, bare Linux without -// cgroups): an explicit GOMEMLIMIT first, then the tightest cgroup limit in -// force on this process — own cgroup, ancestors, and root, so a systemd +// cgroups). Every source is consulted and the smallest wins, because none +// implies the others: GOMEMLIMIT can be set looser than a cgroup limit, and +// the effective cgroup limit — own cgroup, ancestors, and root, so a systemd // MemoryMax= or an inherited pod/slice limit binds, not just a namespaced -// container's root files — then total system memory. +// container's root files — can sit above or below host memory. func detectImageCacheMemoryBytes() int64 { - if limit := debug.SetMemoryLimit(-1); limit > 0 && limit < math.MaxInt64 { - return limit + goLimit := int64(0) + if limit := debug.SetMemoryLimit(-1); limit < math.MaxInt64 { + goLimit = limit } - if limit := nodemetrics.EffectiveMemoryLimitBytes(); limit > 0 { - return limit - } - if mem, err := nodemetrics.ReadMeminfoTotalBytes("/proc/meminfo"); err == nil && mem > 0 { - return mem + hostTotal, _ := nodemetrics.ReadMeminfoTotalBytes("/proc/meminfo") + return tightestMemoryLimit(goLimit, nodemetrics.EffectiveMemoryLimitBytes(), hostTotal) +} + +// tightestMemoryLimit returns the smallest positive limit, or 0 when none is. +func tightestMemoryLimit(limits ...int64) int64 { + tightest := int64(0) + for _, limit := range limits { + if limit > 0 && (tightest == 0 || limit < tightest) { + tightest = limit + } } - return 0 + return tightest } var cacheMetadataImagesWorkers = imageCacheWorkerCount(runtime.GOMAXPROCS(0), detectImageCacheMemoryBytes()) diff --git a/internal/taskmanager/tasks/cache_metadata_images_test.go b/internal/taskmanager/tasks/cache_metadata_images_test.go index b56f1cc09d..d1166b32ff 100644 --- a/internal/taskmanager/tasks/cache_metadata_images_test.go +++ b/internal/taskmanager/tasks/cache_metadata_images_test.go @@ -344,7 +344,10 @@ func TestImageCacheWorkerCount(t *testing.T) { {numCPU: 16, memory: 0, want: 48}, // cap: more cores must not monopolize providers {numCPU: 64, memory: 0, want: 48}, // A many-core container with a small memory limit is bounded by - // memory, never below the original pool of 2. + // memory, never below the original pool of 2 — a sub-1GiB deployment + // already ran 2 workers before this change, so 2 is the floor, not 1. + {numCPU: 16, memory: 256 << 20, want: 2}, + {numCPU: 16, memory: 512 << 20, want: 2}, {numCPU: 16, memory: 1 * gib, want: 2}, {numCPU: 16, memory: 2 * gib, want: 4}, {numCPU: 16, memory: 8 * gib, want: 16}, @@ -371,3 +374,24 @@ func TestImageCacheWorkerCount(t *testing.T) { t.Errorf("claim limit = %d, want %d per worker (%d)", cacheMetadataImagesClaimLimit, cacheMetadataImagesClaimPerWorker, cacheMetadataImagesClaimPerWorker*cacheMetadataImagesWorkers) } } + +// Conflicting memory bounds resolve to the smallest positive one: GOMEMLIMIT +// can be set looser than the cgroup limit and the cgroup limit can sit above +// host memory, so no single source can be preferred outright. +func TestTightestMemoryLimit(t *testing.T) { + const gib = int64(1) << 30 + cases := []struct { + limits []int64 + want int64 + }{ + {limits: []int64{4 * gib, 2 * gib, 8 * gib}, want: 2 * gib}, // smallest wins + {limits: []int64{0, 2 * gib, 0}, want: 2 * gib}, // zeros mean "no bound", not zero bytes + {limits: []int64{0, 0, 0}, want: 0}, // nothing detectable + {limits: []int64{6 * gib}, want: 6 * gib}, + } + for _, tc := range cases { + if got := tightestMemoryLimit(tc.limits...); got != tc.want { + t.Errorf("tightestMemoryLimit(%v) = %d, want %d", tc.limits, got, tc.want) + } + } +} From f95411b71b5beb2e5afbca863e8ff28aeb900df6 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:55:49 -0400 Subject: [PATCH 5/5] docs(metadata): state that the worker floor overrides the memory budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: below 2x the per-worker budget the floor of 2 wins, which is deliberate — a sub-1GiB deployment ran 2 workers before this sizing existed, so the memory cap never reduces a host below its long-standing baseline. Say so on the function instead of leaving the budget to read as a guarantee. Co-Authored-By: Claude Fable 5 --- internal/taskmanager/tasks/cache_metadata_images.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/taskmanager/tasks/cache_metadata_images.go b/internal/taskmanager/tasks/cache_metadata_images.go index f5c2f9ec1f..5bd72eab82 100644 --- a/internal/taskmanager/tasks/cache_metadata_images.go +++ b/internal/taskmanager/tasks/cache_metadata_images.go @@ -44,8 +44,13 @@ const imageCacheWorkerMemoryBudget = 512 << 20 // memoryBytes, when positive, is the tightest detected memory bound for this // process (GOMEMLIMIT, cgroup limit, or system memory) and caps the pool at // one worker per imageCacheWorkerMemoryBudget so a many-core container with a -// small memory limit cannot be OOM-killed by concurrent decodes. The floor of -// 2 is the pool size this task shipped with. +// small memory limit cannot be OOM-killed by concurrent decodes. +// +// The floor of 2 is the pool size this task shipped with, and it deliberately +// overrides the per-worker budget below 2×imageCacheWorkerMemoryBudget: a +// sub-1GiB deployment ran 2 workers before this sizing existed, so the memory +// cap never reduces such a host below its long-standing baseline. The budget +// is a sizing heuristic for how far to scale up, not a reservation. func imageCacheWorkerCount(numCPU int, memoryBytes int64) int { workers := min(48, 4*max(numCPU, 1)) if memoryBytes > 0 {