Skip to content

fix(sync): self-heal absent task-SG load balancer ingress rule on sync#95

Open
stevethomas wants to merge 1 commit into
mainfrom
fix/lb-ingress-self-heal
Open

fix(sync): self-heal absent task-SG load balancer ingress rule on sync#95
stevethomas wants to merge 1 commit into
mainfrom
fix/lb-ingress-self-heal

Conversation

@stevethomas
Copy link
Copy Markdown
Member

Hey, I made a thing! 🥳

Great! Now please answer the following questions to help out your assigned reviewer:

What problems are you solving?

  • ensureLoadBalancerIngressRule (in SyncTaskSecurityGroupStep) wrote the task-SG → load-balancer ingress rule but recorded no Change on the plan (dry-run) pass. The runner's planEntryHasWork filter only keeps a step for the apply pass if it returns a WOULD_* status or recorded a change — so this step got pruned before apply.
  • Net effect: the rule was only ever authorised at create-time. A task SG that exists without the rule — e.g. a sync whose create was interrupted mid-flight — could never be self-healed by a later sync. sync production reported clean while the SG was silently missing its container-port ingress.
  • This surfaced on convict: its task-SG create was interrupted by the #93 crash, leaving it permanently stuck (CL got the rule at create-time, so it was unaffected).

Fix: move recordChange() to before the $dryRun guard (mirroring AuthorisesTaskIngress), so the plan flags the step pending → it survives the prune → apply self-heals any SG missing the rule. The reconcile stays purely additive — it never revokes an out-of-band rule, and records nothing when a matching rule already exists (no phantom drift).

Is there anything the reviewer needs to know to deploy this?

  • Behaviour change is plan-pass only: an SG that already has the rule still records nothing and stays clean; an SG missing it now shows ingress 8000/tcp from load balancer security group as a pending change and authorises it on apply. No revokes, ever.
  • Once merged, yolo sync production on convict will detect and add the missing rule — the permanent manual unblock is no longer required (a one-off manual rule-add was the only prior workaround).
  • 621 Pest green; phpstan clean. Two regression tests added: the change is recorded under --dry-run (survives the prune), and an already-authorised SG records no ingress change.

🤖 Generated with Claude Code

…-heals

ensureLoadBalancerIngressRule wrote the task-SG ingress rule but recorded no
Change on the plan (dry-run) pass, so the runner's pending filter pruned the
step before apply. A task SG that existed without the rule — e.g. a create
interrupted mid-flight — could therefore never be self-healed by a later sync;
it was only ever authorised at create-time.

Move the recordChange() before the dry-run guard (mirroring AuthorisesTaskIngress)
so the plan flags the step pending, it survives the prune, and apply adds the rule.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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