feat: post checks to PR commit for dry run results#1023
feat: post checks to PR commit for dry run results#1023adityachoudhari26 merged 4 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds GitHub Check Run integration for deployment plan results. It introduces two new database queries to fetch target context and result history, extends the data access interfaces, implements GitHub check aggregation and upsert logic that renders formatted output with plan diffs, and updates the controller to invoke the check creation/update after terminal result states. Changes
Sequence DiagramsequenceDiagram
participant Controller
participant Getter
participant Database
participant GitHub
Controller->>Controller: Process reaches terminal state
Controller->>Controller: MaybeUpdateTargetCheck(ctx, getter, resultID)
Note over Controller: Span started & attributes set
Controller->>Getter: GetTargetContextByResultID(resultID)
Getter->>Database: Query (joins plan/target/workspace/env/resource)
Database-->>Getter: context row
Getter-->>Controller: targetContext
Controller->>Getter: ListDeploymentPlanTargetResultsByTargetID(targetID)
Getter->>Database: Query (results ordered by timestamp)
Database-->>Getter: result rows
Getter-->>Controller: agent results slice
Note over Controller: Aggregate statuses, count changes, render Markdown
Controller->>GitHub: Create or update check run
GitHub-->>Controller: success/error
Note over Controller: Record result to span, return error if any
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
There was a problem hiding this comment.
Pull request overview
Adds a GitHub Checks integration for deployment plan target results so dry-run outputs (including diffs) appear directly on the PR commit, aligning with the native GitHub UX described in #1012.
Changes:
- Introduces a new controller helper to render and upsert per-target GitHub Check Runs (status/conclusion/title + markdown diff output).
- Extends the deploymentplanresult Getter to load target context + list all agent results for a target, backed by new SQLC queries.
- Wires Check Run updates into the deploymentplanresult controller when results reach terminal states (completed/errored/unsupported).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go | New GitHub Check Run rendering + upsert logic for target plan results |
| apps/workspace-engine/svc/controllers/deploymentplanresult/controller.go | Triggers check updates after persisting terminal result statuses |
| apps/workspace-engine/svc/controllers/deploymentplanresult/getters.go | Extends Getter interface for target context + per-target results listing |
| apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go | Postgres implementation of the new Getter methods |
| apps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.go | Updates mock Getter to satisfy the expanded interface |
| apps/workspace-engine/pkg/db/queries/deployment_plan.sql | Adds SQLC queries to fetch target context and list target result rows |
| apps/workspace-engine/pkg/db/deployment_plan.sql.go | Regenerates SQLC output for the new queries/types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if r.HasChanges == nil || !*r.HasChanges { | ||
| sb.WriteString("\nNo changes\n") | ||
| return sb.String() | ||
| } | ||
|
|
||
| diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ | ||
| A: difflib.SplitLines(r.Current), | ||
| B: difflib.SplitLines(r.Proposed), | ||
| FromFile: "current", | ||
| ToFile: "proposed", | ||
| Context: 3, | ||
| }) | ||
| if err != nil { | ||
| diff = "(failed to compute diff)" | ||
| } | ||
|
|
||
| sb.WriteString("\n```diff\n") | ||
| sb.WriteString(diff) | ||
| sb.WriteString("```\n") | ||
| return sb.String() |
There was a problem hiding this comment.
formatAgentSection posts diffs derived directly from Current/Proposed into the GitHub CheckRun output. Those fields can contain sensitive data (e.g., Terraform Cloud planner stores full plan JSON in Proposed, and Argo manifests may include Secret objects), which would be exposed to anyone with repo/PR access. Consider gating this behind an explicit config/feature flag, redacting known-sensitive fields/resources, or only posting a high-level summary plus a link to the Ctrlplane UI for the full diff.
| // buildCheckOutput builds the full check output (title + summary + text) | ||
| // from the target's current state and all its agents' results. | ||
| func buildCheckOutput( | ||
| tc targetContext, | ||
| results []agentResult, | ||
| agg aggregate, | ||
| ) *github.CheckRunOutput { | ||
| title := agg.checkTitle() | ||
|
|
||
| var summary strings.Builder | ||
| fmt.Fprintf(&summary, "**Version:** `%s`\n\n", tc.VersionTag) | ||
| fmt.Fprintf(&summary, "[View full plan →](%s)\n", targetDetailsURL(tc)) | ||
|
|
||
| var text strings.Builder | ||
| for i, r := range results { | ||
| if i > 0 { | ||
| text.WriteString("\n---\n\n") | ||
| } | ||
| text.WriteString(formatAgentSection(r)) | ||
| } | ||
|
|
||
| summaryStr := summary.String() | ||
| textStr := text.String() | ||
| return &github.CheckRunOutput{ | ||
| Title: &title, | ||
| Summary: &summaryStr, | ||
| Text: &textStr, | ||
| } | ||
| } |
There was a problem hiding this comment.
GitHub check run output fields have size limits (notably output.text), and diff here can become very large for Terraform plan JSON or large manifest sets. If the payload exceeds GitHub’s limits, CreateCheckRun/UpdateCheckRun will fail and the check won’t render. Consider truncating summaryStr/textStr to a safe max length (with a "truncated" note + link to full plan) and/or limiting diff size (e.g., max lines/bytes).
| func agentResultFromRow(row db.ListDeploymentPlanTargetResultsByTargetIDRow) agentResult { | ||
| var dc oapi.DispatchContext | ||
| _ = json.Unmarshal(row.DispatchContext, &dc) | ||
|
|
||
| var hasChanges *bool | ||
| if row.HasChanges.Valid { | ||
| v := row.HasChanges.Bool | ||
| hasChanges = &v | ||
| } | ||
|
|
||
| return agentResult{ | ||
| AgentName: dc.JobAgent.Name, | ||
| AgentType: dc.JobAgent.Type, | ||
| Status: row.Status, |
There was a problem hiding this comment.
agentResultFromRow ignores JSON unmarshal errors for DispatchContext, which can lead to empty/misleading agent name/type in the rendered check without any signal of why. It’d be more diagnosable to handle the error (e.g., set AgentName/Type to placeholders and/or include an error message in the section) rather than silently dropping it.
| // MaybeUpdateTargetCheck rebuilds the target's GitHub check run from | ||
| // current DB state and upserts it on the PR's head commit. It silently | ||
| // returns when the bot is not configured or required GitHub metadata | ||
| // is missing. | ||
| func MaybeUpdateTargetCheck( | ||
| ctx context.Context, | ||
| getter Getter, | ||
| resultID uuid.UUID, | ||
| ) error { | ||
| ctx, span := tracer.Start(ctx, "MaybeUpdateTargetCheck") | ||
| defer span.End() | ||
|
|
||
| span.SetAttributes(attribute.String("result_id", resultID.String())) | ||
|
|
||
| tc, results, err := loadTargetContext(ctx, getter, resultID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !tc.hasGitHubMetadata() { | ||
| span.AddEvent("skipped: missing github metadata") | ||
| return nil | ||
| } | ||
|
|
||
| span.SetAttributes( | ||
| attribute.String("github.owner", tc.Owner), | ||
| attribute.String("github.repo", tc.Repo), | ||
| attribute.String("git.sha", tc.SHA), | ||
| attribute.String("target_id", tc.TargetID.String()), | ||
| ) | ||
|
|
||
| client, err := gh.CreateClientForRepo(ctx, tc.Owner, tc.Repo) | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "create github client") | ||
| return fmt.Errorf("create github client: %w", err) | ||
| } | ||
| if client == nil { | ||
| span.AddEvent("skipped: github bot not configured") | ||
| return nil | ||
| } | ||
|
|
||
| agg := aggregateResults(results) | ||
| output := buildCheckOutput(tc, results, agg) | ||
|
|
||
| if err := upsertCheckRun(ctx, client, tc, agg, output); err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "upsert check run") | ||
| return err | ||
| } | ||
|
|
||
| span.AddEvent("check run upserted") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
New GitHub check-run behavior is introduced (aggregation, conclusions, diff rendering, upsert logic) but isn’t covered by unit tests in this package. Adding tests for aggregateResults/checkTitle and for formatAgentSection/buildCheckOutput (including large output truncation/redaction behavior if added) would help prevent regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/svc/controllers/deploymentplanresult/github_check.go`:
- Around line 187-220: The current logic treats all-unsupported or mixed
unsupported results as a successful "no changes" run; update checkConclusion()
to return "failure" when no plan was produced (i.e., when Unsupported equals
Total or when Unsupported > 0 and there are zero
Completed/Changed/Errored/Unchanged), and update checkTitle() to include the
Unsupported count in its summaries and to produce an explicit message like "All
agents unsupported" when everything is Unsupported (and include Unsupported in
intermediate "Computing..."/errored summaries so unsupported agents are not
hidden); make these changes in the checkConclusion and checkTitle methods and
consider using the existing allDone() helper to detect final vs in-progress
states.
- Around line 432-446: Wrap the GitHub client creation and the subsequent upsert
call in a bounded context with a timeout (e.g., 15–30s) inside
MaybeUpdateTargetCheck: create a child ctxWithTimeout via
context.WithTimeout(ctx, timeout) and defer cancel(), then use ctxWithTimeout
when calling gh.CreateClientForRepo(...) and upsertCheckRun(...). This ensures
gh.CreateClientForRepo and upsertCheckRun have a deadline and prevents the
reconcile worker from being blocked by slow GitHub operations.
- Around line 334-342: The current flow (findCheckRunByName -> createCheckRun /
updateCheckRun) has a race where two workers can both see no existing check and
create duplicates; fix by serializing creation per target: acquire a
target-scoped advisory lock (or DB row lock) before calling findCheckRunByName
in the function that decides create vs update, re-check/persist the check-run ID
inside the lock, and only then call createCheckRun or updateCheckRun;
alternatively, if you persist a mapping from (owner, repo, sha, name) to
checkRunID in the DB, read and upsert that mapping inside the same lock so
subsequent workers read the persisted ID and call updateCheckRun instead of
creating a duplicate.
- Around line 276-290: The check output text built by buildCheckOutput (the
strings.Builder accumulating formatAgentSection results) can exceed GitHub's
65,535-byte limit and must be truncated before creating the
github.CheckRunOutput (textStr). After building text.String(), convert to bytes,
enforce a max byte length (65,535 minus a small margin for an added truncation
message), trim to the last valid UTF-8 rune boundary (so you don't cut in the
middle of a multi-byte character), append a short sentinel like
"\n\n...truncated..." and assign that to textStr; then return the
github.CheckRunOutput with Title, Summary and the truncated Text. Ensure you
reference the variables text (strings.Builder), textStr, formatAgentSection, and
the function that returns github.CheckRunOutput when implementing this change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21707dda-fb7d-451a-bebe-184be6be7eeb
📒 Files selected for processing (8)
apps/workspace-engine/pkg/db/deployment_plan.sql.goapps/workspace-engine/pkg/db/queries/deployment_plan.sqlapps/workspace-engine/svc/controllers/deploymentplanresult/controller.goapps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.goapps/workspace-engine/svc/controllers/deploymentplanresult/getters.goapps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.goapps/workspace-engine/svc/controllers/deploymentplanresult/github_check.goapps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go
| // checkConclusion returns the GitHub "conclusion" field. Only | ||
| // meaningful when shouldFinalize() is true. | ||
| func (a aggregate) checkConclusion() string { | ||
| if a.Errored > 0 { | ||
| return "failure" | ||
| } | ||
| if a.Changed > 0 { | ||
| return "neutral" | ||
| } | ||
| return "success" | ||
| } | ||
|
|
||
| // checkTitle returns a short, human-readable summary line for the check. | ||
| func (a aggregate) checkTitle() string { | ||
| done := a.Completed + a.Errored + a.Unsupported | ||
|
|
||
| if a.Errored > 0 && !a.allDone() { | ||
| return fmt.Sprintf("%d errored (%d/%d agents complete)", a.Errored, done, a.Total) | ||
| } | ||
| if !a.allDone() { | ||
| return fmt.Sprintf("Computing... (%d/%d agents)", done, a.Total) | ||
| } | ||
| if a.Errored > 0 { | ||
| return fmt.Sprintf( | ||
| "%d errored, %d changed, %d unchanged", | ||
| a.Errored, | ||
| a.Changed, | ||
| a.Unchanged, | ||
| ) | ||
| } | ||
| if a.Changed > 0 { | ||
| return fmt.Sprintf("%d changed, %d unchanged", a.Changed, a.Unchanged) | ||
| } | ||
| return "No changes" |
There was a problem hiding this comment.
Don’t report unsupported agents as a successful no-op.
If every result is Unsupported, checkConclusion() returns success and checkTitle() returns "No changes", even though no plan was produced. Mixed clean + unsupported results are also hidden as success/no changes.
🐛 Proposed fix
func (a aggregate) checkConclusion() string {
if a.Errored > 0 {
return "failure"
}
+ if a.Unsupported > 0 {
+ return "neutral"
+ }
if a.Changed > 0 {
return "neutral"
}
return "success"
}
@@
if a.Errored > 0 {
return fmt.Sprintf(
"%d errored, %d changed, %d unchanged",
a.Errored,
a.Changed,
a.Unchanged,
)
}
+ if a.Unsupported > 0 {
+ return fmt.Sprintf(
+ "%d unsupported, %d changed, %d unchanged",
+ a.Unsupported,
+ a.Changed,
+ a.Unchanged,
+ )
+ }
if a.Changed > 0 {
return fmt.Sprintf("%d changed, %d unchanged", a.Changed, a.Unchanged)
}📝 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.
| // checkConclusion returns the GitHub "conclusion" field. Only | |
| // meaningful when shouldFinalize() is true. | |
| func (a aggregate) checkConclusion() string { | |
| if a.Errored > 0 { | |
| return "failure" | |
| } | |
| if a.Changed > 0 { | |
| return "neutral" | |
| } | |
| return "success" | |
| } | |
| // checkTitle returns a short, human-readable summary line for the check. | |
| func (a aggregate) checkTitle() string { | |
| done := a.Completed + a.Errored + a.Unsupported | |
| if a.Errored > 0 && !a.allDone() { | |
| return fmt.Sprintf("%d errored (%d/%d agents complete)", a.Errored, done, a.Total) | |
| } | |
| if !a.allDone() { | |
| return fmt.Sprintf("Computing... (%d/%d agents)", done, a.Total) | |
| } | |
| if a.Errored > 0 { | |
| return fmt.Sprintf( | |
| "%d errored, %d changed, %d unchanged", | |
| a.Errored, | |
| a.Changed, | |
| a.Unchanged, | |
| ) | |
| } | |
| if a.Changed > 0 { | |
| return fmt.Sprintf("%d changed, %d unchanged", a.Changed, a.Unchanged) | |
| } | |
| return "No changes" | |
| // checkConclusion returns the GitHub "conclusion" field. Only | |
| // meaningful when shouldFinalize() is true. | |
| func (a aggregate) checkConclusion() string { | |
| if a.Errored > 0 { | |
| return "failure" | |
| } | |
| if a.Unsupported > 0 { | |
| return "neutral" | |
| } | |
| if a.Changed > 0 { | |
| return "neutral" | |
| } | |
| return "success" | |
| } | |
| // checkTitle returns a short, human-readable summary line for the check. | |
| func (a aggregate) checkTitle() string { | |
| done := a.Completed + a.Errored + a.Unsupported | |
| if a.Errored > 0 && !a.allDone() { | |
| return fmt.Sprintf("%d errored (%d/%d agents complete)", a.Errored, done, a.Total) | |
| } | |
| if !a.allDone() { | |
| return fmt.Sprintf("Computing... (%d/%d agents)", done, a.Total) | |
| } | |
| if a.Errored > 0 { | |
| return fmt.Sprintf( | |
| "%d errored, %d changed, %d unchanged", | |
| a.Errored, | |
| a.Changed, | |
| a.Unchanged, | |
| ) | |
| } | |
| if a.Unsupported > 0 { | |
| return fmt.Sprintf( | |
| "%d unsupported, %d changed, %d unchanged", | |
| a.Unsupported, | |
| a.Changed, | |
| a.Unchanged, | |
| ) | |
| } | |
| if a.Changed > 0 { | |
| return fmt.Sprintf("%d changed, %d unchanged", a.Changed, a.Unchanged) | |
| } | |
| return "No changes" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`
around lines 187 - 220, The current logic treats all-unsupported or mixed
unsupported results as a successful "no changes" run; update checkConclusion()
to return "failure" when no plan was produced (i.e., when Unsupported equals
Total or when Unsupported > 0 and there are zero
Completed/Changed/Errored/Unchanged), and update checkTitle() to include the
Unsupported count in its summaries and to produce an explicit message like "All
agents unsupported" when everything is Unsupported (and include Unsupported in
intermediate "Computing..."/errored summaries so unsupported agents are not
hidden); make these changes in the checkConclusion and checkTitle methods and
consider using the existing allDone() helper to detect final vs in-progress
states.
| existing, err := findCheckRunByName(ctx, client, tc.Owner, tc.Repo, tc.SHA, name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if existing == nil { | ||
| return createCheckRun(ctx, client, tc, name, status, detailsURL, agg, output) | ||
| } | ||
| return updateCheckRun(ctx, client, tc, existing.GetID(), status, detailsURL, agg, output) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether check-run creation has any target-scoped serialization or persisted identity.
# Expected if this concern is fixed: evidence of a lock/advisory lock, stored check_run_id, or another single-writer mechanism around create.
rg -nP 'findCheckRunByName|CreateCheckRun|UpdateCheckRun|ExternalID|advisory|check_run|MaybeUpdateTargetCheck|upsertCheckRun' -C3Repository: ctrlplanedev/ctrlplane
Length of output: 20007
🏁 Script executed:
# Get the complete upsertCheckRun function and check for any locking/synchronization
sed -n '321,342p' apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.goRepository: ctrlplanedev/ctrlplane
Length of output: 760
🏁 Script executed:
# Check for advisory locks or database-level synchronization in the codebase
rg -i 'advisory|pg_advisory|lock|mutex|sema' apps/workspace-engine/svc/controllers/deploymentplanresult/ -A2 -B2Repository: ctrlplanedev/ctrlplane
Length of output: 634
🏁 Script executed:
# Search for where check_run_id or github check run ID is persisted
rg 'check_run_id|checkRunID|CheckRunID' apps/workspace-engine --type go -B2 -A2Repository: ctrlplanedev/ctrlplane
Length of output: 1112
🏁 Script executed:
# Verify if there's any target-scoped locking in the controller's reconcile loop
sed -n '1,100p' apps/workspace-engine/svc/controllers/deploymentplanresult/controller.go | head -60Repository: ctrlplanedev/ctrlplane
Length of output: 1798
🏁 Script executed:
# Search for database schema definitions and migrations
find apps/workspace-engine -name '*.sql' | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 1196
🏁 Script executed:
# Look for deployment_plan_result or check run related schema
rg 'deployment_plan_result|check_run|checks' apps/workspace-engine --type sql -B2 -A2Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Search for mutex, locks, or synchronization primitives in controller
rg 'sync\.|Mutex|Lock|RWMutex' apps/workspace-engine/svc/controllers/deploymentplanresult/ -B2 -A2Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there's a postgres locking utility used elsewhere
rg 'advisory|pg_advisory|LeaseManager|lease' apps/workspace-engine/pkg --type go -lRepository: ctrlplanedev/ctrlplane
Length of output: 7270
🏁 Script executed:
# Check the postgres locking mechanism
cat apps/workspace-engine/pkg/reconcile/postgres/pg.goRepository: ctrlplanedev/ctrlplane
Length of output: 10262
🏁 Script executed:
# Look for the full Process method and transaction context
sed -n '42,250p' apps/workspace-engine/svc/controllers/deploymentplanresult/controller.goRepository: ctrlplanedev/ctrlplane
Length of output: 5433
🏁 Script executed:
# Check if findCheckRunByName uses any database locking
sed -n '293,320p' apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.goRepository: ctrlplanedev/ctrlplane
Length of output: 792
🏁 Script executed:
# Search for any database migrations or constraints on checks
find apps/workspace-engine -name '*.sql' -exec grep -l 'check' {} \;Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there's idempotent check creation logic in GitHub or retry handling
rg 'CreateCheckRun|check.*already.*exists|duplicate' apps/workspace-engine/svc/controllers/deploymentplanresult/ -B3 -A3Repository: ctrlplanedev/ctrlplane
Length of output: 1511
Race condition remains unaddressed: concurrent workers can create duplicate check runs.
Two workers processing results on the same target can both find no existing check, then both create one. The lease-based locking is per-item (resultID), not per-target, so concurrent processing of different results on the same commit is allowed. The findCheckRunByName call is a plain GitHub API query with no database synchronization, and there is no unique constraint on check names per commit.
Serialize check creation per target using an advisory lock, persist the check-run ID and query it first, or merge target results before upserting checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`
around lines 334 - 342, The current flow (findCheckRunByName -> createCheckRun /
updateCheckRun) has a race where two workers can both see no existing check and
create duplicates; fix by serializing creation per target: acquire a
target-scoped advisory lock (or DB row lock) before calling findCheckRunByName
in the function that decides create vs update, re-check/persist the check-run ID
inside the lock, and only then call createCheckRun or updateCheckRun;
alternatively, if you persist a mapping from (owner, repo, sha, name) to
checkRunID in the DB, read and upsert that mapping inside the same lock so
subsequent workers read the persisted ID and call updateCheckRun instead of
creating a duplicate.
Ctrlplane Deployment PlanVersion: |
Ctrlplane Deployment PlanVersion: |
fixes #1012
Summary by CodeRabbit