fix(deploy): guard SetTTL against permanent + terminal deploys (bug-bash #3/#5/#6)#228
Merged
Merged
Conversation
…ash #3/#5/#6) SetTTL unconditionally overwrote ttl_policy/expires_at, so a user could flip a deployment they'd made permanent back to an expiring TTL, and could set a TTL on an expired/deleted/stopped deploy. Both violate the lifecycle contract. - handler: reject ttl_policy=='permanent' with 409 already_permanent and a terminal status with 409 invalid_state, before the write. - model: SetDeploymentTTL UPDATE now carries `AND ttl_policy != 'permanent'` — defense-in-depth so a MakePermanent racing the UPDATE can't silently un-permanent the deploy (the row just isn't touched). - agent_action entry for already_permanent. Tests (DB-gated): make-permanent→SetTTL→409 (row stays permanent), terminal deploy→SetTTL→409, and the model WHERE-guard leaves a permanent row untouched on a direct call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mastermanas805
added a commit
that referenced
this pull request
Jun 3, 2026
…lose TOCTOU (#237) Design-review follow-up to #228. SetTTL guarded permanent + terminal deploys via a PRE-READ of the looked-up row, then called SetDeploymentTTL whose UPDATE only guarded `ttl_policy != 'permanent'`. Two problems the panel surfaced: 1. TOCTOU: a deploy reaped to expired/deleted/stopped (or made permanent) between the handler's status read and the UPDATE would still get expires_at re-armed + reminders_sent reset to 0 — re-entering the 6-email reminder cycle for a deploy the user thinks is gone. A lost race also returned 200 "TTL set" for a write that never happened. 2. The terminal SQL guard was missing entirely (only permanent was guarded). Fix: make the status-guarded UPDATE the AUTHORITATIVE guard. SetDeploymentTTL now `WHERE id=$2 AND ttl_policy != 'permanent' AND status NOT IN ('deleted','expired','stopped')` and returns whether a row was actually updated. The handler drops the stale pre-read; on !applied it re-reads (now authoritative, not racy) ONLY to pick the precise code — already_permanent vs invalid_state — so a concurrent change yields an honest 409, never a phantom 200. Added lifecycleTerminalStatusesSQL (deleted/expired/stopped) as a NAMED const, deliberately distinct from terminalDeploymentStatusesSQL (deleted/expired): the former is "reject lifecycle mutations" (stopped is terminal), the latter is "hide from the user's list" (stopped is still visible). Documented why they differ — the panel's "unify them" call was wrong; they answer different questions. Tests: model SetDeploymentTTL now 100% (added the 0-rows→applied=false sqlmock case); the existing TestSetTTL_{Permanent,Terminal}Rejected now deterministically cover the handler's re-read !applied sub-branches (permanent→already_permanent, terminal→invalid_state). Signature change threaded through 3 callers. Verified vs real Postgres+Redis; all changed lines covered. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
SetTTL unconditionally overwrote ttl_policy/expires_at, so a user could flip a
deployment they'd made permanent back to an expiring TTL, and could set a TTL on
an expired/deleted/stopped deploy. Both violate the lifecycle contract.
terminal status with 409 invalid_state, before the write.
AND ttl_policy != 'permanent'—defense-in-depth so a MakePermanent racing the UPDATE can't silently
un-permanent the deploy (the row just isn't touched).
Tests (DB-gated): make-permanent→SetTTL→409 (row stays permanent), terminal
deploy→SetTTL→409, and the model WHERE-guard leaves a permanent row untouched
on a direct call.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com