fix(containerprofilemanager): repair timestamp chain on LRU eviction and retry exhaustion - #884
Conversation
…xAttempts exhaustion (kubescape#871) enforceMaxSize and the MaxAttempts drop path discarded a queued chunk with no replacement, forking the container's report-timestamp chain and hanging the profile in Learning forever with no field-visible symptom. Both paths now go through the same stitch-repair mechanism kubescape#866 introduced for its own split-chunk drops: MaxAttempts exhaustion routes through dropChunk, and enforceMaxSize replaces an evicted item with a stitch instead of discarding it. Since replacing every eviction with a same-count stitch makes no net progress toward MaxQueueSize by itself, enforceMaxSize bounds the in-flight stitch backlog (10% of MaxQueueSize, floor 1); once exhausted, further evictions fall back to the original unrepaired drop rather than let the queue grow without bound or convert itself entirely into stitches in one call. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com>
📝 WalkthroughWalkthroughThe queue repairs report chains after retry exhaustion and LRU eviction by creating bounded stitch replacements. It tracks stitch backlog usage, records dedicated drop reasons, exposes backlog statistics, and adds tests and documentation for repair behavior and fallback limits. ChangesContainer profile queue repair
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Queue
participant enforceMaxSize
participant newStitchFor
participant dropChunk
Queue->>enforceMaxSize: evict queued chunk
enforceMaxSize->>newStitchFor: create replacement stitch
enforceMaxSize->>Queue: enqueue replacement when backlog permits
Queue->>dropChunk: route retry-exhausted or unrepairable chunk
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/containerprofilemanager/v1/queue/containerprofile_queue.go (1)
665-669: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce
maxStitchBacklogfor all replacement stitches.
dropChunkdequeues its source item before this enqueue. If resident stitches already equalmaxStitchBacklog, the queue is belowmaxQueueSize, soenqueueLockeddoes not callenforceMaxSize. This path then incrementsstitchBacklogabove its configured bound. The lost-split path has the same bypass.
pkg/containerprofilemanager/v1/queue/containerprofile_queue.go#L665-L669: check backlog capacity before enqueuing the replacement stitch. If full, record a suitable fallback drop reason and leave the chain forked.pkg/containerprofilemanager/v1/queue/containerprofile_queue.go#L606-L607: use the same guarded stitch-admission path.- Add a test that exhausts retries while the resident stitch backlog is already full.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/containerprofilemanager/v1/queue/containerprofile_queue.go` around lines 665 - 669, Enforce maxStitchBacklog before admitting replacement stitches: update the dropChunk path around qd.newStitchFor(dropped) and the lost-split path around the sibling site at pkg/containerprofilemanager/v1/queue/containerprofile_queue.go:606-607 to use the same guarded stitch-admission logic. When backlog is full, record the appropriate fallback drop reason, leave the chain forked, and do not enqueue or increment stitchBacklog; add a test covering exhausted retries with the resident stitch backlog already full.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/container-profile-split-on-413.md`:
- Line 129: Update the documentation around dropReasonStitchRejected to state
that stitch rejection is not the only drop path leaving the report chain forked;
qualify the LRU eviction repair statement to explicitly exempt
dropReasonLRUBacklogExhausted, while preserving the existing MaxAttempts repair
behavior.
---
Outside diff comments:
In `@pkg/containerprofilemanager/v1/queue/containerprofile_queue.go`:
- Around line 665-669: Enforce maxStitchBacklog before admitting replacement
stitches: update the dropChunk path around qd.newStitchFor(dropped) and the
lost-split path around the sibling site at
pkg/containerprofilemanager/v1/queue/containerprofile_queue.go:606-607 to use
the same guarded stitch-admission logic. When backlog is full, record the
appropriate fallback drop reason, leave the chain forked, and do not enqueue or
increment stitchBacklog; add a test covering exhausted retries with the resident
stitch backlog already full.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6607dd0-aee4-4d61-9aac-673c909dd9e0
📒 Files selected for processing (4)
docs/features/container-profile-split-on-413.mdpkg/containerprofilemanager/v1/queue/containerprofile_queue.gopkg/containerprofilemanager/v1/queue/containerprofile_queue_errors_test.gopkg/containerprofilemanager/v1/queue/containerprofile_queue_test.go
…eSplit too enforceMaxSize checked the in-flight stitch backlog before repairing an eviction, but dropChunk and requeueSplit's lost-first-half case built and enqueued their own replacement stitch unconditionally. Left unfixed, a sustained run of MaxAttempts exhaustions or unsplittable drops could grow the backlog past the bound the eviction loop itself relies on to terminate promptly, since only enforceMaxSize's own path was gated. Factor the check into stitchBacklogFull and call it from all three stitch-admission sites; once the backlog is spent, dropChunk and requeueSplit now fall back to the same unrepaired, forked drop (dropReasonStitchBacklogExhausted) enforceMaxSize already used. Also tightens the docs page's claim that dropReasonStitchRejected is the only drop path that can leave the chain forked - dropReasonLRUBacklogExhausted already qualified that, and now dropReasonStitchBacklogExhausted does too. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com>
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Review
Verified locally on ad06ebf (fresh clone of the PR head):
go build ./... ok
go vet ./pkg/containerprofilemanager/... ok
go test ./pkg/containerprofilemanager/... -race ok (4.99s)
The direction is right and the newStitchFor extraction is a faithful refactor — the struct literals it replaces were byte-for-byte identical at all three sites. The docs update is thorough and CodeRabbit's one actionable comment is addressed.
I'm not approving yet: two of the findings below are defects against invariants this PR itself states, and both have small, mechanical fixes. The third is a design question that decides whether the MaxAttempts half of this fix actually does anything in the scenario it targets.
🔴 Blocker 1 — stitchBacklog drifts negative across a restart, permanently loosening the bound
Reproduced (probe test, since removed):
run1: size=10 stitchBacklog=1 # one eviction repaired with a stitch
--- restart (same queue dir) ---
run2 after processAllItems: stitchBacklog=-1 # bound=1
CONFIRMED: effective budget is now 2 instead of 1
The counter starts at 0 every process start, but the disk queue can already hold stitches from the previous run. Every one of those decrements the counter as it leaves (:387 in enforceMaxSize, :489 in processAllItems), driving it negative — and nothing ever pulls it back except enqueueing an equal number of fresh stitches. If the persisted stitch succeeds on its first try, the deficit is permanent for the life of the process.
The field comment says the worst case is "a brief burst of extra replacements right after a restart before it settles." It doesn't settle on its own, and the burst isn't bounded by anything small: the drift is bounded by the number of stitches resident at startup, i.e. up to maxQueueSize. At the defaults (MaxQueueSize=1000, maxStitchBacklog=100), a process that starts after a heavy-overload run can end up with an effective budget near 1100 — which is precisely the pathology enforceMaxSize's own doc comment says the bound exists to prevent: "a single call here walk[ing] through and convert[ing] the whole queue before ever making room for the new item that triggered it" — 1000 disk enqueues in one enforceMaxSize call, all under qd.mu.
Cheapest fix is to make the release monotonic at the floor:
// releaseStitch decrements the resident-stitch count without letting it go below zero:
// the counter starts at zero each process start while the disk queue may already hold
// stitches from a prior run, and an unclamped decrement would leave a permanent deficit
// that silently widens maxStitchBacklog.
func (qd *QueueData) releaseStitch() {
for {
cur := qd.stitchBacklog.Load()
if cur <= 0 {
return
}
if qd.stitchBacklog.CompareAndSwap(cur, cur-1) {
return
}
}
}...called from both decrement sites, plus a test that reopens a queue over a persisted stitch and asserts stitchBacklog >= 0. (Reconciling the real count at startup would be more accurate, but you already note dque can't count a predicate cheaply — clamping is enough to keep the bound honest.)
🔴 Blocker 2 — chunksDropped counts one chunk twice on the backlog-exhausted and enqueue-failed paths
Reproduced: a single dropChunk call with the backlog at its bound yields
chunksDropped=2, reasons=[max-attempts-exhausted stitch-backlog-exhausted]
dropChunk increments at :686 for the drop itself, then again at :699 when the repair is refused. Same shape at :620+:629 (requeueSplit) and :407+:418 (enforceMaxSize's failed enqueue). Meanwhile enforceMaxSize's own backlog-full path at :396-404 increments exactly once for the same logical event.
So chunksDropped is not a count of dropped chunks — it over-reports by up to 2× exactly when things are going worst, and it's internally inconsistent between two paths that describe the same outcome. The drop-reason counter has the same problem: sum(chunk_dropped_total) by (reason) no longer equals chunks dropped, so any dashboard or alert built on it silently double-counts overload.
Two coherent options — either is fine, but please pick one and apply it at all four sites:
- One increment per chunk. Keep the double
ReportContainerProfileChunkDropped(the two labels do carry distinct information: why dropped and why not repaired) but incrementchunksDroppedonly once. Then document on the metric that a chunk emits one or two reason samples, so nobody sums them as a chunk count. - Separate the concerns. Keep
chunksDroppedas a pure chunk count and move "repair refused" onto its own counter/metric (stitchesRefused), so both series stay individually summable.
TestDropChunk_StitchBacklogExhaustedForksWithoutReplacement currently asserts only Contains on the reasons, so it passes under either — worth tightening it to pin the counter value once you've decided.
🟠 Discuss — the MaxAttempts repair stitch inherits an exhausted budget, so it gets exactly one try
newStitchFor copies Attempts: dropped.Attempts. Coming from dropReasonMaxAttemptsExhausted that value is already >= maxAttempts, so the stitch is attempted once, fails, and is dropped un-restitched. Your own test pins this: assert.Equal(t, maxAttempts+1, creator.callCount(), "the original gets its full budget, the stitch exactly one try").
Now walk the motivating scenario from #871: storage is unreachable, an item burns 360 retries over ~30 minutes, dropChunk fires, the stitch is tried once ~5s later against the same unreachable storage, fails, and the chain forks anyway. For a sustained outage — the case the retry budget exists to survive — the MaxAttempts half of this fix changes nothing but the log line. It does genuinely help the other case (an item-specific persistent rejection, where a metadata-only stitch succeeds immediately), and that's worth having; I just don't think the PR description's framing holds for the outage case.
Inheriting Attempts is clearly correct for the 413 lineage — it's what bounds the split/stitch cascade. For the retry-exhaustion path the trade looks different: a fresh budget is still bounded (a stitch is never re-stitched, so total exposure is 30 min + 30 min, not unbounded), and a stitch is a couple of KB. The cost is queue slots, not bytes — which is real, but it's the same cost you already accepted by enqueueing the stitch at all.
If you keep the current behaviour, could you say so explicitly in the dropReasonMaxAttemptsExhausted comment and in the docs' "Known limitations" — something to the effect that the repair is best-effort and does not survive the outage that caused the drop? Right now the docs read as though this path is fixed outright.
🟡 Minor
-
enqueueStitchNoEvict's lock-discipline claim is wrong (:313): "enforceMaxSize always runs under qd.mu (it's only ever called from enqueueLocked, whose callers must already hold the lock)".NewQueueData:250callsenforceMaxSize()directly, without the lock. It's safe there — no other goroutine exists yet — but this is a concurrency-critical file and the comment asserts an invariant the code doesn't hold. Please either note the constructor exception or move the startup sweep under the lock. -
A restart at capacity now sheds two real chunks instead of one. Confirmed in the same probe: reopening a full queue took it 10 → 9, evicting one real chunk into a stitch (no net progress) and then forking a second. That startup sweep runs with no new item waiting for room, so the second eviction buys nothing. Pre-fix this cost one delta per restart; it now costs two, every restart, on every node. Worth a line in the docs at minimum — or skipping the repair entirely when
enforceMaxSizeis called outside an enqueue. -
GetQueueStatscan report a negativestitchBacklog— falls out of Blocker 1, and it'll look like a bug to whoever reads the stats endpoint. Fixed by the clamp. -
stitchBacklogFull()is a soft bound indropChunk(:693checks outsideqd.mu,:706enqueues inside). AdropChunkon the processor goroutine can race anenforceMaxSizeunder the lock and overshoot by one. Harmless in practice, but the doc comment reads as a hard invariant — a one-line "advisory, may overshoot by one under concurrent admission" would set expectations correctly.
👍 Confirmed correct
enforceMaxSizeterminates: every iteration either shrinks the queue or spends one unit of a bounded budget, so the loop is bounded atmaxStitchBacklog + 1iterations — modulo Blocker 1 inflating that bound.- I traced
TestQueueLRUEviction's rewritten expectations by hand againstenqueueStitchNoEvictappending at the tail;[stitch(0), profile-3, profile-4]withchunksDropped == 3is right. - The new drop reasons are compile-time constants from a closed set, so Prometheus/OTEL label cardinality stays bounded.
EmptyQueue'sstitchBacklog.Store(0)correctly resets the counter alongside the drain.
Happy to re-review as soon as 1 and 2 are in — they're both small, and I'd merge on your answer to the third.
…, fresh retry budget for MaxAttempts stitches Addresses review findings on the queue-drop-lineage fix: - stitchBacklog started at zero every process start but the disk queue can already hold stitches from a prior run; decrementing for one of those (which never incremented the fresh counter) drove it negative permanently, silently widening maxStitchBacklog by up to maxQueueSize. releaseStitch clamps the decrement at zero. - chunksDropped was incremented twice for a single lost chunk whenever its repair also failed (dropChunk, requeueSplit's lost-first-half case, and enforceMaxSize's failed stitch enqueue all double-counted). Each site now increments the counter once per chunk and reports both drop reasons as separate metric samples instead. - A stitch repairing a MaxAttempts-exhaustion drop inherited the parent's already-exhausted Attempts, so it got exactly one try - a no-op against a storage outage that hasn't recovered. dropChunk now gives that stitch a fresh retry budget (newStitchFor's freshAttempts); a stitch is never re-stitched, so total exposure stays bounded at two retry windows. - dropChunk, requeueSplit's lost-first-half stitch, and enforceMaxSize's own eviction loop now all gate on stitchBacklogFull before admitting a stitch, not just enforceMaxSize's loop. - NewQueueData's startup enforceMaxSize sweep now runs under qd.mu, matching the invariant enqueueStitchNoEvict's doc comment already claimed. - Documented two residual trade-offs: the MaxAttempts repair is best-effort under a sustained outage, and a restart at capacity can now shed two deltas instead of one. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/container-profile-split-on-413.md`:
- Line 129: Update the stitch-size wording in the documentation to say that
stitches are normally smaller, rather than guaranteeing they cannot exceed the
413 cap. Preserve the existing behavior and explanation of 413 rejection,
backlog eviction, and retry exhaustion.
In `@pkg/containerprofilemanager/v1/queue/containerprofile_queue_errors_test.go`:
- Around line 595-608: The queue must restore the resident stitch count when
reopening persisted data so new admissions remain within maxStitchBacklog. In
pkg/containerprofilemanager/v1/queue/containerprofile_queue_errors_test.go:595-608,
replace the zero-baseline assertions with coverage that reopens persisted
stitches, admits new stitches, and verifies the total remains bounded. In
docs/features/container-profile-split-on-413.md:209-213, retain the all-path
bound claim only if startup restoration is implemented; otherwise document the
restart limitation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d22c794c-f8b6-46ce-86c5-ffed307cc76a
📒 Files selected for processing (3)
docs/features/container-profile-split-on-413.mdpkg/containerprofilemanager/v1/queue/containerprofile_queue.gopkg/containerprofilemanager/v1/queue/containerprofile_queue_errors_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/containerprofilemanager/v1/queue/containerprofile_queue.go
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-review of 39bcdf8 — all findings addressed, approving ✅
Re-ran everything on the new head:
go build ./... ok
go vet ./pkg/containerprofilemanager/... ok
go test ./pkg/containerprofilemanager/... -race ok (4.96s)
I re-ran my two original repro probes unchanged against the new code rather than reading the commit message, and added two more to check the fixes didn't just move the problem.
Blocker 1 — stitchBacklog negative drift → fixed
Same probe as before, same restart-over-a-persisted-stitch setup:
before (ad06ebf): stitchBacklog=-1 bound=1 → effective budget 2
after (39bcdf8): stitchBacklog=0 bound=1 → full=false, budget intact
releaseStitch's CAS loop is correct — the cur <= 0 early return means a lost race retries against a fresh load, and the loop can't spin indefinitely since every competing operation makes progress.
I also checked the direction of the residual error, since clamping trades one inaccuracy for another. Interleaving untracked (prior-run) releases with tracked enqueues, the counter never exceeded the true resident count — so it can under-count and permit an extra repair, but it can never over-count and refuse one. That matches what the new field comment claims, and it's the safe direction: the failure mode is a slightly generous budget for a bounded window, not a silently unrepaired chain.
Blocker 2 — chunksDropped double-count → fixed
before (ad06ebf): one dropChunk → chunksDropped=2
after (39bcdf8): one dropChunk → chunksDropped=1, reasons=[max-attempts-exhausted stitch-backlog-exhausted]
Option 1, which is the one I'd have picked: one increment per lost chunk, both reason labels retained since they answer different questions. I audited all five increment sites in the final file — :455, :465, :682, :716, :746 — and every follow-up "repair also failed" branch (:478, :691, :703, :762, :781) now reports a reason without touching the counter. The rule holds uniformly, including on the path that was already correct.
Tightening the assertions from Contains to exact-slice equality in TestDropChunk_StitchBacklogExhaustedForksWithoutReplacement was the right call — that test would otherwise have passed under either scheme.
Discussion item — fresh retry budget → adopted, with a consequence worth naming
newStitchFor(dropped, freshAttempts) with dropChunk passing reason == dropReasonMaxAttemptsExhausted is exactly the narrow application I was hoping for: the 413/split lineage keeps inheriting Attempts (which is what bounds the split/stitch cascade), and only the retry-exhaustion repair gets a fresh window. Since a stitch is never re-stitched, total exposure is two retry windows — I confirmed that bound holds by draining a queue against a permanently-failing creator and seeing exactly N+1 drops for N originals, never N+2.
One behavioural consequence to be aware of, not a blocker: because a fresh-budget stitch now occupies a backlog slot for a full retry window (~30 min at defaults) instead of ~5s, maxStitchBacklog has become a cap on how many chains can be repaired concurrently during a sustained outage — roughly 100 at defaults. Probed with maxStitchBacklog=1 and 5 dropping containers: exactly one got a repair stitch, the other four reported stitch-backlog-exhausted.
I think that's the right trade — the repairs it displaces were the one-shot kind that couldn't have succeeded anyway — and it's bounded at 10% of the queue and directly observable through the drop-reason metric. Just worth knowing that during a long outage the stitch-backlog-exhausted counter is expected to be non-zero and isn't by itself a sign of a bug.
Minors — all four addressed
NewQueueDatanow takesqd.muaround the startup sweep, soenqueueStitchNoEvict's invariant is true as written rather than documented-around. Good call taking the lock instead of carving out an exception.- The two-deltas-per-restart-at-capacity cost is now documented in Known limitations, and the doc correctly notes that exactly-
MaxQueueSizealready triggered an eviction pre-change. - Negative
stitchBacklogcan no longer surface inGetQueueStats; the new test asserts it via the stats map, which is the right place to pin it. stitchBacklogFull's advisory nature is now stated on the function.
Also checked, no issues
- No deadlock from the new
NewQueueDatalocking —enforceMaxSizereaches onlyenqueueStitchNoEvict, which doesn't takeqd.mu. - A stitch that exhausts its own fresh budget still hits the
dropped.IsStitchearly return indropChunk, so it's dropped un-restitched; no loop. - Queue-occupancy cost of longer-lived stitches stays capped at 10% of
MaxQueueSizeby the existing bound. requeueSplit's second-half-failure path was already single-counting and was correctly left alone.
Nice work — the fixes are minimal, the reasoning is in the comments rather than the commit message, and each one landed with a test that would actually catch the regression. LGTM.
…and retry exhaustion (kubescape#884) * fix(containerprofile): repair the report chain on LRU eviction and MaxAttempts exhaustion (kubescape#871) enforceMaxSize and the MaxAttempts drop path discarded a queued chunk with no replacement, forking the container's report-timestamp chain and hanging the profile in Learning forever with no field-visible symptom. Both paths now go through the same stitch-repair mechanism kubescape#866 introduced for its own split-chunk drops: MaxAttempts exhaustion routes through dropChunk, and enforceMaxSize replaces an evicted item with a stitch instead of discarding it. Since replacing every eviction with a same-count stitch makes no net progress toward MaxQueueSize by itself, enforceMaxSize bounds the in-flight stitch backlog (10% of MaxQueueSize, floor 1); once exhausted, further evictions fall back to the original unrepaired drop rather than let the queue grow without bound or convert itself entirely into stitches in one call. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> * fix(containerprofile): bound stitch admission in dropChunk and requeueSplit too enforceMaxSize checked the in-flight stitch backlog before repairing an eviction, but dropChunk and requeueSplit's lost-first-half case built and enqueued their own replacement stitch unconditionally. Left unfixed, a sustained run of MaxAttempts exhaustions or unsplittable drops could grow the backlog past the bound the eviction loop itself relies on to terminate promptly, since only enforceMaxSize's own path was gated. Factor the check into stitchBacklogFull and call it from all three stitch-admission sites; once the backlog is spent, dropChunk and requeueSplit now fall back to the same unrepaired, forked drop (dropReasonStitchBacklogExhausted) enforceMaxSize already used. Also tightens the docs page's claim that dropReasonStitchRejected is the only drop path that can leave the chain forked - dropReasonLRUBacklogExhausted already qualified that, and now dropReasonStitchBacklogExhausted does too. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> * fix(containerprofile): clamp stitch backlog, fix double-counted drops, fresh retry budget for MaxAttempts stitches Addresses review findings on the queue-drop-lineage fix: - stitchBacklog started at zero every process start but the disk queue can already hold stitches from a prior run; decrementing for one of those (which never incremented the fresh counter) drove it negative permanently, silently widening maxStitchBacklog by up to maxQueueSize. releaseStitch clamps the decrement at zero. - chunksDropped was incremented twice for a single lost chunk whenever its repair also failed (dropChunk, requeueSplit's lost-first-half case, and enforceMaxSize's failed stitch enqueue all double-counted). Each site now increments the counter once per chunk and reports both drop reasons as separate metric samples instead. - A stitch repairing a MaxAttempts-exhaustion drop inherited the parent's already-exhausted Attempts, so it got exactly one try - a no-op against a storage outage that hasn't recovered. dropChunk now gives that stitch a fresh retry budget (newStitchFor's freshAttempts); a stitch is never re-stitched, so total exposure stays bounded at two retry windows. - dropChunk, requeueSplit's lost-first-half stitch, and enforceMaxSize's own eviction loop now all gate on stitchBacklogFull before admitting a stitch, not just enforceMaxSize's loop. - NewQueueData's startup enforceMaxSize sweep now runs under qd.mu, matching the invariant enqueueStitchNoEvict's doc comment already claimed. - Documented two residual trade-offs: the MaxAttempts repair is best-effort under a sustained outage, and a restart at capacity can now shed two deltas instead of one. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> --------- Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> Signed-off-by: entlein <einentlein@gmail.com>
2 participants
Overview
enforceMaxSize(LRU eviction) andMaxAttemptsretry exhaustion dropped queued chunks without replacement, breaking the container's(previousReportTimestamp, reportTimestamp)chain. Storage requires an unbroken chain tracing back to the container's first report to complete a profile; dropping a chunk caused the affected container to remain stuck inLearningmode indefinitely with no visible errors.MaxAttemptsexhaustion triggersdropChunk(dropReasonMaxAttemptsExhausted), and LRU evictions inenforceMaxSizeenqueue a metadata-only stitch chunk. Stitch creation is bounded to 10% ofMaxQueueSize(maxStitchBacklogFor) to prevent infinite queue growth during eviction, falling back todropReasonLRUBacklogExhaustedwhen full.Additional Information
dropChunk,requeueSplit, andenforceMaxSizeinto a sharednewStitchForhelper function.dropReasonMaxAttemptsExhausted,dropReasonLRUEvicted,dropReasonLRUBacklogExhausted) and exposedstitchBacklog/maxStitchBackloginGetQueueStats.docs/features/container-profile-split-on-413.mdto reflect the resolution of this known limitation.How to Test
Run the full package validation and race detection test suite:
go build ./... go vet ./pkg/containerprofilemanager/... go test ./pkg/containerprofilemanager/... -raceCovered by existing rewritten tests (
TestQueueLRUEviction,TestQueueDropsProfileAfterMaxAttempts) and new unit tests (TestEnforceMaxSize_EvictionEnqueuesStitch,TestEnforceMaxSize_BacklogBoundsRepairCost).Examples/Screenshots
N/A (backend queue and timestamp continuity repair)
Related issues/PRs:
Checklist before requesting a review
Please open the PR against the
devbranch (Unless the PR contains only documentation changes)Summary by CodeRabbit
Bug Fixes
Documentation