simplify logic #715
simplify logic #715
Conversation
📊 DB Package Test Coveragepkg/db coverage: |
|
Warning Rate limit exceeded@jsbroks has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 23 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (10)
WalkthroughRemoved ChangeSet wiring and the taint/targets materialized-view subsystem; stores migrated to plain Map-backed adapters. ReleaseTargets became an in-memory map with Items() (no context), Upsert/Get/Remove/GetFor* helpers. Event handlers now compute/upsert/remove ReleaseTargets and synchronously call ReleaseManager.ReconcileTarget. Changes
Sequence Diagram(s)sequenceDiagram
participant H as Events Handler
participant S as Store (repo / maps)
participant RM as ReleaseManager
participant T as Tracing
rect rgb(230,240,255)
note right of H: Old flow (ChangeSet + taint)
H->>T: WithChangeSet(ctx, changeSet)
H->>RM: ProcessChanges(ctx, changeSet)
RM-->>H: targets tainted / targets manager
T-->>T: emit span attr "release-target.changed"
end
rect rgb(240,255,230)
note right of H: New flow (map-backed targets)
H->>S: persist entity (Upsert/Remove)
H->>S: compute & Upsert ReleaseTargets (selector-driven)
H->>RM: ReconcileTarget(ctx, releaseTarget, false)
RM-->>H: reconciliation result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/workspace-engine/pkg/workspace/store/diffcheck/resource.go (1)
76-90: Field path elements after the first are not lowercased, breaking field matching.The
convertPathToFieldNamefunction only lowercases the first path element. However, the comment examples show all elements should be lowercase (e.g., "config.replicas"). Since struct field names are capitalized by Go, a path like["Config", "Replicas"]would currently produce"config.Replicas"instead of the expected"config.replicas". This breaks matching inisIgnoredFieldwhere ignored fields are all-lowercase strings.Apply this diff to lowercase all path elements:
func convertPathToFieldName(path []string) string { if len(path) == 0 { return "" } - // Convert first element to lowercase - result := strings.ToLower(path[0]) + // Convert path elements to lowercase + result := strings.ToLower(path[0]) // Append remaining path elements with dots for i := 1; i < len(path); i++ { - result += "." + path[i] + result += "." + strings.ToLower(path[i]) } return result }apps/workspace-engine/pkg/events/handler/jobs/jobs.go (1)
91-100: Line 100 contains unreachable code with potential nil pointer dereference.After the AgentId and ExternalId presence checks (lines 89-95), the code attempts
ws.Jobs().Get(*job.Id). This is broken for two reasons:
- If job.Id was non-nil and non-empty, the function already returned at line 87
- If job.Id is nil or empty, dereferencing it at line 100 will panic
The comment indicates this should look up jobs by agent ID + external ID, but there's no method in the Jobs store that supports this. The removed
GetByJobAgentAndExternalIdmethod was not replaced.Change line 100 to return
nil, false:- // Try finding by job agent ID + external ID - return ws.Jobs().Get(*job.Id) + return nil, falseIf lookup by agent + external ID is still needed, implement a new method on the Jobs store to support this.
apps/workspace-engine/pkg/workspace/store/resource_providers.go (1)
52-59: Avoid mutating the map while iterating it.Inside the range over
r.resources.Items()you callr.resources.Upsert, which writes back into the same map thatItems()just exposed. In Go this triggers a fatal “concurrent map iteration and map write” panic at runtime. Collect the affected resources first, then upsert them after you finish iterating.Apply this diff:
- for _, resource := range r.resources.Items() { - if resource.ProviderId != nil && *resource.ProviderId == id { - resource.ProviderId = nil - if _, err := r.resources.Upsert(ctx, resource); err != nil { - return err - } - } - } + var toDettach []*oapi.Resource + for _, resource := range r.resources.Items() { + if resource.ProviderId != nil && *resource.ProviderId == id { + resource.ProviderId = nil + toDettach = append(toDettach, resource) + } + } + + for _, resource := range toDettach { + if _, err := r.resources.Upsert(ctx, resource); err != nil { + return err + } + }apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
10-65: Reintroduce concurrency safety for repository maps.Replacing
cmap.ConcurrentMapwith a bare Go map means every store (resources, deployments, environments, etc.) now uses an unsynchronized map. Those stores are accessed and mutated from multiple goroutines (e.g., ingest, watchers, reconciler loops). Without locking, concurrent reads/writes will race and can triggerfatal error: concurrent map read and map writeat runtime. Critical correctness issue: we previously relied oncmapspecifically to avoid this.Please keep the in-memory backing concurrency-safe—either retain
cmap.ConcurrentMap, or wrap the new Map in async.RWMutex/other synchronization so Set/Get/Remove can be used safely across goroutines.apps/workspace-engine/pkg/workspace/store/relationships.go (1)
51-55: Userelationship.Referenceas the storage key.
GetRelationshipRule(reference string)(and the relation graph) expect the repository to be keyed byrelationship.Reference, butUpsertnow stores byrelationship.Id. That makesGetRelationshipRulereturn nil for newly upserted rules. Please restore the reference-based key.Apply this diff:
func (r *RelationshipRules) Upsert(ctx context.Context, relationship *oapi.RelationshipRule) error { - r.repo.RelationshipRules.Set(relationship.Id, relationship) + r.repo.RelationshipRules.Set(relationship.Reference, relationship) if cs, ok := changeset.FromContext[any](ctx); ok { cs.Record(changeset.ChangeTypeUpsert, relationship) }
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/store/diffcheck/deployment.go (1)
45-58: Ignored fields list may be incomplete.Only
"id"is in the ignored-fields list. Consider whether system-managed or auto-generated fields (e.g.,createdAt,updatedAt,version, etc.) should also be ignored.If the list grows, consider using a map for O(1) lookup instead of linear search:
var ignoredFields = map[string]bool{ "id": true, // Add other ignored fields as needed } func isIgnoredDeploymentField(fieldPath string) bool { return ignoredFields[fieldPath] }apps/workspace-engine/pkg/workspace/store/policy.go (1)
38-38: Consider removing unused context parameter.Similar to
Upsert, thectxparameter is unused inRemove. If context is no longer needed, consider removing it for consistency.
📜 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 (34)
apps/workspace-engine/pkg/events/handler/handler.go(1 hunks)apps/workspace-engine/pkg/events/handler/jobs/jobs.go(1 hunks)apps/workspace-engine/pkg/events/handler/resources/resources.go(1 hunks)apps/workspace-engine/pkg/events/handler/tick/tick.go(1 hunks)apps/workspace-engine/pkg/oapi/oapi.go(1 hunks)apps/workspace-engine/pkg/persistence/integration_test.go(2 hunks)apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go(2 hunks)apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.go(8 hunks)apps/workspace-engine/pkg/workspace/releasemanager/versions/manager.go(1 hunks)apps/workspace-engine/pkg/workspace/store/deployment_variable_values.go(1 hunks)apps/workspace-engine/pkg/workspace/store/deployment_variables.go(2 hunks)apps/workspace-engine/pkg/workspace/store/deployment_versions.go(1 hunks)apps/workspace-engine/pkg/workspace/store/deployments.go(1 hunks)apps/workspace-engine/pkg/workspace/store/diffcheck/deployment.go(3 hunks)apps/workspace-engine/pkg/workspace/store/diffcheck/environment.go(3 hunks)apps/workspace-engine/pkg/workspace/store/diffcheck/resource.go(3 hunks)apps/workspace-engine/pkg/workspace/store/environments.go(1 hunks)apps/workspace-engine/pkg/workspace/store/github_entities.go(1 hunks)apps/workspace-engine/pkg/workspace/store/job_agents.go(3 hunks)apps/workspace-engine/pkg/workspace/store/jobs.go(2 hunks)apps/workspace-engine/pkg/workspace/store/policy.go(1 hunks)apps/workspace-engine/pkg/workspace/store/relationships.go(2 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(1 hunks)apps/workspace-engine/pkg/workspace/store/releases.go(2 hunks)apps/workspace-engine/pkg/workspace/store/repository/adapter.go(1 hunks)apps/workspace-engine/pkg/workspace/store/repository/repo.go(3 hunks)apps/workspace-engine/pkg/workspace/store/resource_providers.go(2 hunks)apps/workspace-engine/pkg/workspace/store/resource_variables.go(0 hunks)apps/workspace-engine/pkg/workspace/store/resources.go(7 hunks)apps/workspace-engine/pkg/workspace/store/restore_test.go(5 hunks)apps/workspace-engine/pkg/workspace/store/store.go(0 hunks)apps/workspace-engine/pkg/workspace/store/systems.go(1 hunks)apps/workspace-engine/pkg/workspace/store/user_approval_records.go(1 hunks)apps/workspace-engine/test/e2e/engine_presistence_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- apps/workspace-engine/pkg/workspace/store/resource_variables.go
- apps/workspace-engine/pkg/workspace/store/store.go
🧰 Additional context used
📓 Path-based instructions (2)
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/resources/resources.goapps/workspace-engine/pkg/events/handler/jobs/jobs.goapps/workspace-engine/pkg/workspace/store/diffcheck/environment.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/releasemanager/versions/manager.goapps/workspace-engine/pkg/workspace/store/deployment_variable_values.goapps/workspace-engine/pkg/workspace/store/github_entities.goapps/workspace-engine/test/e2e/engine_presistence_test.goapps/workspace-engine/pkg/workspace/store/resource_providers.goapps/workspace-engine/pkg/workspace/store/diffcheck/deployment.goapps/workspace-engine/pkg/workspace/store/job_agents.goapps/workspace-engine/pkg/workspace/store/deployment_versions.goapps/workspace-engine/pkg/workspace/store/jobs.goapps/workspace-engine/pkg/workspace/store/diffcheck/resource.goapps/workspace-engine/pkg/oapi/oapi.goapps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/releases.goapps/workspace-engine/pkg/events/handler/tick/tick.goapps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.goapps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/workspace/store/deployment_variables.goapps/workspace-engine/pkg/workspace/store/repository/repo.goapps/workspace-engine/pkg/workspace/store/systems.goapps/workspace-engine/pkg/workspace/store/release_targets.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/workspace/store/relationships.goapps/workspace-engine/pkg/persistence/integration_test.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/workspace/store/repository/adapter.goapps/workspace-engine/pkg/workspace/store/environments.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/test/e2e/engine_presistence_test.goapps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/persistence/integration_test.go
🧠 Learnings (17)
📚 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/diffcheck/environment.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/store/github_entities.goapps/workspace-engine/pkg/workspace/store/deployment_versions.goapps/workspace-engine/pkg/workspace/store/diffcheck/resource.goapps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/deployment_variables.goapps/workspace-engine/pkg/workspace/store/repository/repo.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/workspace/store/environments.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/diffcheck/environment.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/store/diffcheck/deployment.goapps/workspace-engine/pkg/workspace/store/diffcheck/resource.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/diffcheck/environment.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/store/github_entities.goapps/workspace-engine/pkg/workspace/store/diffcheck/deployment.goapps/workspace-engine/pkg/workspace/store/diffcheck/resource.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/store/diffcheck/environment.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/store/github_entities.goapps/workspace-engine/pkg/workspace/store/diffcheck/deployment.goapps/workspace-engine/pkg/workspace/store/diffcheck/resource.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/user_approval_records.goapps/workspace-engine/test/e2e/engine_presistence_test.goapps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/persistence/integration_test.go
📚 Learning: 2025-08-19T22:58:15.170Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 658
File: apps/workspace-engine/pkg/engine/policy/rules/environment-version-rollout/rollout_start_time.go:65-80
Timestamp: 2025-08-19T22:58:15.170Z
Learning: In the environment-version-rollout rule's rollout start time logic, the code intentionally returns the latest ApprovedAt time (first non-nil record from records ordered by updatedAt) rather than the earliest. This is because the goal is to find the latest approval that caused all the rules to pass. While the complete logic for determining "the latest that caused all rules to pass" would be too complex, this approximation works well given that the UI prevents approving if all rules are already passing.
Applied to files:
apps/workspace-engine/pkg/workspace/store/user_approval_records.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/releasemanager/versions/manager.goapps/workspace-engine/pkg/workspace/store/deployment_variable_values.goapps/workspace-engine/pkg/workspace/store/diffcheck/deployment.goapps/workspace-engine/pkg/workspace/store/deployment_versions.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/workspace/store/deployment_variables.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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/test/e2e/engine_presistence_test.goapps/workspace-engine/pkg/workspace/store/resources.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 : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/test/e2e/engine_presistence_test.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/workspace/store/resources.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 : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/test/e2e/engine_presistence_test.goapps/workspace-engine/pkg/workspace/store/resources.go
📚 Learning: 2025-10-24T00:02:29.723Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 696
File: apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go:63-67
Timestamp: 2025-10-24T00:02:29.723Z
Learning: In workspace-engine replay mode (apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go), jobs are created with Pending status even during replay because job updates are still accepted and processed. If a job remains Pending after replay completes, it genuinely is still pending because no updates occurred during replay.
Applied to files:
apps/workspace-engine/pkg/workspace/store/jobs.go
📚 Learning: 2025-04-21T18:34:54.764Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 500
File: packages/db/src/schema/job.ts:117-120
Timestamp: 2025-04-21T18:34:54.764Z
Learning: In the system's job schema, the relationship between job and releaseJob is a true one-to-one relationship - a release job should only ever point to one job and vice versa. The implementation uses `one(releaseJob, ...)` in the jobRelations to reflect this business rule.
Applied to files:
apps/workspace-engine/pkg/workspace/store/releases.go
📚 Learning: 2025-04-28T18:41:58.813Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.813Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployments.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/deployments.goapps/workspace-engine/pkg/workspace/store/resources.go
📚 Learning: 2025-10-07T16:44:54.938Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 673
File: apps/workspace-engine/pkg/db/deployment_variables.go:51-65
Timestamp: 2025-10-07T16:44:54.938Z
Learning: In `apps/workspace-engine/pkg/db/deployment_variables.go`, the `default_value_id` field is intentionally scanned from the database but not assigned to the `DefaultValueId` field in the proto struct. This is a temporary decision and the field assignment is deferred for later implementation.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployment_variables.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/types.go : Add the new condition type to pkg/model/selector/types.go
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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/**/*.go : When adding a new condition type, create a new condition struct in pkg/model/selector implementing the Condition interface
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.go
🧬 Code graph analysis (20)
apps/workspace-engine/pkg/events/handler/jobs/jobs.go (2)
apps/workspace-engine/pkg/workspace/store/jobs.go (1)
Jobs(17-20)apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Id(58-58)
apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Status(64-64)ApprovalStatusApproved(19-19)UserApprovalRecord(556-563)
apps/workspace-engine/pkg/workspace/releasemanager/versions/manager.go (1)
apps/workspace-engine/pkg/workspace/store/deployment_versions.go (1)
DeploymentVersions(16-19)
apps/workspace-engine/test/e2e/engine_presistence_test.go (5)
apps/workspace-engine/pkg/workspace/store/systems.go (1)
Systems(16-19)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(22-25)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(16-19)apps/workspace-engine/pkg/workspace/store/job_agents.go (1)
JobAgents(17-20)
apps/workspace-engine/pkg/workspace/store/jobs.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (7)
Job(266-278)Status(64-64)Pending(48-48)Id(58-58)JobAgentId(60-60)ReleaseTarget(445-449)ReleaseId(62-62)apps/workspace-engine/pkg/workspace/store/releases.go (1)
Releases(17-20)
apps/workspace-engine/pkg/oapi/oapi.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
ReleaseTarget(445-449)
apps/workspace-engine/pkg/workspace/store/policy.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Policy(342-355)Metadata(61-61)Id(58-58)
apps/workspace-engine/pkg/workspace/store/releases.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Job(266-278)ReleaseId(62-62)Id(58-58)apps/workspace-engine/pkg/workspace/store/jobs.go (1)
Jobs(17-20)
apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(22-26)
apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(22-26)
apps/workspace-engine/pkg/workspace/store/deployments.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Id(58-58)DeploymentVariable(147-153)Deployment(129-138)
apps/workspace-engine/pkg/workspace/store/deployment_variables.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
DeploymentVariable(147-153)Id(58-58)apps/workspace-engine/pkg/workspace/store/deployment_variable_values.go (1)
DeploymentVariableValues(13-16)
apps/workspace-engine/pkg/workspace/store/repository/repo.go (3)
apps/workspace-engine/pkg/persistence/router.go (2)
RepositoryRouter(10-12)NewRepositoryRouter(14-18)apps/workspace-engine/pkg/workspace/store/repository/adapter.go (1)
MapStoreAdapter(51-53)apps/workspace-engine/pkg/cmap/concurrent_map.go (1)
New(46-48)
apps/workspace-engine/pkg/workspace/store/release_targets.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
ReleaseTarget(445-449)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)
apps/workspace-engine/pkg/workspace/store/restore_test.go (3)
apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
New(45-67)apps/workspace-engine/pkg/workspace/store/store.go (1)
New(10-34)
apps/workspace-engine/pkg/workspace/store/relationships.go (4)
apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/oapi/oapi.gen.go (5)
Deployment(129-138)Environment(205-212)RelationshipRule(415-428)RelatableEntity(404-406)EntityRelation(194-202)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(22-25)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(16-19)
apps/workspace-engine/pkg/persistence/integration_test.go (1)
apps/workspace-engine/pkg/workspace/store/job_agents.go (1)
JobAgents(17-20)
apps/workspace-engine/pkg/workspace/store/resources.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (6)
Id(58-58)Resource(475-489)ResourceVariable(501-505)Selector(535-537)Environment(205-212)Deployment(129-138)apps/workspace-engine/pkg/selector/resources.go (1)
Selector(10-10)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
apps/workspace-engine/pkg/workspace/store/repository/adapter.go (1)
apps/workspace-engine/pkg/persistence/changelog.go (1)
Entity(9-13)
apps/workspace-engine/pkg/workspace/store/environments.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Environment(205-212)Id(58-58)
⏰ 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: tests
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: workspace-engine-tests
🔇 Additional comments (16)
apps/workspace-engine/pkg/persistence/integration_test.go (2)
492-493: LGTM! Accessor path correctly updated.The JobAgents accessor has been correctly updated to use the top-level store accessor, consistent with the broader refactoring to simplify the store layer.
667-669: LGTM! Consistent with the refactor.The accessor path change is correctly applied here as well, maintaining consistency with the store layer simplification across all tests.
apps/workspace-engine/pkg/workspace/store/diffcheck/deployment.go (2)
14-43: Clarify nil-handling semantics and field naming consistency.The function comment (lines 9–13) describes paths like
"jobAgentConfig.key"in camelCase, buthasDeploymentChangesBasicreturns keys in lowercase ("jobagentid","systemid","resourceselector"). This inconsistency could cause downstream code relying on field names to fail if it expects camelCase.Also, line 16 returns
{"all": true}for any nil input. Verify this is the intended behavior—should it distinguish betweenold == nil,new == nil, or both?Does the field naming need to be consistent across both code paths (diff-based and fallback)? Review how consumers of
HasDeploymentChangesexpect field keys to be formatted.
30-30: No action needed — functions are accessible.Both
convertPathToFieldName(defined at resource.go:76) anddeepEqual(defined at resource.go:137) are in the samediffcheckpackage, making them directly accessible to deployment.go. No issues found.apps/workspace-engine/pkg/workspace/store/policy.go (2)
29-29: Unused context parameter requires API-wide refactoring; verify if removal is intended.The
ctxparameter is indeed unused in the method body. However, removing it from the public method signature would require updating 100+ call sites across the codebase (test files, event handlers, and other store methods). This is a breaking change that extends far beyond the local scope of this file.Before removing the parameter, verify:
- Whether this method is part of a versioned or stable API
- Whether the parameter is intentionally retained for forward compatibility
- Whether the removal justifies the cascading updates across the codebase
Note: Similar methods in the store (e.g.,
Releases.Upsert,RelationshipRules.Upsert) usechangeset.FromContext(ctx), while others (e.g.,Policies.Upsert,Jobs.Upsert) do not. This pattern suggests intentional design choices, not universal oversight.
21-23: The review comment is incorrect. The code contains no type inconsistency or compilation errors.
Map[T any]is a type alias formap[string]Twith receiver methods (Get, Set, Remove) defined on it. This is a valid Go pattern. The fieldp.repo.Policieshas typeMap[*oapi.Policy], which is equivalent tomap[string]*oapi.Policy, and the method calls.Get(),.Set(), and.Remove()are valid receiver method calls on this type.The return type on line 22 is correct, and the code compiles without errors. The suggested diffs would break the code by replacing valid method calls with invalid direct map access.
Likely an incorrect or invalid review comment.
apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go (1)
32-32: LGTM! Consistent API usage update.The removal of the context parameter from
Items()calls is applied consistently across all three methods. The change aligns with the broader refactoring to simplify the store API.Also applies to: 39-39, 94-94
apps/workspace-engine/pkg/workspace/store/deployment_variable_values.go (1)
18-35: LGTM! Simplified API with preserved changeset tracking.The refactoring correctly simplifies
Items()to return the underlying map directly while preserving changeset recording inUpsertandRemoveoperations. The change aligns with the broader removal of context-based changeset handling.apps/workspace-engine/pkg/workspace/store/deployment_versions.go (1)
21-42: LGTM! Consistent refactoring pattern.The changes mirror the refactoring in
deployment_variable_values.go: simplifiedItems()access while maintaining changeset tracking. The implementation is consistent and correct.apps/workspace-engine/pkg/workspace/releasemanager/versions/manager.go (1)
31-42: Verify span attribute semantics change is intentional.The refactoring moves span attribute recording to before filtering (line 34), so
versions.countnow represents the total number of deployment versions rather than the filtered count. This changes the observability semantics compared to the previous implementation.If the intent is to track filtered versions, move the span attribute after filtering:
allVersions := m.store.DeploymentVersions.Items() - span.SetAttributes( - attribute.Int("versions.count", len(allVersions)), - ) - filtered := []*oapi.DeploymentVersion{} for _, version := range allVersions { if version.DeploymentId == releaseTarget.DeploymentId { filtered = append(filtered, version) } } + + span.SetAttributes( + attribute.Int("versions.count", len(filtered)), + ) // Sort by CreatedAt (descending: newest first), then by Id (descending) if CreatedAt is the sameapps/workspace-engine/pkg/events/handler/handler.go (1)
173-179: Verify removal of release target processing is intentional.The release target change processing has been completely disabled. This appears intentional as part of the broader refactoring to remove context-based changeset handling, but it represents a significant functional change.
If this code is permanently removed, consider deleting it entirely rather than leaving it commented out:
- // releaseTargetChanges, err := ws.ReleaseManager().ProcessChanges(ctx, changeSet) - // if err != nil { - // span.RecordError(err) - // span.SetStatus(codes.Error, "failed to process target changes") - // log.Error("Failed to process target changes", "error", err) - // return ws, fmt.Errorf("failed to process target changes: %w", err) - // } -Alternatively, if this functionality needs to be restored later, document the rationale with a TODO comment.
apps/workspace-engine/pkg/events/handler/tick/tick.go (1)
56-56: LGTM! Consistent with API refactoring.The removal of the context parameter from
Items()is consistent with the same change applied across the codebase.apps/workspace-engine/pkg/workspace/store/systems.go (1)
21-36: Systems upsert/removal simplification looks solid.Direct repository writes plus changeset bookkeeping keep the behavior intact and align with the broader refactor. No further changes needed here.
apps/workspace-engine/pkg/workspace/store/resources.go (1)
27-185: Resource store refactor reads clean.The trimmed recompute pipeline and the new selector helpers make the resource path much easier to follow while preserving changeset tracking.
apps/workspace-engine/pkg/workspace/store/environments.go (1)
21-44: Environment store cleanup looks good.Delegating straight to the repository with changeset recording retains prior semantics without the extra materialized-view plumbing.
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
14-105: Release target simplification makes sense.The direct in-memory map with changeset updates keeps the API straightforward and aligns with the rest of the refactor.
apps/workspace-engine/pkg/events/handler/resources/resources.go
Outdated
Show resolved
Hide resolved
| vars := make(map[string]*oapi.DeploymentVariable) | ||
| // Use IterCb for more efficient iteration (no channel overhead) | ||
| e.repo.DeploymentVariables.IterCb(func(key string, variable *oapi.DeploymentVariable) { | ||
| for _, variable := range e.repo.DeploymentVariables { | ||
| if variable.DeploymentId == deploymentId { | ||
| vars[variable.Key] = variable | ||
| vars[variable.Id] = variable | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Restore variable map keyed by Key.
Variables now indexes the result map by variable.Id. Callers historically look up deployment variables by their logical name (DeploymentVariable.Key), so this silently breaks those lookups and returns an unusable map of opaque IDs. Please keep the map keyed by variable.Key (and guard against nil entries if needed).
Apply this diff:
func (e *Deployments) Variables(deploymentId string) map[string]*oapi.DeploymentVariable {
vars := make(map[string]*oapi.DeploymentVariable)
for _, variable := range e.repo.DeploymentVariables {
- if variable.DeploymentId == deploymentId {
- vars[variable.Id] = variable
- }
+ if variable != nil && variable.DeploymentId == deploymentId {
+ vars[variable.Key] = variable
+ }
}
return vars
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vars := make(map[string]*oapi.DeploymentVariable) | |
| // Use IterCb for more efficient iteration (no channel overhead) | |
| e.repo.DeploymentVariables.IterCb(func(key string, variable *oapi.DeploymentVariable) { | |
| for _, variable := range e.repo.DeploymentVariables { | |
| if variable.DeploymentId == deploymentId { | |
| vars[variable.Key] = variable | |
| vars[variable.Id] = variable | |
| } | |
| }) | |
| } | |
| vars := make(map[string]*oapi.DeploymentVariable) | |
| for _, variable := range e.repo.DeploymentVariables { | |
| if variable != nil && variable.DeploymentId == deploymentId { | |
| vars[variable.Key] = variable | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/deployments.go around lines 52 to
57, the map of deployment variables is currently keyed by variable.Id but
callers expect keys to be the logical variable.Key; change the map to use
variable.Key as the map key (e.g., vars[variable.Key] = variable) and skip or
safely handle nil/empty variable or variable.Key values to avoid panics (guard
before inserting). Ensure the rest of function behavior is unchanged.
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/store/releases.go (1)
22-59: Consider adding documentation for exported methods.Per Go conventions and the coding guidelines, exported methods should have doc comments explaining their purpose. Consider adding documentation for
Upsert,Get,Items,Remove, andJobs.Based on coding guidelines.
📜 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 (5)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go(2 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.go(5 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary.go(1 hunks)apps/workspace-engine/pkg/workspace/store/releases.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/store/releases.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go
🧠 Learnings (6)
📚 Learning: 2025-04-21T18:34:54.764Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 500
File: packages/db/src/schema/job.ts:117-120
Timestamp: 2025-04-21T18:34:54.764Z
Learning: In the system's job schema, the relationship between job and releaseJob is a true one-to-one relationship - a release job should only ever point to one job and vice versa. The implementation uses `one(releaseJob, ...)` in the jobRelations to reflect this business rule.
Applied to files:
apps/workspace-engine/pkg/workspace/store/releases.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/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.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 : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.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/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.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 : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/workspace/store/releases.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Job(266-278)ReleaseId(62-62)Id(58-58)apps/workspace-engine/pkg/workspace/store/jobs.go (1)
Jobs(17-20)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(22-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: tests
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: workspace-engine-tests
🔇 Additional comments (7)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go (1)
429-430: Test adaptations to context-free Items() API look correct.The removal of context from
Items()calls aligns with the broader API simplification in this PR.Also applies to: 794-795
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.go (1)
141-142: Test updates correctly reflect the context-free Items() API.All test calls to
Items()have been consistently updated to remove the context parameter, maintaining proper error handling.Also applies to: 265-266, 377-378, 478-479, 579-580
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary.go (1)
67-67: Let me verify the full context of theEvaluatemethod and surrounding code:No context propagation needed for in-memory cache accessor.
The
Items()method is a simple getter that returns an in-memory map (return r.targets, nil). It performs no I/O, external calls, or long-running computations, so context propagation would provide no functional benefit. The method signature doesn't accept context, and adding it would require a broader API change unrelated to the core concern at hand.Likely an incorrect or invalid review comment.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.go (1)
25-25: No changes needed—Items() is a simple in-memory getter with no I/O operations.Items() returns cached data from memory with no blocking, network, or database operations. Context is unnecessary for this trivial accessor. The tracing span created in getReleaseTargets already provides observability; Items() doesn't perform work requiring context propagation.
However, there is a compile error: gradualrollout.go:256 calls
Items(ctx)but the method signature accepts no context parameter.Likely an incorrect or invalid review comment.
apps/workspace-engine/pkg/workspace/store/releases.go (3)
40-48: Simplification looks good.The removal of context-based changeset checking aligns with the PR's objective to simplify changeset wiring. The logic correctly handles the case where the release doesn't exist.
50-59: LGTM - cleaner implementation.The refactored implementation is more straightforward, directly iterating over the jobs map and filtering by
ReleaseId. The logic correctly builds a result map keyed byjob.Id.
36-38: Clarify the architectural shift from concurrent to plain maps and verify synchronization strategy.The PR simplifies store logic by replacing
cmap.ConcurrentMapwith plainMap[T]aliases. TheItems()method now returns the internal map directly (previously called.Items()on a concurrent-safe wrapper), andJobs()uses direct iteration instead ofIterBuffered().Verify:
- Is single-threaded-only access enforced elsewhere (e.g., caller must serialize all store operations)?
- How do concurrent goroutines interact with this store, if at all?
Without synchronization guarantees at the Store level, exposing plain maps directly to callers creates potential data races. If this is intentional, document the concurrency model.
|
|
||
| func getReleaseTargets(ctx context.Context, store *store.Store, version *oapi.DeploymentVersion, environment *oapi.Environment) []*oapi.ReleaseTarget { | ||
| ctx, span := jobTrackerTracer.Start(ctx, "getReleaseTargets", trace.WithAttributes( | ||
| _, span := jobTrackerTracer.Start(ctx, "getReleaseTargets", trace.WithAttributes( |
There was a problem hiding this comment.
Discarding span context breaks trace propagation.
The tracing span is created with Start(ctx, ...), which returns an updated context containing trace metadata. Discarding this context (using _) prevents the trace from propagating to child operations, breaking distributed tracing.
Apply this diff to preserve the trace context:
- _, span := jobTrackerTracer.Start(ctx, "getReleaseTargets", trace.WithAttributes(
+ ctx, span := jobTrackerTracer.Start(ctx, "getReleaseTargets", trace.WithAttributes(
attribute.String("environment.id", environment.Id),
attribute.String("deployment.id", version.DeploymentId),
attribute.String("version.id", version.Id),
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, span := jobTrackerTracer.Start(ctx, "getReleaseTargets", trace.WithAttributes( | |
| ctx, span := jobTrackerTracer.Start(ctx, "getReleaseTargets", trace.WithAttributes( | |
| attribute.String("environment.id", environment.Id), | |
| attribute.String("deployment.id", version.DeploymentId), | |
| attribute.String("version.id", version.Id), | |
| )) |
🤖 Prompt for AI Agents
In
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.go
around line 18, the trace Start call discards the returned context which breaks
propagation; capture the returned context (e.g., ctx, span :=
jobTrackerTracer.Start(ctx, "getReleaseTargets", ...)) instead of using `_`,
assign the updated context to be used for downstream operations and ensure the
span is ended (defer span.End()) so child calls use the propagated trace
context.
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/server/openapi/deployments/server.go (1)
58-78: Restore deployment-variable values lookup correctness
valuesByVariableIdis populated usingvalue.DeploymentVariableId, but this lookup now usesvariable.Key. Unless every variable’s key equals its ID, we’ll never attach any values and the API regresses to always returning an emptyValuesslice. Please index by the variable’s ID instead.- values := valuesByVariableId[variable.Key] + values := valuesByVariableId[variable.Id]
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/store/deployments.go (1)
53-61: Map key should bevariable.Key, notvariable.Id.This issue was previously flagged: callers expect deployment variables to be keyed by their logical name (
variable.Key), but the map is currently keyed byvariable.Id. This breaks lookups.Apply this diff:
func (e *Deployments) Variables(deploymentId string) map[string]*oapi.DeploymentVariable { vars := make(map[string]*oapi.DeploymentVariable) for _, variable := range e.repo.DeploymentVariables { - if variable.DeploymentId == deploymentId { - vars[variable.Id] = variable - } + if variable != nil && variable.DeploymentId == deploymentId { + vars[variable.Key] = variable + } } return vars }
🧹 Nitpick comments (4)
apps/workspace-engine/test/e2e/engine_system_test.go (2)
403-403: Consider checking error returns from Items().The error return from
ReleaseTargets().Items()is ignored. While in-memory stores rarely produce errors, explicitly checking (or at least asserting nil in tests) would improve robustness and catch unexpected issues.Example for line 403:
- releaseTargets, _ := engine.Workspace().ReleaseTargets().Items() + releaseTargets, err := engine.Workspace().ReleaseTargets().Items() + if err != nil { + t.Fatalf("failed to get release targets: %v", err) + }Also applies to: 492-492, 506-506
462-465: Remove duplicate verification.Lines 463-465 duplicate the exact same check performed at lines 458-460. The second verification is redundant.
Apply this diff:
// Verify system is removed if _, ok := engine.Workspace().Systems().Get(systemId); ok { t.Fatalf("system should be removed after deletion") } - - // Verify Get returns not found - if _, ok := engine.Workspace().Systems().Get(systemId); ok { - t.Fatalf("Get should return not found after deletion") - }apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
108-139: Consider removing redundant selector nil-check.Lines 126-128 pre-filter selectors that have any nil component, but
selector.MatchPolicyalready performs the same nil checks internally. This duplication could drift if the matching logic evolves.Apply this diff to remove the redundant check:
for _, policy := range r.store.Policies.Items() { for _, sel := range policy.Selectors { - if sel.ResourceSelector == nil || sel.EnvironmentSelector == nil || sel.DeploymentSelector == nil { - continue - } - isMatch := selector.MatchPolicy(ctx, policy, selector.NewResolvedReleaseTarget(environment, deployment, resource)) if isMatch { policiesSlice = append(policiesSlice, policy)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
67-89: Consider a direct slice-to-slice filter for efficiency.The method converts resources slice → map (inside
selector.FilterResources) → slice. While functionally correct, a dedicatedFilterResourcesSlicehelper would eliminate the round-trip conversion.
📜 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 (28)
apps/workspace-engine/pkg/persistence/integration_test.go(4 hunks)apps/workspace-engine/pkg/selector/resources.go(2 hunks)apps/workspace-engine/pkg/server/openapi/deployments/server.go(3 hunks)apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go(1 hunks)apps/workspace-engine/pkg/server/openapi/environments/server.go(2 hunks)apps/workspace-engine/pkg/server/openapi/policies/policies.go(2 hunks)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go(1 hunks)apps/workspace-engine/pkg/workspace/store/deployments.go(2 hunks)apps/workspace-engine/pkg/workspace/store/environments.go(1 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(2 hunks)apps/workspace-engine/pkg/workspace/store/resource_variables.go(1 hunks)apps/workspace-engine/pkg/workspace/store/systems.go(1 hunks)apps/workspace-engine/test/e2e/engine_bench_test.go(2 hunks)apps/workspace-engine/test/e2e/engine_deployment_test.go(8 hunks)apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go(4 hunks)apps/workspace-engine/test/e2e/engine_deployment_version_test.go(4 hunks)apps/workspace-engine/test/e2e/engine_environment_selector_update_test.go(4 hunks)apps/workspace-engine/test/e2e/engine_environments_bench_test.go(8 hunks)apps/workspace-engine/test/e2e/engine_jobs_test.go(10 hunks)apps/workspace-engine/test/e2e/engine_policy_test.go(8 hunks)apps/workspace-engine/test/e2e/engine_presistence_test.go(11 hunks)apps/workspace-engine/test/e2e/engine_redeploy_test.go(10 hunks)apps/workspace-engine/test/e2e/engine_release_target_test.go(24 hunks)apps/workspace-engine/test/e2e/engine_releasemanager_test.go(3 hunks)apps/workspace-engine/test/e2e/engine_releasetarget_state_test.go(7 hunks)apps/workspace-engine/test/e2e/engine_selector_and_conditions_test.go(4 hunks)apps/workspace-engine/test/e2e/engine_system_test.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/workspace-engine/test/e2e/engine_presistence_test.go
- apps/workspace-engine/pkg/workspace/store/resource_variables.go
- apps/workspace-engine/pkg/persistence/integration_test.go
🧰 Additional context used
📓 Path-based instructions (2)
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/releasemanager/policy/evaluator/gradualrollout/gradualrollout.goapps/workspace-engine/pkg/server/openapi/deploymentversions/server.goapps/workspace-engine/test/e2e/engine_releasetarget_state_test.goapps/workspace-engine/test/e2e/engine_redeploy_test.goapps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_deployment_version_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_releasemanager_test.goapps/workspace-engine/test/e2e/engine_jobs_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/pkg/workspace/store/environments.goapps/workspace-engine/pkg/server/openapi/environments/server.goapps/workspace-engine/test/e2e/engine_system_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.goapps/workspace-engine/test/e2e/engine_deployment_test.goapps/workspace-engine/pkg/server/openapi/deployments/server.goapps/workspace-engine/test/e2e/engine_bench_test.goapps/workspace-engine/test/e2e/engine_environments_bench_test.goapps/workspace-engine/pkg/server/openapi/policies/policies.goapps/workspace-engine/pkg/workspace/store/release_targets.goapps/workspace-engine/pkg/workspace/store/systems.goapps/workspace-engine/pkg/selector/resources.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.goapps/workspace-engine/test/e2e/engine_policy_test.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/test/e2e/engine_releasetarget_state_test.goapps/workspace-engine/test/e2e/engine_redeploy_test.goapps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_deployment_version_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_releasemanager_test.goapps/workspace-engine/test/e2e/engine_jobs_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/test/e2e/engine_system_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.goapps/workspace-engine/test/e2e/engine_deployment_test.goapps/workspace-engine/test/e2e/engine_bench_test.goapps/workspace-engine/test/e2e/engine_environments_bench_test.goapps/workspace-engine/test/e2e/engine_policy_test.go
🧠 Learnings (13)
📚 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/server/openapi/deploymentversions/server.goapps/workspace-engine/test/e2e/engine_releasetarget_state_test.goapps/workspace-engine/test/e2e/engine_deployment_version_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_jobs_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/test/e2e/engine_system_test.goapps/workspace-engine/test/e2e/engine_deployment_test.goapps/workspace-engine/pkg/server/openapi/deployments/server.goapps/workspace-engine/pkg/workspace/store/deployments.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/test/e2e/engine_releasetarget_state_test.goapps/workspace-engine/test/e2e/engine_redeploy_test.goapps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_deployment_version_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_releasemanager_test.goapps/workspace-engine/test/e2e/engine_jobs_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/test/e2e/engine_system_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.goapps/workspace-engine/test/e2e/engine_deployment_test.goapps/workspace-engine/test/e2e/engine_policy_test.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/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.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 : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/test/e2e/engine_system_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.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 : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_release_target_test.goapps/workspace-engine/test/e2e/engine_environment_selector_update_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/test/e2e/engine_release_target_test.go
📚 Learning: 2024-10-29T02:04:50.312Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Applied to files:
apps/workspace-engine/test/e2e/engine_selector_and_conditions_test.goapps/workspace-engine/pkg/server/openapi/deployments/server.go
📚 Learning: 2024-10-30T23:10:58.869Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 187
File: apps/jobs/src/ephemeral-env-checker/index.ts:57-0
Timestamp: 2024-10-30T23:10:58.869Z
Learning: In the codebase, deployments are decoupled from environments. When deleting environments (e.g., in `apps/jobs/src/ephemeral-env-checker/index.ts`), associated deployments should not be deleted.
Applied to files:
apps/workspace-engine/test/e2e/engine_release_target_test.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 : 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/test/e2e/engine_system_test.goapps/workspace-engine/test/e2e/engine_bench_test.goapps/workspace-engine/pkg/workspace/store/systems.goapps/workspace-engine/test/e2e/engine_policy_test.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/test/e2e/engine_deployment_version_jobs_list_test.goapps/workspace-engine/test/e2e/engine_bench_test.go
📚 Learning: 2025-10-07T16:44:54.938Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 673
File: apps/workspace-engine/pkg/db/deployment_variables.go:51-65
Timestamp: 2025-10-07T16:44:54.938Z
Learning: In `apps/workspace-engine/pkg/db/deployment_variables.go`, the `default_value_id` field is intentionally scanned from the database but not assigned to the `DefaultValueId` field in the proto struct. This is a temporary decision and the field assignment is deferred for later implementation.
Applied to files:
apps/workspace-engine/pkg/server/openapi/deployments/server.goapps/workspace-engine/pkg/workspace/store/deployments.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/types.go : Add the new condition type to pkg/model/selector/types.go
Applied to files:
apps/workspace-engine/pkg/selector/resources.go
📚 Learning: 2025-04-28T18:41:58.813Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.813Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployments.go
🧬 Code graph analysis (20)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_releasetarget_state_test.go (2)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_redeploy_test.go (2)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_selector_and_conditions_test.go (2)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_releasemanager_test.go (3)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_jobs_test.go (3)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/store/environments.go (5)
apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
InMemoryStore(72-96)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Environment(205-212)Id(58-58)Resource(475-489)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/resources.go (1)
FilterResources(12-26)
apps/workspace-engine/pkg/server/openapi/environments/server.go (2)
apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_system_test.go (5)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/systems.go (1)
Systems(16-19)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/events/handler/handler.go (1)
SystemDelete(61-61)
apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go (2)
apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_deployment_test.go (6)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/server/openapi/deployments/server.go (1)
Deployments(37-37)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)
apps/workspace-engine/test/e2e/engine_bench_test.go (3)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_environments_bench_test.go (2)
apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)
apps/workspace-engine/pkg/server/openapi/policies/policies.go (2)
apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/store/release_targets.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
ReleaseTarget(445-449)Policy(342-355)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/selector/policies.go (2)
MatchPolicy(34-57)NewResolvedReleaseTarget(8-14)
apps/workspace-engine/pkg/workspace/store/systems.go (5)
apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
InMemoryStore(72-96)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
System(548-553)Id(58-58)Deployment(129-138)Environment(205-212)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)
apps/workspace-engine/pkg/selector/resources.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Selector(535-537)Resource(475-489)apps/workspace-engine/pkg/selector/match.go (1)
Matchable(27-62)
apps/workspace-engine/pkg/workspace/store/deployments.go (4)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Id(58-58)DeploymentVariable(147-153)Deployment(129-138)Resource(475-489)apps/workspace-engine/pkg/workspace/store/variables.go (1)
Variables(11-14)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/resources.go (1)
FilterResources(12-26)
apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/test/e2e/engine_policy_test.go (2)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/workspace/store/policy.go (1)
Policies(16-19)
⏰ 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: tests
- GitHub Check: Format
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: workspace-engine-tests
🔇 Additional comments (22)
apps/workspace-engine/test/e2e/engine_system_test.go (2)
24-24: LGTM! Simplified API usage.The removal of context from
Items()aligns with the PR's simplification objectives and makes the API cleaner.
314-314: LGTM! Idiomatic Get() pattern.The refactoring from
Has()toGet()with ok checks is idiomatic Go and avoids potential double-lookup patterns. The consistent application across all tests is appropriate.Also applies to: 329-329, 450-450, 458-458, 501-501, 540-540, 543-543, 546-546, 554-554, 557-557, 560-560, 568-568, 571-571, 574-574, 608-608, 650-650
apps/workspace-engine/test/e2e/engine_redeploy_test.go (1)
53-53: LGTM: Consistent API update across all test cases.The removal of the context parameter from
ReleaseTargets().Items()calls is applied consistently across all test functions. This aligns with the broader simplification effort where the ReleaseTargets store now uses simple in-memory maps instead of context-bound operations.Also applies to: 168-168, 269-269, 372-372, 429-429, 550-550, 648-648, 729-729, 855-855, 927-927
apps/workspace-engine/pkg/server/openapi/policies/policies.go (1)
75-81: LGTM: Proper error handling maintained.The API update removes the context parameter while maintaining proper error handling. The error return from
Items()is correctly checked and returned to the client.apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go (1)
56-68: LGTM: Error handling properly maintained.The context parameter removal is correctly applied, and the error return is properly handled.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
251-269: LGTM: Consistent with API simplification.The context parameter removal is correctly applied in the
getReleaseTargetshelper method, with proper error propagation maintained.apps/workspace-engine/test/e2e/engine_bench_test.go (1)
354-354: LGTM: Benchmark statistics collection updated correctly.The API changes are correctly applied in the benchmark statistics reporting. Ignoring errors is acceptable here since these calls are for final statistics collection after the benchmark execution.
Also applies to: 461-461
apps/workspace-engine/test/e2e/engine_releasemanager_test.go (1)
87-87: LGTM: Test assertions updated consistently.The context parameter removal is applied consistently across all release manager test cases. The changes maintain the existing test structure and validation logic.
Also applies to: 323-323, 484-484
apps/workspace-engine/test/e2e/engine_environment_selector_update_test.go (1)
70-76: LGTM: Critical test scenarios properly maintained.The API changes are correctly applied in these important tests that verify job cancellation behavior when selectors change. The error handling in these tests is appropriate and helps ensure test reliability.
Also applies to: 116-122, 470-476, 512-518
apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go (1)
76-79: LGTM: Jobs list tests updated correctly.The removal of the context parameter is consistently applied across all test cases, with appropriate error handling maintained. The test logic for verifying release target creation and job creation remains intact.
Also applies to: 127-130, 178-181, 354-357
apps/workspace-engine/pkg/workspace/store/release_targets.go (7)
15-21: LGTM!The simplified initialization using a plain map is clean and removes complexity from the previous materialized-view approach.
23-27: LGTM!The struct simplification aligns well with the refactor goals.
30-32: LGTM!Removing the unused context parameter simplifies the API appropriately.
34-38: LGTM!The Upsert implementation correctly stores the target and records the change.
48-51: LGTM!The Remove implementation properly records the deletion before removing from the map.
53-106: LGTM!The business logic remains unchanged and correctly operates on the new in-memory structure.
40-46: Nil handling verified across all call sites.All three callers of
ReleaseTargets().Get()properly check forniland return a 404 response when the release target is not found. The API change is correctly handled throughout the codebase.apps/workspace-engine/pkg/workspace/store/deployments.go (5)
15-22: LGTM!The simplified initialization aligns with the removal of materialized views.
24-27: LGTM!The struct simplification is appropriate for the refactor.
33-41: LGTM!The simplified Upsert correctly stores the deployment and records the change without unnecessary complexity.
43-51: LGTM!The simplified Remove properly handles deletion.
63-65: LGTM!Direct map return is clean and appropriate.
| // If no selector is provided, return empty slice | ||
| if sel == nil { | ||
| return []T{}, nil | ||
| } |
There was a problem hiding this comment.
Keep nil selector semantics intact
Returning []T{} when sel is nil drops every resource whenever the caller omits a selector. Callers routinely pass nil to mean “no filtering” (for example, FilterResources forwards sel directly), so this change empties those responses. Please return the original resources slice in this branch and mirror the same adjustment in FilterConcurrent.
- if sel == nil {
- return []T{}, nil
- }
+ if sel == nil {
+ return append([]T(nil), resources...), nil
+ }🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/selector/resources.go around lines 52 to 55, the
function currently returns an empty slice when sel == nil which changes the
semantics callers expect (nil meaning “no filter”); change that branch to return
the original resources slice (return resources, nil) instead of []T{} and apply
the identical fix in the FilterConcurrent implementation so both functions
preserve the original resources when sel is nil.
| policies := map[string]*oapi.Policy{} | ||
|
|
||
| rt, _ := ws.ReleaseTargets().Items(c.Request.Context()) | ||
| rt, _ := ws.ReleaseTargets().Items() |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error handling is acceptable.
The error from Items() is being ignored here. This might be intentional if errors are not expected from the in-memory map implementation, but consider whether error handling should be added for consistency.
🏁 Script executed:
#!/bin/bash
# Check if error handling for Items() is consistent across the codebase
rg -n -C3 'ReleaseTargets\(\)\.Items\(\)' --type=go -g '!*_test.go' | head -50Length of output: 4019
🏁 Script executed:
#!/bin/bash
# Get full function context around line 187 to understand purpose and constraints
sed -n '160,210p' apps/workspace-engine/pkg/server/openapi/policies/policies.goLength of output: 1265
🏁 Script executed:
#!/bin/bash
# Check if ReleaseTargets().Items() ever returns an error in the implementation
fd --name "*.go" -g '!*_test.go' | xargs grep -l "func.*Items()" | head -5Length of output: 242
🏁 Script executed:
#!/bin/bash
# Check type definition and Items() method signature
rg -n "func.*Items\(\)" --type=go -g '!*_test.go' -A3 | head -30Length of output: 2133
🏁 Script executed:
#!/bin/bash
# Look for any comments near line 187 that might explain error suppression
sed -n '180,195p' apps/workspace-engine/pkg/server/openapi/policies/policies.goLength of output: 494
🏁 Script executed:
#!/bin/bash
# Check if there are any TODOs or FIXMEs around this error suppression
sed -n '175,200p' apps/workspace-engine/pkg/server/openapi/policies/policies.go | grep -i "TODO\|FIXME\|ignore\|suppress"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get a broader view of the function containing line 187
sed -n '140,210p' apps/workspace-engine/pkg/server/openapi/policies/policies.goLength of output: 1749
🏁 Script executed:
#!/bin/bash
# Check whether this file has any pattern of intentional error suppression
rg -n "_\s*:=.*\(\)" apps/workspace-engine/pkg/server/openapi/policies/policies.go --type=goLength of output: 41
🏁 Script executed:
#!/bin/bash
# Verify consistency: how many error suppressions vs error handling in this file
echo "=== Error handling patterns in policies.go ===" && \
rg -n "err\s*:=" apps/workspace-engine/pkg/server/openapi/policies/policies.go --type=go | wc -l && \
echo "Error suppressions:" && \
rg -n "_\s*:=.*\(\)" apps/workspace-engine/pkg/server/openapi/policies/policies.go --type=go | wc -lLength of output: 70
Add error handling for consistency with all other ReleaseTargets().Items() call sites in the codebase.
Line 187 suppresses the error from Items(), which is inconsistent with 5+ other locations in the codebase (deploymentversions/server.go:58, environments/server.go:171, deployments/server.go:18, tick/tick.go:56, and policies.go:75) that properly handle it. Although the current implementation always returns nil, the method signature declares an error return type, making this defensive programming critical. Change rt, _ := ws.ReleaseTargets().Items() to properly assign and handle the error:
rt, err := ws.ReleaseTargets().Items()
if err != nil {
// handle error (log, return, continue, etc.)
}🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/server/openapi/policies/policies.go around line
187, the call to ReleaseTargets().Items() currently discards the error (rt, _ :=
...); change it to capture the error (rt, err := ...) and handle it consistently
with other call sites — e.g., check if err != nil then log the error and return
or propagate it as appropriate for this function's behavior to avoid silently
ignoring potential errors.
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/events/handler/resources/resources.go (1)
134-148: Resource updates still skip reconciling surviving release targetsThe new delta logic only reconciles freshly added targets. When a resource changes but continues to match the same deployment/environment combo (the common case), its release targets stay in place and never get reprocessed. Previously
TaintDependentReleaseTargetsOnChangetriggered that refresh; now those release targets hold stale config and downstream releases never see the update. Please reintroduce reconciliation for the targets that remain after the diff—either by restoring the taint hook or explicitly looping the surviving targets and callingReconcileTargeton them—before returning from the handler.+ currentByKey := make(map[string]*oapi.ReleaseTarget, len(releaseTargets)) + for _, target := range releaseTargets { + currentByKey[target.Key()] = target + } + + for _, oldTarget := range oldReleaseTargets { + if _, stillPresent := currentByKey[oldTarget.Key()]; stillPresent { + ws.ReleaseManager().ReconcileTarget(ctx, oldTarget, false) + } + }
🧹 Nitpick comments (7)
apps/workspace-engine/pkg/workspace/store/environments.go (2)
38-46: Simplify redundant nil check.The condition
!ok || env == nilis redundant. When a map lookup returnsok == false, the value is guaranteed to benilfor pointer types. Theenv == nilcheck adds no additional safety.Apply this diff to simplify:
func (e *Environments) Remove(ctx context.Context, id string) { env, ok := e.Get(id) - if !ok || env == nil { + if !ok { return } e.repo.Environments.Remove(id) e.store.changeset.RecordDelete(env) }
48-70: Consider optimizing resource filtering to avoid unnecessary conversions.The current implementation converts map→slice→map→slice:
- Lines 54-57: Convert resources map to slice
- Line 59:
FilterResourcesinternally converts slice→map- Lines 64-67: Convert filtered map back to slice
Based on the relevant code snippets,
selector.Filterreturns a slice directly. Consider using it instead ofFilterResourcesto eliminate the intermediate map conversions.If
selector.Filteris available, apply this approach:func (e *Environments) Resources(ctx context.Context, environmentId string) ([]*oapi.Resource, error) { environment, ok := e.Get(environmentId) if !ok { return nil, fmt.Errorf("environment %s not found", environmentId) } allResourcesSlice := make([]*oapi.Resource, 0) for _, resource := range e.store.Resources.Items() { allResourcesSlice = append(allResourcesSlice, resource) } - resources, err := selector.FilterResources(ctx, environment.ResourceSelector, allResourcesSlice) + resourcesSlice, err := selector.Filter(ctx, environment.ResourceSelector, allResourcesSlice) if err != nil { return nil, err } - resourcesSlice := make([]*oapi.Resource, 0, len(resources)) - for _, resource := range resources { - resourcesSlice = append(resourcesSlice, resource) - } - return resourcesSlice, nil }apps/workspace-engine/pkg/events/handler/environment/environment.go (2)
13-36: Consider performance implications of nested iteration.The nested loops iterate over all deployments and their resources, resulting in O(deployments × resources) complexity. For workspaces with many deployments and resources, this could become a bottleneck.
Consider these optimizations if performance becomes an issue:
- Pre-index resources by deployment ID
- Add early termination if resource matching fails fast
- Batch resource fetching instead of per-deployment calls
Also verify error propagation is correct - errors from
selector.Matchare properly returned at line 24.
63-95: Delta helpers have O(n²) complexity.Both
getRemovedReleaseTargetsandgetAddedReleaseTargetsuse nested loops, resulting in O(n²) complexity. For large numbers of release targets, this could be inefficient.Refactor to use a map for O(n) complexity:
-func getRemovedReleaseTargets(oldReleaseTargets []*oapi.ReleaseTarget, newReleaseTargets []*oapi.ReleaseTarget) []*oapi.ReleaseTarget { - removedReleaseTargets := make([]*oapi.ReleaseTarget, 0) - for _, oldReleaseTarget := range oldReleaseTargets { - found := false - for _, newReleaseTarget := range newReleaseTargets { - if oldReleaseTarget.Key() == newReleaseTarget.Key() { - found = true - break - } - } - if !found { - removedReleaseTargets = append(removedReleaseTargets, oldReleaseTarget) - } - } - return removedReleaseTargets -} +func getRemovedReleaseTargets(oldReleaseTargets []*oapi.ReleaseTarget, newReleaseTargets []*oapi.ReleaseTarget) []*oapi.ReleaseTarget { + newKeys := make(map[string]bool, len(newReleaseTargets)) + for _, rt := range newReleaseTargets { + newKeys[rt.Key()] = true + } + + removedReleaseTargets := make([]*oapi.ReleaseTarget, 0) + for _, oldReleaseTarget := range oldReleaseTargets { + if !newKeys[oldReleaseTarget.Key()] { + removedReleaseTargets = append(removedReleaseTargets, oldReleaseTarget) + } + } + return removedReleaseTargets +} -func getAddedReleaseTargets(oldReleaseTargets []*oapi.ReleaseTarget, newReleaseTargets []*oapi.ReleaseTarget) []*oapi.ReleaseTarget { - addedReleaseTargets := make([]*oapi.ReleaseTarget, 0) - for _, newReleaseTarget := range newReleaseTargets { - found := false - for _, oldReleaseTarget := range oldReleaseTargets { - if oldReleaseTarget.Key() == newReleaseTarget.Key() { - found = true - break - } - } - if !found { - addedReleaseTargets = append(addedReleaseTargets, newReleaseTarget) - } - } - return addedReleaseTargets -} +func getAddedReleaseTargets(oldReleaseTargets []*oapi.ReleaseTarget, newReleaseTargets []*oapi.ReleaseTarget) []*oapi.ReleaseTarget { + oldKeys := make(map[string]bool, len(oldReleaseTargets)) + for _, rt := range oldReleaseTargets { + oldKeys[rt.Key()] = true + } + + addedReleaseTargets := make([]*oapi.ReleaseTarget, 0) + for _, newReleaseTarget := range newReleaseTargets { + if !oldKeys[newReleaseTarget.Key()] { + addedReleaseTargets = append(addedReleaseTargets, newReleaseTarget) + } + } + return addedReleaseTargets +}Additionally, verify if similar delta logic exists in other handler files (mentioned in AI summary) to avoid code duplication:
#!/bin/bash # Search for similar delta computation patterns echo "=== Searching for similar getRemoved/getAdded patterns ===" rg -nP 'func get(Removed|Added)ReleaseTargets' --type=go echo "" echo "=== Searching for similar nested loop patterns with Key() comparison ===" ast-grep --pattern $'for _, $OLD := range $OLDS { $$$ for _, $NEW := range $NEWS { if $OLD.Key() == $NEW.Key() { $$$ } } $$$ }'apps/workspace-engine/pkg/workspace/store/resources.go (1)
165-177: Consider logging selector matching errors.The method silently ignores errors from
selector.Match(line 170). While this prevents a single malformed selector from blocking all results, it hides potential configuration issues. Consider logging these errors to aid debugging.Apply this diff to add error logging:
func (r *Resources) ForSelector(ctx context.Context, sel *oapi.Selector) map[string]*oapi.Resource { resources := make(map[string]*oapi.Resource) for _, resource := range r.Items() { matched, err := selector.Match(ctx, sel, resource) if err != nil { + log.Warn("Selector matching error", "resourceId", resource.Id, "error", err) continue } if matched { resources[resource.Id] = resource } } return resources }apps/workspace-engine/pkg/workspace/store/deployments.go (2)
67-89: Consider simplifying the resource filtering logic.The method converts resources to a slice, filters them to a map, then converts back to a slice. This is inefficient and can be simplified by using
selector.Filterdirectly, which operates on slices and returns a slice.Apply this diff to simplify the implementation:
func (e *Deployments) Resources(ctx context.Context, deploymentId string) ([]*oapi.Resource, error) { deployment, ok := e.Get(deploymentId) if !ok { return nil, fmt.Errorf("deployment %s not found", deploymentId) } allResourcesSlice := make([]*oapi.Resource, 0) for _, resource := range e.store.Resources.Items() { allResourcesSlice = append(allResourcesSlice, resource) } - resources, err := selector.FilterResources(ctx, deployment.ResourceSelector, allResourcesSlice) + resources, err := selector.Filter(ctx, deployment.ResourceSelector, allResourcesSlice) if err != nil { return nil, err } - resourcesSlice := make([]*oapi.Resource, 0, len(resources)) - for _, resource := range resources { - resourcesSlice = append(resourcesSlice, resource) - } - - return resourcesSlice, nil + return resources, nil }
91-103: Consider logging selector matching errors.Similar to
ForSelectorin resources.go, this method silently ignores errors fromselector.Match(line 96). Consider logging these errors to aid debugging selector configuration issues.Apply this diff to add error logging:
func (e *Deployments) ForResource(ctx context.Context, resource *oapi.Resource) ([]*oapi.Deployment, error) { deployments := make([]*oapi.Deployment, 0) for _, deployment := range e.Items() { matched, err := selector.Match(ctx, deployment.ResourceSelector, resource) if err != nil { + log.Warn("Selector matching error", "deploymentId", deployment.Id, "error", err) return nil, err } if matched { deployments = append(deployments, deployment) } } return deployments, nil }
📜 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 (11)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go(4 hunks)apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.go(4 hunks)apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.go(3 hunks)apps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.go(3 hunks)apps/workspace-engine/pkg/events/handler/environment/environment.go(4 hunks)apps/workspace-engine/pkg/events/handler/resources/resources.go(4 hunks)apps/workspace-engine/pkg/workspace/releasemanager/manager.go(3 hunks)apps/workspace-engine/pkg/workspace/store/deployments.go(2 hunks)apps/workspace-engine/pkg/workspace/store/environments.go(1 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(2 hunks)apps/workspace-engine/pkg/workspace/store/resources.go(7 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/deploymentversion/deploymentversion.goapps/workspace-engine/pkg/events/handler/deployment/deployment.goapps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.goapps/workspace-engine/pkg/events/handler/environment/environment.goapps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.goapps/workspace-engine/pkg/events/handler/resources/resources.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/workspace/store/release_targets.goapps/workspace-engine/pkg/workspace/releasemanager/manager.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/workspace/store/environments.go
🧠 Learnings (10)
📚 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/events/handler/deploymentversion/deploymentversion.goapps/workspace-engine/pkg/events/handler/deployment/deployment.goapps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.goapps/workspace-engine/pkg/workspace/releasemanager/manager.goapps/workspace-engine/pkg/workspace/store/deployments.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 : 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/resources.goapps/workspace-engine/pkg/workspace/store/environments.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/resources.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 : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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/types.go : Add the new condition type to pkg/model/selector/types.go
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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 : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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/**/*.go : When adding a new condition type, create a new condition struct in pkg/model/selector implementing the Condition interface
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.go
📚 Learning: 2025-04-28T18:41:58.813Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.813Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployments.go
📚 Learning: 2025-10-07T16:44:54.938Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 673
File: apps/workspace-engine/pkg/db/deployment_variables.go:51-65
Timestamp: 2025-10-07T16:44:54.938Z
Learning: In `apps/workspace-engine/pkg/db/deployment_variables.go`, the `default_value_id` field is intentionally scanned from the database but not assigned to the `DefaultValueId` field in the proto struct. This is a temporary decision and the field assignment is deferred for later implementation.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployments.go
🧬 Code graph analysis (11)
apps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go (7)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Deployment(129-138)ReleaseTarget(445-449)Id(58-58)JobAgentId(60-60)apps/workspace-engine/pkg/workspace/store/systems.go (1)
Systems(16-19)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/environment/environment.go (8)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Environment(205-212)ReleaseTarget(445-449)Id(58-58)apps/workspace-engine/pkg/workspace/store/systems.go (1)
Systems(16-19)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)
apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.go (3)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
DeploymentVariableValue(156-162)ReleaseTarget(445-449)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/resources/resources.go (6)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Resource(475-489)ReleaseTarget(445-449)Id(58-58)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)
apps/workspace-engine/pkg/workspace/store/resources.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (6)
Id(58-58)Resource(475-489)ResourceVariable(501-505)Selector(535-537)Environment(205-212)Deployment(129-138)apps/workspace-engine/pkg/selector/resources.go (1)
Selector(10-10)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
apps/workspace-engine/pkg/workspace/store/release_targets.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
ReleaseTarget(445-449)Policy(342-355)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/selector/policies.go (2)
MatchPolicy(34-57)NewResolvedReleaseTarget(8-14)
apps/workspace-engine/pkg/workspace/releasemanager/manager.go (2)
apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go (1)
Manager(24-29)apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
ReleaseTarget(445-449)
apps/workspace-engine/pkg/workspace/store/deployments.go (6)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Id(58-58)DeploymentVariable(147-153)Deployment(129-138)Resource(475-489)apps/workspace-engine/pkg/workspace/store/variables.go (1)
Variables(11-14)apps/workspace-engine/pkg/workspace/store/deployment_variables.go (1)
DeploymentVariables(16-19)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/resources.go (1)
FilterResources(12-26)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
apps/workspace-engine/pkg/workspace/store/environments.go (6)
apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
InMemoryStore(72-96)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Environment(205-212)Id(58-58)Resource(475-489)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/resources.go (1)
FilterResources(12-26)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
⏰ 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: workspace-engine-tests
- GitHub Check: tests
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (17)
apps/workspace-engine/pkg/workspace/store/environments.go (1)
72-84: LGTM!The
ForResourceimplementation correctly filters environments by matching their selectors against the provided resource. Error handling is appropriate, and the logic is clear.apps/workspace-engine/pkg/workspace/releasemanager/manager.go (3)
195-195: LGTM - Redeploy uses exported ReconcileTarget.The
Redeploymethod correctly uses the exportedReconcileTargetwithforceRedeploy=true.
235-235: Exporting ReconcileTarget increases API surface.The method is now part of the public API. Ensure this is intentional and that external callers understand the concurrency implications, especially since per-target locking has been removed.
122-122: Clarify intent of lock removal: Is ProcessChanges remaining disabled or will concurrency be managed differently?The concern about concurrent reconciliation is technically valid.
ProcessChangesspawns goroutines that callReconcileTargetwithout per-target locking (lines 116-122), and the lock that previously serialized this has been removed (lines 241-247). However,ProcessChangesitself is currently disabled (commented at handler.go:173), so there's no active concurrency risk today.This inconsistency needs clarification:
- If
ProcessChangesremains disabled indefinitely, the lock removal is safe but indicates incomplete cleanup- If
ProcessChangeswill be re-enabled, the removed lock creates a race condition risk that must be addressed (either restore locking, add synchronization at theProcessChangeslevel, or ensure targets cannot appear in multiple goroutines)Verify the architectural intent and ensure concurrency safety is handled explicitly before re-enabling
ProcessChanges.apps/workspace-engine/pkg/events/handler/environment/environment.go (1)
148-157: LGTM - Environment deletion properly cleans up release targets.The cleanup logic correctly removes all release targets associated with the deleted environment.
apps/workspace-engine/pkg/workspace/store/resources.go (7)
6-6: LGTM!The selector import is necessary for the new filtering methods introduced later in the file.
27-37: LGTM!The simplified Upsert flow correctly stores the resource and records the change without triggering complex recomputation logic.
43-56: LGTM!The simplified Remove flow correctly deletes the resource and records the change.
58-69: LGTM!Both methods correctly use direct map access patterns, simplifying the implementation while maintaining correctness.
71-80: LGTM!The Variables method correctly iterates the map and keys by
variable.Key, which is the expected behavior for variable lookups.
89-163: LGTM!The Set method correctly implements the provider-scoped resource replacement logic with efficient O(1) identifier lookups and proper handling of cross-provider conflicts.
179-195: LGTM!All three helper methods correctly implement resource filtering by environment, deployment, and provider respectively.
apps/workspace-engine/pkg/workspace/store/deployments.go (5)
15-22: LGTM!The simplified constructor correctly initializes the Deployments store with minimal state.
33-41: LGTM!The simplified Upsert correctly stores the deployment and records the change without triggering recomputation.
43-51: LGTM!The simplified Remove correctly deletes the deployment and records the change.
53-61: LGTM!The Variables method now correctly keys the map by
variable.Key(line 57), addressing the concern from the previous review.
63-65: LGTM!The Items method correctly returns the deployments map directly.
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, releaseTarget := range releaseTargets { | ||
| ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) | ||
| } |
There was a problem hiding this comment.
Check the error from ReconcileTarget.
The error returned by ReconcileTarget at line 57 is not checked. Reconciliation failures will be silently ignored, which could lead to release targets not being properly deployed.
Apply this diff to handle reconciliation errors:
for _, releaseTarget := range releaseTargets {
- ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false)
+ if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil {
+ return err
+ }
}Alternatively, if you want to continue on error and log failures:
+ var errs []error
for _, releaseTarget := range releaseTargets {
- ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false)
+ if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil {
+ errs = append(errs, fmt.Errorf("failed to reconcile target %s: %w", releaseTarget.Key(), err))
+ }
}
+ if len(errs) > 0 {
+ return fmt.Errorf("reconciliation errors: %v", errs)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | |
| if err != nil { | |
| return err | |
| } | |
| for _, releaseTarget := range releaseTargets { | |
| ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) | |
| } | |
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | |
| if err != nil { | |
| return err | |
| } | |
| for _, releaseTarget := range releaseTargets { | |
| if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/events/handler/environment/environment.go around
lines 52 to 58, the call to ws.ReleaseManager().ReconcileTarget(...) ignores its
returned error; update the loop to check that error after each call and handle
it appropriately — either return the error immediately (e.g., if reconciliation
must stop on failure) or log the failure with context (release target identifier
and error) and continue; if you choose to aggregate errors, collect them and
return a combined error after the loop so failures are not silently dropped.
| oldReleaseTargets, err := ws.ReleaseTargets().GetForEnvironment(ctx, environment.Id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := ws.Environments().Upsert(ctx, environment); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| removedReleaseTargets := getRemovedReleaseTargets(oldReleaseTargets, releaseTargets) | ||
| addedReleaseTargets := getAddedReleaseTargets(oldReleaseTargets, releaseTargets) | ||
|
|
||
| for _, removedReleaseTarget := range removedReleaseTargets { | ||
| ws.ReleaseTargets().Remove(removedReleaseTarget.Key()) | ||
| } | ||
| for _, addedReleaseTarget := range addedReleaseTargets { | ||
| err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false) | ||
| } |
There was a problem hiding this comment.
Check the error from ReconcileTarget and consider reconciling unchanged targets.
Two issues:
-
Missing error check (line 132): Same as in
HandleEnvironmentCreated, the error fromReconcileTargetis not checked. -
Unchanged targets not reconciled: The logic only reconciles newly added targets. Existing targets that remain after the update are not reconciled. If environment properties changed (e.g., resource selector), existing targets might need reconciliation to reflect the new environment configuration.
Apply this diff to fix error handling:
for _, addedReleaseTarget := range addedReleaseTargets {
err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget)
if err != nil {
return err
}
- ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false)
+ if err := ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false); err != nil {
+ return err
+ }
}Consider whether unchanged targets should also be reconciled if environment properties changed:
+ // Reconcile all current targets to reflect environment updates
+ for _, releaseTarget := range releaseTargets {
+ if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil {
+ return err
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oldReleaseTargets, err := ws.ReleaseTargets().GetForEnvironment(ctx, environment.Id) | |
| if err != nil { | |
| return err | |
| } | |
| if err := ws.Environments().Upsert(ctx, environment); err != nil { | |
| return err | |
| } | |
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | |
| if err != nil { | |
| return err | |
| } | |
| removedReleaseTargets := getRemovedReleaseTargets(oldReleaseTargets, releaseTargets) | |
| addedReleaseTargets := getAddedReleaseTargets(oldReleaseTargets, releaseTargets) | |
| for _, removedReleaseTarget := range removedReleaseTargets { | |
| ws.ReleaseTargets().Remove(removedReleaseTarget.Key()) | |
| } | |
| for _, addedReleaseTarget := range addedReleaseTargets { | |
| err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget) | |
| if err != nil { | |
| return err | |
| } | |
| ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false) | |
| } | |
| oldReleaseTargets, err := ws.ReleaseTargets().GetForEnvironment(ctx, environment.Id) | |
| if err != nil { | |
| return err | |
| } | |
| if err := ws.Environments().Upsert(ctx, environment); err != nil { | |
| return err | |
| } | |
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | |
| if err != nil { | |
| return err | |
| } | |
| removedReleaseTargets := getRemovedReleaseTargets(oldReleaseTargets, releaseTargets) | |
| addedReleaseTargets := getAddedReleaseTargets(oldReleaseTargets, releaseTargets) | |
| for _, removedReleaseTarget := range removedReleaseTargets { | |
| ws.ReleaseTargets().Remove(removedReleaseTarget.Key()) | |
| } | |
| for _, addedReleaseTarget := range addedReleaseTargets { | |
| err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget) | |
| if err != nil { | |
| return err | |
| } | |
| if err := ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false); err != nil { | |
| return err | |
| } | |
| } | |
| // Reconcile all current targets to reflect environment updates | |
| for _, releaseTarget := range releaseTargets { | |
| if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/events/handler/environment/environment.go around
lines 107 to 133, the call to ws.ReleaseManager().ReconcileTarget ignores its
returned error and only reconciles newly added targets, missing unchanged ones
that may need reconciliation when the environment changed; update the code to
check and return any error from ReconcileTarget, and reconcile all current
releaseTargets (not just added ones) after Upsert so existing targets reflect
the updated environment—i.e., after computing releaseTargets and applying Upsert
for added targets, iterate over releaseTargets and call ReconcileTarget for
each, checking the error and returning it if non-nil (or aggregate/handle errors
per project convention).
| // targetKey := releaseTarget.Key() | ||
| // lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{}) | ||
| // lock := lockInterface.(*sync.Mutex) | ||
|
|
||
| // Serialize processing for this specific release target | ||
| lock.Lock() | ||
| defer lock.Unlock() | ||
| // // Serialize processing for this specific release target | ||
| // lock.Lock() | ||
| // defer lock.Unlock() |
There was a problem hiding this comment.
Critical: Removed per-target locking breaks atomicity.
Commenting out the per-target lock removes concurrency safety for the three-phase deployment flow (Planning → Eligibility → Execution). This allows multiple goroutines to concurrently process the same release target, which can lead to:
- Race conditions: Two goroutines read the same state, both decide to create a job, and both execute
- Duplicate jobs: Both goroutines pass eligibility checks and create separate jobs for the same target
- Inconsistent state: Concurrent writes to the store without serialization
The three-phase pattern is NOT atomic without this lock. Each phase reads/writes shared state (jobs, releases, release targets), and concurrent execution can violate business logic constraints (e.g., "only one job should be created per reconciliation decision").
If the lock removal is intentional for performance reasons, you need alternative concurrency control:
Option 1: Re-enable the per-target lock (recommended for correctness):
- // targetKey := releaseTarget.Key()
- // lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{})
- // lock := lockInterface.(*sync.Mutex)
+ targetKey := releaseTarget.Key()
+ lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{})
+ lock := lockInterface.(*sync.Mutex)
- // // Serialize processing for this specific release target
- // lock.Lock()
- // defer lock.Unlock()
+ // Serialize processing for this specific release target
+ lock.Lock()
+ defer lock.Unlock()Option 2: If removing the lock is required, add:
- Optimistic concurrency control in the executor (e.g., check-and-set with version numbers)
- Idempotency keys for job creation
- Database-level uniqueness constraints on (releaseTargetKey, releaseID) to prevent duplicate jobs
Verify the concurrency safety with the script below:
#!/bin/bash
# Check if ReconcileTarget is called concurrently from multiple places
echo "=== Searching for ReconcileTarget call sites ==="
rg -nP '\bReconcileTarget\s*\(' --type=go -C5
echo ""
echo "=== Searching for goroutine usage in callers ==="
rg -nP '\bgo func\b' --type=go -A10 | rg -P 'ReconcileTarget|ProcessChanges' -C3
echo ""
echo "=== Checking for concurrent ProcessChanges patterns ==="
ast-grep --pattern $'go func($$$) {
$$$
ProcessChanges($$$)
$$$
}($$$)'🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/releasemanager/manager.go around lines
241-247, the per-target mutex was commented out which breaks atomicity across
the Planning→Eligibility→Execution phases and allows concurrent goroutines to
create duplicate jobs and corrupt state; restore the per-target lock by
re-adding the LoadOrStore creation of a *sync.Mutex for releaseTarget.Key(),
cast it, then wrap the three-phase processing with lock.Lock() and defer
lock.Unlock() so only one goroutine processes a given target at a time; if you
intentionally remove the lock for performance instead, implement alternative
concurrency controls: add check-and-set/versioned updates in the executor,
require idempotency keys on job creation, and enforce a database uniqueness
constraint on (releaseTargetKey, releaseID), plus add a concurrency test that
spawns multiple goroutines calling ReconcileTarget to verify only one job is
created.
| func (e *Environments) Upsert(ctx context.Context, environment *oapi.Environment) error { | ||
| e.repo.Environments.Set(environment.Id, environment) | ||
| e.store.changeset.RecordUpsert(environment) | ||
|
|
||
| _ = mv.WaitRecompute() | ||
| allResources := mv.Get() | ||
| if envResources, ok := allResources[resourceId]; ok { | ||
| return envResources != nil | ||
| } | ||
| return false | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Unused error return.
The Upsert method always returns nil. If validation or persistence errors are not expected, consider changing the signature to omit the error return. Otherwise, add validation logic before persisting.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/environments.go around lines 31 to
36, the Upsert method always returns nil which makes the error return unused;
either remove the error from the method signature (and update any interface and
callers to match) if no error conditions are possible, or add proper
validation/persistence checks before calling e.repo.Environments.Set and
e.store.changeset.RecordUpsert and return descriptive errors when validation or
persistence fails; ensure consistency by updating any interfaces, tests, and
call sites accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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 (12)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go(4 hunks)apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.go(4 hunks)apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.go(3 hunks)apps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.go(3 hunks)apps/workspace-engine/pkg/events/handler/environment/environment.go(4 hunks)apps/workspace-engine/pkg/events/handler/resources/resources.go(4 hunks)apps/workspace-engine/pkg/events/handler/resourcevariables/resource_variables.go(3 hunks)apps/workspace-engine/pkg/workspace/releasemanager/manager.go(3 hunks)apps/workspace-engine/pkg/workspace/store/deployments.go(2 hunks)apps/workspace-engine/pkg/workspace/store/environments.go(1 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(2 hunks)apps/workspace-engine/pkg/workspace/store/resources.go(7 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/deploymentversion/deploymentversion.goapps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.goapps/workspace-engine/pkg/events/handler/environment/environment.goapps/workspace-engine/pkg/workspace/releasemanager/manager.goapps/workspace-engine/pkg/events/handler/resourcevariables/resource_variables.goapps/workspace-engine/pkg/workspace/store/release_targets.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/events/handler/resources/resources.goapps/workspace-engine/pkg/events/handler/deployment/deployment.goapps/workspace-engine/pkg/workspace/store/environments.goapps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.goapps/workspace-engine/pkg/workspace/store/deployments.go
🧠 Learnings (11)
📚 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/events/handler/deploymentversion/deploymentversion.goapps/workspace-engine/pkg/events/handler/deployment/deployment.goapps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.goapps/workspace-engine/pkg/workspace/store/deployments.go
📚 Learning: 2025-08-12T21:56:42.349Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 655
File: apps/workspace-engine/pkg/events/handler/deployment-version/new.go:15-17
Timestamp: 2025-08-12T21:56:42.349Z
Learning: In Go constructor naming conventions, when a struct is named with a "New" prefix (like NewDeploymentVersionHandler), the constructor function should be named "New" + the struct name, resulting in names like NewNewDeploymentVersionHandler(). This is not redundant but follows the standard Go pattern of New<TypeName>() for constructors.
Applied to files:
apps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.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 : 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/resources.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/resources.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 : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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/types.go : Add the new condition type to pkg/model/selector/types.go
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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 : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.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/**/*.go : When adding a new condition type, create a new condition struct in pkg/model/selector implementing the Condition interface
Applied to files:
apps/workspace-engine/pkg/workspace/store/resources.go
📚 Learning: 2025-04-28T18:41:58.813Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.813Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployments.go
📚 Learning: 2025-10-07T16:44:54.938Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 673
File: apps/workspace-engine/pkg/db/deployment_variables.go:51-65
Timestamp: 2025-10-07T16:44:54.938Z
Learning: In `apps/workspace-engine/pkg/db/deployment_variables.go`, the `default_value_id` field is intentionally scanned from the database but not assigned to the `DefaultValueId` field in the proto struct. This is a temporary decision and the field assignment is deferred for later implementation.
Applied to files:
apps/workspace-engine/pkg/workspace/store/deployments.go
🧬 Code graph analysis (12)
apps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.go (4)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
DeploymentVariableValue(156-162)ReleaseTarget(445-449)apps/workspace-engine/pkg/workspace/store/deployment_variables.go (1)
DeploymentVariables(16-19)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/environment/environment.go (4)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Environment(205-212)ReleaseTarget(445-449)Id(58-58)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/releasemanager/manager.go (2)
apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go (1)
Manager(24-29)apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
ReleaseTarget(445-449)
apps/workspace-engine/pkg/events/handler/resourcevariables/resource_variables.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/store/release_targets.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
ReleaseTarget(445-449)Policy(342-355)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/selector/policies.go (2)
MatchPolicy(34-57)NewResolvedReleaseTarget(8-14)
apps/workspace-engine/pkg/workspace/store/resources.go (5)
apps/workspace-engine/pkg/oapi/oapi.gen.go (6)
Id(58-58)Resource(475-489)ResourceVariable(501-505)Selector(535-537)Environment(205-212)Deployment(129-138)apps/workspace-engine/pkg/workspace/store/variables.go (1)
Variables(11-14)apps/workspace-engine/pkg/workspace/store/resource_variables.go (1)
ResourceVariables(16-19)apps/workspace-engine/pkg/selector/resources.go (1)
Selector(10-10)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
apps/workspace-engine/pkg/events/handler/resources/resources.go (6)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Resource(475-489)ReleaseTarget(445-449)Id(58-58)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go (4)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Deployment(129-138)ReleaseTarget(445-449)Id(58-58)JobAgentId(60-60)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/store/environments.go (6)
apps/workspace-engine/pkg/workspace/store/repository/repo.go (1)
InMemoryStore(72-96)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Environment(205-212)Id(58-58)Resource(475-489)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/resources.go (1)
FilterResources(12-26)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/store/deployments.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Id(58-58)DeploymentVariable(147-153)Deployment(129-138)Resource(475-489)apps/workspace-engine/pkg/selector/resources.go (1)
FilterResources(12-26)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)
🪛 GitHub Actions: Apps / Workspace Engine
apps/workspace-engine/pkg/workspace/store/resources.go
[error] 34-34: TestEngine_ResourceProviderSetResources_TimestampBehavior: CreatedAt missing after resource creation; timestamp not set
🔇 Additional comments (7)
apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variable_value.go (1)
42-44: Propagate reconciliation failures
ReconcileTargetreturns an error, but this path drops it. If reconciliation fails we silently succeed, leaving the release target stale. Please capture the error and return it (same fix applies to the update and delete handlers below).for _, releaseTarget := range releaseTargets { - ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } }apps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.go (1)
29-31: Bubble up ReconcileTarget errorsSame issue here: we ignore reconciliation failures. That masks deployment-version drift and the handler reports success even when nothing updated. Please return the error (also in the update/delete handlers).
for _, releaseTarget := range releaseTargets { - ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } }apps/workspace-engine/pkg/events/handler/deploymentvariables/deployment_variables.go (1)
28-30: Handle reconciliation errorsDropping the error from
ReconcileTargetmakes these handlers report success even when reconciliation fails. Please return the error (likewise for the update/delete variants).for _, releaseTarget := range releaseTargets { - ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } }apps/workspace-engine/pkg/events/handler/resourcevariables/resource_variables.go (1)
28-30: Return reconciliation failures
ReconcileTargetcan fail, but we ignore that outcome here (and in the sibling handlers). That leaves resource variables out of sync without surfacing the problem. Please return the error.for _, releaseTarget := range releaseTargets { - ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } }apps/workspace-engine/pkg/events/handler/resources/resources.go (1)
61-68: Surface reconcile failures
ReconcileTargetreturns an error but we discard it. If reconciliation fails, the resource stays stale and the handler still returns nil. Please bubble the error up here and in the update handler’s loop.err := ws.ReleaseTargets().Upsert(ctx, releaseTarget) if err != nil { return err } - ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } }apps/workspace-engine/pkg/events/handler/deployment/deployment.go (2)
62-69: Don’t swallow reconcile errorsReconciliation errors are ignored here. If the release manager fails, we return nil and leave targets stale. Please propagate the error (same applies in the update handler’s loop).
err := ws.ReleaseTargets().Upsert(ctx, releaseTarget) if err != nil { return err } - if deployment.JobAgentId != nil && *deployment.JobAgentId != "" { - ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) + if deployment.JobAgentId != nil && *deployment.JobAgentId != "" { + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } }
140-148: Reconcile existing targets on deployment updatesWhen a deployment changes but keeps the same release-target set,
addedReleaseTargetsis empty, so we never runReconcileTarget. That means config/image updates never reach existing targets. We need to reconcile every target (or at least force the existing ones) after any meaningful deployment change.- for _, addedReleaseTarget := range addedReleaseTargets { - err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget) + for _, releaseTarget := range releaseTargets { + err := ws.ReleaseTargets().Upsert(ctx, releaseTarget) if err != nil { return err } - if deployment.JobAgentId != nil && *deployment.JobAgentId != "" { - ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false) + if deployment.JobAgentId != nil && *deployment.JobAgentId != "" { + if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { + return err + } } }
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, releaseTarget := range releaseTargets { | ||
| ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) | ||
| } |
There was a problem hiding this comment.
Persist release targets before reconciliation
HandleEnvironmentCreated now computes the correct ReleaseTargets but never records them in the workspace store. As a result, subsequent environment updates/deletes (and any other code that inspects ws.ReleaseTargets()) will miss these targets entirely. Please upsert each target before reconciling it, and make sure reconciliation failures bubble up instead of being silently dropped.
- for _, releaseTarget := range releaseTargets {
- ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false)
- }
+ for _, releaseTarget := range releaseTargets {
+ if err := ws.ReleaseTargets().Upsert(ctx, releaseTarget); err != nil {
+ return err
+ }
+ if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil {
+ return err
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | |
| if err != nil { | |
| return err | |
| } | |
| for _, releaseTarget := range releaseTargets { | |
| ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) | |
| } | |
| releaseTargets, err := makeReleaseTargets(ctx, ws, environment) | |
| if err != nil { | |
| return err | |
| } | |
| for _, releaseTarget := range releaseTargets { | |
| if err := ws.ReleaseTargets().Upsert(ctx, releaseTarget); err != nil { | |
| return err | |
| } | |
| if err := ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/events/handler/environment/environment.go around
lines 52 to 58, the code computes releaseTargets but never persists them and
ignores reconciliation errors; for each releaseTarget call
ws.UpsertReleaseTarget(ctx, releaseTarget) (or the appropriate workspace store
upsert method) and handle/return any upsert error, then call
ws.ReleaseManager().ReconcileTarget(ctx, releaseTarget, false) and return any
error from ReconcileTarget instead of dropping it; ensure both upsert and
reconcile errors bubble up to the caller.
| for _, removedReleaseTarget := range removedReleaseTargets { | ||
| ws.ReleaseTargets().Remove(removedReleaseTarget.Key()) | ||
| } | ||
| for _, addedReleaseTarget := range addedReleaseTargets { | ||
| err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false) | ||
| } |
There was a problem hiding this comment.
Surface reconciliation errors on environment updates
We already guard the Upsert, but the follow-up call to ReconcileTarget can still fail (planner/eligibility/execution). Right now those failures disappear, leaving the environment out of sync with no signal to the caller. Please return the error so the event handler can retry or report the failure.
for _, addedReleaseTarget := range addedReleaseTargets {
- err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget)
- if err != nil {
+ if err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget); err != nil {
return err
}
- ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false)
+ if err := ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false); err != nil {
+ return err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, removedReleaseTarget := range removedReleaseTargets { | |
| ws.ReleaseTargets().Remove(removedReleaseTarget.Key()) | |
| } | |
| for _, addedReleaseTarget := range addedReleaseTargets { | |
| err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget) | |
| if err != nil { | |
| return err | |
| } | |
| ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false) | |
| } | |
| for _, removedReleaseTarget := range removedReleaseTargets { | |
| ws.ReleaseTargets().Remove(removedReleaseTarget.Key()) | |
| } | |
| for _, addedReleaseTarget := range addedReleaseTargets { | |
| if err := ws.ReleaseTargets().Upsert(ctx, addedReleaseTarget); err != nil { | |
| return err | |
| } | |
| if err := ws.ReleaseManager().ReconcileTarget(ctx, addedReleaseTarget, false); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/events/handler/environment/environment.go around
lines 124 to 133, the call to ws.ReleaseManager().ReconcileTarget(...) currently
ignores any error; modify the code so that after Upsert you capture the return
value of ReconcileTarget into an err variable and if err != nil return that
error (so the event handler can retry/report failures). Keep the existing Upsert
error handling and ensure the ReconcileTarget call is checked immediately and
returned on failure.
| // targetKey := releaseTarget.Key() | ||
| // lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{}) | ||
| // lock := lockInterface.(*sync.Mutex) | ||
|
|
||
| // Serialize processing for this specific release target | ||
| lock.Lock() | ||
| defer lock.Unlock() | ||
| // // Serialize processing for this specific release target | ||
| // lock.Lock() | ||
| // defer lock.Unlock() |
There was a problem hiding this comment.
Keep per-target serialization to avoid duplicate jobs
Removing the mutex means concurrent calls (e.g., from ProcessChanges) can race on the same release target. Both goroutines will plan, pass eligibility, and execute, yielding duplicate jobs—a regression from the previous behaviour. Please restore the per-target lock so only one reconciliation for a given ReleaseTarget.Key() runs at a time.
- // targetKey := releaseTarget.Key()
- // lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{})
- // lock := lockInterface.(*sync.Mutex)
-
- // // Serialize processing for this specific release target
- // lock.Lock()
- // defer lock.Unlock()
+ targetKey := releaseTarget.Key()
+ lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{})
+ lock := lockInterface.(*sync.Mutex)
+
+ // Serialize processing for this specific release target
+ lock.Lock()
+ defer lock.Unlock()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // targetKey := releaseTarget.Key() | |
| // lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{}) | |
| // lock := lockInterface.(*sync.Mutex) | |
| // Serialize processing for this specific release target | |
| lock.Lock() | |
| defer lock.Unlock() | |
| // // Serialize processing for this specific release target | |
| // lock.Lock() | |
| // defer lock.Unlock() | |
| targetKey := releaseTarget.Key() | |
| lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{}) | |
| lock := lockInterface.(*sync.Mutex) | |
| // Serialize processing for this specific release target | |
| lock.Lock() | |
| defer lock.Unlock() |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/releasemanager/manager.go around lines
241 to 247, the per-target mutex was removed causing concurrent reconciliations
to race; restore the per-target lock by reintroducing the LoadOrStore of a
*sync.Mutex keyed by releaseTarget.Key(), assert the stored value to
*sync.Mutex, call lock.Lock() before the reconciliation/processing block and
defer lock.Unlock() immediately after acquiring it so only one goroutine
processes a given ReleaseTarget.Key() at a time.
| func (r *Resources) Upsert(ctx context.Context, resource *oapi.Resource) (*oapi.Resource, error) { | ||
| ctx, span := tracer.Start(ctx, "Upsert", trace.WithAttributes( | ||
| _, span := tracer.Start(ctx, "Upsert", trace.WithAttributes( | ||
| attribute.String("resource.id", resource.Id), | ||
| )) | ||
| defer span.End() | ||
|
|
||
| hasChanges, err := r.upsertWithoutRecompute(ctx, resource) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Only trigger recomputation if there are actual changes | ||
| if hasChanges { | ||
| span.SetAttributes(attribute.Bool("recompute.triggered", true)) | ||
| envs := r.store.Environments.ApplyResourceUpsert(ctx, resource) | ||
| deploys := r.store.Deployments.ApplyResourceUpsert(ctx, resource) | ||
| rt := []*oapi.ReleaseTarget{} | ||
| for _, environment := range envs { | ||
| for _, deployment := range deploys { | ||
| rt = append(rt, &oapi.ReleaseTarget{ | ||
| EnvironmentId: environment.Id, | ||
| DeploymentId: deployment.Id, | ||
| ResourceId: resource.Id, | ||
| }) | ||
| } | ||
| } | ||
| r.store.ReleaseTargets.AddReleaseTargets(ctx, rt) | ||
| // Invalidate this resource AND all entities that might have relationships to it | ||
| r.store.Relationships.InvalidateEntityAndPotentialSources(resource.Id, oapi.RelatableEntityTypeResource) | ||
| } else { | ||
| span.SetAttributes(attribute.Bool("recompute.triggered", false)) | ||
| span.AddEvent("Skipped recomputation - no meaningful changes detected") | ||
| } | ||
| r.repo.Resources.Set(resource.Id, resource) | ||
| r.store.changeset.RecordUpsert(resource) | ||
|
|
There was a problem hiding this comment.
Restore resource timestamps on upsert.
Line [33] now writes new resources without initializing CreatedAt, so newly created entries keep the zero value. CI is already red (TestEngine_ResourceProviderSetResources_TimestampBehavior) because it expects CreatedAt to be set on creation. We should also preserve the original CreatedAt when updating and bump the UpdatedAt timestamp. Please apply:
@@
-import (
- "context"
- "workspace-engine/pkg/oapi"
- "workspace-engine/pkg/selector"
- "workspace-engine/pkg/workspace/store/repository"
+import (
+ "context"
+ "time"
+ "workspace-engine/pkg/oapi"
+ "workspace-engine/pkg/selector"
+ "workspace-engine/pkg/workspace/store/repository"
@@
func (r *Resources) Upsert(ctx context.Context, resource *oapi.Resource) (*oapi.Resource, error) {
+ now := time.Now().UTC()
+ if existing, ok := r.repo.Resources.Get(resource.Id); ok && existing != nil {
+ if resource.CreatedAt.IsZero() {
+ resource.CreatedAt = existing.CreatedAt
+ }
+ } else if resource.CreatedAt.IsZero() {
+ resource.CreatedAt = now
+ }
+ resource.UpdatedAt = &now
+
_, span := tracer.Start(ctx, "Upsert", trace.WithAttributes(
attribute.String("resource.id", resource.Id),
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Resources) Upsert(ctx context.Context, resource *oapi.Resource) (*oapi.Resource, error) { | |
| ctx, span := tracer.Start(ctx, "Upsert", trace.WithAttributes( | |
| _, span := tracer.Start(ctx, "Upsert", trace.WithAttributes( | |
| attribute.String("resource.id", resource.Id), | |
| )) | |
| defer span.End() | |
| hasChanges, err := r.upsertWithoutRecompute(ctx, resource) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Only trigger recomputation if there are actual changes | |
| if hasChanges { | |
| span.SetAttributes(attribute.Bool("recompute.triggered", true)) | |
| envs := r.store.Environments.ApplyResourceUpsert(ctx, resource) | |
| deploys := r.store.Deployments.ApplyResourceUpsert(ctx, resource) | |
| rt := []*oapi.ReleaseTarget{} | |
| for _, environment := range envs { | |
| for _, deployment := range deploys { | |
| rt = append(rt, &oapi.ReleaseTarget{ | |
| EnvironmentId: environment.Id, | |
| DeploymentId: deployment.Id, | |
| ResourceId: resource.Id, | |
| }) | |
| } | |
| } | |
| r.store.ReleaseTargets.AddReleaseTargets(ctx, rt) | |
| // Invalidate this resource AND all entities that might have relationships to it | |
| r.store.Relationships.InvalidateEntityAndPotentialSources(resource.Id, oapi.RelatableEntityTypeResource) | |
| } else { | |
| span.SetAttributes(attribute.Bool("recompute.triggered", false)) | |
| span.AddEvent("Skipped recomputation - no meaningful changes detected") | |
| } | |
| r.repo.Resources.Set(resource.Id, resource) | |
| r.store.changeset.RecordUpsert(resource) | |
| func (r *Resources) Upsert(ctx context.Context, resource *oapi.Resource) (*oapi.Resource, error) { | |
| now := time.Now().UTC() | |
| if existing, ok := r.repo.Resources.Get(resource.Id); ok && existing != nil { | |
| if resource.CreatedAt.IsZero() { | |
| resource.CreatedAt = existing.CreatedAt | |
| } | |
| } else if resource.CreatedAt.IsZero() { | |
| resource.CreatedAt = now | |
| } | |
| resource.UpdatedAt = &now | |
| _, span := tracer.Start(ctx, "Upsert", trace.WithAttributes( | |
| attribute.String("resource.id", resource.Id), | |
| )) | |
| defer span.End() | |
| r.repo.Resources.Set(resource.Id, resource) | |
| r.store.changeset.RecordUpsert(resource) | |
🧰 Tools
🪛 GitHub Actions: Apps / Workspace Engine
[error] 34-34: TestEngine_ResourceProviderSetResources_TimestampBehavior: CreatedAt missing after resource creation; timestamp not set
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/resources.go around lines 27 to 35,
the Upsert currently writes new resources without initializing CreatedAt and
overwrites existing CreatedAt on update; change Upsert to (1) look up an
existing resource by resource.Id, (2) if none exists set resource.CreatedAt to
the current timestamp, (3) if an existing resource exists preserve its CreatedAt
on the incoming object, (4) always set resource.UpdatedAt to the current
timestamp before persisting, and (5) persist the modified resource and record
the upsert to the changeset so timestamps are correct for both create and update
paths.
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
101-103: Remove redundant length check.This check is redundant since
len(jobs)was already verified to be non-zero on line 87, andjobsListis built by iterating overjobs.Apply this diff:
- if len(jobsList) == 0 { - return nil, fmt.Errorf("no jobs found for release target") - } - return jobsList[0], nil
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/workspace/store/release_targets.go (2)
124-136: Simplify policy matching logic.The outer loop iterates through
policy.Selectorsand checks each selector for nil fields (lines 126-128), but then callsselector.MatchPolicy(ctx, policy, ...)on line 130, which internally iterates throughpolicy.Selectorsagain. This means:
- The outer loop and nil-checks on lines 125-128 don't actually filter what
MatchPolicyseesMatchPolicywill re-check all selectors, including those skipped bycontinueConsider simplifying to call
MatchPolicyonce per policy without the inner loop:for _, policy := range r.store.Policies.Items() { - for _, sel := range policy.Selectors { - if sel.ResourceSelector == nil || sel.EnvironmentSelector == nil || sel.DeploymentSelector == nil { - continue - } - - isMatch := selector.MatchPolicy(ctx, policy, selector.NewResolvedReleaseTarget(environment, deployment, resource)) - if isMatch { - policiesSlice = append(policiesSlice, policy) - break - } + isMatch := selector.MatchPolicy(ctx, policy, selector.NewResolvedReleaseTarget(environment, deployment, resource)) + if isMatch { + policiesSlice = append(policiesSlice, policy) } }
30-32: Consider simplifying return signature.The error return is always
nil. If no error conditions are anticipated, consider removing the error return to simplify the API.apps/workspace-engine/pkg/events/handler/deployment/deployment.go (1)
75-107: Consider optimizing diff functions for larger datasets.Both
getRemovedReleaseTargetsandgetAddedReleaseTargetsuse nested loops with O(n²) complexity. For small lists this is fine, but if deployments have many release targets, consider using a map-based approach for O(n) performance.Example optimization:
func getRemovedReleaseTargets(oldReleaseTargets []*oapi.ReleaseTarget, newReleaseTargets []*oapi.ReleaseTarget) []*oapi.ReleaseTarget { newKeys := make(map[string]bool, len(newReleaseTargets)) for _, rt := range newReleaseTargets { newKeys[rt.Key()] = true } removedReleaseTargets := make([]*oapi.ReleaseTarget, 0) for _, oldReleaseTarget := range oldReleaseTargets { if !newKeys[oldReleaseTarget.Key()] { removedReleaseTargets = append(removedReleaseTargets, oldReleaseTarget) } } return removedReleaseTargets }
📜 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/deployment/deployment.go(4 hunks)apps/workspace-engine/pkg/events/handler/system/system.go(1 hunks)apps/workspace-engine/pkg/workspace/store/release_targets.go(2 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/system/system.goapps/workspace-engine/pkg/events/handler/deployment/deployment.goapps/workspace-engine/pkg/workspace/store/release_targets.go
🧠 Learnings (1)
📚 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/events/handler/deployment/deployment.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/events/handler/system/system.go (4)
apps/workspace-engine/pkg/workspace/store/systems.go (1)
Systems(16-19)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go (7)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Deployment(129-138)ReleaseTarget(445-449)Id(58-58)JobAgentId(60-60)apps/workspace-engine/pkg/workspace/store/systems.go (1)
Systems(16-19)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(22-25)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/store/release_targets.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
ReleaseTarget(445-449)Policy(342-355)Id(58-58)apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/selector/policies.go (2)
MatchPolicy(34-57)NewResolvedReleaseTarget(8-14)
⏰ 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: tests
- GitHub Check: workspace-engine-tests
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: Format
🔇 Additional comments (1)
apps/workspace-engine/pkg/events/handler/system/system.go (1)
63-71: No changes needed—deletion order concern does not apply to this implementation.The review comment assumes database constraints enforcing referential integrity, but this codebase uses an in-memory store pattern without constraint validation. The
Remove()methods are simple in-memory operations that don't cascade or validate references. Additionally, all entities are retrieved before any deletion begins (lines 56-61), so no data is lost during removal. The current order is correct for this architecture.Likely an incorrect or invalid review comment.
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/store/relationships.go (1)
276-403: Consolidate the duplicated finder helpers
findMatchingResources,findMatchingEnvironments, andfindMatchingDeploymentsdiffer only in the loader and entity constructor, so maintaining three copies of the same loop/selector/matcher plumbing makes future tweaks harder. Consider extracting a generic helper (e.g., parameterize the slice source andrelationships.New*Entityconstructor) so any matcher/selector fixes land once.
📜 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 (2)
apps/workspace-engine/pkg/workspace/relationships/matcher.go(1 hunks)apps/workspace-engine/pkg/workspace/store/relationships.go(2 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/relationships/matcher.goapps/workspace-engine/pkg/workspace/store/relationships.go
🧠 Learnings (1)
📚 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 : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/relationships/matcher.go
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/workspace/relationships/matcher.go (1)
apps/workspace-engine/test/integration/creators/relationship.go (1)
NewPropertyMatcher(26-32)
apps/workspace-engine/pkg/workspace/store/relationships.go (5)
apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(36-58)apps/workspace-engine/pkg/oapi/oapi.gen.go (13)
Resource(475-489)Deployment(129-138)Environment(205-212)RelationshipRule(415-428)RelatableEntityType(409-409)Selector(535-537)RelatableEntity(404-406)EntityRelation(194-202)RelatableEntityTypeResource(87-87)RelatableEntityTypeDeployment(85-85)RelatableEntityTypeEnvironment(86-86)To(93-93)From(92-92)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/relationships/match.go (3)
NewResourceEntity(17-21)NewEnvironmentEntity(11-15)NewDeploymentEntity(5-9)apps/workspace-engine/pkg/workspace/relationships/matcher.go (1)
Matches(20-58)
⏰ 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: Lint
- GitHub Check: Format
- GitHub Check: workspace-engine-tests
- GitHub Check: tests
| pm, err := matcher.AsPropertiesMatcher() | ||
|
|
||
| if err != nil { | ||
| log.Warn("failed to get properties matcher", "error", err) | ||
| } | ||
|
|
||
| if err == nil && len(pm.Properties) > 0 { | ||
| for _, pm := range pm.Properties { | ||
| matcher := NewPropertyMatcher(&pm) | ||
| if !matcher.Evaluate(ctx, from, to) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| cm, err := matcher.AsCelMatcher() | ||
| if err != nil { | ||
| log.Warn("failed to get cel matcher", "error", err) | ||
| } | ||
|
|
||
| if err == nil && cm.Cel != "" { | ||
| matcher, err := NewCelMatcher(&cm) | ||
| if err != nil { | ||
| log.Warn("failed to new cel matcher", "error", err) | ||
| return false | ||
| } | ||
|
|
||
| // Always convert entities to maps without any cache | ||
| fromMap, _ := entityToMap(from.Item()) | ||
| toMap, _ := entityToMap(to.Item()) | ||
|
|
||
| return matcher.Evaluate(ctx, fromMap, toMap) | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard nil matcher before dereferencing
Matches is called with rule.GetMatcher() in several flows, and rules that rely only on selectors leave Matcher unset—GetMatcher() returns nil. With the new inlined logic we now dereference matcher immediately, so those calls will panic instead of following the “match by selectors only” fallback. Please short-circuit when the matcher is absent.
func Matches(ctx context.Context, matcher *oapi.RelationshipRule_Matcher, from *oapi.RelatableEntity, to *oapi.RelatableEntity) bool {
+ if matcher == nil {
+ return true
+ }
pm, err := matcher.AsPropertiesMatcher()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/relationships/matcher.go around lines
21–55, the code calls matcher.AsPropertiesMatcher() and matcher.AsCelMatcher()
without first checking whether matcher is nil; when rules omit Matcher
(selectors-only) this panics. Add an early guard: if matcher == nil { return
true } (or the appropriate selectors-only success value) before any dereference,
then proceed with the existing AsPropertiesMatcher/AsCelMatcher logic so we
short-circuit safely for nil matchers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/store/relationships.go (2)
94-146: Consider extracting common rule collection logic.Both
collectFromRulesandcollectToRulesfollow nearly identical patterns (type check, selector matching). While the current implementation is correct, extracting a parameterized helper function could reduce duplication and improve maintainability.
148-173: Consider using generics to eliminate triplication.The three
getAll*methods are identical except for the entity type. With Go generics, a single parameterized function could eliminate this duplication.Example approach:
func getAllEntities[T any](items map[string]T) []T { slice := make([]T, 0, len(items)) for _, item := range items { slice = append(slice, item) } return slice }
📜 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 (1)
apps/workspace-engine/pkg/workspace/store/relationships.go(2 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/store/relationships.go
⏰ 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). (1)
- GitHub Check: Lint
🔇 Additional comments (6)
apps/workspace-engine/pkg/workspace/store/relationships.go (6)
16-30: LGTM! Simplified to direct map access.The refactoring to return direct map references from the repository is cleaner and eliminates the indirection through Items() methods.
48-77: LGTM! CRUD operations properly handle changesets.The Upsert and Remove operations correctly maintain changeset recording for both context-based and store-level changesets, with appropriate existence checks.
79-92: LGTM! Efficient selector matching with proper guards.The method correctly validates type compatibility and handles the nil selector case before delegating to the selector matching logic.
175-213: LGTM! Clear orchestration of relationship computation.The method properly separates the collection of bidirectional rules and merges results correctly, with appropriate error handling throughout.
215-260: LGTM! Correctly collects outgoing relationships.The method properly constructs
EntityRelationobjects with the correct direction (oapi.To) for each matching target entity. The logic correctly handles all three entity types.Note: The shift to on-demand computation means relationships are now computed via O(rules × entities) traversal. Verify this performance trade-off is acceptable for your typical dataset sizes.
262-308: LGTM! Correctly collects incoming relationships.The method mirrors
collectFromRelationswith the correct direction (oapi.From) for entities that reference the target entity. The logic is sound.
| func (r *RelationshipRules) findMatchingResources( | ||
| ctx context.Context, | ||
| rule *oapi.RelationshipRule, | ||
| entitySelector *oapi.Selector, | ||
| sourceEntity *oapi.RelatableEntity, | ||
| evaluateFromTo bool, | ||
| ) ([]*oapi.RelatableEntity, error) { | ||
| resources := r.getAllResources() | ||
|
|
||
| results := make([]*oapi.RelatableEntity, 0) | ||
|
|
||
| for _, resource := range resources { | ||
| // Check entity selector if provided | ||
| if entitySelector != nil { | ||
| matched, err := selector.Match(ctx, entitySelector, resource) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !matched { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| span.SetAttributes( | ||
| attribute.String("entity.id", entity.GetID()), | ||
| attribute.String("entity.type", string(entity.GetType())), | ||
| ) | ||
| resourceEntity := relationships.NewResourceEntity(resource) | ||
|
|
||
| relatedEntities, err := r.graph.GetRelatedEntitiesWithCompute(ctx, entity.GetID()) | ||
| if err != nil { | ||
| return nil, err | ||
| // Check matcher rule | ||
| var matches bool | ||
| if evaluateFromTo { | ||
| matches = relationships.Matches(ctx, &rule.Matcher, sourceEntity, resourceEntity) | ||
| } else { | ||
| matches = relationships.Matches(ctx, &rule.Matcher, resourceEntity, sourceEntity) | ||
| } | ||
|
|
||
| if !matches { | ||
| continue | ||
| } | ||
|
|
||
| results = append(results, resourceEntity) | ||
| } | ||
|
|
||
| return results, nil | ||
| } | ||
|
|
||
| func (r *RelationshipRules) findMatchingEnvironments( | ||
| ctx context.Context, | ||
| rule *oapi.RelationshipRule, | ||
| entitySelector *oapi.Selector, | ||
| sourceEntity *oapi.RelatableEntity, | ||
| evaluateFromTo bool, | ||
| ) ([]*oapi.RelatableEntity, error) { | ||
| environments := r.getAllEnvironments() | ||
|
|
||
| results := make([]*oapi.RelatableEntity, 0) | ||
|
|
||
| for _, environment := range environments { | ||
| // Check entity selector if provided | ||
| if entitySelector != nil { | ||
| matched, err := selector.Match(ctx, entitySelector, environment) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !matched { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| environmentEntity := relationships.NewEnvironmentEntity(environment) | ||
|
|
||
| // Check matcher rule | ||
| var matches bool | ||
| if evaluateFromTo { | ||
| matches = relationships.Matches(ctx, &rule.Matcher, sourceEntity, environmentEntity) | ||
| } else { | ||
| matches = relationships.Matches(ctx, &rule.Matcher, environmentEntity, sourceEntity) | ||
| } | ||
|
|
||
| if !matches { | ||
| continue | ||
| } | ||
|
|
||
| results = append(results, environmentEntity) | ||
| } | ||
|
|
||
| for reference, relations := range relatedEntities { | ||
| var rels []string | ||
| for _, relation := range relations { | ||
| if jsonBytes, err := json.Marshal(relation); err == nil { | ||
| rels = append(rels, string(jsonBytes)) | ||
| return results, nil | ||
| } | ||
|
|
||
| func (r *RelationshipRules) findMatchingDeployments( | ||
| ctx context.Context, | ||
| rule *oapi.RelationshipRule, | ||
| entitySelector *oapi.Selector, | ||
| sourceEntity *oapi.RelatableEntity, | ||
| evaluateFromTo bool, | ||
| ) ([]*oapi.RelatableEntity, error) { | ||
| deployments := r.getAllDeployments() | ||
|
|
||
| results := make([]*oapi.RelatableEntity, 0) | ||
|
|
||
| for _, deployment := range deployments { | ||
| // Check entity selector if provided | ||
| if entitySelector != nil { | ||
| matched, err := selector.Match(ctx, entitySelector, deployment) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !matched { | ||
| continue | ||
| } | ||
| } | ||
| span.AddEvent(fmt.Sprintf("related_entities.reference.%s", reference), trace.WithAttributes(attribute.String("relations", strings.Join(rels, ", ")))) | ||
|
|
||
| deploymentEntity := relationships.NewDeploymentEntity(deployment) | ||
|
|
||
| // Check matcher rule | ||
| var matches bool | ||
| if evaluateFromTo { | ||
| matches = relationships.Matches(ctx, &rule.Matcher, sourceEntity, deploymentEntity) | ||
| } else { | ||
| matches = relationships.Matches(ctx, &rule.Matcher, deploymentEntity, sourceEntity) | ||
| } | ||
|
|
||
| if !matches { | ||
| continue | ||
| } | ||
|
|
||
| results = append(results, deploymentEntity) | ||
| } | ||
| return relatedEntities, nil | ||
|
|
||
| return results, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant duplication across findMatching methods.*
The three findMatching* methods (findMatchingResources, findMatchingEnvironments, findMatchingDeployments) share 95% identical logic, differing only in:
- The entity type being iterated
- The wrapper function used (e.g.,
relationships.NewResourceEntity)
This triplication increases maintenance burden and the risk of inconsistent updates.
Consider refactoring using one of these approaches:
Option 1: Interface-based approach
Define an interface for entity operations:
type entityMatcher interface {
GetAll() []*oapi.RelatableEntity
Match(ctx context.Context, selector *oapi.Selector, item any) (bool, error)
}Then implement a single findMatchingEntities method.
Option 2: Callback-based approach
Extract common logic into a single method that accepts callbacks:
func (r *RelationshipRules) findMatchingEntities(
ctx context.Context,
rule *oapi.RelationshipRule,
entitySelector *oapi.Selector,
sourceEntity *oapi.RelatableEntity,
evaluateFromTo bool,
getAllFn func() []any,
wrapFn func(any) *oapi.RelatableEntity,
) ([]*oapi.RelatableEntity, error) {
// Common logic here
}
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/workspace/store/store.go (1)
66-66: Remove the unusedsetStatusparameter.The
setStatusparameter is never used in the function body. This appears to be leftover from the previous implementation that included status reporting during materialized-view recomputation.Apply this diff to remove the unused parameter:
-func (s *Store) Restore(ctx context.Context, changes persistence.Changes, setStatus func(status string)) error { +func (s *Store) Restore(ctx context.Context, changes persistence.Changes) error {
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/workspace/store/store.go (3)
78-78: Comment restates the code rather than explaining intent.This comment describes what the code does (the iteration order) rather than explaining why. Per the coding guidelines, comments should explain why or provide non-obvious context.
As per coding guidelines
Consider removing this comment or replacing it with something that explains the rationale, e.g., "Iterate by environment to minimize redundant selector evaluations" (if that's the intent).
87-97: Consider logging or collecting selector evaluation errors.Errors from
selector.Matchare silently ignored (lines 88, 95), which could hide selector configuration issues during restore. While continuing on error maintains resilience, it makes debugging malformed selectors difficult.Consider at least logging these errors:
for _, resource := range s.Resources.Items() { isInEnv, err := selector.Match(ctx, environment.ResourceSelector, resource) - if err != nil || !isInEnv { + if err != nil { + // Log selector evaluation error for debugging + continue + } + if !isInEnv { continue } for _, deployment := range matchingDeployments { isInDeployment, err := selector.Match(ctx, deployment.ResourceSelector, resource) - if err != nil || !isInDeployment { + if err != nil { + // Log selector evaluation error for debugging + continue + } + if !isInDeployment { continue }
72-106: Algorithmic complexity consideration for large workspaces.The triple-nested iteration (environments × resources × deployments) could have performance implications during restore for workspaces with many environments, resources, and deployments. The deploymentsBySystem optimization helps, but the overall complexity is still O(E × R × D_per_system) selector evaluations.
This is likely acceptable for typical workspace sizes, but consider adding a benchmark or performance test if restore times become problematic.
📜 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 (1)
apps/workspace-engine/pkg/workspace/store/store.go(2 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/store/store.go
🧠 Learnings (1)
📚 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/store.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/store/store.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Deployment(129-138)ReleaseTarget(445-449)Id(58-58)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
⏰ 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: workspace-engine-tests
- GitHub Check: Lint
- GitHub Check: tests
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/store/relationships.go (1)
310-447: Critical duplication persists across findMatching methods.*The three
findMatching*methods share 95% identical logic, differing only in entity type and wrapper function. This was previously flagged and remains unaddressed.As previously suggested, consider one of these refactoring approaches:
Option 1: Generic helper with type parameters
func findMatchingEntities[T any]( ctx context.Context, r *RelationshipRules, rule *oapi.RelationshipRule, entitySelector *oapi.Selector, sourceEntity *oapi.RelatableEntity, evaluateFromTo bool, entities map[string]T, getID func(T) string, entityType oapi.RelatableEntityType, wrapFn func(T) *oapi.RelatableEntity, ) ([]*oapi.RelatableEntity, error) { results := make([]*oapi.RelatableEntity, 0) for _, entity := range entities { if sourceEntity.GetType() == entityType && sourceEntity.GetID() == getID(entity) { continue } if entitySelector != nil { matched, err := selector.Match(ctx, entitySelector, entity) if err != nil { return nil, err } if !matched { continue } } wrappedEntity := wrapFn(entity) var matches bool if evaluateFromTo { matches = relationships.Matches(ctx, &rule.Matcher, sourceEntity, wrappedEntity) } else { matches = relationships.Matches(ctx, &rule.Matcher, wrappedEntity, sourceEntity) } if matches { results = append(results, wrappedEntity) } } return results, nil }Then each method becomes a thin wrapper:
func (r *RelationshipRules) findMatchingResources(...) ([]*oapi.RelatableEntity, error) { return findMatchingEntities( ctx, r, rule, entitySelector, sourceEntity, evaluateFromTo, r.store.Resources.Items(), func(res *oapi.Resource) string { return res.Id }, oapi.RelatableEntityTypeResource, relationships.NewResourceEntity, ) }
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/store/relationships.go (2)
148-173: Consider iterating maps directly to avoid allocation overhead.These three methods convert maps to slices, which allocates memory and copies all entries. Since the
findMatching*methods only iterate through entities, they could work directly with the maps returned byItems(), eliminating these intermediate conversions.Example refactor for
findMatchingResources:-func (r *RelationshipRules) findMatchingResources( - ctx context.Context, - rule *oapi.RelationshipRule, - entitySelector *oapi.Selector, - sourceEntity *oapi.RelatableEntity, - evaluateFromTo bool, -) ([]*oapi.RelatableEntity, error) { - resources := r.getAllResources() +func (r *RelationshipRules) findMatchingResources( + ctx context.Context, + rule *oapi.RelationshipRule, + entitySelector *oapi.Selector, + sourceEntity *oapi.RelatableEntity, + evaluateFromTo bool, +) ([]*oapi.RelatableEntity, error) { + resources := r.store.Resources.Items() results := make([]*oapi.RelatableEntity, 0) - for _, resource := range resources { + for _, resource := range resources {Apply the same pattern to
findMatchingEnvironmentsandfindMatchingDeployments, then remove the now-unusedgetAll*methods.
215-308: Consider reducing duplication in relation collection.Both
collectFromRelationsandcollectToRelationscontain similar structures with three consecutive if statements for entity types. While the logic is correct, consolidating this pattern could improve maintainability.A map-based dispatch could simplify the pattern:
type entityFinder func(context.Context, *oapi.RelationshipRule, *oapi.Selector, *oapi.RelatableEntity, bool) ([]*oapi.RelatableEntity, error) var entityFinders = map[oapi.RelatableEntityType]entityFinder{ oapi.RelatableEntityTypeResource: (*RelationshipRules).findMatchingResources, oapi.RelatableEntityTypeDeployment: (*RelationshipRules).findMatchingDeployments, oapi.RelatableEntityTypeEnvironment: (*RelationshipRules).findMatchingEnvironments, }Then use the map to dispatch calls instead of separate if statements.
📜 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 (1)
apps/workspace-engine/pkg/workspace/store/relationships.go(2 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/store/relationships.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/store/relationships.go (8)
apps/workspace-engine/pkg/workspace/store/store.go (1)
Store(38-60)apps/workspace-engine/pkg/oapi/oapi.gen.go (14)
Resource(475-489)Deployment(129-138)Environment(205-212)RelationshipRule(415-428)RelatableEntityType(409-409)Selector(535-537)RelatableEntity(404-406)EntityRelation(194-202)RelatableEntityTypeResource(87-87)RelatableEntityTypeDeployment(85-85)RelatableEntityTypeEnvironment(86-86)To(93-93)From(92-92)Id(58-58)apps/workspace-engine/pkg/workspace/store/resources.go (1)
Resources(23-26)apps/workspace-engine/pkg/workspace/store/deployments.go (1)
Deployments(24-27)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/selector/match.go (1)
Match(64-75)apps/workspace-engine/pkg/workspace/relationships/match.go (3)
NewResourceEntity(17-21)NewEnvironmentEntity(11-15)NewDeploymentEntity(5-9)apps/workspace-engine/pkg/workspace/relationships/matcher.go (1)
Matches(20-58)
⏰ 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: Format
- GitHub Check: Typecheck
- GitHub Check: tests
- GitHub Check: Lint
- GitHub Check: workspace-engine-tests
🔇 Additional comments (6)
apps/workspace-engine/pkg/workspace/store/relationships.go (6)
7-8: LGTM: Imports align with new computation approach.The new imports for
selectorandrelationshipspackages support the refactored relationship computation logic.
16-30: LGTM: Simplified entity access.Direct map returns from the repository eliminate unnecessary indirection and align with the PR's goal to simplify logic.
48-77: LGTM: CRUD operations are correct.The implementation follows standard patterns with proper change tracking. The simplified
Items()method aligns with the refactored approach.
79-92: LGTM: Clean selector matching logic.The method efficiently handles type mismatches and nil selectors with early returns before delegating to
selector.Match.
94-146: LGTM: Rule collection logic is sound.Both
collectFromRulesandcollectToRulescorrectly filter rules based on entity type and selector matching. The symmetric structure is appropriate for their complementary roles.
175-213: LGTM: Clear orchestration of relationship computation.The method properly separates rule collection, relation computation, and result aggregation. The logic correctly handles both "from" and "to" relationships.
| // Check entity selector if provided | ||
| if entitySelector != nil { |
There was a problem hiding this comment.
Remove obvious inline comments.
As per coding guidelines for Go files in this directory: "Do not add comments that simply restate what the code does."
The comments like // Check entity selector if provided and // Check matcher rule merely repeat what the subsequent code statements clearly express and should be removed.
Based on coding guidelines.
Also applies to: 339-340, 372-373, 385-386, 418-419, 431-432
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/relationships.go around lines
326-327 (and also at 339-340, 372-373, 385-386, 418-419, 431-432), remove the
redundant inline comments such as "// Check entity selector if provided" and "//
Check matcher rule" that simply restate the code; delete those comment lines so
the code conforms to the directory's guideline against comments that only repeat
what the code does.
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.go (2)
12-35: Helper function implementation looks good.The logic correctly resolves version, environment, and filters targets by DeploymentId. Error handling is appropriate for missing entities.
Optional: If performance matters and the typical number of matching targets is predictable, you could pre-allocate the slice capacity:
- releaseTargets := make([]*oapi.ReleaseTarget, 0) + releaseTargets := make([]*oapi.ReleaseTarget, 0, len(environmentTargets))However, this is a micro-optimization and likely unnecessary unless profiling shows this as a bottleneck.
49-56: Optional: Consider extracting common reconciliation logic.The reconciliation loop pattern is duplicated across all three handlers. You could extract it into a helper:
func reconcileTargetsForApprovalRecord(ctx context.Context, ws *workspace.Workspace, userApprovalRecord *oapi.UserApprovalRecord) error { relevantTargets, err := getRelevantTargets(ctx, ws, userApprovalRecord) if err != nil { return err } for _, target := range relevantTargets { ws.ReleaseManager().ReconcileTarget(ctx, target, false) } return nil }However, given the PR's goal to "simplify logic," the current straightforward duplication might be preferred over adding another layer of abstraction.
Also applies to: 72-79, 95-102
📜 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 (1)
apps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.go(4 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/userapprovalrecords/userapprovalrecords.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.go (5)
apps/workspace-engine/pkg/workspace/workspace.go (1)
Workspace(28-34)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
UserApprovalRecord(556-563)ReleaseTarget(445-449)Id(58-58)apps/workspace-engine/pkg/workspace/store/deployment_versions.go (1)
DeploymentVersions(16-19)apps/workspace-engine/pkg/workspace/store/environments.go (1)
Environments(18-21)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
⏰ 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: workspace-engine-tests
- GitHub Check: tests
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (2)
apps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.go (2)
49-56: Reconciliation logic looks correct.After upserting the approval record, relevant targets are computed and reconciled synchronously. The error handling ensures that if the version or environment is missing, the handler fails before attempting reconciliation.
72-79: Update handler follows the same correct pattern.Consistent with the Created handler—upserts the record, computes relevant targets, and reconciles them.
| relevantTargets, err := getRelevantTargets(ctx, ws, userApprovalRecord) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, target := range relevantTargets { | ||
| ws.ReleaseManager().ReconcileTarget(ctx, target, false) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify error handling strategy for missing entities.
The approval record is removed (line 93) before calling getRelevantTargets. If the version or environment has been deleted concurrently, getRelevantTargets returns an error, causing the handler to fail even though the approval record was already removed. This could lead to:
- Inconsistent state (record deleted, but targets not reconciled)
- Event replay failures if the version/environment remain missing
Consider one of these approaches:
- Option 1: Make
getRelevantTargetsmore lenient—log a warning and return an empty slice when entities are missing, allowing reconciliation to proceed gracefully. - Option 2: Verify version/environment existence before removing the approval record.
Run this script to check if there are any lifecycle dependencies or cascade deletion patterns:
🏁 Script executed:
#!/bin/bash
# Check for version/environment deletion patterns and lifecycle management
# Search for version deletion logic
echo "=== Version deletion patterns ==="
rg -n -A5 -B5 'DeploymentVersions.*Remove|DeploymentVersions.*Delete' --type=go
# Search for environment deletion logic
echo "=== Environment deletion patterns ==="
rg -n -A5 -B5 'Environments.*Remove|Environments.*Delete' --type=go
# Check if there are any cascade deletion or foreign key constraints
echo "=== Cascade or dependency checks ==="
rg -n 'UserApprovalRecord.*VersionId|UserApprovalRecord.*EnvironmentId' --type=go -A3 -B3Length of output: 10180
Wait, I need to keep the cloning. Let me revise:
</parameter>
</invoke>
---
🏁 Script executed:
```shell
#!/bin/bash
# Find getRelevantTargets implementation and understand error handling
echo "=== getRelevantTargets implementation ==="
ast-grep --pattern $'func getRelevantTargets($_, $_, $_) {
$$$
}'
echo "=== Schema/constraint information ==="
fd -e sql | head -20
echo "=== Check database setup for user approval records ==="
rg -n -A10 'UserApprovalRecord' --type=sql -g '*schema*' -g '*migration*' 2>/dev/null || echo "No SQL schema files found with UserApprovalRecord"
Length of output: 1917
🏁 Script executed:
#!/bin/bash
# Find getRelevantTargets implementation and understand error handling
echo "=== getRelevantTargets implementation ==="
ast-grep --pattern $'func getRelevantTargets($_) {
$$$
}'
echo "=== userapprovalrecords handler file context ==="
cat apps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.go | head -150Length of output: 2758
Verify and address error handling for missing version/environment in deletion handler.
The concern is valid. When HandleUserApprovalRecordDeleted calls getRelevantTargets after removing the approval record, if the version or environment has been concurrently deleted, the handler fails with an error even though the record was already removed. This leaves the corresponding release targets unreconciled.
The pattern is inconsistent across handlers: the environment deletion handler fetches release targets before removal (safer), while the deployment version handler deletes first then fetches (same issue as this one).
Consider updating getRelevantTargets to handle missing entities gracefully—return an empty slice and log a debug message instead of erroring—so reconciliation can proceed even when referenced entities are already deleted.
🤖 Prompt for AI Agents
In
apps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.go
around lines 95 to 102, the deletion handler currently errors out if
getRelevantTargets fails (e.g., referenced version/environment already deleted)
which prevents reconciling release targets; update getRelevantTargets so that
when referenced entities are not found it returns an empty slice (no error) and
logs a debug message, and adjust callers (including
HandleUserApprovalRecordDeleted) to treat a nil/empty slice as "no targets to
reconcile" and continue processing; ensure getRelevantTargets still returns real
errors for unexpected failures and propagate those normally.
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/manager.go (1)
205-211: Restore per-target locking to prevent race conditions.The commented-out per-target mutex removes concurrency safety for the three-phase reconciliation flow (Planning → Eligibility → Execution). Without this lock, concurrent calls to
ReconcileTargetfor the same release target can create duplicate jobs and corrupt state.This issue has been flagged in previous reviews. Please restore the per-target lock:
- // targetKey := releaseTarget.Key() - // lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{}) - // lock := lockInterface.(*sync.Mutex) - - // // Serialize processing for this specific release target - // lock.Lock() - // defer lock.Unlock() + targetKey := releaseTarget.Key() + lockInterface, _ := m.releaseTargetLocks.LoadOrStore(targetKey, &sync.Mutex{}) + lock := lockInterface.(*sync.Mutex) + + // Serialize processing for this specific release target + lock.Lock() + defer lock.Unlock()
📜 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 (8)
apps/workspace-engine/pkg/events/handler/handler.go(1 hunks)apps/workspace-engine/pkg/oapi/oapi.go(1 hunks)apps/workspace-engine/pkg/statechange/statechange.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/manager.go(5 hunks)apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go(0 hunks)apps/workspace-engine/pkg/workspace/releasemanager/targets/taint_test.go(0 hunks)apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go(0 hunks)apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.go(0 hunks)
💤 Files with no reviewable changes (4)
- apps/workspace-engine/pkg/workspace/releasemanager/targets/taint_test.go
- apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager_test.go
- apps/workspace-engine/pkg/workspace/releasemanager/targets/targets_manager.go
- apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go
✅ Files skipped from review due to trivial changes (1)
- apps/workspace-engine/pkg/statechange/statechange.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspace-engine/pkg/oapi/oapi.go
🧰 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/handler.goapps/workspace-engine/pkg/workspace/releasemanager/manager.go
🧠 Learnings (5)
📚 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/events/handler/handler.goapps/workspace-engine/pkg/workspace/releasemanager/manager.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 : 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/events/handler/handler.goapps/workspace-engine/pkg/workspace/releasemanager/manager.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/releasemanager/manager.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/releasemanager/manager.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/releasemanager/manager.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/releasemanager/manager.go (2)
apps/workspace-engine/pkg/statechange/statechange.go (3)
ChangeSet(33-36)StateChangeUpsert(19-19)StateChangeDelete(20-20)apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
ReleaseTarget(445-449)Status(64-64)Cancelled(42-42)UpdatedAt(65-65)
⏰ 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: Lint
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: tests
- GitHub Check: workspace-engine-tests
| if err := ws.ReleaseManager().ProcessChanges(ctx, ws.Changeset()); err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "failed to process target changes") | ||
| log.Error("Failed to process target changes", "error", err) | ||
| return ws, fmt.Errorf("failed to process target changes: %w", err) | ||
| } |
There was a problem hiding this comment.
Return the error to prevent silent failures.
The error from ProcessChanges is logged but not returned, allowing the handler to continue and save changes to persistence even when reconciliation fails. This can lead to inconsistent state between the workspace and persistence layers.
Apply this diff to propagate the error:
if err := ws.ReleaseManager().ProcessChanges(ctx, ws.Changeset()); err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "failed to process target changes")
log.Error("Failed to process target changes", "error", err)
+ return ws, fmt.Errorf("failed to process target changes: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := ws.ReleaseManager().ProcessChanges(ctx, ws.Changeset()); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to process target changes") | |
| log.Error("Failed to process target changes", "error", err) | |
| return ws, fmt.Errorf("failed to process target changes: %w", err) | |
| } | |
| if err := ws.ReleaseManager().ProcessChanges(ctx, ws.Changeset()); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to process target changes") | |
| log.Error("Failed to process target changes", "error", err) | |
| return ws, fmt.Errorf("failed to process target changes: %w", err) | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/events/handler/handler.go around lines 173 to 177,
the handler logs errors from ws.ReleaseManager().ProcessChanges(...) but does
not return them, allowing the function to continue and persist state despite
reconciliation failures; change the flow so that after span.RecordError(err) and
span.SetStatus(...) you return the error (or a wrapped error with context) to
propagate the failure to the caller instead of swallowing it, and ensure the
enclosing function signature supports returning an error.
| } | ||
| }(rt) | ||
| for _, target := range targetsUpserted { | ||
| fmt.Printf("upserting release target: %+v\n", target) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace debug print with proper logging.
The fmt.Printf statement should use the structured logger for consistency with the rest of the codebase.
Apply this diff:
- fmt.Printf("upserting release target: %+v\n", target)
+ log.Debug("upserting release target", "target", target.Key())Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/releasemanager/manager.go around line 98,
replace the debugging fmt.Printf call with the package's structured logger used
elsewhere in this file: log the event at an appropriate level (Debug/Info),
include the release target as a structured field (not just formatted text), and
remove the fmt import if it becomes unused; ensure the log message provides
context like "upserting release target" and the target object as a field.
| job.Status = oapi.Cancelled | ||
| job.UpdatedAt = time.Now() | ||
| m.store.Jobs.Upsert(ctx, job) | ||
| fmt.Printf("cancelled job: %+v\n", job) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace debug print with proper logging.
The fmt.Printf statement should use the structured logger for consistency with the rest of the codebase.
Apply this diff:
- fmt.Printf("cancelled job: %+v\n", job)
+ log.Debug("cancelled job", "jobId", job.Id, "status", job.Status)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/releasemanager/manager.go around line
110, replace the debug fmt.Printf call with the module's structured logger
(e.g., logger.Debug/Info) and emit the job details as structured fields (job ID,
status or the whole job object) instead of plain formatted output; update
imports to remove fmt if it becomes unused. Ensure the log call matches the
existing logger variable/name and log level conventions in this file.
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
📊 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/releasemanager/manager.go (2)
122-132: Replace debug prints with structured logging.The
fmt.Printfstatements at lines 127 and 132 should use the structured logger for consistency with the rest of the codebase.
229-235: Restore per-target lock to prevent race conditions and duplicate jobs.The commented-out per-target lock allows concurrent invocations of
ReconcileTargetfor the same release target to race through the three-phase deployment flow (Planning → Eligibility → Execution), potentially creating duplicate jobs and corrupting state. This issue was extensively discussed in previous reviews.
📜 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 (1)
apps/workspace-engine/pkg/workspace/releasemanager/manager.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/releasemanager/manager.go
🧠 Learnings (5)
📚 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/releasemanager/manager.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/releasemanager/manager.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/releasemanager/manager.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/releasemanager/manager.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/releasemanager/manager.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/releasemanager/manager.go (4)
apps/workspace-engine/pkg/statechange/statechange.go (3)
ChangeSet(33-36)StateChangeUpsert(19-19)StateChangeDelete(20-20)apps/workspace-engine/pkg/changeset/changeset.go (1)
ChangeSet(24-30)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Status(64-64)Cancelled(42-42)UpdatedAt(65-65)apps/workspace-engine/pkg/workspace/store/jobs.go (1)
Jobs(17-20)
⏰ 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
🔇 Additional comments (3)
apps/workspace-engine/pkg/workspace/releasemanager/manager.go (3)
11-11: LGTM: Import aligns with new state change processing model.The import change to
statechangepackage is consistent with the newProcessChangessignature and the simplified processing flow described in the PR objectives.
55-55: LGTM: Cache initialization correctly simplified.The removal of explicit type parameters from
NewCacheis appropriate since they're already specified in thestateCacheConfigtype.
183-183: LGTM: Clean delegation to unified ReconcileTarget API.The refactoring to use the public
ReconcileTargetmethod withforceRedeploy=truecorrectly bypasses eligibility checks for manual redeploys while maintaining the three-phase deployment pattern.
| job.Status = oapi.Cancelled | ||
| job.UpdatedAt = time.Now() | ||
| cancelledJobs.Set(job.Id, job) | ||
| m.store.Jobs.Upsert(ctx, job) |
There was a problem hiding this comment.
Handle Jobs.Upsert error to prevent silent cancellation failures.
If Jobs.Upsert fails, the job cancellation will not persist to storage, but the code continues as if the cancellation succeeded. This could lead to jobs remaining in a processing state when they should be cancelled.
Apply this diff:
if job != nil && job.IsInProcessingState() {
job.Status = oapi.Cancelled
job.UpdatedAt = time.Now()
- m.store.Jobs.Upsert(ctx, job)
- fmt.Printf("cancelled job: %+v\n", job)
+ if err := m.store.Jobs.Upsert(ctx, job); err != nil {
+ log.Warn("failed to cancel job", "jobId", job.Id, "error", err.Error())
+ continue
+ }
+ log.Debug("cancelled job", "jobId", job.Id, "status", job.Status)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/releasemanager/manager.go around line
126, the call to m.store.Jobs.Upsert(ctx, job) ignores its return value so a
failed persistence of the cancelled job is silently dropped; capture the
returned error, log it with context (job ID and operation), and propagate it
(return or wrap) so the caller knows the cancellation did not persist; do not
continue as if Upsert succeeded.
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores