perf: cache release target jobs query#862
Merged
adityachoudhari26 merged 1 commit intomainfrom Mar 23, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughA new store implementation Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
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 (2)
apps/workspace-engine/svc/controllers/policyeval/controller.go (1)
56-69:⚠️ Potential issue | 🟠 MajorUse a separate, shorter TTL for
jobsForRTcache.Line 56 defines a 5-minute cache and Line 66-68 applies it to job status reads. Job statuses change frequently; this can serve stale verification state and delay policy progression decisions.
Suggested adjustment
cacheTTL := 5 * time.Minute + jobsCacheTTL := 15 * time.Second rtForDep := releasetargets.NewGetReleaseTargetsForDeployment( releasetargets.WithCache(cacheTTL), ) ... jobsForRT := releasetargets.NewGetJobsForReleaseTarget( - releasetargets.WithCache(cacheTTL), + releasetargets.WithCache(jobsCacheTTL), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policyeval/controller.go` around lines 56 - 69, The current code reuses cacheTTL (5 * time.Minute) for all getters including jobsForRT, which risks stale job status reads; change to use a shorter TTL for jobsForRT by defining a separate jobCacheTTL (e.g., 30s or 1m) and pass releasetargets.WithCache(jobCacheTTL) when constructing jobsForRT (NewGetJobsForReleaseTarget) while leaving rtForDep, rtForDepEnv, and policiesForRT using the original cacheTTL; ensure NewPostgresGetter still receives the updated jobsForRT instance.apps/workspace-engine/svc/controllers/desiredrelease/controller.go (1)
53-67:⚠️ Potential issue | 🟠 Major
jobsForRTshould not inherit the 5-minute generic cache TTL.Line 53 sets a broad TTL and Line 63-65 applies it to job status lookups. For desired-release reconciliation, stale job status can postpone or mis-sequence decisions.
Suggested adjustment
cacheTTL := 5 * time.Minute + jobsCacheTTL := 15 * time.Second rtForDep := releasetargets.NewGetReleaseTargetsForDeployment( releasetargets.WithCache(cacheTTL), ) ... jobsForRT := releasetargets.NewGetJobsForReleaseTarget( - releasetargets.WithCache(cacheTTL), + releasetargets.WithCache(jobsCacheTTL), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/controller.go` around lines 53 - 67, The jobsForRT lookup is using the generic cacheTTL (5m) which can make job status stale; change the code so NewGetJobsForReleaseTarget (jobsForRT) gets a much shorter TTL (e.g., 30s) instead of cacheTTL: introduce a separate variable (e.g., jobsCacheTTL := 30 * time.Second) and pass releasetargets.WithCache(jobsCacheTTL) when constructing jobsForRT, leaving rtForDep, rtForDepEnv, and policiesForRT using the original cacheTTL, then instantiate NewPostgresGetter with the updated jobsForRT.
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/getters.go (2)
36-36: Add a doc comment forNewPostgresGetters.This exported constructor changed here and is still undocumented.
✏️ Suggested comment
+// NewPostgresGetters returns the Postgres-backed getters used by the +// version cooldown evaluator. func NewPostgresGetters(queries *db.Queries, jobsForRT releasetargets.GetJobsForReleaseTarget) *PostgresGetters {As per coding guidelines "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/getters.go` at line 36, Add a single-line doc comment above the exported constructor NewPostgresGetters describing its purpose (constructs a PostgresGetters instance for version cooldown policy evaluation), and briefly document its parameters and return value — mention that it accepts a *db.Queries and a releasetargets.GetJobsForReleaseTarget and returns *PostgresGetters — so the exported function is properly documented for consumers and godoc.
37-39: MakejobsForRTexplicit in this constructor.This PR is threading a cached
GetJobsForReleaseTargetthrough the evaluator, but the nil fallback makes a missed injection silently work with a default instance instead.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/versioncooldown_test.goalready provides a mock getter at Lines 67-75, so tests can stay explicit without depending on this implicit default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/getters.go` around lines 37 - 39, The constructor currently silently falls back to a default by doing "if jobsForRT == nil { jobsForRT = releasetargets.NewGetJobsForReleaseTarget() }"; remove that fallback and make the constructor require an explicit non-nil jobsForRT injection—validate the jobsForRT parameter (the variable named jobsForRT passed into the constructor in getters.go) and return an error or panic if it is nil instead of creating a default via releasetargets.NewGetJobsForReleaseTarget(), so callers and tests (e.g., the mock provided in versioncooldown_test.go) must inject the dependency explicitly.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go (2)
82-87: Add a doc comment forNewPostgresGetters.This exported constructor changed here and is still undocumented.
✏️ Suggested comment
+// NewPostgresGetters returns the Postgres-backed getters used by the +// environment progression evaluator. func NewPostgresGetters(As per coding guidelines "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go` around lines 82 - 87, The exported constructor NewPostgresGetters lacks a doc comment; add a concise GoDoc comment immediately above the NewPostgresGetters function that describes what it constructs (a PostgresGetters), summarizes its purpose, and documents the parameters (queries, rtForDep, rtForDepEnv, jobsForRT) and the return value so the exported function is properly documented for users and tools.
94-96: MakejobsForRTexplicit in this constructor.
apps/workspace-engine/svc/controllers/desiredrelease/policyeval/getter_postgres.goat Lines 28-46 andapps/workspace-engine/svc/controllers/policyeval/controller.goat Lines 60-75 already thread a sharedjobsForRTthrough the production wiring. Keeping this fallback means a future missed injection will still work, but without the intended shared-cache configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go` around lines 94 - 96, Remove the silent fallback that instantiates releasetargets.NewGetJobsForReleaseTarget() when jobsForRT is nil and make jobsForRT a required dependency: delete the if-block that creates the default, validate that jobsForRT is non-nil at constructor entry (or return an error/panic with a clear message like "jobsForRT must be provided to ensure shared-cache behavior"), and update the constructor signature/validation in the function containing that code so callers must supply the shared jobsForRT instance (reference symbols: jobsForRT and releasetargets.NewGetJobsForReleaseTarget()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/workspace-engine/pkg/store/releasetargets/get_jobs_for_release_target.go`:
- Around line 15-20: The current GetJobsForReleaseTarget interface and its
implementations hide parse/DB errors by returning nil maps (in
GetJobsForReleaseTarget implementations at the locations referenced), making
error paths indistinguishable from “no jobs”; change the GetJobsForReleaseTarget
signature to return (map[string]*oapi.Job, error), update every implementation
of GetJobsForReleaseTarget to return a non-nil error on parse/DB failures
(instead of nil), and propagate those errors to callers so callers can
distinguish transient failures from empty results; ensure to update any call
sites to handle the new error return and adjust tests accordingly.
---
Outside diff comments:
In `@apps/workspace-engine/svc/controllers/desiredrelease/controller.go`:
- Around line 53-67: The jobsForRT lookup is using the generic cacheTTL (5m)
which can make job status stale; change the code so NewGetJobsForReleaseTarget
(jobsForRT) gets a much shorter TTL (e.g., 30s) instead of cacheTTL: introduce a
separate variable (e.g., jobsCacheTTL := 30 * time.Second) and pass
releasetargets.WithCache(jobsCacheTTL) when constructing jobsForRT, leaving
rtForDep, rtForDepEnv, and policiesForRT using the original cacheTTL, then
instantiate NewPostgresGetter with the updated jobsForRT.
In `@apps/workspace-engine/svc/controllers/policyeval/controller.go`:
- Around line 56-69: The current code reuses cacheTTL (5 * time.Minute) for all
getters including jobsForRT, which risks stale job status reads; change to use a
shorter TTL for jobsForRT by defining a separate jobCacheTTL (e.g., 30s or 1m)
and pass releasetargets.WithCache(jobCacheTTL) when constructing jobsForRT
(NewGetJobsForReleaseTarget) while leaving rtForDep, rtForDepEnv, and
policiesForRT using the original cacheTTL; ensure NewPostgresGetter still
receives the updated jobsForRT instance.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go`:
- Around line 82-87: The exported constructor NewPostgresGetters lacks a doc
comment; add a concise GoDoc comment immediately above the NewPostgresGetters
function that describes what it constructs (a PostgresGetters), summarizes its
purpose, and documents the parameters (queries, rtForDep, rtForDepEnv,
jobsForRT) and the return value so the exported function is properly documented
for users and tools.
- Around line 94-96: Remove the silent fallback that instantiates
releasetargets.NewGetJobsForReleaseTarget() when jobsForRT is nil and make
jobsForRT a required dependency: delete the if-block that creates the default,
validate that jobsForRT is non-nil at constructor entry (or return an
error/panic with a clear message like "jobsForRT must be provided to ensure
shared-cache behavior"), and update the constructor signature/validation in the
function containing that code so callers must supply the shared jobsForRT
instance (reference symbols: jobsForRT and
releasetargets.NewGetJobsForReleaseTarget()).
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/getters.go`:
- Line 36: Add a single-line doc comment above the exported constructor
NewPostgresGetters describing its purpose (constructs a PostgresGetters instance
for version cooldown policy evaluation), and briefly document its parameters and
return value — mention that it accepts a *db.Queries and a
releasetargets.GetJobsForReleaseTarget and returns *PostgresGetters — so the
exported function is properly documented for consumers and godoc.
- Around line 37-39: The constructor currently silently falls back to a default
by doing "if jobsForRT == nil { jobsForRT =
releasetargets.NewGetJobsForReleaseTarget() }"; remove that fallback and make
the constructor require an explicit non-nil jobsForRT injection—validate the
jobsForRT parameter (the variable named jobsForRT passed into the constructor in
getters.go) and return an error or panic if it is nil instead of creating a
default via releasetargets.NewGetJobsForReleaseTarget(), so callers and tests
(e.g., the mock provided in versioncooldown_test.go) must inject the dependency
explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebff0fa0-77ba-4965-9199-136d2818fb58
📒 Files selected for processing (10)
apps/workspace-engine/pkg/store/releasetargets/get_jobs_for_release_target.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/getters_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/getters.goapps/workspace-engine/svc/controllers/desiredrelease/controller.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/getter_postgres.goapps/workspace-engine/svc/controllers/policyeval/controller.goapps/workspace-engine/svc/controllers/policyeval/getters_postgres.go
Comment on lines
+15
to
+20
| type GetJobsForReleaseTarget interface { | ||
| GetJobsForReleaseTarget( | ||
| ctx context.Context, | ||
| releaseTarget *oapi.ReleaseTarget, | ||
| ) map[string]*oapi.Job | ||
| } |
Contributor
There was a problem hiding this comment.
Error paths are currently indistinguishable from “no jobs.”
Line 51, Line 55, Line 59, and Line 67 return nil for parse/DB failures, but the interface (Line 15-20) cannot surface errors. That makes transient failures look like empty results, which can mis-evaluate policy decisions.
Proposed direction
type GetJobsForReleaseTarget interface {
GetJobsForReleaseTarget(
ctx context.Context,
releaseTarget *oapi.ReleaseTarget,
- ) map[string]*oapi.Job
+ ) (map[string]*oapi.Job, error)
}
func (s *PostgresGetJobsForReleaseTarget) GetJobsForReleaseTarget(
ctx context.Context,
releaseTarget *oapi.ReleaseTarget,
-) map[string]*oapi.Job {
+) (map[string]*oapi.Job, error) {
...
deploymentID, err := uuid.Parse(releaseTarget.DeploymentId)
if err != nil {
- return nil
+ return nil, fmt.Errorf("parse deployment id: %w", err)
}
...
if err != nil {
- return nil
+ return nil, fmt.Errorf("list jobs by release target: %w", err)
}
...
- return jobs
+ return jobs, nil
}Also applies to: 49-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/store/releasetargets/get_jobs_for_release_target.go`
around lines 15 - 20, The current GetJobsForReleaseTarget interface and its
implementations hide parse/DB errors by returning nil maps (in
GetJobsForReleaseTarget implementations at the locations referenced), making
error paths indistinguishable from “no jobs”; change the GetJobsForReleaseTarget
signature to return (map[string]*oapi.Job, error), update every implementation
of GetJobsForReleaseTarget to return a non-nil error on parse/DB failures
(instead of nil), and propagate those errors to callers so callers can
distinguish transient failures from empty results; ensure to update any call
sites to handle the new error return and adjust tests accordingly.
This was referenced Mar 28, 2026
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
1 participant
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 by CodeRabbit