Skip to content

Fix review app release runner resource defaults - #802

Open
justin808 wants to merge 2 commits into
masterfrom
fix/review-app-release-runner-resources
Open

Fix review app release runner resource defaults#802
justin808 wants to merge 2 commits into
masterfrom
fix/review-app-release-runner-resources

Conversation

@justin808

@justin808 justin808 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Why

Review-app builds could finish successfully but then stall while the release runner never exposed a replica. The previous resource-only repair did not solve that observation failure and left the workflow without a bounded, useful outcome.

What changed

  • Give review-app one-off runners app-sized CPU and memory defaults and a 15-minute server deadline.
  • Pin only the review-app deploy wrapper to the immutable repair revision from Bound runner replica observation by a monotonic deadline control-plane-flow#435.
  • Keep every non-canary cpflow wrapper on one stable release while allowing only the exact review-app deploy job to use a full 40-character canary SHA.
  • Record the exact first-party upstream repository in the closed action-trust seam; no wildcard trust was added.

How to review

  1. Confirm .controlplane/controlplane.yml changes only the review-app runner defaults and deadline.
  2. Confirm the workflow diff changes only the immutable upstream revision; triggers, permissions, conditions, and credential mapping are unchanged.
  3. Review bin/test-cpflow-github-flow for the narrow canary exception and its rejection paths.
  4. Treat the hosted replay below as release-blocking evidence: it proved the new bound, but it did not produce a usable deployment.

Validation

  • Workflow YAML parse: passed.
  • Targeted actionlint: passed.
  • Full cpflow GitHub-flow validation against the exact upstream checkout: passed.
  • Negative validator cases rejected a moving canary, wrong upstream workflow, second divergent wrapper, and duplicate canary entry.
  • Secure workflow scan: zero candidate-only findings and zero findings on the changed workflow; 53 unrelated trusted-base findings remain unchanged.
  • git diff --check: passed.
  • Independent maker-distinct Sol/xhigh audit: no blocking code finding; publish verdict for this exact head.

Authorized hosted replay

Current disposition: external-gate-failing; do not merge. No retry or waiver is inferred.

Tracking and post-merge exercise

#784 remains the required exercise tracker for this semantic workflow change. If a later authorized repair succeeds and this PR becomes mergeable, that issue owns one fresh-default verification PR, deployed behavior and endpoint evidence, cleanup, and closing the verification PR unmerged. Owner: batch coordinator.

Review decisions:

  • The immutable control-plane-flow canary is temporary. Issue Follow-up: Repair review-app setup and exercise deployed smoke #784 owns replacing it with the next durable released revision after upstream PR Update gems, RoR to 10.1.4 #435 merges and releases; this PR is not proceeding to merge on the canary.
  • The 900-second deadline deliberately applies to every one-off runner in review apps. The available configuration seam is app-level rather than release-job-specific, and the shared bound prevents any review-app one-off job from waiting indefinitely.
Agent details

QA Evidence

  • QA required: yes; workflow, deployment, and runner behavior changed.
  • QA lane: maker-distinct ror17-fleet-c-tutorial-checker-4, observed codex-collaboration/gpt-5.6-sol/xhigh.
  • Exact head: 1fc3f390bea00655b0846d83b2831a38b3589e34.
  • Result: local gates and independent review passed; the sole hosted deployment failed; cleanup passed.
  • Hosted QA receipt: unavailable because no successful deployment identity or behavioral criteria exist.

Coordination

@github-actions

Copy link
Copy Markdown

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:
They require the repository to have cpflow review apps configured, including the CPLN_TOKEN_STAGING secret.

+review-app-deploy

Deploy your PR branch for testing.

+review-app-delete

Remove the review app when done.

+review-app-help

Show detailed instructions, environment setup, and configuration options.

Comment +review-app-help for full setup details.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 28f77fd7-1ec6-4c56-91fc-2410e972db13

📥 Commits

Reviewing files that changed from the base of the PR and between ec08723 and 1fc3f39.

📒 Files selected for processing (3)
  • .agents/agent-workflow.yml
  • .github/workflows/cpflow-deploy-review-app.yml
  • bin/test-cpflow-github-flow

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The Control Plane configuration adds runner resources and timeout defaults, trusts the Control Plane action, and pins the review-app workflow to a commit. GitHub Actions validation now separates stable references from the review-app canary.

Changes

Control Plane review-app workflow

Layer / File(s) Summary
Configure review-app execution
.controlplane/controlplane.yml, .agents/agent-workflow.yml, .github/workflows/cpflow-deploy-review-app.yml
Adds CPU, memory, and timeout defaults. Trusts shakacode/control-plane-flow. Pins the reusable workflow to a commit SHA.
Validate stable and canary references
bin/test-cpflow-github-flow
Separates review-app canary references from stable references. Requires one canary and at least one stable reference. Allows a differing canary reference only when it is a full commit SHA.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 1fc3f

The PR makes a localized review-app resource-default change with the supplied checks passing, and no actionable merge-blocking risk remains beyond normal review and checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: correcting review-app release runner resource defaults.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/review-app-release-runner-resources

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread .controlplane/controlplane.yml
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review

Overview

Single-file, additive config change to .controlplane/controlplane.yml. It adds three cpflow one-off-job overrides (runner_job_default_cpu: "300m", runner_job_default_memory: "1Gi", runner_job_timeout: 900) scoped to the qa-react-webpack-rails-tutorial (review-app) entry, aiming to give one-off/release jobs the same baseline resources as the Rails workload. Production, staging, and the shared common alias are untouched.

Code quality / correctness

  • Change is minimal, well-scoped, and placed logically within the existing qa-react-webpack-rails-tutorial block (merges via <<: *common, doesn't touch the alias itself).
  • CPU/memory values are correctly quoted as strings (Kubernetes-style resource quantities with units), while the integer timeout is left unquoted — consistent with how these fields are typically consumed.
  • The explanatory comment is a little redundant with the PR description but is reasonable to keep for future readers who won't see the PR body.
  • No YAML anchors/aliases were changed, so react-webpack-rails-tutorial-staging and -production are unaffected, matching the stated intent.

Risk / potential issues

  • Scope of the timeout: runner_job_timeout: 900 applies to all one-off jobs run against QA/review apps, not just the release/migration job. A legitimate longer-running one-off (manual debugging task, data backfill, etc.) run against a review app will now be killed after 15 minutes. The PR description acknowledges this explicitly — left an inline comment on the relevant line for visibility.
  • Unproven root-cause fix: per the PR's own description, this is a speculative mitigation for issue Follow-up: Repair review-app setup and exercise deployed smoke #784 (missing runner replica/logs) — live deployment verification is still pending. The resource bump may not address the actual root cause if it's a scheduler/provider-side issue rather than a resource-starvation issue. Since this is low-risk, reversible config (review apps only), that seems like an acceptable way to test the hypothesis, but worth confirming the follow-up verification actually happens before treating Follow-up: Repair review-app setup and exercise deployed smoke #784 as resolved.
  • No automated test coverage for this (expected, since it's a deploy-tooling YAML config, not app code) — the PR description's manual cpflow config / YAML-load validation steps are a reasonable substitute.

Security

No credentials, identities, images, permissions, or endpoints are touched — consistent with the PR's stated config-security assertions. No concerns here.

Suggestions

  • Consider whether the 900s timeout should be documented as applying to all review-app one-offs (not just release) somewhere more discoverable than the PR description, e.g., in .controlplane/docs/ or the inline comment, so a future engineer debugging a killed one-off job in a review app has a pointer back to this decision.

@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown

Greptile Summary

Updates review-app one-off runner defaults.

  • Assigns 300m CPU and 1Gi memory to one-off jobs for prefix-matched QA/review apps.
  • Adds a 900-second active deadline for those jobs.
  • Leaves shared, staging, and production runner defaults unchanged.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or non-blocking defects identified in the changed configuration.

The runner overrides are confined to the prefix-matched QA configuration, align with the Rails workload resources, and do not alter production or staging settings.

Important Files Changed

Filename Overview
.controlplane/controlplane.yml Adds internally consistent, review-app-scoped CPU, memory, and timeout defaults without an identified actionable defect.

Reviews (1): Last reviewed commit: "Fix release runner scheduling resource r..." | Re-trigger Greptile

@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: full PR history after the complete current-head review wave.

Mattered

  • None.

Skipped

  • One current-head inline concern was verified as already documented and intentionally scoped to QA/review apps. The broader one-off timeout tradeoff was accepted, production and staging remain unchanged, and no code change was warranted. Rationale was posted and the thread was resolved: Fix review app release runner resource defaults #802 (comment)

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

@justin808

Copy link
Copy Markdown
Member Author

+review-app-deploy

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

❌ Review App Deployment Failed

Deployment failed for PR #802, commit ec08723

🎮 Control Plane Console
📋 View Failed Action Build and Deploy Logs

@github-actions
github-actions Bot had a problem deploying to review/qa-react-webpack-rails-tutorial-802 July 24, 2026 13:51 Failure
@justin808

Copy link
Copy Markdown
Member Author

+review-app-delete

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

✅ Review App Deleted

Review app for PR #802 is deleted

🎮 Control Plane Console
📋 View Workflow Logs

@justin808
justin808 force-pushed the fix/review-app-release-runner-resources branch from ec08723 to 1fc3f39 Compare August 31, 2026 11:53
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-31T11:59:52.822088Z 1fc3f39 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Comment thread .github/workflows/cpflow-deploy-review-app.yml
Comment thread .controlplane/controlplane.yml
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review summary

Small, well-scoped infra change (50/-11 across 4 files) that (1) gives review-app one-off jobs the same CPU/memory as the Rails workload and a 15-minute active deadline, (2) points the review-app deploy reusable workflow at an unreleased control-plane-flow commit to test a fix, and (3) extends bin/test-cpflow-github-flow to allow that one workflow to diverge from the shared stable ref, but only via a full 40-char commit SHA.

Checked and looks correct:

  • runner_job_default_cpu: "300m" / runner_job_default_memory: "1Gi" in .controlplane/controlplane.yml match the Rails workload template (.controlplane/templates/rails.yml), consistent with the PR's stated intent.
  • YAML nesting is correctly scoped under qa-react-webpack-rails-tutorial only; common, production, and staging are untouched.
  • The new commit SHA in cpflow-deploy-review-app.yml is a well-formed 40-char hex string, and the updated bin/test-cpflow-github-flow canary logic correctly requires exactly one such divergent entry and validates it's a full SHA (not a mutable tag/branch).
  • .agents/agent-workflow.yml's new trusted_actions: [shakacode/control-plane-flow] entry appears intended to authorize exactly this kind of pinned-ref change to an already same-org-controlled repo.

Two things worth a second look (left as inline comments):

  1. .github/workflows/cpflow-deploy-review-app.yml — pinning to a commit on an unmerged PR (control-plane-flow#435) rather than a released tag is provisional by design (per the PR description), but carries a real risk: if that PR branch is later force-pushed/rebased/deleted before merging, the SHA could become unresolvable and silently break every review-app deploy. Recommend a tracked follow-up to swap to a released tag once Update gems, RoR to 10.1.4 #435 ships.
  2. .controlplane/controlplane.ymlrunner_job_timeout: 900 applies to all one-off jobs on qa-* apps, not just the release job. The PR description already flags this tradeoff, but it's a real behavior change for anyone running longer manual one-off tasks against review apps.

No security issues found (no credentials, permissions, or identity fields touched; SHA-pinning is actually a security improvement over the previous floating @v5.2.0 tag for that one workflow). No test coverage concerns beyond the existing bin/test-cpflow-github-flow script, which was appropriately extended.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

❌ Review App Deployment Failed

Deployment failed for PR #802, commit 1fc3f39

🎮 Control Plane Console
📋 View Failed Action Build and Deploy Logs

@github-actions
github-actions Bot had a problem deploying to review/qa-react-webpack-rails-tutorial-802 August 31, 2026 11:56 Failure
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

✅ Review App Deleted

Review app for PR #802 is deleted

🎮 Control Plane Console
📋 View Workflow Logs

@justin808

Copy link
Copy Markdown
Member Author

Hosted replay result for head 1fc3f390bea00655b0846d83b2831a38b3589e34: external-gate-failing; do not merge.

The conditional fresh-default verification deployment is not eligible because this repair deployment did not succeed. No protected app URL, platform identifier/value, log, token, or secret name is included here.

@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: since the previous summary at 2026-07-24T04:08:12Z.

Status: cutoff-safe review summary. Detailed review outcomes are collapsed below. The PR itself remains not merge-ready because the sole authorized hosted deployment failed.

Detailed review outcomes

Mattered

  • Deferred the temporary immutable canary lifecycle to existing tracker Follow-up: Repair review-app setup and exercise deployed smoke #784. The PR decision log records replacement with a durable released revision after upstream PR Update gems, RoR to 10.1.4 #435 merges and releases; the review thread is complete.
  • Declined a narrower timeout-scope code change. The configuration seam is review-app-wide, so the PR decision log records that the 900-second bound intentionally covers every review-app one-off runner; the review thread is complete.
  • The deployment gate remains external-gate-failing; no retry, merge, or fresh-default verification was performed.

Skipped

  • Nine general PR comments since the prior cutoff were operator evidence or automated review/workflow status receipts. Metadata triage found no separate action requirement; they remain preserved as durable status evidence.

Deferred-work tracking: existing issue #784.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant