Conversation
* The desired-release reconciler previously buffered up to 500 candidate
deployment versions per release target, silently skipping any version
beyond that window. Replace the upfront fetch with a keyset-paginated
iter.Seq2 that streams versions newest-first and short-circuits as soon
as FindDeployableVersion picks one, removing the hard cap.
Burst patterns (many RTs of the same deployment reconciling concurrently
after a fanout event) now collapse the first-batch fetch through
singleflight keyed by (deploymentID, hash(pushdown clauses)), so worker
overlap costs one round trip instead of N.
For policies whose versionselector CEL can be statically translated to
SQL, push the predicate into the candidate query via cel2sql. Translatable
shapes (literal eq, in-list, startsWith, boolean composition over the
version variable) AND-join into the WHERE clause so Postgres returns only
plausible candidates instead of the full LIMIT 500. Untranslatable
selectors (referencing environment/resource/deployment, complex CEL,
JSONB metadata access) fall back to the row-by-row evaluator unchanged —
pushdown is a candidate-set narrowing optimization, not a replacement
for runtime evaluation.
Span instrumentation:
- FindDeployableVersion: versions.scanned, version.found, deployment.id
- queryCandidateVersionsBatch: pushdown.applied, pushdown.clauses_count,
cursor.paginated, rows.returned, rows.below_limit
SQL injection safety on the inlined pushdown fragments is verified by
TestTryPushDown_StringEscaping; cel2sql emits Postgres-standard doubled
single-quote escapes for string literals.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates candidate version fetching from eager, buffered loading to lazy, keyset-paginated iteration with optional SQL-level pushdown filters. Dependencies are bumped, a new paginated SQL query and CEL-to-SQL pushdown module are introduced, the ChangesDependency Updates
Lazy Version Iteration with SQL Pushdown
Sequence DiagramsequenceDiagram
participant Reconciler
participant Pushdown as Pushdown Module
participant Getter
participant Database
participant PolicyEval
participant Iterator
Reconciler->>Pushdown: Collect clause translations<br/>from enabled policy selectors
Pushdown-->>Reconciler: []clause strings (deterministic order)
Reconciler->>Getter: IterCandidateVersions<br/>(deploymentID, pushdown clauses)
Getter->>Getter: Hash pushdown clauses
Getter->>Database: First batch: SELECT versions<br/>WHERE deployment_id=? AND (clause AND...)<br/>ORDER BY created_at DESC, id DESC<br/>LIMIT 500
Database-->>Getter: First batch: []*DeploymentVersion
Getter-->>Reconciler: iter.Seq2[*DeploymentVersion]<br/>(yields first batch)
Reconciler->>PolicyEval: FindDeployableVersion<br/>(iterator, policies, evaluators)
PolicyEval->>Iterator: for version := range iter
Iterator->>Reconciler: (next version, nil)
PolicyEval->>PolicyEval: Eval version against policies
alt Version is deployable
PolicyEval-->>Reconciler: Found: DeploymentVersion
else More versions needed
Iterator->>Getter: Next batch needed
Getter->>Database: SELECT versions WHERE<br/>(created_at, id) < (cursor)<br/>LIMIT 500
Database-->>Iterator: []*DeploymentVersion
Iterator-->>PolicyEval: (next version, nil)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (8)
apps/workspace-engine/test/controllers/harness/mocks.go (1)
170-182: 💤 Low valueMock ignores
extraWhere— fine today, but worth a comment so future test authors know.A unit test that wants to verify "policy X with selector
version.tag == 'v1.0.0'filters out other versions" can't do so against this mock — every version ing.Versionsis yielded regardless of pushdown clauses. That's perfectly correct for the existing tests (which test reconcile logic, not the pushdown itself; pushdown has its own tests inpushdown_test.goand the integration-style coverage hits the real Postgres path). But it's a non-obvious limitation.A one-line comment above the function would prevent surprise:
// IterCandidateVersions yields every version in g.Versions in order. It // intentionally ignores extraWhere — pushdown filtering is exercised // against the real Postgres getter; this mock is for testing reconcile // logic given a candidate stream.🤖 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 `@apps/workspace-engine/test/controllers/harness/mocks.go` around lines 170 - 182, Add a one-line comment above the DesiredReleaseGetter.IterCandidateVersions function explaining that this mock intentionally ignores the extraWhere parameter and yields every version in g.Versions in order (i.e., it does not apply pushdown filtering), and note that pushdown filtering is tested against the real Postgres getter (pushdown_test.go) so this mock is only for testing reconcile logic given a candidate stream.apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go (1)
233-248: ⚖️ Poor tradeoffTrust boundary on
extraWhereis documented, but a defensive sentinel would harden it further.The doc at lines 127–130 plus the interface‑level note in
getters.gomake the contract clear: onlyversionselector.TryPushDownoutput should ever flow in here. That's the right call given cel2sql's escaping is what makes pushdown safe. One small belt‑and‑suspenders option: haveTryPushDownreturn a typed wrapper (e.g.type SafeWhereClause string) instead of a barestring, and accept[]SafeWhereClausehere. That makes "I assembled this fragment by hand" a compile error rather than a code-review catch. Optional.Also worth a sanity check at call time: panic-or-skip if a fragment contains a literal
;outside of a quoted run, since cel2sql's translatable shapes never need one. Cheap defense against a future regression in the translator.🤖 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 `@apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go` around lines 233 - 248, The extraWhere loop currently accepts raw strings which relies on external discipline; change the API so TryPushDown (e.g. versionselector.TryPushDown) returns a typed wrapper like type SafeWhereClause string and update this function to accept []SafeWhereClause instead of []string, then iterate over those wrappers when building the SQL (referencing extraWhere and the candidateVersionColumns builder) to make accidental raw fragments a compile-time error; additionally add a cheap runtime sanity-check inside the loop to detect any fragment containing an unquoted semicolon (panic or skip the fragment) to guard against future translator regressions.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go (2)
67-81: 💤 Low valueEmpty selector subtest name is unprintable — give it a label.
t.Run(sel, ...)withsel == ""produces a subtest path likeTestTryPushDown_FailsClosed/which is awkward in-voutput and can collide if more empty-named cases ever get added. Switch the case list to{name, sel}pairs.♻️ Proposed fix
-func TestTryPushDown_FailsClosed(t *testing.T) { - cases := []string{ - `environment.name == "prod"`, - `resource.kind == "Server"`, - `deployment.name == "api"`, - `version.tag == "x" && environment.name == "prod"`, - ``, // empty - } - for _, sel := range cases { - t.Run(sel, func(t *testing.T) { - _, ok := TryPushDown(sel) - assert.False(t, ok, "selector %q must NOT push down", sel) - }) - } -} +func TestTryPushDown_FailsClosed(t *testing.T) { + cases := []struct { + name string + sel string + }{ + {"environment ref", `environment.name == "prod"`}, + {"resource ref", `resource.kind == "Server"`}, + {"deployment ref", `deployment.name == "api"`}, + {"mixed version+environment", `version.tag == "x" && environment.name == "prod"`}, + {"empty", ``}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, ok := TryPushDown(tc.sel) + assert.False(t, ok, "selector %q must NOT push down", tc.sel) + }) + } +}🤖 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 `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go` around lines 67 - 81, The subtest for an empty selector uses t.Run(sel, ...) so the empty name is unprintable; update TestTryPushDown_FailsClosed to use a slice of {name, sel} pairs (or a small struct) instead of []string and change the loop to t.Run(name, func(t *testing.T) { _, ok := TryPushDown(sel); assert.False(t, ok, "selector %q must NOT push down", sel) }), giving the empty case an explicit label like "empty" (or "empty-case") so all subtest names are printable and stable; keep references to TryPushDown and TestTryPushDown_FailsClosed when making the change.
100-113: 💤 Low valueThe
parameterizedheuristic is loose — anchor it to theassert.NotContainsfor actual safety.
parameterized := strings.Contains(clause, "$") && !strings.Contains(clause, "DROP")is fragile: a$can appear inside an escaped Postgres E-string (E'$1') without meaning a placeholder, and a clause that legitimately mentionsDROP(say in a future test fixture) would flip the flag for an unrelated reason.The real guarantee is on Line 111:
assert.NotContains(t, clause, "test'; DROP TABLE"). That's the assertion that catches actual injection. Thesafe := doubled || backslash || parameterizedblock is a positive sanity check that would silently pass on something like'test\'; DROP TABLE...'where the backslash escape isn't actually honored by Postgres' defaultstandard_conforming_strings = on.Tightening suggestion: drop the
parameterizedheuristic, keep only the doubled-quote check (cel2sql's actual behavior — confirmed in their docs which emit'John Doe'-style literals), and rely on theNotContainsfor the hard fail.🤖 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 `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go` around lines 100 - 113, The parameterized/backslash heuristic is too loose — remove the parameterized and backslash checks and make the positive sanity check rely only on the doubled check; specifically, delete or stop using the parameterized and backslash variables, set safe := doubled (using the existing doubled := strings.Contains(clause, `''`)), and leave the assert.NotContains(t, clause, `test'; DROP TABLE`) as the definitive safety check; also update the assert.True message to reflect that only doubled-quote escaping is considered a positive sanity check.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown.go (1)
53-70: 💤 Low valueConsider distinguishing "won't translate" from "translator failed".
Right now compilation errors, conversion errors, and "empty SQL" all collapse into the same
(", false)return. That's correct for the runtime fallback contract, but it makes operational debugging harder when a selector that should push down silently doesn't (e.g. a cel2sql regression after a library bump). A trace span attribute or debug log on the failure paths — recording the selector hash and which step failed — would let you spot capability regressions without changing the public contract.Not a blocker for merge; the silent fallback behavior is intentional. As per coding guidelines: "Write comments that explain why, document complex logic/algorithms, provide non-obvious context".
🤖 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 `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown.go` around lines 53 - 70, The TryPushDown function currently collapses all failures into ("", false), which hides whether compilation (env.Compile), conversion (cel2sql.Convert) or an empty-SQL outcome failed; update TryPushDown to emit a debug/trace record on each failure path (e.g., using your tracing/logging helper) that records the selector identifier (hash of selector) and which step failed (getPushdownEnv error, env.Compile issues, cel2sql.Convert error, or empty SQL) without changing the public return values; reference TryPushDown, getPushdownEnv, env.Compile and cel2sql.Convert when adding the logs/trace attributes so operational teams can detect regressions while preserving the ("", false) contract.apps/workspace-engine/pkg/db/queries/deployment_versions.sql (1)
29-38: Add a composite index to back this keyset query.The query filters on
deployment_id, excludes twostatusvalues, and paginates by(created_at DESC, id DESC)with a tuple‑comparison cursor. Without a matching composite index, Postgres will fall back to a sort over the full deployable subset for every page, which defeats the purpose of moving to keyset pagination — especially under the singleflight burst fan‑out and per‑batch second/third page fetches added ingetters_postgres.go.A partial index on the deployable subset is the tightest fit:
CREATE INDEX deployment_version_deployable_keyset_idx ON deployment_version (deployment_id, created_at DESC, id DESC) WHERE status NOT IN ('rejected', 'building');If you can't use a partial index (e.g.
statusset churns), a covering(deployment_id, status, created_at DESC, id DESC)works too but is bulkier.🤖 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 `@apps/workspace-engine/pkg/db/queries/deployment_versions.sql` around lines 29 - 38, Add a composite index to support the keyset pagination used by the SQL query named ListDeployableVersionsByDeploymentIDAfter on table deployment_version: create a partial index (recommended) named deployment_version_deployable_keyset_idx on (deployment_id, created_at DESC, id DESC) with a WHERE clause excluding status IN ('rejected','building'); if partial indexes aren’t allowed, create a covering index including status (deployment_id, status, created_at DESC, id DESC) instead so the (created_at,id) tuple comparison and ORDER BY can use the index without a full sort.apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go (1)
124-176: ⚡ Quick winSet span attributes via
deferso error paths still recordversions.scanned/version.found.The early returns on lines 143 and 148 (when
GetPolicySkipsorevaluateVersionfails) bypass thespan.SetAttributes(...)block at lines 169-173, so traces for those failure modes lose the deployment id and the scan progress. Moving the attribute set into adeferkeeps the instrumentation contract uniform on all paths (success, iterator error, getter error).♻️ Proposed refactor
_, span := tracer.Start(ctx, "FindDeployableVersion") defer span.End() var earliest *time.Time var allEvaluations []VersionedEvaluation var found *oapi.DeploymentVersion var iterErr error var scanned int + + defer func() { + span.SetAttributes( + attribute.String("deployment.id", rt.DeploymentId), + attribute.Int("versions.scanned", scanned), + attribute.Bool("version.found", found != nil), + ) + }() for version, err := range versions { ... } - span.SetAttributes( - attribute.String("deployment.id", rt.DeploymentId), - attribute.Int("versions.scanned", scanned), - attribute.Bool("version.found", found != nil), - ) if iterErr != nil { return nil, iterErr }🤖 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 `@apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go` around lines 124 - 176, Move the span attribute setting into a deferred closure immediately after creating the span so attributes are always recorded even on early returns: after tracer.Start(ctx, "FindDeployableVersion") add defer func() { span.SetAttributes(attribute.String("deployment.id", rt.DeploymentId), attribute.Int("versions.scanned", scanned), attribute.Bool("version.found", found != nil)) }() (leave span.End() deferred as-is or combine end in the same defer), so errors from GetPolicySkips or evaluateVersion and the iterator error still record deployment id, scanned and found using the existing scanned and found variables.apps/workspace-engine/svc/controllers/desiredrelease/reconcile.go (1)
188-206: ⚡ Quick winUse
slices.Sortinstead of a hand-rolled insertion sort.The "keep the import surface small" rationale doesn't hold —
slicesis part of the Go standard library and is already imported elsewhere in this module (e.g.policyeval.go). The custom O(n²) implementation adds maintenance surface without any benefit, andslices.Sortwill outperform it once a deployment accumulates many translatable selectors.♻️ Proposed refactor
import ( "context" "fmt" + "slices" "time" "github.com/charmbracelet/log" @@ - if len(clauses) > 1 { - // Stable order so the singleflight key is deterministic across - // reconciles that see the same logical clause set. - sortStrings(clauses) - } + // Stable order so the singleflight key is deterministic across + // reconciles that see the same logical clause set. + slices.Sort(clauses) return clauses } - -// sortStrings is an in-place insertion sort. We use it instead of -// sort.Strings to keep the import surface small for this hot path. -func sortStrings(s []string) { - for i := 1; i < len(s); i++ { - j := i - for j > 0 && s[j-1] > s[j] { - s[j-1], s[j] = s[j], s[j-1] - j-- - } - } -}
slices.Sortno-ops on length-0/1 slices, so thelen(clauses) > 1guard isn't needed.🤖 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 `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile.go` around lines 188 - 206, Replace the hand-rolled insertion sort with the standard library implementation: remove the custom sortStrings function and its call, and use slices.Sort(clauses) where clauses is sorted (you can drop the len(clauses) > 1 guard since slices.Sort is a no-op for short slices); update any references to sortStrings accordingly (e.g., the call site in reconcile.go that currently calls sortStrings(clauses)). Ensure the package imports include "slices" (it already exists elsewhere) and remove the unused sortStrings function to avoid dead code.
🤖 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 `@apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go`:
- Around line 161-167: queryCandidateVersionsBatch is already wrapping errors
with fmt.Errorf("list versions for deployment %s: %w", deploymentID, err), so
avoid double-wrapping when calling it from the batch loop: remove the extra
fmt.Errorf wrapper and call yield(nil, err) directly (use the returned err from
queryCandidateVersionsBatch). Ensure the call-site that handles the first-batch
path uses the same strategy (yield the err returned by
queryCandidateVersionsBatch) so error messages are consistent; reference
symbols: queryCandidateVersionsBatch, yield, deploymentID.
- Around line 193-208: fetchFirstBatch uses g.firstBatchSF.Do which runs the
closure in the first caller's goroutine, so when that caller's ctx is cancelled
the shared queryCandidateVersionsBatch call fails for all waiters; change the
closure passed to g.firstBatchSF.Do in fetchFirstBatch to detach from the
caller's cancellation by creating qCtx := context.WithoutCancel(ctx) (and
optionally wrap with a shared timeout via context.WithTimeout and defer
cancel()) and pass qCtx into queryCandidateVersionsBatch instead of the original
ctx so individual caller cancellations don’t abort the shared singleflight work.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/db/queries/deployment_versions.sql`:
- Around line 29-38: Add a composite index to support the keyset pagination used
by the SQL query named ListDeployableVersionsByDeploymentIDAfter on table
deployment_version: create a partial index (recommended) named
deployment_version_deployable_keyset_idx on (deployment_id, created_at DESC, id
DESC) with a WHERE clause excluding status IN ('rejected','building'); if
partial indexes aren’t allowed, create a covering index including status
(deployment_id, status, created_at DESC, id DESC) instead so the (created_at,id)
tuple comparison and ORDER BY can use the index without a full sort.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go`:
- Around line 67-81: The subtest for an empty selector uses t.Run(sel, ...) so
the empty name is unprintable; update TestTryPushDown_FailsClosed to use a slice
of {name, sel} pairs (or a small struct) instead of []string and change the loop
to t.Run(name, func(t *testing.T) { _, ok := TryPushDown(sel); assert.False(t,
ok, "selector %q must NOT push down", sel) }), giving the empty case an explicit
label like "empty" (or "empty-case") so all subtest names are printable and
stable; keep references to TryPushDown and TestTryPushDown_FailsClosed when
making the change.
- Around line 100-113: The parameterized/backslash heuristic is too loose —
remove the parameterized and backslash checks and make the positive sanity check
rely only on the doubled check; specifically, delete or stop using the
parameterized and backslash variables, set safe := doubled (using the existing
doubled := strings.Contains(clause, `''`)), and leave the assert.NotContains(t,
clause, `test'; DROP TABLE`) as the definitive safety check; also update the
assert.True message to reflect that only doubled-quote escaping is considered a
positive sanity check.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown.go`:
- Around line 53-70: The TryPushDown function currently collapses all failures
into ("", false), which hides whether compilation (env.Compile), conversion
(cel2sql.Convert) or an empty-SQL outcome failed; update TryPushDown to emit a
debug/trace record on each failure path (e.g., using your tracing/logging
helper) that records the selector identifier (hash of selector) and which step
failed (getPushdownEnv error, env.Compile issues, cel2sql.Convert error, or
empty SQL) without changing the public return values; reference TryPushDown,
getPushdownEnv, env.Compile and cel2sql.Convert when adding the logs/trace
attributes so operational teams can detect regressions while preserving the ("",
false) contract.
In `@apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go`:
- Around line 233-248: The extraWhere loop currently accepts raw strings which
relies on external discipline; change the API so TryPushDown (e.g.
versionselector.TryPushDown) returns a typed wrapper like type SafeWhereClause
string and update this function to accept []SafeWhereClause instead of []string,
then iterate over those wrappers when building the SQL (referencing extraWhere
and the candidateVersionColumns builder) to make accidental raw fragments a
compile-time error; additionally add a cheap runtime sanity-check inside the
loop to detect any fragment containing an unquoted semicolon (panic or skip the
fragment) to guard against future translator regressions.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go`:
- Around line 124-176: Move the span attribute setting into a deferred closure
immediately after creating the span so attributes are always recorded even on
early returns: after tracer.Start(ctx, "FindDeployableVersion") add defer func()
{ span.SetAttributes(attribute.String("deployment.id", rt.DeploymentId),
attribute.Int("versions.scanned", scanned), attribute.Bool("version.found",
found != nil)) }() (leave span.End() deferred as-is or combine end in the same
defer), so errors from GetPolicySkips or evaluateVersion and the iterator error
still record deployment id, scanned and found using the existing scanned and
found variables.
In `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile.go`:
- Around line 188-206: Replace the hand-rolled insertion sort with the standard
library implementation: remove the custom sortStrings function and its call, and
use slices.Sort(clauses) where clauses is sorted (you can drop the len(clauses)
> 1 guard since slices.Sort is a no-op for short slices); update any references
to sortStrings accordingly (e.g., the call site in reconcile.go that currently
calls sortStrings(clauses)). Ensure the package imports include "slices" (it
already exists elsewhere) and remove the unused sortStrings function to avoid
dead code.
In `@apps/workspace-engine/test/controllers/harness/mocks.go`:
- Around line 170-182: Add a one-line comment above the
DesiredReleaseGetter.IterCandidateVersions function explaining that this mock
intentionally ignores the extraWhere parameter and yields every version in
g.Versions in order (i.e., it does not apply pushdown filtering), and note that
pushdown filtering is tested against the real Postgres getter (pushdown_test.go)
so this mock is only for testing reconcile logic given a candidate stream.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16ecb207-2bf1-4b20-836f-b2c41925b27c
⛔ Files ignored due to path filters (1)
apps/workspace-engine/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
apps/workspace-engine/go.modapps/workspace-engine/pkg/db/deployment_versions.sql.goapps/workspace-engine/pkg/db/queries/deployment_versions.sqlapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.goapps/workspace-engine/svc/controllers/desiredrelease/getters.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres_test.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.goapps/workspace-engine/svc/controllers/desiredrelease/pushdown_test.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.goapps/workspace-engine/test/controllers/harness/mocks.go
💤 Files with no reviewable changes (1)
- apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.go
* singleflight.Group.Do runs the closure in the first caller's goroutine,
so the first caller's ctx cancellation aborted the shared DB query for
every other waiter on the same key. fetchFirstBatch now passes
context.WithoutCancel(ctx) into the closure — cancellation is detached
from any individual caller while trace context is preserved. Per-pool
timeouts still bound runaway queries.
* The batch-loop in IterCandidateVersions wrapped errors a second time
even though queryCandidateVersionsBatch already formats them as
"list versions for deployment %s: %w". The first-batch path passed the
error through unwrapped, so the two sites produced inconsistent messages
("list versions for deployment X: list versions for deployment X: ...").
Drop the outer wrap so both paths surface a single layer.
mleonidas
deleted the
1101-bug-updating-version-selector-is-not-triggering-reconcile-of-targets
branch
2 participants
The desired-release reconciler previously buffered up to 500 candidate deployment versions per release target, silently skipping any version beyond that window. Replace the upfront fetch with a keyset-paginated iter.Seq2 that streams versions newest-first and short-circuits as soon as FindDeployableVersion picks one, removing the hard cap.
Burst patterns (many RTs of the same deployment reconciling concurrently after a fanout event) now collapse the first-batch fetch through singleflight keyed by (deploymentID, hash(pushdown clauses)), so worker overlap costs one round trip instead of N.
For policies whose versionselector CEL can be statically translated to SQL, push the predicate into the candidate query via cel2sql. Translatable shapes (literal eq, in-list, startsWith, boolean composition over the version variable) AND-join into the WHERE clause so Postgres returns only plausible candidates instead of the full LIMIT 500. Untranslatable selectors (referencing environment/resource/deployment, complex CEL, JSONB metadata access) fall back to the row-by-row evaluator unchanged — pushdown is a candidate-set narrowing optimization, not a replacement for runtime evaluation.
Span instrumentation:
SQL injection safety on the inlined pushdown fragments is verified by
TestTryPushDown_StringEscaping; cel2sql emits Postgres-standard doubled
single-quote escapes for string literals.
Summary by CodeRabbit
Release Notes
New Features
Performance
Tests