fix(storage): stop pinning one connection for the whole ContainerProfile consolidation pass#344
Open
matthyx wants to merge 1 commit into
Open
Conversation
matthyx
added
the
ai-assisted
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
5 tasks
There was a problem hiding this comment.
Pull request overview
This PR reduces SQLite pool contention during the ContainerProfile time-series consolidation pass by scoping pooled connections more narrowly and processing keys concurrently with a bounded worker pool, while also ensuring key de-duplication and per-key failure isolation.
Changes:
- Make SQLite pool sizing explicit via
DefaultPoolSizeand use it as the default inNewPooland the main wiring. - Update
ConsolidateTimeSeriesto (1) list keys using a short-lived connection, (2) de-duplicate keys with expired precedence, and (3) process keys concurrently usingerrgroup.Groupwith a configured worker limit. - Add concurrency-focused tests for deadlock avoidance, exactly-once de-duplication, and error isolation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/file/sqlite.go | Introduces DefaultPoolSize and applies it when pool size is unset. |
| pkg/registry/file/containerprofile_processor.go | Implements de-duplicated, bounded-concurrency consolidation with per-key connections. |
| pkg/registry/file/containerprofile_processor_test.go | Adds concurrency, de-dup, and error-isolation tests for the new consolidator behavior. |
| main.go | Wires the pool size explicitly using file.DefaultPoolSize. |
| go.mod | Promotes golang.org/x/sync to a direct dependency for errgroup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
30
to
33
| // NewPool creates a new SQLite connection pool at the given path. | ||
| // It returns an error if the connection cannot be opened or the database cannot be initialized. | ||
| // It is your responsibility to call conn.Close() when you no longer need conn. | ||
| func NewPool(path string, size int) *sqlitemigration.Pool { |
Comment on lines
+310
to
+312
| // in both lists. Expired precedence: a key present in both lists (or multiple | ||
| // times within either list) is processed EXACTLY ONCE, as expired — preserving | ||
| // today's expired-first semantics and avoiding two workers racing one key. |
|
Summary:
|
matthyx
force-pushed
the
fix/collapseconfig-ttl-cache
branch
from
e4149ee to
6abe45d
Compare
…ile consolidation pass ConsolidateTimeSeries held a single pooled *sqlite.Conn (via WithConnection) for its entire serial pass over every active time-series key. Under load this starved REST Create/Get/Update calls competing for the same connection pool (default size 10, shared between REST and the consolidator), an independent path to the same request-timeout symptom the other two commits address. Scope the outer connection to just the two listing calls (ListTimeSeriesExpired /ListTimeSeriesWithData), then give each key its own connection inside consolidateKeyTimeSeries and dispatch keys through a bounded worker pool (errgroup.Group + SetLimit(workers), workers = max(1, DefaultPoolSize/4) = 2), reserving the rest of the pool for REST traffic. Two correctness requirements surfaced during design review and are implemented here: - De-duplication: ListTimeSeriesExpired/ListTimeSeriesWithData have no DISTINCT/GROUP BY and the same storage key can appear multiple times within one list and across both lists. The sequential loop tolerated this by accident; concurrent workers would not. Union both lists into one de-duplicated work set keyed by storage key with expired-precedence (preserves today's expired-first semantics), so every key is dispatched exactly once. - Error isolation: a plain errgroup.Group (not errgroup.WithContext) is used with the parent ctx passed to every worker. errgroup.WithContext's cancel-on-first-error child context would, via pool.Take's conn.SetInterrupt, roll back every other concurrently-running worker's in-flight transaction on the first key's failure -- a silent regression from the sequential loop's per-key failure isolation. Also makes the connection pool size an explicit, referenceable constant (DefaultPoolSize = 10 in sqlite.go, threaded from main.go) rather than an implicit library default, so the worker bound has a knowable source of truth. golang.org/x/sync is promoted from an indirect to a direct go.mod requirement (already vendored at the pinned version; go.sum/vendor/modules.txt unchanged). The verified lock-ordering invariant (workload-apKey always acquired before member-cpKey reads, never the reverse) and the MapMutex primitive itself are unchanged -- this only changes where connections are acquired and how keys are dispatched, not per-key business logic or its internal lock order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/consolidator-connection-scoping
branch
from
8983b7f to
9546fdf
Compare
|
Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ConsolidateTimeSeriesheld a single pooled*sqlite.Connfor its entire serial pass over every active time-series key, starving RESTCreate/Get/Updatecalls competing for the same connection pool (default size 10, shared between REST and the consolidator).ListTimeSeriesExpired/ListTimeSeriesWithData), then gives each key its own connection insideconsolidateKeyTimeSeriesand dispatches keys through a bounded worker pool (errgroup.Group+SetLimit(workers),workers = max(1, DefaultPoolSize/4) = 2), reserving the rest of the pool for REST traffic.ListTimeSeriesExpired/ListTimeSeriesWithDatahave noDISTINCT/GROUP BYand the same storage key can appear multiple times within one list and across both lists. Keys are unioned into one de-duplicated work set with expired-precedence, so every key is dispatched exactly once.errgroup.Group(noterrgroup.WithContext) with the parentctxpassed to every worker, so one key's failure doesn't roll back every other concurrently-running worker's in-flight transaction (whicherrgroup.WithContext's cancel-on-error semantics would otherwise cause viapool.Take'sconn.SetInterrupt).DefaultPoolSize = 10) rather than an implicit library default.golang.org/x/syncpromoted from indirect to direct ingo.mod(already vendored;go.sum/vendor/modules.txtunchanged).The verified lock-ordering invariant (workload-
apKeyalways acquired before member-cpKeyreads) and theMapMutexprimitive itself are unchanged — this only changes where connections are acquired and how keys are dispatched.Context
Stacked on #343 (CollapseConfig TTL cache). Part of a stack of independently-revertable fixes for the ContainerProfile 504 regression; see #342 for the shared root-cause context and validation methodology.
Test plan
go build ./...,go vet,gofmtcleango test -race ./pkg/registry/file/...passes, including a dedicated concurrency test suite (no-deadlock/isolation over shared-apKeykeys, exactly-once dedup with expired-precedence, error-isolation with N-1 keys committing when one fails)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com