Repository navigation
fcvm-coordinator-release: name the failed step and error code; never fail a release over a lock it no longer holds - #695
Conversation
…fail a release over a lock it no longer holds
The failure result now carries the step that raised (credentials, input, lock, inspect,
register, update, rollout, pointer, rollback, unlock) and, for a botocore ClientError or a
failed release check, its code. Codes are fixed tokens (AWS error codes, or the new
Refused checks: service-unavailable, service-changed, rollout-failed, not-idle,
pointer-mismatch). Messages and SDK payloads are still never returned.
Lock handling, from the code:
- The unlock's conditional DeleteItem is not idempotent under the SDK's retry. If the
first attempt deleted the lock and its reply was lost, the retry failed
ConditionalCheckFailedException and a release whose rollout and pointer write had
completed returned "failed". The same happened when the lock was force-released (and
possibly taken) while the release ran. Both now leave the lock alone, log
{"step": "unlock", ...} and keep the release's own result.
- The lock's conditional PutItem had the same retry hole: a retry of a write that landed
returned "busy" and left the release's own lock behind, blocking Terraform until a
force-unlock. Both writes now ask for ReturnValuesOnConditionCheckFailure=ALL_OLD and
compare the returned Info with the release's token.
- assume_role ran outside the handler's try, so an STS error escaped as an unhandled
function error with its message. It now returns the failure result with step
"credentials".
- A rollback that succeeds reports the step that failed, not "rollback"; an unlock that
succeeds after a failure keeps the original step.
Tested: python3 -S -B scripts/test-fcvm-coordinator-release.py (21 tests, OK). Against
the previous release.py, 14 of them fail.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Failure codes and progress tracking fcvm-coordinator-release/release.py |
Adds coded refusal errors and helpers for validated error codes. Initializes progress tracking for release steps. |
Deployment and lock lifecycle fcvm-coordinator-release/release.py, scripts/test-fcvm-coordinator-release.py |
Tracks deployment steps, limits lock deletion to the release owner, and handles conditional-write retries and lock takeovers. Rollback preserves the original failure step. Tests cover lock retries and unlock failures. |
Handler results and validation fcvm-coordinator-release/release.py, scripts/test-fcvm-coordinator-release.py, AGENTS.md |
The handler returns the failure step and a validated error code when available, including for credential failures. Tests cover handler failures and rollback results. The guidance describes failure fields and lock behavior. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Merge Risk: 
f626a
-
The coordinator release changes are ready to merge after normal checks; no unresolved issue is established.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 15.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 2 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and specifically summarizes the main changes: reporting the failed step and error code, and avoiding release failure when the release no longer owns the lock. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 15.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performed
|
The release function's failure result now says which step failed and the AWS error code, and a release whose rollout and pointer write completed no longer reports "failed" because of a lock it no longer holds.
Context: today's failed staging deploys
The fcvm staging deploy failed on its first attempt in fcvm runs 40261310012, 40265725881 and 40274465086 with
{"status": "failed", "errorType": "RuntimeError"}, and a re-run returned "unchanged" with the new digest. That line is printed by fcvm'sfleet/coord/publish.py, and its RuntimeError there wascommand failed: aws: CloudWatch metrics show each invocation still running when the CLI exited (309 s against 269 s, 272 s against 261 s, 320 s against about 272 s), then 2 throttles in the minute the CLI exited. The CLI's call ends about 260 s in, its own retries are throttled byreserved_concurrent_executions = 1, and it gives up while the release finishes. That client side is handled in ejc3/fcvm#1334. The function's logs (#696) would confirm that those invocations printed "deployed".This PR is the function's half: the result names the failure, and the lock-handling defects below, found by reading the code, are fixed. They are not the cause of today's failures as far as the metrics show.
Failure result
{"status": "failed", "errorType": "ClientError", "step": "unlock", "code": "ConditionalCheckFailedException"}step: credentials, input, lock, inspect, register, update, rollout, pointer, rollback or unlock. A rollback that succeeds reports the step that failed; an unlock that succeeds after a failure keeps the original step.code: a botocoreClientErrorcode, or for the release's own checks (Refused, a RuntimeError) one of service-unavailable, service-changed, rollout-failed, not-idle, pointer-mismatch. Present only when it is a plain token of at most 64 characters. Other exceptions (a botocore timeout, TimeoutError) carry onlyerrorTypeandstep.Lock defects
finallyDeleteItem is conditional on the release's Info token. If its first attempt deleted the lock and the reply was lost (read_timeout 15, botocore retries), the retry fails ConditionalCheckFailedException, the exception replaces thereturn, and a deployed release reports failed. The same happens when the lock was force-released, or force-released and taken, during the release. Now a refused delete leaves the lock alone, logs{"step": "unlock", "code": "ConditionalCheckFailedException", "lock": "absent" | "held by another owner"}, and the release's own result stands. A lock held by someone else is never deleted. Any other unlock error still fails with step "unlock", since the lock may still be held.ReturnValuesOnConditionCheckFailure=ALL_OLD; a refused put whose returned Info equals this release's token (a fresh uuid) owns the lock and proceeds. This uses no new IAM action: the parameter is part of PutItem and DeleteItem, and the policy sets nodynamodb:ReturnValuescondition. If that is wrong, every release now reports"step": "lock", "code": "AccessDeniedException"instead of deploying, and nothing changes in ECS.Tests
Against the previous release.py, 14 of the 21 fail: the lock tests (
test_a_retried_unlock_whose_first_attempt_landed_reports_deployed,test_a_retried_lock_write_whose_first_attempt_landed_owns_the_lock,test_a_lock_taken_over_during_the_release_is_left_alone_and_the_release_reports_deployed) and everyHandlerTestscase. The fake DynamoDB returns the old item only when the request asks for ALL_OLD.test_the_lock_delete_checks_the_unique_owneris replaced by the take-over test: the lock is still never deleted, and the release now reports deployed instead of raising.Apply
Lambda code only (
fcvm-coordinator-release/release.py) plus a sentence in AGENTS.md. A targeted apply ofaws_lambda_function.fcvm_coordinator_releaseis enough.Summary by CodeRabbit