refactor: convert all to hybrid store#831
Merged
adityachoudhari26 merged 1 commit intomainfrom Mar 5, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds OpenTelemetry tracing instrumentation to store Upsert operations for error visibility. Introduces hybrid repository layer that coordinates between in-memory and persistent DB storage using a write-through pattern. Updates store configuration and workspace consumer to utilize these hybrid repositories. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/workspace-engine/pkg/workspace/store/deployment_variables.go (1)
39-50:⚠️ Potential issue | 🟠 MajorUse
idto enforce Upsert identity consistency.
Upsertstill ignoresid. IfidanddeploymentVariable.Iddiverge, writes/changeset records can drift to the wrong logical key. Validate or normalize befored.repo.Set(...).Suggested fix
func (d *DeploymentVariables) Upsert(ctx context.Context, id string, deploymentVariable *oapi.DeploymentVariable) { _, span := deploymentVariablesTracer.Start(ctx, "UpsertDeploymentVariable") defer span.End() + if deploymentVariable == nil { + log.Error("Failed to upsert deployment variable", "error", "nil deployment variable") + return + } + if deploymentVariable.Id != "" && deploymentVariable.Id != id { + log.Error("Failed to upsert deployment variable", "error", "id mismatch") + return + } + deploymentVariable.Id = id + if err := d.repo.Set(deploymentVariable); err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to upsert deployment variable") log.Error("Failed to upsert deployment variable", "error", err) return } d.store.changeset.RecordUpsert(deploymentVariable) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/deployment_variables.go` around lines 39 - 50, DeploymentVariables.Upsert currently ignores the incoming id parameter which can cause identity drift between the provided id and deploymentVariable.Id; update the function (DeploymentVariables.Upsert) to enforce consistency by validating that id equals deploymentVariable.Id (and log/return on mismatch) or by normalizing by assigning deploymentVariable.Id = id before calling d.repo.Set and before calling d.store.changeset.RecordUpsert so the repo write and changeset use the canonical id.apps/workspace-engine/pkg/workspace/store/systems.go (1)
40-47:⚠️ Potential issue | 🔴 Critical
Upsertshould return the repository error immediately.Line 40 handles
repo.Setfailure but still proceeds to Line 45 (RecordUpsert) and returnsnilon Line 47. That marks a failed write as successful.Suggested fix
func (s *Systems) Upsert(ctx context.Context, system *oapi.System) error { _, span := systemsTracer.Start(ctx, "UpsertSystem") defer span.End() if err := s.repo.Set(system); err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to upsert system") log.Error("Failed to upsert system", "error", err) + return err } s.store.changeset.RecordUpsert(system) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/systems.go` around lines 40 - 47, The Upsert flow currently logs errors from s.repo.Set but continues to call s.store.changeset.RecordUpsert and returns nil; change it so that when s.repo.Set(system) returns an error you record the error with span.RecordError and span.SetStatus, log it (log.Error), and then immediately return that error (do not call s.store.changeset.RecordUpsert). Ensure the successful path still calls s.store.changeset.RecordUpsert(system) and returns nil.apps/workspace-engine/pkg/workspace/store/deployments.go (1)
44-51:⚠️ Potential issue | 🔴 CriticalDo not report successful upsert after
repo.Setfails.On Line 44, failure is logged but execution continues; Line 49 records an upsert and Line 51 returns
nil. This can desync state and hide write failures from callers.Suggested fix
func (e *Deployments) Upsert(ctx context.Context, deployment *oapi.Deployment) error { _, span := deploymentsTracer.Start(ctx, "UpsertDeployment") defer span.End() if err := e.repo.Set(deployment); err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to upsert deployment") log.Error("Failed to upsert deployment", "error", err) + return err } e.store.changeset.RecordUpsert(deployment) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/deployments.go` around lines 44 - 51, The code currently logs an error when e.repo.Set(deployment) fails but still calls e.store.changeset.RecordUpsert(deployment) and returns nil; change this so that on error from e.repo.Set you record the error (span.RecordError, span.SetStatus, log.Error) and immediately return that error (or wrap it) without calling e.store.changeset.RecordUpsert. Only call e.store.changeset.RecordUpsert(deployment) and return nil when e.repo.Set succeeds; reference e.repo.Set and e.store.changeset.RecordUpsert to locate the changes.apps/workspace-engine/pkg/workspace/store/environments.go (1)
46-53:⚠️ Potential issue | 🔴 CriticalReturn
erron upsert failure to avoid false-success writes.At Line 46,
repo.Setfailure is logged but not returned; Line 51 still records the upsert and Line 53 returnsnil. This can corrupt logical state and mask persistence errors.Suggested fix
func (e *Environments) Upsert(ctx context.Context, environment *oapi.Environment) error { _, span := environmentsTracer.Start(ctx, "UpsertEnvironment") defer span.End() if err := e.repo.Set(environment); err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to upsert environment") log.Error("Failed to upsert environment", "error", err) + return err } e.store.changeset.RecordUpsert(environment) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/environments.go` around lines 46 - 53, The current upsert swallows errors returned by e.repo.Set: if e.repo.Set(environment) returns err you should record the span and log, then return that err instead of continuing; additionally only call e.store.changeset.RecordUpsert(environment) after a successful Set. Update the method around e.repo.Set to return err on failure (preserve span.RecordError/span.SetStatus/log.Error) and move e.store.changeset.RecordUpsert(environment) into the success path so it is not executed when e.repo.Set fails.
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/store/workflow_job_templates.go (1)
52-61: Consider adding tracing toRemovefor consistency.The
Upsertmethod now has OpenTelemetry tracing, butRemovedoes not. If the goal is uniform observability for store mutations, consider adding similar instrumentation here.♻️ Optional: Add tracing to Remove
func (w *WorkflowJobTemplates) Remove(ctx context.Context, id string) { + _, span := workflowJobTemplatesTracer.Start(ctx, "RemoveWorkflowJobTemplate") + defer span.End() + workflowJobTemplate, ok := w.repo.Get(id) if !ok || workflowJobTemplate == nil { return } if err := w.repo.Remove(id); err != nil { + span.RecordError(err) + span.SetStatus(codes.Error, "failed to remove workflow job template") log.Error("Failed to remove workflow job template", "error", err) return } w.store.changeset.RecordDelete(workflowJobTemplate) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/workflow_job_templates.go` around lines 52 - 61, The Remove method lacks OpenTelemetry tracing; update WorkflowJobTemplates.Remove to mirror Upsert by starting a span from the incoming ctx (e.g., spanName "WorkflowJobTemplates.Remove"), use the span to set relevant attributes (like "workflow_job_template.id"), record any errors returned by w.repo.Remove on the span and log them, and ensure the span is ended before returning; keep calls to w.repo.Get and w.store.changeset.RecordDelete inside the traced context so the full mutation is captured.apps/workspace-engine/pkg/workspace/store/repository/hybrid/workflows.go (1)
10-152: Add doc comments for newly exported types and constructors.
WorkflowRepo,WorkflowJobTemplateRepo,WorkflowRunRepo,WorkflowJobRepo, and theirNew*constructors are exported but undocumented. Please add concise comments for exported declarations in this new file.As per coding guidelines: "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/workflows.go` around lines 10 - 152, Add concise Go doc comments for each exported type and constructor: WorkflowRepo, NewWorkflowRepo, WorkflowJobTemplateRepo, NewWorkflowJobTemplateRepo, WorkflowRunRepo, NewWorkflowRunRepo, WorkflowJobRepo, and NewWorkflowJobRepo. For each comment, place it immediately above the declaration and briefly describe the purpose and behavior (e.g., that WorkflowRepo is a hybrid repo delegating to an in-memory repo and persisting to dbRepo, and NewWorkflowRepo constructs it). Keep comments short (one or two sentences) and mention any non-obvious behavior such as write-through to dbRepo.apps/workspace-engine/pkg/workspace/store/store.go (2)
30-35: Document newly exportedWithHybrid*StoreOption helpers.The newly added exported
WithHybrid*functions are missing doc comments. Please add concise comments for each exported helper.As per coding guidelines: "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
Also applies to: 46-51, 62-67, 78-83, 94-99, 110-115, 144-149, 160-165, 176-181, 210-215, 226-231, 242-247, 258-263, 274-279, 290-295, 306-311, 322-327, 338-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/store.go` around lines 30 - 35, Add concise Go doc comments for all exported WithHybrid* StoreOption helpers (e.g., WithHybridDeploymentVersions) explaining their purpose and when to use them; place a short comment immediately above each function declaration, follow Go doc style (start with the function name), mention that they configure the Store to use hybrid repos (what repo is set/changed) and any non-obvious behavior or side-effects, and ensure every exported helper noted in the review (the WithHybrid* functions) has such a comment.
30-343: Consider extracting a small helper for repeated hybrid wiring.These new option helpers repeat the same setup pattern (
NewDBRepo+SetRepo(New*Repo)), which increases maintenance overhead and copy/paste drift. A tiny internal helper would reduce duplication and keep future additions safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/store.go` around lines 30 - 343, The helper recommendation: factor the repeated pattern of calling dbrepo.NewDBRepo(ctx, s.id) and then s.<Something>.SetRepo(hybridrepo.NewX(dbRepo, s.repo)) into a small internal helper (e.g., newHybridSetter) and update each WithHybrid* option (like WithHybridDeployments, WithHybridResources, WithHybridSystems, WithHybridJobs, WithHybridReleases, WithHybridPolicies, WithHybridEnvironments, WithHybridWorkflows, WithHybridWorkflowJobs, WithHybridResourceProviders, WithHybridSystemDeployments, WithHybridSystemEnvironments, WithHybridUserApprovalRecords, WithHybridDeploymentVariables, WithHybridDeploymentVariableValues, WithHybridDeploymentVersions, WithHybridWorkflowJobTemplates, WithHybridWorkflowRuns, WithHybridResourceVariables) to call that helper passing the context, store and a factory function that returns the hybridrepo.NewXXX(dbRepo, s.repo) for that repo; ensure the helper handles creating dbRepo via dbrepo.NewDBRepo(ctx, s.id) and invoking SetRepo on the target repo to preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/workspace/store/deployment_versions.go`:
- Around line 48-53: The current code calls
d.store.changeset.RecordUpsert(version) unconditionally even when
d.repo.Set(version) fails; update the flow so that
d.store.changeset.RecordUpsert(version) only runs on successful repo writes —
e.g., after the if err := d.repo.Set(version); err != nil { ... } block either
return or place RecordUpsert in the success path. Ensure you keep the existing
error handling (span.RecordError, span.SetStatus, log.Error) and only record the
changeset when d.repo.Set completes without error.
In `@apps/workspace-engine/pkg/workspace/store/job_agents.go`:
- Around line 36-41: The RecordUpsert call must only run when the repository
write succeeds: check the error returned by j.repo.Set(jobAgent) and return or
skip recording if err != nil. Specifically, in the function where
j.repo.Set(jobAgent) is invoked (the upsert flow in job_agents.go), move or gate
j.store.changeset.RecordUpsert(jobAgent) so it executes only after a successful
Set, and ensure the error path (where span.RecordError/span.SetStatus/log.Error
are called) does not call RecordUpsert.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/deployment_variables.go`:
- Around line 30-35: The in-memory repo is updated before the DB write in
DeploymentVariableRepo.Set (and similarly in its Remove and the other hybrid
methods), causing state divergence on DB failures; update the persistence order
or add rollback: first call dbRepo.DeploymentVariables().Set(...) (or
Remove(...)), check error, and only on success call r.mem.Set(...) (or
r.mem.Remove(...)); alternatively, if you must write mem first, capture the
previous in-memory state and revert it when the DB call returns an error. Ensure
you apply this change to DeploymentVariableRepo.Set,
DeploymentVariableRepo.Remove and the equivalent hybrid methods referenced in
the diff so memory and DB remain consistent.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/deployment_versions.go`:
- Around line 30-35: The in-memory-first ordering in DeploymentVersionRepo.Set
(and similarly in the Remove method) can leave mem state mutated when the DB
write fails; change the order so you persist to the durable store first by
calling r.dbRepo.DeploymentVersions().Set(entity) (or the DB delete call in
Remove) and only after that succeeds update r.mem.Set(entity) (or
r.mem.Remove(...)); ensure you return DB errors without mutating r.mem when the
DB operation fails.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/deployments.go`:
- Around line 26-38: The current write-through in DeploymentRepo.Set and Remove
mutates r.mem before the DB call, leaving memory inconsistent if the DB
operation fails; fix by performing a compensating rollback: for Set, read and
save the previous entity from r.mem (if any), call r.mem.Set(entity), then call
r.dbRepo.Deployments().Set(entity) and if that DB call returns an error restore
the previous state into r.mem (call r.mem.Set(prev) or r.mem.Remove if prev was
nil) before returning the error; for Remove, read and save the existing entity
from r.mem, call r.mem.Remove(id), then call r.dbRepo.Deployments().Remove(id)
and if DB fails restore the saved entity into r.mem via r.mem.Set(prev) and
return the error. Use the DeploymentRepo.Set, DeploymentRepo.Remove, r.mem and
r.dbRepo.Deployments() symbols to locate changes.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/environments.go`:
- Around line 26-38: The write-order currently updates in-memory first in
EnvironmentRepo.Set and Remove causing divergence if DB writes fail; change to
write the persistent store first (use r.dbRepo.Environments().Set(entity) and
r.dbRepo.Environments().Remove(id)), then update r.mem. For Set: call
r.dbRepo.Environments().Set(entity) first, then r.mem.Set(entity); if r.mem.Set
fails, roll back the DB by removing or restoring the prior record (e.g., call
r.dbRepo.Environments().Remove(entity.Id) or re-set the previous value) and
return the error. For Remove: fetch the existing entity via
r.dbRepo.Environments().Get(id) (or r.mem.Get) before deleting, then call
r.dbRepo.Environments().Remove(id) followed by r.mem.Remove(id); if r.mem.Remove
fails, restore the DB by re-setting the previously fetched entity and return the
error. Ensure all calls reference EnvironmentRepo, r.mem,
r.dbRepo.Environments().Get/Set/Remove and surface the original error.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/job_agents.go`:
- Around line 26-38: The in-memory store is being mutated before the DB write in
JobAgentRepo.Set and Remove (r.mem.Set / r.mem.Remove then
r.dbRepo.JobAgents().Set / Remove), which leaves memory stale if the DB fails;
fix by either performing the durable DB operation first and only updating r.mem
after r.dbRepo.JobAgents().Set / Remove succeeds, or (if mem-first is required)
capture the previous memory state (e.g., read existing entity from r.mem before
mutating) and on DB error perform a compensating call to restore that state
(call r.mem.Set with the previous entity on Set failure, or r.mem.Set the
removed entity back on Remove failure); apply the same pattern to both
JobAgentRepo.Set and JobAgentRepo.Remove.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/links.go`:
- Around line 29-34: The in-memory repo is mutated before the DB operation in
SystemDeploymentRepo.Link/Unlink (methods calling r.mem.Link/r.mem.Unlink then
r.dbRepo.SystemDeployments().Link/Unlink), which leaves memory inconsistent on
DB failure; change the order to perform the persistent DB call first (call
r.dbRepo.SystemDeployments().Link/Unlink) and only if it succeeds apply
r.mem.Link/r.mem.Unlink; additionally, if the mem operation fails after a
successful DB change, attempt to roll back the DB by calling the opposite DB
operation (e.g., call Unlink if Link succeeded) and return a clear error
combining both failures so callers know both outcomes.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/policies.go`:
- Around line 26-31: The current PolicyRepo.Set updates the in-memory store
(r.mem.Set) before persisting to the DB (r.dbRepo.Policies().Set), which can
leave memory reflecting a non-durable state when the DB write fails; change
PolicyRepo.Set to call r.dbRepo.Policies().Set first and only update r.mem.Set
if that call succeeds. Do the same for the delete path (PolicyRepo.Delete): call
r.dbRepo.Policies().Delete before r.mem.Delete, and return the DB error without
mutating memory when the DB operation fails. Ensure you reference and update the
calls to r.dbRepo.Policies().Set / Delete and r.mem.Set / Delete in
PolicyRepo.Set and PolicyRepo.Delete respectively.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/resource_providers.go`:
- Around line 26-38: The Set and Remove methods on ResourceProviderRepo perform
in-memory mutations via r.mem before persisting to the DB, causing non-atomic
writes; if r.dbRepo.ResourceProviders().Set or .Remove fails the in-memory
change must be rolled back. Modify ResourceProviderRepo.Set to first call
r.mem.Set(entity), then call r.dbRepo.ResourceProviders().Set(entity) and, on DB
error, undo the mem change (e.g., remove the newly set entry or restore the
prior value fetched before mutating) and return the DB error; similarly, in
ResourceProviderRepo.Remove call r.mem.Remove(id) only after saving the prior
in-memory entry (or fetch it), call r.dbRepo.ResourceProviders().Remove(id), and
on DB error restore the removed in-memory entry before returning the error.
Ensure these compensation steps reference r.mem,
r.dbRepo.ResourceProviders().Set, r.dbRepo.ResourceProviders().Remove, and the
methods ResourceProviderRepo.Set and ResourceProviderRepo.Remove.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/resource_variables.go`:
- Around line 30-35: The in-memory repo is being mutated before the DB write,
causing state desync if the DB call fails; change the ordering in
ResourceVariableRepo.Set to perform the persistent write first via
r.dbRepo.ResourceVariables().Set(entity) and only on success update the
in-memory layer with r.mem.Set(entity). Apply the same pattern to the other
methods referenced (e.g., BulkUpdate, Delete/Update methods around lines 37-42
and 48-53): run the DB operation first, and only when it returns nil update the
mem store; for bulk operations ensure partial failures do not leave mem
partially updated (either perform DB bulk commit then mem bulk update, or on DB
failure avoid any mem mutation or roll back mem changes).
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/resources.go`:
- Around line 42-47: The current implementation updates the in-memory store
before committing to the DB (e.g., ResourceRepo.Set calling r.mem.Set then
r.dbRepo.Resources().Set), which causes read-side drift on DB failure; change
each write/remove path (Set, BatchSet, Delete, BatchDelete, etc.) to perform the
DB operation first (call r.dbRepo.Resources().Set / BatchSet / Delete /
BatchDelete), check for error, and only on success apply the corresponding
in-memory change (r.mem.Set / BatchSet / Remove / BatchRemove); ensure you keep
existing error return behavior and do not swallow DB errors, and apply the same
reorder to the other methods noted in the comment (lines 49-54, 56-61, 63-68).
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/systems.go`:
- Around line 26-38: The current mem-first mutations in SystemRepo.Set and
SystemRepo.Remove update r.mem before persisting to r.dbRepo, which can leave
memory and DB out of sync if the DB call fails; change them to use the same
compensating-rollback strategy as other hybrid repos: for Set, capture the
previous in-memory entity (via r.mem.Get or equivalent), attempt the DB write
first (or keep mem write but on DB error revert mem to the captured previous
entity by calling r.mem.Set(previous) or r.mem.Remove when previous was nil),
and for Remove, capture the pre-delete entity and on DB Remove failure restore
it into r.mem (or perform DB Remove first and only then mutate r.mem).
Reference: SystemRepo.Set, SystemRepo.Remove, r.mem, r.dbRepo.Systems().Set,
r.dbRepo.Systems().Remove.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/user_approval_records.go`:
- Around line 30-35: The Set/Delete methods on UserApprovalRecordRepo currently
update the in-memory store first (r.mem.Set / r.mem.Delete) before performing
the persistent operation (r.dbRepo.UserApprovalRecords().Set / .Delete), which
can expose unpersisted state if the DB call fails; change the order to perform
the DB operation first and only update r.mem after the DB succeeds, and ensure
errors from the DB call are returned without mutating memory (for Set: call
r.dbRepo.UserApprovalRecords().Set(entity) then r.mem.Set(entity); for Delete:
call r.dbRepo.UserApprovalRecords().Delete(id) then r.mem.Delete(id)); if any
compensating rollback is required in your design, implement it after a failed
second step.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/workflows.go`:
- Around line 26-31: The current WorkflowRepo.Set mutates in-memory state via
mem.Set before persisting to the DB, which can leave memory divergent if
dbRepo.Workflows().Set fails; change the sequence to persist to the DB first
(call dbRepo.Workflows().Set(entity) and return on error) and only if that
succeeds call r.mem.Set(entity); alternatively, if you want to keep mem-first
semantics implement a rollback by capturing the previous in-memory state and
calling r.mem.Remove (or restoring the previous value) when
dbRepo.Workflows().Set returns an error; apply the same pattern to the other
repo methods referenced (the Set/Remove pairs around mem.Set/mem.Remove and
dbRepo.Workflows().Set/dbRepo.Workflows().Remove).
In `@apps/workspace-engine/pkg/workspace/store/resource_providers.go`:
- Around line 46-51: The code currently calls
r.store.changeset.RecordUpsert(resourceProvider) regardless of whether
r.repo.Set(resourceProvider) succeeded; change the control flow in the function
containing r.repo.Set so that RecordUpsert is only executed when err == nil
(i.e., after a successful r.repo.Set). Keep the existing error handling
(span.RecordError, span.SetStatus, log.Error) in the failure branch and move
r.store.changeset.RecordUpsert(resourceProvider) into the success path following
the call to r.repo.Set.
---
Outside diff comments:
In `@apps/workspace-engine/pkg/workspace/store/deployment_variables.go`:
- Around line 39-50: DeploymentVariables.Upsert currently ignores the incoming
id parameter which can cause identity drift between the provided id and
deploymentVariable.Id; update the function (DeploymentVariables.Upsert) to
enforce consistency by validating that id equals deploymentVariable.Id (and
log/return on mismatch) or by normalizing by assigning deploymentVariable.Id =
id before calling d.repo.Set and before calling d.store.changeset.RecordUpsert
so the repo write and changeset use the canonical id.
In `@apps/workspace-engine/pkg/workspace/store/deployments.go`:
- Around line 44-51: The code currently logs an error when
e.repo.Set(deployment) fails but still calls
e.store.changeset.RecordUpsert(deployment) and returns nil; change this so that
on error from e.repo.Set you record the error (span.RecordError, span.SetStatus,
log.Error) and immediately return that error (or wrap it) without calling
e.store.changeset.RecordUpsert. Only call
e.store.changeset.RecordUpsert(deployment) and return nil when e.repo.Set
succeeds; reference e.repo.Set and e.store.changeset.RecordUpsert to locate the
changes.
In `@apps/workspace-engine/pkg/workspace/store/environments.go`:
- Around line 46-53: The current upsert swallows errors returned by e.repo.Set:
if e.repo.Set(environment) returns err you should record the span and log, then
return that err instead of continuing; additionally only call
e.store.changeset.RecordUpsert(environment) after a successful Set. Update the
method around e.repo.Set to return err on failure (preserve
span.RecordError/span.SetStatus/log.Error) and move
e.store.changeset.RecordUpsert(environment) into the success path so it is not
executed when e.repo.Set fails.
In `@apps/workspace-engine/pkg/workspace/store/systems.go`:
- Around line 40-47: The Upsert flow currently logs errors from s.repo.Set but
continues to call s.store.changeset.RecordUpsert and returns nil; change it so
that when s.repo.Set(system) returns an error you record the error with
span.RecordError and span.SetStatus, log it (log.Error), and then immediately
return that error (do not call s.store.changeset.RecordUpsert). Ensure the
successful path still calls s.store.changeset.RecordUpsert(system) and returns
nil.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/workflows.go`:
- Around line 10-152: Add concise Go doc comments for each exported type and
constructor: WorkflowRepo, NewWorkflowRepo, WorkflowJobTemplateRepo,
NewWorkflowJobTemplateRepo, WorkflowRunRepo, NewWorkflowRunRepo,
WorkflowJobRepo, and NewWorkflowJobRepo. For each comment, place it immediately
above the declaration and briefly describe the purpose and behavior (e.g., that
WorkflowRepo is a hybrid repo delegating to an in-memory repo and persisting to
dbRepo, and NewWorkflowRepo constructs it). Keep comments short (one or two
sentences) and mention any non-obvious behavior such as write-through to dbRepo.
In `@apps/workspace-engine/pkg/workspace/store/store.go`:
- Around line 30-35: Add concise Go doc comments for all exported WithHybrid*
StoreOption helpers (e.g., WithHybridDeploymentVersions) explaining their
purpose and when to use them; place a short comment immediately above each
function declaration, follow Go doc style (start with the function name),
mention that they configure the Store to use hybrid repos (what repo is
set/changed) and any non-obvious behavior or side-effects, and ensure every
exported helper noted in the review (the WithHybrid* functions) has such a
comment.
- Around line 30-343: The helper recommendation: factor the repeated pattern of
calling dbrepo.NewDBRepo(ctx, s.id) and then
s.<Something>.SetRepo(hybridrepo.NewX(dbRepo, s.repo)) into a small internal
helper (e.g., newHybridSetter) and update each WithHybrid* option (like
WithHybridDeployments, WithHybridResources, WithHybridSystems, WithHybridJobs,
WithHybridReleases, WithHybridPolicies, WithHybridEnvironments,
WithHybridWorkflows, WithHybridWorkflowJobs, WithHybridResourceProviders,
WithHybridSystemDeployments, WithHybridSystemEnvironments,
WithHybridUserApprovalRecords, WithHybridDeploymentVariables,
WithHybridDeploymentVariableValues, WithHybridDeploymentVersions,
WithHybridWorkflowJobTemplates, WithHybridWorkflowRuns,
WithHybridResourceVariables) to call that helper passing the context, store and
a factory function that returns the hybridrepo.NewXXX(dbRepo, s.repo) for that
repo; ensure the helper handles creating dbRepo via dbrepo.NewDBRepo(ctx, s.id)
and invoking SetRepo on the target repo to preserve existing behavior.
In `@apps/workspace-engine/pkg/workspace/store/workflow_job_templates.go`:
- Around line 52-61: The Remove method lacks OpenTelemetry tracing; update
WorkflowJobTemplates.Remove to mirror Upsert by starting a span from the
incoming ctx (e.g., spanName "WorkflowJobTemplates.Remove"), use the span to set
relevant attributes (like "workflow_job_template.id"), record any errors
returned by w.repo.Remove on the span and log them, and ensure the span is ended
before returning; keep calls to w.repo.Get and w.store.changeset.RecordDelete
inside the traced context so the full mutation is captured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8c3b7c7-8725-4513-a18c-dfa143b458b8
📒 Files selected for processing (34)
apps/workspace-engine/pkg/workspace/store/deployment_variable_values.goapps/workspace-engine/pkg/workspace/store/deployment_variables.goapps/workspace-engine/pkg/workspace/store/deployment_versions.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/workspace/store/environments.goapps/workspace-engine/pkg/workspace/store/job_agents.goapps/workspace-engine/pkg/workspace/store/jobs.goapps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/releases.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/deployment_variables.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/deployment_versions.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/deployments.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/environments.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/job_agents.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/links.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/policies.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/resource_providers.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/resource_variables.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/resources.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/systems.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/user_approval_records.goapps/workspace-engine/pkg/workspace/store/repository/hybrid/workflows.goapps/workspace-engine/pkg/workspace/store/resource_providers.goapps/workspace-engine/pkg/workspace/store/resource_variables.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/pkg/workspace/store/systems.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/store/workflow_job.goapps/workspace-engine/pkg/workspace/store/workflow_job_templates.goapps/workspace-engine/pkg/workspace/store/workflow_runs.goapps/workspace-engine/pkg/workspace/store/workflows.goapps/workspace-engine/svc/workspaceconsumer/consumer.goapps/workspace-engine/test/integration/dbtest.go
Comment on lines
48
to
53
| if err := d.repo.Set(version); err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "failed to upsert deployment version") | ||
| log.Error("Failed to upsert deployment version", "error", err) | ||
| } | ||
| d.store.changeset.RecordUpsert(version) |
Contributor
There was a problem hiding this comment.
Do not record changeset entries when the repository write fails.
If d.repo.Set(version) fails on Line 48, Line 53 still records an upsert. That can emit a false successful change and desync downstream consumers.
💡 Proposed fix
if err := d.repo.Set(version); err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "failed to upsert deployment version")
log.Error("Failed to upsert deployment version", "error", err)
+ return
}
d.store.changeset.RecordUpsert(version)📝 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.
Suggested change
| if err := d.repo.Set(version); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to upsert deployment version") | |
| log.Error("Failed to upsert deployment version", "error", err) | |
| } | |
| d.store.changeset.RecordUpsert(version) | |
| if err := d.repo.Set(version); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to upsert deployment version") | |
| log.Error("Failed to upsert deployment version", "error", err) | |
| return | |
| } | |
| d.store.changeset.RecordUpsert(version) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/deployment_versions.go` around
lines 48 - 53, The current code calls d.store.changeset.RecordUpsert(version)
unconditionally even when d.repo.Set(version) fails; update the flow so that
d.store.changeset.RecordUpsert(version) only runs on successful repo writes —
e.g., after the if err := d.repo.Set(version); err != nil { ... } block either
return or place RecordUpsert in the success path. Ensure you keep the existing
error handling (span.RecordError, span.SetStatus, log.Error) and only record the
changeset when d.repo.Set completes without error.
Comment on lines
36
to
41
| if err := j.repo.Set(jobAgent); err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "failed to upsert job agent") | ||
| log.Error("Failed to upsert job agent", "error", err) | ||
| } | ||
| j.store.changeset.RecordUpsert(jobAgent) |
Contributor
There was a problem hiding this comment.
Avoid recording upserts after a failed repository write.
On Line 36, j.repo.Set(jobAgent) can fail, but Line 41 still records RecordUpsert(jobAgent). In a hybrid write-through flow, this can publish/track an upsert that was not durably persisted.
Suggested fix
func (j *JobAgents) Upsert(ctx context.Context, jobAgent *oapi.JobAgent) {
_, span := jobAgentsTracer.Start(ctx, "UpsertJobAgent")
defer span.End()
if err := j.repo.Set(jobAgent); err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "failed to upsert job agent")
log.Error("Failed to upsert job agent", "error", err)
+ return
}
j.store.changeset.RecordUpsert(jobAgent)
}📝 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.
Suggested change
| if err := j.repo.Set(jobAgent); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to upsert job agent") | |
| log.Error("Failed to upsert job agent", "error", err) | |
| } | |
| j.store.changeset.RecordUpsert(jobAgent) | |
| func (j *JobAgents) Upsert(ctx context.Context, jobAgent *oapi.JobAgent) { | |
| _, span := jobAgentsTracer.Start(ctx, "UpsertJobAgent") | |
| defer span.End() | |
| if err := j.repo.Set(jobAgent); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to upsert job agent") | |
| log.Error("Failed to upsert job agent", "error", err) | |
| return | |
| } | |
| j.store.changeset.RecordUpsert(jobAgent) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/job_agents.go` around lines 36 -
41, The RecordUpsert call must only run when the repository write succeeds:
check the error returned by j.repo.Set(jobAgent) and return or skip recording if
err != nil. Specifically, in the function where j.repo.Set(jobAgent) is invoked
(the upsert flow in job_agents.go), move or gate
j.store.changeset.RecordUpsert(jobAgent) so it executes only after a successful
Set, and ensure the error path (where span.RecordError/span.SetStatus/log.Error
are called) does not call RecordUpsert.
Comment on lines
+30
to
+35
| func (r *DeploymentVariableRepo) Set(entity *oapi.DeploymentVariable) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.DeploymentVariables().Set(entity) | ||
| } |
Contributor
There was a problem hiding this comment.
Both variable repos have failure-path state divergence.
Set/Remove mutate memory first in both adapters. If DB persistence fails, methods return error but in-memory state already changed.
Proposed fix
func (r *DeploymentVariableRepo) Set(entity *oapi.DeploymentVariable) error {
- if err := r.mem.Set(entity); err != nil {
+ if err := r.dbRepo.DeploymentVariables().Set(entity); err != nil {
return err
}
- return r.dbRepo.DeploymentVariables().Set(entity)
+ return r.mem.Set(entity)
}
func (r *DeploymentVariableRepo) Remove(id string) error {
- if err := r.mem.Remove(id); err != nil {
+ if err := r.dbRepo.DeploymentVariables().Remove(id); err != nil {
return err
}
- return r.dbRepo.DeploymentVariables().Remove(id)
+ return r.mem.Remove(id)
}
func (r *DeploymentVariableValueRepo) Set(entity *oapi.DeploymentVariableValue) error {
- if err := r.mem.Set(entity); err != nil {
+ if err := r.dbRepo.DeploymentVariableValues().Set(entity); err != nil {
return err
}
- return r.dbRepo.DeploymentVariableValues().Set(entity)
+ return r.mem.Set(entity)
}
func (r *DeploymentVariableValueRepo) Remove(id string) error {
- if err := r.mem.Remove(id); err != nil {
+ if err := r.dbRepo.DeploymentVariableValues().Remove(id); err != nil {
return err
}
- return r.dbRepo.DeploymentVariableValues().Remove(id)
+ return r.mem.Remove(id)
}Also applies to: 37-42, 68-73, 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/deployment_variables.go`
around lines 30 - 35, The in-memory repo is updated before the DB write in
DeploymentVariableRepo.Set (and similarly in its Remove and the other hybrid
methods), causing state divergence on DB failures; update the persistence order
or add rollback: first call dbRepo.DeploymentVariables().Set(...) (or
Remove(...)), check error, and only on success call r.mem.Set(...) (or
r.mem.Remove(...)); alternatively, if you must write mem first, capture the
previous in-memory state and revert it when the DB call returns an error. Ensure
you apply this change to DeploymentVariableRepo.Set,
DeploymentVariableRepo.Remove and the equivalent hybrid methods referenced in
the diff so memory and DB remain consistent.
Comment on lines
+30
to
+35
| func (r *DeploymentVersionRepo) Set(entity *oapi.DeploymentVersion) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.DeploymentVersions().Set(entity) | ||
| } |
Contributor
There was a problem hiding this comment.
Set/Remove should persist before mutating read path state.
With memory-first ordering, a DB failure returns error after memory has already changed, so reads can expose non-durable state.
Proposed fix
func (r *DeploymentVersionRepo) Set(entity *oapi.DeploymentVersion) error {
- if err := r.mem.Set(entity); err != nil {
+ if err := r.dbRepo.DeploymentVersions().Set(entity); err != nil {
return err
}
- return r.dbRepo.DeploymentVersions().Set(entity)
+ return r.mem.Set(entity)
}
func (r *DeploymentVersionRepo) Remove(id string) error {
- if err := r.mem.Remove(id); err != nil {
+ if err := r.dbRepo.DeploymentVersions().Remove(id); err != nil {
return err
}
- return r.dbRepo.DeploymentVersions().Remove(id)
+ return r.mem.Remove(id)
}Also applies to: 37-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/deployment_versions.go`
around lines 30 - 35, The in-memory-first ordering in DeploymentVersionRepo.Set
(and similarly in the Remove method) can leave mem state mutated when the DB
write fails; change the order so you persist to the durable store first by
calling r.dbRepo.DeploymentVersions().Set(entity) (or the DB delete call in
Remove) and only after that succeeds update r.mem.Set(entity) (or
r.mem.Remove(...)); ensure you return DB errors without mutating r.mem when the
DB operation fails.
Comment on lines
+26
to
+38
| func (r *DeploymentRepo) Set(entity *oapi.Deployment) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.Deployments().Set(entity) | ||
| } | ||
|
|
||
| func (r *DeploymentRepo) Remove(id string) error { | ||
| if err := r.mem.Remove(id); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.Deployments().Remove(id) | ||
| } |
Contributor
There was a problem hiding this comment.
Write-through can leave memory and DB inconsistent on DB errors.
Line 30 and Line 37 return DB errors after memory has already been mutated. Since reads come from memory, failed DB writes can still appear as successful state updates.
💡 Proposed fix (compensating rollback)
func (r *DeploymentRepo) Set(entity *oapi.Deployment) error {
+ prev, hadPrev := r.mem.Get(entity.Id)
if err := r.mem.Set(entity); err != nil {
return err
}
- return r.dbRepo.Deployments().Set(entity)
+ if err := r.dbRepo.Deployments().Set(entity); err != nil {
+ if hadPrev {
+ _ = r.mem.Set(prev)
+ } else {
+ _ = r.mem.Remove(entity.Id)
+ }
+ return err
+ }
+ return nil
}
func (r *DeploymentRepo) Remove(id string) error {
+ prev, hadPrev := r.mem.Get(id)
if err := r.mem.Remove(id); err != nil {
return err
}
- return r.dbRepo.Deployments().Remove(id)
+ if err := r.dbRepo.Deployments().Remove(id); err != nil {
+ if hadPrev {
+ _ = r.mem.Set(prev)
+ }
+ return err
+ }
+ return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/deployments.go`
around lines 26 - 38, The current write-through in DeploymentRepo.Set and Remove
mutates r.mem before the DB call, leaving memory inconsistent if the DB
operation fails; fix by performing a compensating rollback: for Set, read and
save the previous entity from r.mem (if any), call r.mem.Set(entity), then call
r.dbRepo.Deployments().Set(entity) and if that DB call returns an error restore
the previous state into r.mem (call r.mem.Set(prev) or r.mem.Remove if prev was
nil) before returning the error; for Remove, read and save the existing entity
from r.mem, call r.mem.Remove(id), then call r.dbRepo.Deployments().Remove(id)
and if DB fails restore the saved entity into r.mem via r.mem.Set(prev) and
return the error. Use the DeploymentRepo.Set, DeploymentRepo.Remove, r.mem and
r.dbRepo.Deployments() symbols to locate changes.
Comment on lines
+42
to
+47
| func (r *ResourceRepo) Set(entity *oapi.Resource) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.Resources().Set(entity) | ||
| } |
Contributor
There was a problem hiding this comment.
Batch and single writes should not update memory before DB commit.
On DB failure, these methods return error after memory has already been updated/removed, causing read-side drift. Batch paths increase impact.
Proposed fix
func (r *ResourceRepo) Set(entity *oapi.Resource) error {
- if err := r.mem.Set(entity); err != nil {
+ if err := r.dbRepo.Resources().Set(entity); err != nil {
return err
}
- return r.dbRepo.Resources().Set(entity)
+ return r.mem.Set(entity)
}
func (r *ResourceRepo) SetBatch(entities []*oapi.Resource) error {
- if err := r.mem.SetBatch(entities); err != nil {
+ if err := r.dbRepo.Resources().SetBatch(entities); err != nil {
return err
}
- return r.dbRepo.Resources().SetBatch(entities)
+ return r.mem.SetBatch(entities)
}
func (r *ResourceRepo) Remove(id string) error {
- if err := r.mem.Remove(id); err != nil {
+ if err := r.dbRepo.Resources().Remove(id); err != nil {
return err
}
- return r.dbRepo.Resources().Remove(id)
+ return r.mem.Remove(id)
}
func (r *ResourceRepo) RemoveBatch(ids []string) error {
- if err := r.mem.RemoveBatch(ids); err != nil {
+ if err := r.dbRepo.Resources().RemoveBatch(ids); err != nil {
return err
}
- return r.dbRepo.Resources().RemoveBatch(ids)
+ return r.mem.RemoveBatch(ids)
}Also applies to: 49-54, 56-61, 63-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/resources.go`
around lines 42 - 47, The current implementation updates the in-memory store
before committing to the DB (e.g., ResourceRepo.Set calling r.mem.Set then
r.dbRepo.Resources().Set), which causes read-side drift on DB failure; change
each write/remove path (Set, BatchSet, Delete, BatchDelete, etc.) to perform the
DB operation first (call r.dbRepo.Resources().Set / BatchSet / Delete /
BatchDelete), check for error, and only on success apply the corresponding
in-memory change (r.mem.Set / BatchSet / Remove / BatchRemove); ensure you keep
existing error return behavior and do not swallow DB errors, and apply the same
reorder to the other methods noted in the comment (lines 49-54, 56-61, 63-68).
Comment on lines
+26
to
+38
| func (r *SystemRepo) Set(entity *oapi.System) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.Systems().Set(entity) | ||
| } | ||
|
|
||
| func (r *SystemRepo) Remove(id string) error { | ||
| if err := r.mem.Remove(id); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.Systems().Remove(id) | ||
| } |
Contributor
There was a problem hiding this comment.
Mem-first mutation can expose uncommitted system state.
If DB persistence fails on Line 30 or Line 37, memory has already changed, so subsequent reads can return state that is not in the DB.
Please apply the same rollback/compensating-write strategy used to keep the two stores aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/systems.go`
around lines 26 - 38, The current mem-first mutations in SystemRepo.Set and
SystemRepo.Remove update r.mem before persisting to r.dbRepo, which can leave
memory and DB out of sync if the DB call fails; change them to use the same
compensating-rollback strategy as other hybrid repos: for Set, capture the
previous in-memory entity (via r.mem.Get or equivalent), attempt the DB write
first (or keep mem write but on DB error revert mem to the captured previous
entity by calling r.mem.Set(previous) or r.mem.Remove when previous was nil),
and for Remove, capture the pre-delete entity and on DB Remove failure restore
it into r.mem (or perform DB Remove first and only then mutate r.mem).
Reference: SystemRepo.Set, SystemRepo.Remove, r.mem, r.dbRepo.Systems().Set,
r.dbRepo.Systems().Remove.
Comment on lines
+30
to
+35
| func (r *UserApprovalRecordRepo) Set(entity *oapi.UserApprovalRecord) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.UserApprovalRecords().Set(entity) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid mutating memory before the persistent write.
If DB write/delete fails, this still leaves in-memory state changed while returning an error. Since reads are served from memory, unpersisted state becomes visible.
Proposed fix
func (r *UserApprovalRecordRepo) Set(entity *oapi.UserApprovalRecord) error {
- if err := r.mem.Set(entity); err != nil {
+ if err := r.dbRepo.UserApprovalRecords().Set(entity); err != nil {
return err
}
- return r.dbRepo.UserApprovalRecords().Set(entity)
+ return r.mem.Set(entity)
}
func (r *UserApprovalRecordRepo) Remove(key string) error {
- if err := r.mem.Remove(key); err != nil {
+ if err := r.dbRepo.UserApprovalRecords().Remove(key); err != nil {
return err
}
- return r.dbRepo.UserApprovalRecords().Remove(key)
+ return r.mem.Remove(key)
}Also applies to: 37-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/hybrid/user_approval_records.go`
around lines 30 - 35, The Set/Delete methods on UserApprovalRecordRepo currently
update the in-memory store first (r.mem.Set / r.mem.Delete) before performing
the persistent operation (r.dbRepo.UserApprovalRecords().Set / .Delete), which
can expose unpersisted state if the DB call fails; change the order to perform
the DB operation first and only update r.mem after the DB succeeds, and ensure
errors from the DB call are returned without mutating memory (for Set: call
r.dbRepo.UserApprovalRecords().Set(entity) then r.mem.Set(entity); for Delete:
call r.dbRepo.UserApprovalRecords().Delete(id) then r.mem.Delete(id)); if any
compensating rollback is required in your design, implement it after a failed
second step.
Comment on lines
+26
to
+31
| func (r *WorkflowRepo) Set(entity *oapi.Workflow) error { | ||
| if err := r.mem.Set(entity); err != nil { | ||
| return err | ||
| } | ||
| return r.dbRepo.Workflows().Set(entity) | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent memory/DB divergence on failed persistence writes.
All four repos mutate memory first and persist second. If DB Set/Remove fails, the method returns an error but in-memory state is already changed, so reads can diverge from persisted state.
💡 Example rollback pattern (apply similarly across all four repos)
func (r *WorkflowRepo) Set(entity *oapi.Workflow) error {
+ prev, existed := r.mem.Get(entity.Id)
if err := r.mem.Set(entity); err != nil {
return err
}
- return r.dbRepo.Workflows().Set(entity)
+ if err := r.dbRepo.Workflows().Set(entity); err != nil {
+ if existed {
+ _ = r.mem.Set(prev)
+ } else {
+ _ = r.mem.Remove(entity.Id)
+ }
+ return err
+ }
+ return nil
}
func (r *WorkflowRepo) Remove(id string) error {
+ prev, existed := r.mem.Get(id)
if err := r.mem.Remove(id); err != nil {
return err
}
- return r.dbRepo.Workflows().Remove(id)
+ if err := r.dbRepo.Workflows().Remove(id); err != nil {
+ if existed {
+ _ = r.mem.Set(prev)
+ }
+ return err
+ }
+ return nil
}Also applies to: 33-38, 60-65, 67-72, 98-103, 105-110, 136-141, 143-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/hybrid/workflows.go`
around lines 26 - 31, The current WorkflowRepo.Set mutates in-memory state via
mem.Set before persisting to the DB, which can leave memory divergent if
dbRepo.Workflows().Set fails; change the sequence to persist to the DB first
(call dbRepo.Workflows().Set(entity) and return on error) and only if that
succeeds call r.mem.Set(entity); alternatively, if you want to keep mem-first
semantics implement a rollback by capturing the previous in-memory state and
calling r.mem.Remove (or restoring the previous value) when
dbRepo.Workflows().Set returns an error; apply the same pattern to the other
repo methods referenced (the Set/Remove pairs around mem.Set/mem.Remove and
dbRepo.Workflows().Set/dbRepo.Workflows().Remove).
Comment on lines
46
to
51
| if err := r.repo.Set(resourceProvider); err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "failed to upsert resource provider") | ||
| log.Error("Failed to upsert resource provider", "error", err) | ||
| } | ||
| r.store.changeset.RecordUpsert(resourceProvider) |
Contributor
There was a problem hiding this comment.
Prevent false changeset upserts on failed repository writes.
When r.repo.Set(resourceProvider) fails on Line 46, Line 51 still records an upsert. That reports a successful change that never persisted.
💡 Proposed fix
if err := r.repo.Set(resourceProvider); err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "failed to upsert resource provider")
log.Error("Failed to upsert resource provider", "error", err)
+ return
}
r.store.changeset.RecordUpsert(resourceProvider)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/resource_providers.go` around lines
46 - 51, The code currently calls
r.store.changeset.RecordUpsert(resourceProvider) regardless of whether
r.repo.Set(resourceProvider) succeeded; change the control flow in the function
containing r.repo.Set so that RecordUpsert is only executed when err == nil
(i.e., after a successful r.repo.Set). Keep the existing error handling
(span.RecordError, span.SetStatus, log.Error) in the failure branch and move
r.store.changeset.RecordUpsert(resourceProvider) into the success path following
the call to r.repo.Set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit