Skip to content

sec(ci): gate terraform force-unlock behind explicit input (closes #438) - #818

Merged
cristim merged 3 commits into
mainfrom
fix/438-wave7
Jul 17, 2026
Merged

sec(ci): gate terraform force-unlock behind explicit input (closes #438)#818
cristim merged 3 commits into
mainfrom
fix/438-wave7

Conversation

@cristim

@cristim cristim commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

  • The deploy-aws-fargate.yml workflow unconditionally deleted the S3 Terraform state lock before every terraform init, allowing concurrent dispatch runs to race and corrupt state (issue sec(ci): deploy-aws-fargate.yml unconditionally deletes Terraform state-lock file at init time #438).
  • Replaced the automatic deletion with a new step gated on inputs.clear_stale_lock == true (a workflow_dispatch-only boolean input, default false). The existing if: failure() || cancelled() cleanup step is unchanged.
  • Added runbooks/terraform-stuck-lock.md explaining when/how to use the escape hatch and why unconditional clearing is unsafe.

Test plan

  • Verify a normal workflow_dispatch run (without clear_stale_lock) no longer deletes the lock file at init time.
  • Verify a run with clear_stale_lock=true executes the new step and removes the lock object.
  • Verify the Release state lock on failure step still fires on job failure/cancellation.
  • YAML parse-check: python3 -c "import yaml; yaml.safe_load(open('.github/workflows/deploy-aws-fargate.yml'))" passes.

Summary by CodeRabbit

  • New Features

    • Added an operator-controlled option to clear stale Terraform S3 state locks for ECS Fargate deployments via workflow dispatch input, improving recovery from interrupted runs.
  • Documentation

    • Added a runbook for diagnosing Terraform state lock issues and performing safe recovery, including workflow-based and manual AWS CLI steps.
  • Operational Safety

    • Removed automatic state-lock deletion on failure/cancellation to avoid interfering with concurrent deployments; lock clearing is now only operator-triggered.

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-quarter Within the quarter impact/internal Team-internal only effort/xs Trivial / one-liner type/security Security finding labels May 28, 2026
@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fc935300-4ac0-4c1a-9305-b66af809fa79

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The workflow now clears Terraform S3 state locks only when explicitly requested through clear_stale_lock=true. Automatic failure cleanup is removed, and a runbook documents lock symptoms, verification steps, workflow-dispatch recovery, and manual AWS CLI recovery.

Changes

Operator-triggered Terraform state lock recovery

Layer / File(s) Summary
Workflow input and conditional lock-clearing step
.github/workflows/deploy-aws-fargate.yml
Adds the clear_stale_lock dispatch input, conditionally deletes the S3 .tflock object, keeps Terraform Init focused on backend configuration, and removes automatic lock release after failure or cancellation.
Stuck lock recovery procedures
runbooks/terraform-stuck-lock.md
Documents lock-acquisition symptoms, verification of the owning run, workflow-dispatch and AWS CLI recovery paths, and why automatic deletion is avoided.

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

Possibly related PRs

  • LeanerCloud/CUDly#1131: Modifies the same deployment workflow’s Terraform S3 state-lock handling and recovery flow.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: making Terraform force-unlock conditional on an explicit input.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/438-wave7

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

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 30, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 2 seconds before sending another message.

cristim commented Jun 8, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@cristim Sure, triggering a full review of PR #818 now.


🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.
✅ Action performed

Full review finished.

@cristim

cristim commented Jun 8, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@cristim
cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:45
@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 @.github/workflows/deploy-aws-fargate.yml:
- Around line 117-132: The workflow has an automatic failure-path unlock
mechanism that unconditionally removes the Terraform state lock on any failed or
cancelled run, which creates a race condition where one failing concurrent run
can delete another healthy run's active lock, causing state corruption. Either
remove the automatic failure-path unlock mechanism entirely and rely solely on
the operator-triggered clear_stale_lock step (which requires explicit
input.clear_stale_lock == true), or modify the failure-path unlock to include
ownership awareness by adding a unique identifier or token that tracks which run
created the lock before allowing removal. The safest approach is to delete the
automatic failure cleanup step and require manual operator intervention via the
clear_stale_lock input flag.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 56c13b0f-5208-4309-86a4-9983e17993f6

📥 Commits

Reviewing files that changed from the base of the PR and between 451a70f and 8f38465.

📒 Files selected for processing (2)
  • .github/workflows/deploy-aws-fargate.yml
  • runbooks/terraform-stuck-lock.md

Comment thread .github/workflows/deploy-aws-fargate.yml
@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@cristim

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Merge-prep: rebased onto origin/main (27fdb06), no conflicts. Non-Go PR; go vet on internal/... clean. @coderabbitai full review

@cristim

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main (includes #1437 tflint cache + #1438 trivy skip). No Go changes; CI workflow + runbook changes only.

cristim added 3 commits July 17, 2026 19:22
Unconditionally deleting the S3 state lock before every terraform init
allowed concurrent dispatch runs to race and corrupt state. Remove the
automatic deletion and replace it with a gated step that only runs when
the operator explicitly sets clear_stale_lock=true on workflow_dispatch.
The existing failure/cancellation cleanup step is unchanged.

Add runbooks/terraform-stuck-lock.md with diagnosis and recovery steps.
…k.md

markdownlint MD040 requires fenced code blocks to specify a language.
The error-output block at line 7 had a bare ``` opener; add "text" tag.
The "Release state lock on failure" step deleted the S3 tflock on any
failed/cancelled run. A run that fails because it could not acquire the
lock would delete the lock a concurrent run is still actively holding,
re-introducing the state-corruption race this PR aims to close.

Remove the automatic failure-path unlock. Terraform already releases its
own lock on a clean apply error; a lock surviving a run means the run died
abnormally, which requires operator confirmation via clear_stale_lock
before clearing. Update the runbook accordingly.
@cristim

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 4aa4ead into main Jul 17, 2026
19 checks passed
@cristim
cristim deleted the fix/438-wave7 branch July 27, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/security Security finding urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant