fix(storage): fail fast on lock contention instead of hanging to the request timeout#342
fix(storage): fail fast on lock contention instead of hanging to the request timeout#342matthyx wants to merge 2 commits into
Conversation
…request timeout Between v0.0.278 and v0.0.291 several ContainerProfile locking/critical-section changes increased lock-hold time on the per-key MapMutex (pkg/utils/mutex.go). Under load this pushed some Create/Get/Update/Delete calls past the k8s apiserver's outer request timeout (~60s), surfacing as apierrors.NewTimeoutError with RetryAfterSeconds=0 — a status client-go's retry logic cannot act on, since no Retry-After header is emitted without a positive value. node-agent then hot- retried on a plain conflict, producing a retry storm. Bound every entry-acquisition lock/rlock call (CreateWithConn, DeleteWithConn, GetWithConn, get()'s noLock RLock, GuaranteedUpdateWithConn, appendGobObjectFromFile) to a hardcoded 5s child context and return apierrors.NewServerTimeout(..., retryAfterSeconds=1) instead. This sets Details.RetryAfterSeconds > 0, so the apiserver emits a Retry-After header that client-go's retry/backoff logic honors — verified identical to a 504 for retry-eligibility purposes (both are >= 500). A contended request now fails in ~5s with an actionable backoff signal instead of hanging ~60s with none. Migration-path lock re-acquisition sites (inside get()'s gob-migration retry, and appendGobObjectFromFile's write-lock upgrade/restore) are deliberately left on the unbounded ctx with their existing error returns: bounding a lock-restore path risks leaving lock accounting unmatched, and correctness beats fail-fast there. Adds docs/features/containerprofile-locking-hardening.md documenting the new HTTP 500 ServerTimeout + Retry-After:1 contract, per repo docs policy for behavior-visible changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
added
the
ai-assisted
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a hardcoded 5-second lock acquisition timeout and a new ChangesLock timeout hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StorageImpl
participant MapMutex
Client->>StorageImpl: Get/Create/Delete/Update request
StorageImpl->>StorageImpl: context.WithTimeout(ctx, lockTimeout)
StorageImpl->>MapMutex: Lock/RLock(lockCtx, key)
alt lock acquired within timeout
MapMutex-->>StorageImpl: lock granted
StorageImpl-->>Client: proceed with operation
else timeout exceeded
MapMutex-->>StorageImpl: context deadline exceeded
StorageImpl->>StorageImpl: newLockTimeoutError(op, key, err)
StorageImpl-->>Client: 500 ServerTimeout, Retry-After: 1
end
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the file-backed storage implementation by bounding lock acquisition to a short backstop timeout and returning a Kubernetes ServerTimeout error with Retry-After so clients can back off instead of hanging until the full apiserver request deadline.
Changes:
- Add a package-level
lockTimeoutand wrap key lock/RLock acquisitions withcontext.WithTimeout(..., lockTimeout)to fail fast under contention. - Introduce
newLockTimeoutErrorreturningapierrors.NewServerTimeout(..., retryAfterSeconds=1)and use it across CRUD/list lock acquisition sites. - Add unit tests validating the new error shape and that lock contention produces
ServerTimeoutwithRetryAfterSeconds=1, plus documentation for the new behavior/contract.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/registry/file/storage.go | Adds a lock-acquisition timeout backstop and returns ServerTimeout + Retry-After on contention. |
| pkg/registry/file/storage_test.go | Adds unit tests covering the new timeout error and contention behavior. |
| docs/features/containerprofile-locking-hardening.md | Documents the new fail-fast lock acquisition behavior and client-facing HTTP semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newLockTimeoutError(op, key string, err error) *apierrors.StatusError { | ||
| logger.L().Debug("lock acquisition timed out", | ||
| helpers.String("op", op), helpers.String("key", key), helpers.Error(err)) | ||
| return apierrors.NewServerTimeout( | ||
| schema.GroupResource{Group: "spdx.softwarecomposition.kubescape.io", Resource: resourceFromKey(key)}, | ||
| op, 1) | ||
| } |
| _, spanLock := otel.Tracer("").Start(ctx, "waiting for lock") | ||
| beforeLock := time.Now() | ||
| err := s.locks.Lock(ctx, key) | ||
| lockCtx, lockCancel := context.WithTimeout(ctx, lockTimeout) | ||
| defer lockCancel() | ||
| err := s.locks.Lock(lockCtx, key) |
| _, spanLock := otel.Tracer("").Start(ctx, "waiting for lock") | ||
| beforeLock := time.Now() | ||
| err := s.locks.Lock(ctx, key) | ||
| lockCtx, lockCancel := context.WithTimeout(ctx, lockTimeout) | ||
| defer lockCancel() | ||
| err := s.locks.Lock(lockCtx, key) |
| _, spanLock := otel.Tracer("").Start(ctx, "waiting for lock") | ||
| beforeLock := time.Now() | ||
| err := s.locks.RLock(ctx, key) | ||
| lockCtx, lockCancel := context.WithTimeout(ctx, lockTimeout) | ||
| defer lockCancel() | ||
| err := s.locks.RLock(lockCtx, key) |
| _, spanLock := otel.Tracer("").Start(ctx, "waiting for lock") | ||
| beforeLock := time.Now() | ||
| err := s.locks.Lock(ctx, key) | ||
| lockCtx, lockCancel := context.WithTimeout(ctx, lockTimeout) | ||
| defer lockCancel() | ||
| err := s.locks.Lock(lockCtx, key) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/registry/file/storage_test.go (1)
711-740: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGlobal
lockTimeoutmutation is not synchronized.The test mutates the package-level
lockTimeoutvar (Line 713) and restores it viadefer(Line 714). SincelockTimeoutis read directly (unguarded) by other lock-acquisition call sites instorage.go, any future test in this package that runs concurrently witht.Parallel()could race on this variable under-race. Currently safe since Go runs same-package tests sequentially absentt.Parallel(), but it's worth a comment ort.Setenv-style guard to prevent someone unknowingly parallelizing this suite later.🤖 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/registry/file/storage_test.go` around lines 711 - 740, The test mutates the package-level lockTimeout global in TestStorageImpl_LockContentionReturnsServerTimeout, which can race with other lock-acquisition paths if this suite is ever parallelized. Update the test to avoid unsynchronized global state by using a test-local override pattern or add a clear guard/comment around the lockTimeout change and restore in the same test scope. Reference the lockTimeout variable, the TestStorageImpl_LockContentionReturnsServerTimeout test, and the StorageImpl lock usage so it is easy to locate and keep the suite safe for future t.Parallel() usage.
🤖 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.
Nitpick comments:
In `@pkg/registry/file/storage_test.go`:
- Around line 711-740: The test mutates the package-level lockTimeout global in
TestStorageImpl_LockContentionReturnsServerTimeout, which can race with other
lock-acquisition paths if this suite is ever parallelized. Update the test to
avoid unsynchronized global state by using a test-local override pattern or add
a clear guard/comment around the lockTimeout change and restore in the same test
scope. Reference the lockTimeout variable, the
TestStorageImpl_LockContentionReturnsServerTimeout test, and the StorageImpl
lock usage so it is easy to locate and keep the suite safe for future
t.Parallel() usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38070755-faeb-45c2-b983-7c602735d048
📒 Files selected for processing (3)
docs/features/containerprofile-locking-hardening.mdpkg/registry/file/storage.gopkg/registry/file/storage_test.go
|
Summary:
|
Two real bugs and one style nit flagged by Copilot review on PR #342: - spanLock (the "waiting for lock" trace span) was only ended on the success path in CreateWithConn/DeleteWithConn/GetWithConn/GuaranteedUpdateWithConn. On a contended lock that times out, the function returned before spanLock.End() ran, leaving the span open/incomplete -- exactly the observability signal you'd want when diagnosing the contention this PR targets. Move spanLock.End() immediately after the Lock/RLock attempt, on both the success and error path. - newLockTimeoutError always returns ServerTimeout+Retry-After, even when the underlying error is context.Canceled (the caller's own ctx was cancelled, e.g. a client disconnect) rather than lockTimeout's child context expiring. Telling a disconnected client to retry is meaningless and misleading in logs/metrics. Distinguish the two: context.Canceled now returns a plain InternalError with no Retry-After hint; actual lockTimeout expiry keeps the existing ServerTimeout+Retry-After:1 behavior. - Uses the existing softwarecomposition.GroupName constant instead of hardcoding the same literal string, avoiding future drift risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
|
Summary:
|
Summary
CreateWithConn,DeleteWithConn,GetWithConn,get()'s noLock RLock,GuaranteedUpdateWithConn,appendGobObjectFromFile) to a hardcoded 5s child context instead of the outer (~60s) request context.apierrors.NewServerTimeout(..., retryAfterSeconds=1)instead ofapierrors.NewTimeoutError(msg, 0), so the apiserver emits aRetry-Afterheader that client-go's retry logic honors.docs/features/containerprofile-locking-hardening.mddocumenting the new HTTP 500ServerTimeout+Retry-After:1contract.Context
Root-caused via a multi-agent investigation of
armosec/shared-workflowsCI failures (relevantCVEs,network_policy_known_serverstiming out with 504s under load). This is PR 1 of a stack of independently-revertable fixes; see #2/#3/#4 (stacked on this one) for the CollapseConfig cache, consolidator connection scoping, and pool-exhaustion fail-fast.Test plan
go build ./...,go vet,gofmtcleango test ./pkg/registry/file/...and-racevariant passTest_newContentionTimeoutError,TestStorageImpl_LockContentionReturnsServerTimeoutquay.io/matthiasb_1/storage:test, deployed againstarmosec/shared-workflowssystem tests):relevantCVEsnow passes cleanly (was failing with 504s)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Documentation