perf: cache target policies in materialized view #712
Conversation
WalkthroughPolicy Upsert was changed to not return errors, record changesets, and trigger per-target policy recomputation; ReleaseTargets gained per-target materialized policy caches and a public RecomputeTargetPolicies method. Event handlers and workspace initialization now call Upsert without handling errors; GetPolicies reads from per-target caches and errors if absent. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Event Handler
participant Policies as Policies Store
participant ReleaseTargets as ReleaseTargets
participant Cache as Per-Target Cache
rect rgb(220,235,255)
note right of Handler: Policy create/update (new)
Handler->>Policies: Upsert(ctx, policy)
Policies->>Policies: record changeset (changeset.FromContext / RecordUpsert)
Policies->>ReleaseTargets: RecomputeTargetPolicies()
ReleaseTargets->>Cache: recompute & populate per-target views
Cache-->>ReleaseTargets: views ready
ReleaseTargets-->>Policies: recompute done
Policies-->>Handler: returns (no error)
end
rect rgb(255,235,220)
note right of Handler: Policy delete (new)
Handler->>Policies: Remove(ctx, id)
alt exists
Policies->>Policies: delete & record changeset
Policies->>ReleaseTargets: RecomputeTargetPolicies()
ReleaseTargets->>Cache: update views
ReleaseTargets-->>Policies: result
Policies-->>Handler: return nil
else not found
Policies-->>Handler: return error (not found)
end
end
rect rgb(235,255,235)
note right of Consumers: Policy query (new)
Consumer->>ReleaseTargets: GetPolicies(ctx, target)
alt cached view exists
ReleaseTargets->>Cache: read per-target view
Cache-->>ReleaseTargets: policies
ReleaseTargets-->>Consumer: policies
else missing
ReleaseTargets-->>Consumer: return error (missing view)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (2)apps/workspace-engine/**/*.go📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Files:
apps/workspace-engine/**/*_test.go📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Files:
🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T20:49:05.086ZApplied to files:
📚 Learning: 2025-10-07T16:44:54.938ZApplied to files:
🧬 Code graph analysis (5)apps/workspace-engine/pkg/events/handler/policies/policies.go (1)
apps/workspace-engine/pkg/workspace/populate_workspace.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (4)
apps/workspace-engine/pkg/workspace/releasemanager/deployment/planner_bench_test.go (2)
apps/workspace-engine/pkg/workspace/store/policy.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (7)
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 |
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/workspace-engine/pkg/events/handler/policies/policies.go(1 hunks)apps/workspace-engine/pkg/workspace/store/policy.go(3 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/events/handler/policies/policies.goapps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/release_targets.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Applied to files:
apps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/mapping/protobuf_mappings.go : Add mapping functions for the new condition in pkg/mapping/protobuf_mappings.go
Applied to files:
apps/workspace-engine/pkg/workspace/store/policy.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Applied to files:
apps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Do not add extraneous inline comments that state the obvious
Applied to files:
apps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/release_targets.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/events/handler/policies/policies.go (1)
apps/workspace-engine/pkg/workspace/store/policy.go (1)
Policies(19-22)
apps/workspace-engine/pkg/workspace/store/policy.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(29-34)
apps/workspace-engine/pkg/workspace/store/release_targets.go (5)
apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
New(17-39)apps/workspace-engine/pkg/workspace/store/store.go (2)
New(10-34)Store(36-58)apps/workspace-engine/pkg/workspace/store/materialized/view.go (4)
New(34-48)MaterializedView(19-27)IsAlreadyStarted(65-67)RecomputeFunc(10-10)apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Policy(342-355)ReleaseTarget(445-449)apps/workspace-engine/pkg/cmap/concurrent_map.go (1)
ConcurrentMap(23-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: tests
- GitHub Check: workspace-engine-tests
| allTargets := r.targets.Get() | ||
| for targetKey, target := range allTargets { | ||
| t := target | ||
| key := targetKey | ||
| mv := materialized.New(r.targetPoliciesRecomputeFunc(t)) | ||
| r.targetPolicies.Set(key, mv) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Remove stale cache entries for deleted targets.
Line 145: RecomputeTargetPolicies rebuilds entries for current targets but never drops keys that disappeared from allTargets. When release targets are deleted, their per-target caches linger indefinitely, leaking memory and leaving stale views accessible through GetPolicies. Please remove any keys absent from the recomputed target set (for example by clearing the map before repopulating or explicitly calling r.targetPolicies.Remove for those keys) before returning.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/release_targets.go around lines 145
to 153, RecomputeTargetPolicies currently repopulates r.targetPolicies for keys
present in allTargets but never removes entries for keys that were deleted,
leaking cached materializations; modify the function to first remove stale keys
(either by clearing r.targetPolicies entirely before the loop or by iterating
existing r.targetPolicies keys and calling r.targetPolicies.Remove for any key
not present in allTargets) and then repopulate r.targetPolicies with
materialized entries for the current targets so deleted targets no longer retain
cached state.
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/workspace-engine/pkg/workspace/store/release_targets.go (2)
141-149: Stale target policy caches linger after target removalReiterating earlier feedback:
RecomputeTargetPoliciesonly rebuilds entries for keys present inallTargets, but it never clears entries for keys that disappeared. Release targets that have been deleted keep their cached policies indefinitely, leaking memory and exposing stale data throughGetPolicies. Please drop keys absent fromallTargets(by clearing before repopulating, or removing the missing keys as you iterate) before you finish recomputation.
209-214: Restore lazy initialization for per-target cachesThis still regresses callers: if
GetPoliciesruns beforeRecomputeTargetPoliciesseedsr.targetPolicies—for example in a workspace with no policy mutations yet—we now error with “target policies not found…”, whereas previously we computed on demand. Please reinstate lazy creation on cache miss (or eagerly seed during construction) so thatGetPoliciescontinues to work from a cold start.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/workspace-engine/pkg/events/handler/policies/policies.go(2 hunks)apps/workspace-engine/pkg/workspace/populate_workspace.go(1 hunks)apps/workspace-engine/pkg/workspace/store/policy.go(3 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/workspace/populate_workspace.goapps/workspace-engine/pkg/events/handler/policies/policies.goapps/workspace-engine/pkg/workspace/store/release_targets.goapps/workspace-engine/pkg/workspace/store/policy.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Applied to files:
apps/workspace-engine/pkg/workspace/populate_workspace.goapps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Do not add comments that simply restate what the code does
Applied to files:
apps/workspace-engine/pkg/workspace/populate_workspace.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Applied to files:
apps/workspace-engine/pkg/workspace/populate_workspace.goapps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T20:49:05.086Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 655
File: apps/workspace-engine/pkg/engine/workspace/fluent.go:166-171
Timestamp: 2025-08-12T20:49:05.086Z
Learning: The UpdateDeploymentVersions() method with OperationCreate case in apps/workspace-engine/pkg/engine/workspace/fluent.go is specifically designed only for creating new deployment versions, not for handling potential duplicates or existing versions.
Applied to files:
apps/workspace-engine/pkg/workspace/populate_workspace.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Do not add extraneous inline comments that state the obvious
Applied to files:
apps/workspace-engine/pkg/workspace/populate_workspace.goapps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/workspace/store/release_targets.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/release_targets.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/events/handler/policies/policies.go (1)
apps/workspace-engine/pkg/workspace/store/policy.go (1)
Policies(18-21)
apps/workspace-engine/pkg/workspace/store/release_targets.go (4)
apps/workspace-engine/pkg/workspace/store/store.go (2)
New(10-34)Store(36-58)apps/workspace-engine/pkg/workspace/store/materialized/view.go (4)
New(34-48)MaterializedView(19-27)IsAlreadyStarted(65-67)RecomputeFunc(10-10)apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Policy(342-355)ReleaseTarget(445-449)apps/workspace-engine/pkg/cmap/concurrent_map.go (1)
ConcurrentMap(23-26)
apps/workspace-engine/pkg/workspace/store/policy.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Policy(342-355)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(29-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: workspace-engine-tests
- GitHub Check: tests
🔇 Additional comments (3)
apps/workspace-engine/pkg/events/handler/policies/policies.go (1)
22-24: Upsert call aligns with new signatureConfirming the event handler now matches the void-returning
Policies.Upsert; no further handling is required here.apps/workspace-engine/pkg/workspace/populate_workspace.go (1)
45-47: Populate path updated correctlyThe workspace bootstrap now reflects the signature change by dropping error handling around
Policies().Upsert, keeping initialization coherent.apps/workspace-engine/pkg/workspace/store/policy.go (1)
39-52: Policy mutations now fan out recomputationPersisting the policy, normalizing metadata, and then triggering
RecomputeTargetPolicieskeeps the cache in sync with policy writes.
| func (r *ReleaseTargets) Recompute(ctx context.Context) error { | ||
| return r.targets.StartRecompute(ctx) | ||
| if err := r.targets.StartRecompute(ctx); err != nil && !materialized.IsAlreadyStarted(err) { | ||
| return err | ||
| } | ||
| r.RecomputeTargetPolicies() | ||
| return nil |
There was a problem hiding this comment.
Wait for target recompute before rebuilding caches
Recompute fires a recompute of r.targets and then immediately pulls r.targets.Get(). Because StartRecompute is asynchronous, the subsequent cache rebuild can run against the old target map, so new targets never receive a policy view unless another recompute happens later. Please block on the recompute finishing (e.g., WaitIfRunning) before calling RecomputeTargetPolicies, and surface any resulting error.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/release_targets.go around lines 64
to 69, the Recompute function starts an asynchronous recompute and then
immediately rebuilds target policies, which can race with StartRecompute and use
an old target map; change the flow to wait for the recompute to finish (use the
provided materialized.WaitIfRunning or equivalent on r.targets) after
StartRecompute returns and before calling RecomputeTargetPolicies, and if
WaitIfRunning (or the wait call) returns an error, return that error instead of
proceeding so the caller sees recompute failures.
adityachoudhari26
force-pushed
the
cache-target-policies
branch
from
413f832 to
1619f16
Compare
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
Summary by CodeRabbit
New Features
Behavioral Changes
Performance