Repository navigation
🐛 runtime: stop unbounded memory growth in dashboard stage tracker - #14699
Conversation
/api/stage-tracker held stageTrackerCacheMu across the whole tracker build (one review-evidence lookup per open PR, each a Glob of the repo evidence directory on NFS: ~40s in the owner's hive). Every webhook delivery and every status publish invalidated that cache through the same mutex, so webhook handlers (~177/min), the governor's publish and every dashboard poll queued behind back-to-back rebuilds that never drained: blocked handler goroutines, connections, threads and heap grew without bound. - invalidation flips a stale flag under the cache mutex and never waits - at most one rebuild runs, outside the mutex; polls during it get the previous body as source=stale, max-age=0 - a waiter with no body honours its request context (503 on disconnect) - the evidence-directory sibling listing is memoised per repo dir (60s) instead of re-read per PR per build - regression tests: invalidation returns during a held build, N polls during a build cost one build and leave no goroutines behind, first waiters join the one build; sibling memo semantics Fixes #14681 Signed-off-by: Andy Anderson <andy@clubanderson.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this hive's repositories have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 77cbb9b. |
Fixes #14681
Root cause
A lock convoy in the dashboard stage tracker, fed by the webhook stream.
src/pkg/dashboard/stage_tracker.gostageTrackerPayloadstageTrackerCacheMufor the entirebuildStageTrackerstage_tracker.gobuildStageTracker→stageTrackerSentinel→github.LoadVerifiedReviewEvidencesrc/pkg/github/review_evidence.goreviewEvidenceDirsfilepath.Glob(<repo-dir>/<pr>.uid*)= a fullReadDirof the repo's evidence directory (thousands of entries, NFSv3)stage_tracker.goinvalidateStageTrackerCacheserver.goUpdateStatusIfFresh) and from every webhook that touched a tracked item (webhook_tracker_refresh.goinvalidateStageTrackerForWebhook, both the sync handler path and the debounced re-read'sapplyWebhookItemSnapshots)Evidence from the live hive (read-only)
GET /api/stage-trackerthrough a port-forward took 40 s to answer (91 KB body); the first attempt timed out at 60 s.hive:3002:POST /api/webhook/github: 1766 (≈177/min), avg upstream time 16 s, 931 × 499 (hub relay gave up at its 30 s timeout). Each one blocked ininvalidateStageTrackerCachebehind a build.GET /api/stage-tracker: 72 × 504, each with nginx retrying the upstream 3 × (60 s each); the SPA re-requests every ~2 s, so the queue of GET handlers never drained.overview export encode failed … broken pipe54 per 70 s — handlers finishing a build after the client/ingress was gone./proc/<hive>/status: Threads 224→278 in 10 min, fds 911→1190,CLOSE_WAITsockets 79→216 (/proc/net/tcp) while RSS grew ~350 MiB/min. With mutex FIFO hand-off, after each 40 s build a webhook invalidator typically runs next, re-emptying the cache, and the next GET rebuilds — so blocked GET handlers, their connections (each pinning a 13 KB request body + bufio + the*StatusPayloadit captured), the builders' transient garbage and the NFS-blocked threads all accumulate, and the rate grows with the number of open PRs / evidence dirs, matching the "worse across today's builds" observation.Fix
invalidateStageTrackerCacheonly flipsstaleand bumps a generation counter under the cache mutex. Webhook handlers and the governor's publish return immediately.Server.stageTrackerBuild). Polls that land during a build get the previous body withsource: stale,X-Hive-Stage-Tracker-Source: stale,max-age=0instead of queueing. A build that was invalidated while running stores its result already marked stale, so the next poll rebuilds from newer status but nobody blocks.r.Context()(503 if the client is gone).reviewEvidenceDirsmemoises the repo directory's.uid*sibling names for 60 s (evidenceSiblingListTTL, bounded to 512 parents) and drops the entry when this process writes a fallback sibling. A build is now oneReadDirper repo plus one stat/read per PR.Tests (new, no
time.Sleep)TestStageTrackerInvalidationNeverWaitsForABuild— holds a build open via an injected build func;invalidateStageTrackerCacheandinvalidateStageTrackerForWebhookmust return.TestStageTrackerBuildsAreSingleFlightAndServeStale— 200 concurrent polls (each preceded by a webhook invalidation) during a held build: all return200 staleat once, exactly one build,runtime.NumGoroutine()returns to baseline; cancelled-context waiter gets 503; post-build staleness and quiet-state cache hits asserted.TestStageTrackerFirstWaitersJoinTheOneBuild— 50 first polls, one build.TestReviewEvidenceDirsMemoisesTheRepoListing— memo hit/TTL/forget/cap semantics with an injected clock.Docs:
api-reference.md(source: stale),operator-reference.md(webhook/publish section). Changelogchangelog.d/fixed-memory-growth-dashboard-stage-tracker.md.Caveat
The heap-retention mechanism is inferred from the convoy (blocked handlers + connections + repeated builds), not yet from a heap profile — #14682 adds the loopback pprof listener and periodic memstats so it can be confirmed on the next deploy (
kubectl port-forward deploy/hive 6060:6060,go tool pprof -top http://127.0.0.1:6060/debug/pprof/heap). Remaining small unbounded maps noted during the audit (automerge engine maps,StageTracker.checks, stageengine anomalies,certCache,HivePushedHeads) are orders of magnitude too small to explain 350 MiB/min and are left for a follow-up.