HYPERFLEET-1192 - fix: prevent premature parent hard-delete with soft…#269
HYPERFLEET-1192 - fix: prevent premature parent hard-delete with soft…#269ldornele wants to merge 8 commits into
Conversation
…-deleted children Adds ExistsSoftDeletedByOwner DAO method to check for soft-deleted children. Parents are now soft-deleted (not hard-deleted) when they have soft-deleted children waiting for adapter finalization, preventing referential integrity issues and ensuring proper cleanup ordering. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/services/resource_test.go (1)
108-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntested error path for
ExistsSoftDeletedByOwner.The mock can't simulate a DAO failure here, so
deleteResourceTree'serrors.GeneralErrorbranch (pkg/services/resource.go, "Unable to check soft-deleted %s children") is never exercised.As per path instructions, "Error paths SHOULD be tested, not just happy paths."
🧪 Proposed error-injection support
type mockResourceDao struct { resources map[string]*api.Resource createErr error saveErr error deleteErr error + existsSoftDeletedErr error } func (d *mockResourceDao) ExistsSoftDeletedByOwner(_ context.Context, kind, ownerID string) (bool, error) { + if d.existsSoftDeletedErr != nil { + return false, d.existsSoftDeletedErr + } for _, r := range d.resources { if r.Kind == kind && r.OwnerID != nil && *r.OwnerID == ownerID && r.DeletedTime != nil { return true, nil } } return false, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource_test.go` around lines 108 - 116, Add error-injection support to the mockResourceDao.ExistsSoftDeletedByOwner method so it can return a DAO failure instead of only scanning resources. Update the resource tests to force that error path and assert deleteResourceTree handles it by returning the errors.GeneralError branch with the "Unable to check soft-deleted %s children" message.Source: Path instructions
pkg/services/resource.go (1)
164-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract soft-delete decision into a helper — function now exceeds 50 lines / 5 branches.
deleteResourceTreegrew past the HyperFleet QUAL-03 threshold with three loops plus nested conditionals. Extract the Reason 1/Reason 2 decision into a dedicated method for readability and unit-testability.As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
♻️ Proposed extraction
- // Determine whether to soft-delete or hard-delete this resource. - // Soft-delete is required when: - // 1. Resource has RequiredAdapters (existing behavior) - // 2. Resource has soft-deleted children waiting for adapter finalization - desc := registry.MustGet(resource.Kind) - shouldSoftDelete := false - - // Reason 1: Resource has RequiredAdapters - if len(desc.RequiredAdapters) > 0 { - shouldSoftDelete = true - } - - // Reason 2: Resource has soft-deleted children - // Parent must remain in DB until all children (active or soft-deleted) are gone - if !shouldSoftDelete { - for _, child := range children { - exists, err := s.resourceDao.ExistsSoftDeletedByOwner(ctx, child.Kind, resource.ID) - if err != nil { - return errors.GeneralError( - "Unable to check soft-deleted %s children: %s", child.Kind, err, - ) - } - if exists { - shouldSoftDelete = true - break - } - } - } - - // Execute delete based on decision - if shouldSoftDelete { + shouldSoftDelete, svcErr := s.shouldSoftDelete(ctx, resource, children) + if svcErr != nil { + return svcErr + } + if shouldSoftDelete { if saveErr := s.resourceDao.Save(ctx, resource); saveErr != nil { return handleSoftDeleteError(resource.Kind, saveErr) } return nil } +// shouldSoftDelete reports whether resource must remain soft-deleted: +// (1) its own descriptor requires adapter finalization, or +// (2) it still has soft-deleted children awaiting finalization/cleanup. +func (s *sqlResourceService) shouldSoftDelete( + ctx context.Context, resource *api.Resource, children []registry.EntityDescriptor, +) (bool, *errors.ServiceError) { + if desc := registry.MustGet(resource.Kind); len(desc.RequiredAdapters) > 0 { + return true, nil + } + for _, child := range children { + exists, err := s.resourceDao.ExistsSoftDeletedByOwner(ctx, child.Kind, resource.ID) + if err != nil { + return false, errors.GeneralError( + "Unable to check soft-deleted %s children: %s", child.Kind, err, + ) + } + if exists { + return true, nil + } + } + return false, nil +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource.go` around lines 164 - 240, The deleteResourceTree method in sqlResourceService is too large and branchy; extract the soft-delete decision logic into a dedicated helper to improve readability and testability. Move the “Reason 1/Reason 2” checks (RequiredAdapters and ExistsSoftDeletedByOwner) into a new method on sqlResourceService or a small helper near deleteResourceTree, then have deleteResourceTree call that helper before deciding between Save and Delete. Keep the existing cascade/restrict behavior in deleteResourceTree and use the new helper to encapsulate only the soft-delete decision.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/services/resource.go`:
- Around line 198-228: The delete flow in resource.go can leave a soft-deleted
parent permanently orphaned because Delete() short-circuits once deleted_time is
set, so it never rechecks whether the soft-deleted child is still present.
Update the logic around the delete decision in the resource deletion path (the
block using shouldSoftDelete, registry.MustGet, and ExistsSoftDeletedByOwner) so
a previously soft-deleted parent can be re-evaluated and hard-deleted once its
child dependency is gone, rather than returning immediately on deleted_time
alone.
---
Nitpick comments:
In `@pkg/services/resource_test.go`:
- Around line 108-116: Add error-injection support to the
mockResourceDao.ExistsSoftDeletedByOwner method so it can return a DAO failure
instead of only scanning resources. Update the resource tests to force that
error path and assert deleteResourceTree handles it by returning the
errors.GeneralError branch with the "Unable to check soft-deleted %s children"
message.
In `@pkg/services/resource.go`:
- Around line 164-240: The deleteResourceTree method in sqlResourceService is
too large and branchy; extract the soft-delete decision logic into a dedicated
helper to improve readability and testability. Move the “Reason 1/Reason 2”
checks (RequiredAdapters and ExistsSoftDeletedByOwner) into a new method on
sqlResourceService or a small helper near deleteResourceTree, then have
deleteResourceTree call that helper before deciding between Save and Delete.
Keep the existing cascade/restrict behavior in deleteResourceTree and use the
new helper to encapsulate only the soft-delete decision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 14e66e45-f1ed-49d3-b67c-35728d416532
📒 Files selected for processing (4)
pkg/dao/mocks/resource.gopkg/dao/resource.gopkg/services/resource.gopkg/services/resource_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 552 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/dao pkg/dao/mocks pkg/registry | +1 |
Computed by hyperfleet-risk-scorer
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/resource_test.go (1)
1243-1256: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExercise the already-soft-deleted parent path.
This test deletes fresh
ch-2, so it does not verify the regression fix: re-deleting previously soft-deletedch-1afterv-1is gone should hard-delete the parent. Cover that path to prevent CWE-459 incomplete cleanup regressions.Proposed test fix
- // Note: In production, a cleanup job would detect that Channel has no children - // and hard-delete it. For now, we verify that a fresh delete (of a non-deleted Channel) - // with no children does hard-delete. - - // Test with fresh Channel (no children) - channel2 := testResource("Channel", "ch-2", "beta") - mockDao.addResource(channel2) - - _, svcErr = svc.Delete(context.Background(), "Channel", "ch-2") + // Re-delete the already soft-deleted Channel now that the child is gone. + _, svcErr = svc.Delete(context.Background(), "Channel", "ch-1") Expect(svcErr).To(BeNil()) - // Channel2 should be hard-deleted immediately (no children) - channel2Gone := mockDao.resources[resourceKey("Channel", "ch-2")] - Expect(channel2Gone).To(BeNil(), "Channel should be hard-deleted when no children exist") + channelGone := mockDao.resources[resourceKey("Channel", "ch-1")] + Expect(channelGone).To(BeNil(), "previously soft-deleted Channel should be hard-deleted once children are gone")As per path instructions, “New exported functions and critical logic paths SHOULD have tests”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource_test.go` around lines 1243 - 1256, The current test only covers deleting a fresh Channel with no children, so it misses the regression path for re-deleting an already soft-deleted parent. Update the test in resource_test.go around the Delete flow to exercise the existing soft-deleted Channel path: first soft-delete ch-1, remove its child v-1, then call svc.Delete on ch-1 again and assert it is hard-deleted. Use the existing testResource, mockDao, resourceKey, and svc.Delete helpers to locate and verify the final cleanup behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/services/resource_test.go`:
- Around line 1243-1256: The current test only covers deleting a fresh Channel
with no children, so it misses the regression path for re-deleting an already
soft-deleted parent. Update the test in resource_test.go around the Delete flow
to exercise the existing soft-deleted Channel path: first soft-delete ch-1,
remove its child v-1, then call svc.Delete on ch-1 again and assert it is
hard-deleted. Use the existing testResource, mockDao, resourceKey, and
svc.Delete helpers to locate and verify the final cleanup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f55a7b70-3387-413c-8c3c-50869c94686b
📒 Files selected for processing (2)
pkg/services/resource.gopkg/services/resource_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration/resource_delete_test.go (2)
17-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegression coverage silently depends on live registry config.
Both
ParentSoftDeletedWhileChildSoftDeleted(Lines 23-26) andChildHardDeletedImmediately(Lines 148-151) skip based on whetherVersioncurrently hasRequiredAdaptersconfigured, andParentHardDeletedAfterChildrenGone(Lines 96-112) branches its assertions the same way. If the registry config forVersion/Channelchanges, the specific scenario this PR fixes — parent staying soft-deleted while a child is soft-deleted — can silently stop being exercised while CI stays green.Since this is the core regression test for HYPERFLEET-1192, prefer a fixture/kind whose
RequiredAdaptersstate is guaranteed by the test itself (e.g., a dedicated test-only kind or explicit registry override) rather than relying on ambient global config andt.Skip.Also applies to: 81-113, 143-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/resource_delete_test.go` around lines 17 - 26, The delete regression tests currently depend on ambient registry state for Version/Channel RequiredAdapters, which can cause the HYPERFLEET-1192 scenario to stop being covered without failing. Update TestResourceDelete_ParentChildWithRequiredAdapters and the related ParentHardDeletedAfterChildrenGone / ChildHardDeletedImmediately paths to use a test-controlled fixture or explicit registry override so RequiredAdapters is guaranteed by the test itself, and remove the t.Skip/branching based on live registry config. Use the existing setupResourceTest, registry.MustGet, and the Version/Channel test cases as the anchor points when making the test deterministic.
18-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing convergence test: child hard-deleted → parent becomes hard-deletable.
This test stops after both Channel and Version are soft-deleted (Lines 63-77). Per the architecture's force-deletion design, hard-delete should only proceed once dependent subresources are actually gone — but no test verifies that once the soft-deleted Version is truly removed (e.g., via
resourceDao.Deletesimulating adapter finalization),shouldSoftDeleteflips back to false viaExistsSoftDeletedByOwnerand the parent Channel can then be hard-deleted.Without this, the "premature hard-delete" fix is only tested at the entry point, not through to completion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/resource_delete_test.go` around lines 18 - 79, Add a convergence test in the existing ParentSoftDeletedWhileChildSoftDeleted flow around svc.Delete, shouldSoftDelete, and ExistsSoftDeletedByOwner to cover the case where the soft-deleted Version is later hard-removed. After verifying both Channel and Version are soft-deleted, simulate child finalization by deleting the Version through the underlying resourceDao, then assert the parent Channel becomes hard-deletable (Delete succeeds without soft-delete behavior and the record is actually gone). Use the existing setupResourceTest, checkResourceCount, and createdChannel/createdVersion IDs to verify the transition end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/integration/resource_delete_test.go`:
- Around line 17-26: The delete regression tests currently depend on ambient
registry state for Version/Channel RequiredAdapters, which can cause the
HYPERFLEET-1192 scenario to stop being covered without failing. Update
TestResourceDelete_ParentChildWithRequiredAdapters and the related
ParentHardDeletedAfterChildrenGone / ChildHardDeletedImmediately paths to use a
test-controlled fixture or explicit registry override so RequiredAdapters is
guaranteed by the test itself, and remove the t.Skip/branching based on live
registry config. Use the existing setupResourceTest, registry.MustGet, and the
Version/Channel test cases as the anchor points when making the test
deterministic.
- Around line 18-79: Add a convergence test in the existing
ParentSoftDeletedWhileChildSoftDeleted flow around svc.Delete, shouldSoftDelete,
and ExistsSoftDeletedByOwner to cover the case where the soft-deleted Version is
later hard-removed. After verifying both Channel and Version are soft-deleted,
simulate child finalization by deleting the Version through the underlying
resourceDao, then assert the parent Channel becomes hard-deletable (Delete
succeeds without soft-delete behavior and the record is actually gone). Use the
existing setupResourceTest, checkResourceCount, and
createdChannel/createdVersion IDs to verify the transition end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 55c4713a-0596-424a-80e6-8a40b2511300
📒 Files selected for processing (2)
pkg/services/resource_test.gotest/integration/resource_delete_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/resource_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/resource_delete_test.go (1)
22-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated global registry mutation without isolation guarantee.
Same mutate-then-defer-restore pattern against the shared
registrysingleton is duplicated three times. This works only because subtests run strictly sequentially — any futuret.Parallel()addition on these or sibling tests readingRequiredAdapterswill race (CWE-362, unsynchronized shared state). Extract a single helper to centralize the mutation/restore and make the sequential-only assumption explicit/enforced.♻️ Proposed helper extraction
func withRequiredAdapters(t *testing.T, kind string, adapters []string) { t.Helper() desc := registry.MustGet(kind) original := desc.RequiredAdapters desc.RequiredAdapters = adapters registry.Register(desc) t.Cleanup(func() { desc.RequiredAdapters = original registry.Register(desc) }) }- versionDesc := registry.MustGet("Version") - originalAdapters := versionDesc.RequiredAdapters - if len(originalAdapters) == 0 { - versionDesc.RequiredAdapters = []string{"test-adapter"} - registry.Register(versionDesc) - defer func() { - versionDesc.RequiredAdapters = originalAdapters - registry.Register(versionDesc) - }() - } + if len(registry.MustGet("Version").RequiredAdapters) == 0 { + withRequiredAdapters(t, "Version", []string{"test-adapter"}) + }Also applies to: 117-127, 183-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/resource_delete_test.go` around lines 22 - 32, Repeated mutate-and-restore logic against the shared registry singleton should be centralized and made safe for test isolation. Extract a helper like withRequiredAdapters that wraps the registry.MustGet/Register mutation for the Version descriptor, uses t.Helper and t.Cleanup to restore the original RequiredAdapters, and call it from each affected test block instead of duplicating the pattern. This makes the sequential-only assumption explicit in one place and reduces the risk of future races if any related tests use t.Parallel or access registry.RequiredAdapters concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/integration/resource_delete_test.go`:
- Around line 22-32: Repeated mutate-and-restore logic against the shared
registry singleton should be centralized and made safe for test isolation.
Extract a helper like withRequiredAdapters that wraps the
registry.MustGet/Register mutation for the Version descriptor, uses t.Helper and
t.Cleanup to restore the original RequiredAdapters, and call it from each
affected test block instead of duplicating the pattern. This makes the
sequential-only assumption explicit in one place and reduces the risk of future
races if any related tests use t.Parallel or access registry.RequiredAdapters
concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3d6e7286-0054-4110-81a3-934bc3dfa8f0
📒 Files selected for processing (2)
test/integration/resource_delete_test.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| Kind: "Version", | ||
| Plural: "versions", | ||
| ParentKind: "Channel", | ||
| OnParentDelete: registry.OnParentDeleteRestrict, |
There was a problem hiding this comment.
Warning
Blocking
Category: JIRA
JIRA AC #4 explicitly requires cascade behavior: "For generic resources using OnParentDeleteCascade, a parent with a soft-deleted child that has RequiredAdapters is not hard-deleted while the child row remains." But all tests here use OnParentDeleteRestrict. Consider adding a cascade variant, e.g.:
func setupDescriptorsWithCascadeAndRequiredAdapters() {
registry.Reset()
registry.Register(registry.EntityDescriptor{Kind: "Workspace", Plural: "workspaces"})
registry.Register(registry.EntityDescriptor{
Kind: "Task",
Plural: "tasks",
ParentKind: "Workspace",
OnParentDelete: registry.OnParentDeleteCascade,
RequiredAdapters: []string{"adapter1"},
})
}
func TestResourceService_Delete_CascadeParentSoftDeletedWhileChildSoftDeleted(t *testing.T) {
RegisterTestingT(t)
setupDescriptorsWithCascadeAndRequiredAdapters()
mockDao := newMockResourceDao()
svc, _, _ := newTestResourceService(mockDao)
workspace := testResource("Workspace", "ws-1", "dev")
mockDao.addResource(workspace)
task := testResourceWithOwner("Task", "t-1", "build", "ws-1")
mockDao.addResource(task)
// Delete Workspace → cascade-deletes Task (soft-delete, RequiredAdapters)
// → Workspace should be soft-deleted (soft-deleted child exists)
_, svcErr := svc.Delete(context.Background(), "Workspace", "ws-1")
Expect(svcErr).To(BeNil())
ws := mockDao.resources[resourceKey("Workspace", "ws-1")]
Expect(ws).ToNot(BeNil(), "Workspace should still exist (soft-deleted)")
Expect(ws.DeletedTime).ToNot(BeNil())
tk := mockDao.resources[resourceKey("Task", "t-1")]
Expect(tk).ToNot(BeNil(), "Task should still exist (soft-deleted)")
Expect(tk.DeletedTime).ToNot(BeNil())
}This validates the cascade path end-to-end and closes AC #4.
There was a problem hiding this comment.
Fixed! Added a cascade test. TY!
| child := testResource("Version", "v-1", "1.0.0") | ||
| child.OwnerID = &parent.ID |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
The other two new tests use testResourceWithOwner (which this PR introduces). This test uses the older testResource + manual owner pattern instead. For consistency:
| child := testResource("Version", "v-1", "1.0.0") | |
| child.OwnerID = &parent.ID | |
| child := testResourceWithOwner("Version", "v-1", "1.0.0", parent.ID) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/registry/registry.go (1)
123-130: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnsynchronized global map mutation — data race if tests ever run parallel.
UpdateDescriptorreads and writes the package-leveldescriptorsmap with zero locking. CWE-362 (race condition). Currently call sites inresource_delete_test.goare sequential subtests, but any futuret.Parallel()adoption on tests that call this (orRegister/Reset/Getconcurrently) triggers Go's fatalconcurrent map writesor silently corrupts the map under-race.🔒 Proposed fix
+var mu sync.RWMutex + func UpdateDescriptor(kind string, updateFn func(*EntityDescriptor)) { + mu.Lock() + defer mu.Unlock() desc, ok := descriptors[kind] if !ok { panic(fmt.Sprintf("entity kind %q not registered", kind)) } updateFn(&desc) descriptors[kind] = desc }Apply the same
mutoRegister,Get,All, andResetfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/registry.go` around lines 123 - 130, UpdateDescriptor mutates the package-level descriptors map without synchronization, which can race with Register, Get, All, or Reset. Protect all access to descriptors with the existing mu in pkg/registry/registry.go, and make UpdateDescriptor follow the same locking pattern as the other registry helpers so concurrent tests or future t.Parallel usage cannot trigger concurrent map writes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/registry/registry.go`:
- Around line 123-130: UpdateDescriptor mutates the package-level descriptors
map without synchronization, which can race with Register, Get, All, or Reset.
Protect all access to descriptors with the existing mu in
pkg/registry/registry.go, and make UpdateDescriptor follow the same locking
pattern as the other registry helpers so concurrent tests or future t.Parallel
usage cannot trigger concurrent map writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 251f01b7-0aa1-4336-a2f0-d44a8f7a4762
📒 Files selected for processing (2)
pkg/registry/registry.gotest/integration/resource_delete_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/resource_delete_test.go
| // Ensure Version has RequiredAdapters for this test | ||
| versionDesc := registry.MustGet("Version") | ||
| originalAdapters := versionDesc.RequiredAdapters | ||
| if len(originalAdapters) == 0 { |
There was a problem hiding this comment.
Warning
Blocking
Category: Bug
This test will always skip in CI because the production Version registration (plugins/versions/plugin.go:22-29) does not set RequiredAdapters. The new soft-delete retention logic is only validated at the unit test level.
Consider using registry.UpdateDescriptor (designed for tests) to temporarily inject RequiredAdapters instead of skipping:
svc, h := setupResourceTest(t)
// Temporarily add RequiredAdapters to Version for this test
registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) {
d.RequiredAdapters = []string{"test-adapter"}
})
t.Cleanup(func() {
registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) {
d.RequiredAdapters = nil
})
})This way the test exercises the real DB path with ExistsSoftDeletedByOwner and actually validates the fix.
There was a problem hiding this comment.
Fixed - removed conditional skip logic
2 participants
Summary
Prevents premature hard-deletion of parent resources when they have soft-deleted children waiting for adapter finalization.
Problem
When a parent resource is deleted, the current implementation only checks for active children before deciding whether to soft-delete or hard-delete. This creates a gap:
RequiredAdaptersis deleted → soft-deleted (waiting for adapter finalization)This violates the HyperFleet deletion semantic where parents must remain in the database until all children (active or soft-deleted) are removed.
Solution
Added a new DAO method
ExistsSoftDeletedByOwner(ctx, kind, ownerID)to check for soft-deleted children. The service layer now considers both conditions when deciding soft vs hard delete:RequiredAdapters(existing behavior)This ensures parents remain in the database until:
Finalized: True, ANDChanges
DAO Layer (
pkg/dao/resource.go):ExistsSoftDeletedByOwnertoResourceDaointerfaceSELECT EXISTS(...WHERE kind = ? AND owner_id = ? AND deleted_time IS NOT NULL)Service Layer (
pkg/services/resource.go):deleteResourceTreeto check for soft-deleted childrenRequiredAdaptersand soft-deleted childrenMocks (
pkg/dao/mocks/resource.go,pkg/services/resource_test.go):Tests (
pkg/services/resource_test.go):TestResourceService_Delete_ParentSoftDeletedWhileChildSoftDeleted: Verifies parent is soft-deleted when child is soft-deletedTestResourceService_Delete_ParentHardDeletedAfterChildGone: Verifies parent is hard-deleted after all children removedReview
✅ Architecture review: Aligned with HyperFleet soft-delete semantics and adapter finalization patterns
✅ Standards review: No violations found
✅ CodeRabbit: No findings
✅ All tests pass: 1308 tests in 21.268s