optimising ground-control satellite state reconciliation by centralis…#449
Open
devlopharsh wants to merge 1 commit into
Open
Conversation
…ation Signed-off-by: devlopharsh <harsh237hk@gmail.com>
Contributor
📝 WalkthroughWalkthroughThis PR consolidates duplicate satellite state artifact reconstruction logic across multiple handlers into two centralized helpers: ChangesSatellite State Reconciliation Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -5 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ground-control/internal/server/helpers.go (1)
229-245: ⚡ Quick winAvoid loading each group twice while building reconciliation details.
getGroupStatesalready does aGetGroupByIDper group, and this loop immediately repeats the same lookup to collectProjects. In the add/remove/delete flows that extra N+1 runs inside an open transaction for every satellite, so folding state/project aggregation into one pass would cut the new reconciliation overhead noticeably.🤖 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 `@ground-control/internal/server/helpers.go` around lines 229 - 245, The code is doing duplicate GetGroupByID calls: getGroupStates(ctx, groupList, q) already queries each group, then the subsequent loop re-calls q.GetGroupByID for every group to collect grp.Projects, causing N+1 overhead inside the transaction. Fix by folding project aggregation into the single pass: modify getGroupStates (or its return type) to also return each group's Projects (e.g., return a map or include Projects in the returned groupStates), then use that returned data instead of calling q.GetGroupByID again in the loop that builds projects; update callers that use getGroupStates accordingly to avoid the second q.GetGroupByID calls.
🤖 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 `@ground-control/internal/server/helpers.go`:
- Around line 260-267: reconcileSatelliteState currently calls
CreateOrUpdateSatStateArtifact (which mutates Harbor) before the DB transaction
is committed, exposing non-durable state if the transaction rolls back; change
the flow so that artifact publication happens only after a successful commit —
e.g., have reconcileSatelliteState return the prepared details (or enqueue an
outbox record) instead of calling CreateOrUpdateSatStateArtifact directly, then
invoke CreateOrUpdateSatStateArtifact (or a worker that processes the outbox)
after tx.Commit() completes; update callers that expect reconcileSatelliteState
to no longer side-effect Harbor and use the new after-commit/outbox handoff to
perform the Harbor mutation reliably.
- Around line 73-84: The helper currently only returns an error when
harbor.ListReplication returns an error, but ignores the case where err==nil and
replications is empty, allowing unknown groups; change the logic in the block
using harbor.ListReplication (the replications variable from
harbor.ListReplication(ctx, harbor.ListParams{Q: fmt.Sprintf("name=%s",
groupName)})) so that you first handle the err != nil case and return an
AppError on failure, and then separately check if len(replications) < 1 and
return an AppError stating the group does not exist (using the same AppError
shape used elsewhere), ensuring the function refuses empty results even when err
== nil.
---
Nitpick comments:
In `@ground-control/internal/server/helpers.go`:
- Around line 229-245: The code is doing duplicate GetGroupByID calls:
getGroupStates(ctx, groupList, q) already queries each group, then the
subsequent loop re-calls q.GetGroupByID for every group to collect grp.Projects,
causing N+1 overhead inside the transaction. Fix by folding project aggregation
into the single pass: modify getGroupStates (or its return type) to also return
each group's Projects (e.g., return a map or include Projects in the returned
groupStates), then use that returned data instead of calling q.GetGroupByID
again in the loop that builds projects; update callers that use getGroupStates
accordingly to avoid the second q.GetGroupByID calls.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: de2f06ad-fc08-4e62-a5e3-44b6719cbbb7
📒 Files selected for processing (4)
ground-control/internal/server/config_handlers.goground-control/internal/server/group_handlers.goground-control/internal/server/helpers.goground-control/internal/server/satellite_handlers.go
Comment on lines
+73
to
+84
| replications, err := harbor.ListReplication(ctx, harbor.ListParams{ | ||
| Q: fmt.Sprintf("name=%s", groupName), | ||
| }) | ||
| if len(replications) < 1 { | ||
| if err != nil { | ||
| log.Println(err) | ||
| return nil, &AppError{ | ||
| Message: fmt.Sprintf("Error: Group Name: %s, does not exist in replication, Please give a Valid Group Name", groupName), | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Reject groups missing from Harbor even when the lookup itself succeeds.
An empty ListReplication result with err == nil currently falls through and still links the satellite to the DB group. That accepts groups that are not declared in Harbor, which breaks the validation this helper is trying to enforce.
Proposed fix
replications, err := harbor.ListReplication(ctx, harbor.ListParams{
Q: fmt.Sprintf("name=%s", groupName),
})
- if len(replications) < 1 {
- if err != nil {
- log.Println(err)
- return nil, &AppError{
- Message: fmt.Sprintf("Error: Group Name: %s, does not exist in replication, Please give a Valid Group Name", groupName),
- Code: http.StatusBadRequest,
- }
- }
+ if err != nil {
+ log.Println(err)
+ return nil, &AppError{
+ Message: "Error: failed to validate group replication",
+ Code: http.StatusBadGateway,
+ }
+ }
+ if len(replications) == 0 {
+ return nil, &AppError{
+ Message: fmt.Sprintf("Error: Group Name: %s, does not exist in replication, Please give a Valid Group Name", groupName),
+ Code: http.StatusBadRequest,
+ }
}📝 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
| replications, err := harbor.ListReplication(ctx, harbor.ListParams{ | |
| Q: fmt.Sprintf("name=%s", groupName), | |
| }) | |
| if len(replications) < 1 { | |
| if err != nil { | |
| log.Println(err) | |
| return nil, &AppError{ | |
| Message: fmt.Sprintf("Error: Group Name: %s, does not exist in replication, Please give a Valid Group Name", groupName), | |
| Code: http.StatusBadRequest, | |
| } | |
| } | |
| } | |
| replications, err := harbor.ListReplication(ctx, harbor.ListParams{ | |
| Q: fmt.Sprintf("name=%s", groupName), | |
| }) | |
| if err != nil { | |
| log.Println(err) | |
| return nil, &AppError{ | |
| Message: "Error: failed to validate group replication", | |
| Code: http.StatusBadGateway, | |
| } | |
| } | |
| if len(replications) == 0 { | |
| return nil, &AppError{ | |
| Message: fmt.Sprintf("Error: Group Name: %s, does not exist in replication, Please give a Valid Group Name", groupName), | |
| Code: http.StatusBadRequest, | |
| } | |
| } |
🤖 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 `@ground-control/internal/server/helpers.go` around lines 73 - 84, The helper
currently only returns an error when harbor.ListReplication returns an error,
but ignores the case where err==nil and replications is empty, allowing unknown
groups; change the logic in the block using harbor.ListReplication (the
replications variable from harbor.ListReplication(ctx, harbor.ListParams{Q:
fmt.Sprintf("name=%s", groupName)})) so that you first handle the err != nil
case and return an AppError on failure, and then separately check if
len(replications) < 1 and return an AppError stating the group does not exist
(using the same AppError shape used elsewhere), ensuring the function refuses
empty results even when err == nil.
Comment on lines
+260
to
+267
| func reconcileSatelliteState(ctx context.Context, q *database.Queries, satelliteID int32) error { | ||
| details, err := buildSatelliteStateDetails(ctx, q, satelliteID) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to fetch satellite config: %v", err) | ||
| return database.Config{}, &AppError{ | ||
| Message: "Error: Failed to fetch satellite config", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| configObject, err := dbQueries.GetConfigByID(ctx, satelliteConfig.ConfigID) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to fetch satellite config: %v", err) | ||
| return database.Config{}, &AppError{ | ||
| Message: "Error: Failed to fetch satellite config", | ||
| if err := utils.CreateOrUpdateSatStateArtifact(ctx, details.Satellite.Name, details.GroupStates, details.Config.ConfigName); err != nil { | ||
| log.Printf("failed to update satellite state artifact for %s: %v", details.Satellite.Name, err) |
Contributor
There was a problem hiding this comment.
Don’t publish Harbor state before the DB transaction is durable.
CreateOrUpdateSatStateArtifact mutates Harbor immediately, but all of the new callers invoke this helper before tx.Commit(). If the transaction later rolls back or the commit fails, Harbor exposes a satellite state that never became durable in Postgres. This centralized helper is the right place to move to an after-commit or outbox-style handoff.
🤖 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 `@ground-control/internal/server/helpers.go` around lines 260 - 267,
reconcileSatelliteState currently calls CreateOrUpdateSatStateArtifact (which
mutates Harbor) before the DB transaction is committed, exposing non-durable
state if the transaction rolls back; change the flow so that artifact
publication happens only after a successful commit — e.g., have
reconcileSatelliteState return the prepared details (or enqueue an outbox
record) instead of calling CreateOrUpdateSatStateArtifact directly, then invoke
CreateOrUpdateSatStateArtifact (or a worker that processes the outbox) after
tx.Commit() completes; update callers that expect reconcileSatelliteState to no
longer side-effect Harbor and use the new after-commit/outbox handoff to perform
the Harbor mutation reliably.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ground-control/internal/server/satellite_handlers.go">
<violation number="1" location="ground-control/internal/server/satellite_handlers.go:1109">
P2: Duplicate state-rebuild work in add/remove group handlers: `buildSatelliteStateDetails` is called to get `details.Projects`, then `reconcileSatelliteState` is called which internally rebuilds the same state again, causing all DB queries to run twice within one request path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| // Update the state artifact to also track the new group state artifact | ||
| err = utils.CreateOrUpdateSatStateArtifact(r.Context(), sat.Name, groupStates, configObject.ConfigName) | ||
| if err != nil { | ||
| if err := reconcileSatelliteState(r.Context(), q, sat.ID); err != nil { |
Contributor
There was a problem hiding this comment.
P2: Duplicate state-rebuild work in add/remove group handlers: buildSatelliteStateDetails is called to get details.Projects, then reconcileSatelliteState is called which internally rebuilds the same state again, causing all DB queries to run twice within one request path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ground-control/internal/server/satellite_handlers.go, line 1109:
<comment>Duplicate state-rebuild work in add/remove group handlers: `buildSatelliteStateDetails` is called to get `details.Projects`, then `reconcileSatelliteState` is called which internally rebuilds the same state again, causing all DB queries to run twice within one request path.</comment>
<file context>
@@ -1197,9 +1106,7 @@ func (s *Server) addSatelliteToGroup(w http.ResponseWriter, r *http.Request) {
- // Update the state artifact to also track the new group state artifact
- err = utils.CreateOrUpdateSatStateArtifact(r.Context(), sat.Name, groupStates, configObject.ConfigName)
- if err != nil {
+ if err := reconcileSatelliteState(r.Context(), q, sat.ID); err != nil {
log.Printf("Error: Failed to update satellite state artifact: %v", err)
HandleAppError(w, err)
</file context>
shri771
suggested changes
May 29, 2026
shri771
left a comment
shri771
left a comment
There was a problem hiding this comment.
Left a suggestion. Also, the AI Bot suggestion seems reasonable to me.
There was a problem hiding this comment.
In the robot-account paths, buildSatelliteStateDetails is called for details. Projects and then reconcileSatelliteState runs right after, which rebuilds the same details internally. Could we add a variant of reconcileSatelliteState next to the existing one in helpers.go:260 that accepts a prebuilt details value and use it at those three sites?
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
2 participants
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.
Description
Centralizes Ground Control satellite state reconciliation by moving repeated state rebuild/publish logic into a shared helper.
Changes
Testing
go test ./internal/server/...Additional context
Summary by CodeRabbit