Support -parallel > 1 for real apply - #306
Conversation
Duplicate names make GetRuleByName return the same *Rule to multiple parallel workers (data race in parallel diff / apply -dry-run) and silently shadow later rules in sequential apply. BREAKING: configs containing duplicate rule names now fail to load for every subcommand. Regenerate with 'ecschedule dump -region ... -cluster ...'.
First signal cancels the context so subcommands stop at API-call boundaries instead of dying mid-write; a second signal restores default fatal handling as an escape hatch. Interrupt now exits with the normal error path (status 1) instead of signal death.
plan() runs validations + remote diff and returns data (no logging); execute() runs PutRule → PutTargets → TagResource under WithTimeout(WithoutCancel(ctx), 2m) so a sibling failure or first SIGINT never interrupts a started sequence, while a stalled call cannot hang forever. TagResource gets an app-level retry scoped to error classes the SDK does not retry, and tag-only failure is reported via a dedicated error type whose Error() is exactly the underlying error string. validateTaskDefinition wraps with %w so a context cancellation stays identifiable through the error chain (message unchanged). Golden-log tests stub AWS at the HTTP layer (aws.Config.HTTPClient with X-Amz-Target routing) and pin the exact log output and call sequence of apply, proving the sequential path is unchanged across the refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EytbJ4CKmkZZPA8wWmZQEb
Adds a three-phase engine behind the existing -parallel flag: Phase 1 plans every rule read-only (all validation errors are listed and any error blocks all writes; interruption is classified separately from failure). Phase 2 executes only the changed rules continue-on-error with per-region clients (RetryMaxAttempts=10) and atomic per-rule output blocks; a tag-only failure prints a fully substituted repair command. Phase 3 prints a summary. The engine returns nil only on full uninterrupted success, which is what gates the -prune block. Underneath, executeJobsCollect is a new runner alongside executeJobsInParallel: it never cancels in-flight jobs (plain errgroup, errors recorded per outcome), and emits exactly one outcome per name on every path including panics and admission skips. parallel == 1 and dry-run keep their existing code paths untouched. README documents the split semantics, interrupt behavior and CI grace-period advice, the scoped idempotency story with its two exceptions, quota guidance, and the duplicate-rule-name rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EytbJ4CKmkZZPA8wWmZQEb
Planning validated the task definition once per rule. Real configs point many rules at a handful of definitions, so a 150-rule config issued 150 identical DescribeTaskDefinition calls at -parallel 10 — enough for ECS to answer ThrottlingException. The engine's ECS client also kept the SDK default of 3 attempts while its EventBridge clients used 10, so the retries ran out and Phase 1 aborted the whole run. Because Phase 1 is fail-closed, that surfaced as "1 rule(s) failed validation/planning; no writes were performed" with a message blaming a task definition that exists. Found running apply -all -parallel 10 against a real 150-rule config. taskDefValidator memoizes the result per region + task definition, and the engine gives it the same raised retry budget as its EventBridge clients: planning is where call volume peaks, so it is the phase most exposed to throttling. The dry-run path fans applyInternal out over workers instead of using the engine, so applyInternalWith lets it share one validator too; parallel diff -validate shares the same memoization. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EytbJ4CKmkZZPA8wWmZQEb
There was a problem hiding this comment.
Pull request overview
This PR enables real parallel execution for apply -parallel N (N > 1) by introducing a conservative plan → execute → summarize workflow, while also hardening config loading and operational behavior (signals, retries, throttling). It fits into the codebase by expanding the existing parallel infrastructure (previously used for diff / apply -dry-run) to safely support parallel writes to EventBridge.
Changes:
- Add a new parallel apply engine (
runParallelApply) that validates/diffs all rules before any write, then applies changed rules with aggregated failures and an end-of-run summary. - Reject duplicate rule names at config load time (breaking change) to avoid shadowing and parallel data races.
- Improve resilience/operability: memoize
DescribeTaskDefinition, add continue-on-error parallel runner, and add SIGINT/SIGTERM cancellation viasignal.NotifyContext.
Show a summary per file
| File | Description |
|---|---|
| rule.go | Refactors apply into plan/execute, adds task-definition validation memoization and tagging retry/error typing. |
| parallel.go | Adds executeJobsInParallelContinueOnError to run all jobs and aggregate outcomes without fail-fast cancellation. |
| parallel_apply.go | Implements the new three-phase parallel apply flow with per-rule logging and summary output. |
| cmd_apply.go | Removes the “dry-run only” parallel guard; routes real apply to runParallelApply when -parallel > 1. |
| cmd_diff.go | Shares task-definition validation across workers to reduce ECS API amplification. |
| config.go | Adds validateUniqueRuleNames and enforces it at config load time (breaking). |
| config_test.go | Adds coverage for duplicate rule name rejection. |
| cmd/ecschedule/main.go | Introduces SIGINT/SIGTERM handling using signal.NotifyContext, with a “second signal kills immediately” escape hatch. |
| README.md | Updates docs for real parallel apply behavior, failure semantics, and signal handling. |
| rule_apply_stub_test.go | Adds HTTP-layer AWS stubs and golden-log tests to pin output/call sequences. |
| parallel_continue_on_error_test.go | Tests the continue-on-error parallel runner behavior (success/failure/skip/panic). |
| parallel_apply_test.go | Tests parallel apply semantics (barrier planning, aggregation, memoization, interruption). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
| func(ctx context.Context, name string) (string, error) { | ||
| ru := rules[name] | ||
| log.Printf("applying rule %q", name) | ||
| if err := ru.execute(ctx, clients[ru.Region]); err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Good catch — the wording was wrong, not the code, so I fixed the wording in 6291751.
You are right that the two log.Printf calls interleave. A real 150-rule run looks like this:
[ecschedule] applying rule "perf-test-rule-001"
[ecschedule] applying rule "perf-test-rule-004"
[ecschedule] ✅ rule "perf-test-rule-004" applied
💡 applied changes: ...
[ecschedule] applying rule "perf-test-rule-012"
What is indivisible is each rule's result: the ✅ rule … applied line, its diff and the resulting YAML go out in one log.Printf, so two results can never be spliced into each other. The claim of "one block per rule" overstated that, and the README and the PR body now say "result" explicitly.
I kept the progress line rather than folding it into the final block, for two reasons. A parallel apply is otherwise completely silent for its whole duration — the 150-rule run above takes ~25s — and this line is what tells you which rules are in flight. It also matches the sequential path, which prints the same applying the rule "…" before writing; dropping it only under -parallel > 1 would make the two paths diverge for no gain.
Happy to fold it in anyway if you would rather have exactly one line per rule.
|
|
||
| ### Signal Handling | ||
|
|
||
| Since the introduction of parallel apply, all subcommands handle SIGINT/SIGTERM gracefully: the first signal cancels in-progress work at API-call boundaries (for parallel apply, no new rules start and in-flight rules finish within the per-rule timeout) and the command exits through the normal error path (exit status 1 with a `💢` message) instead of dying to the signal (previously exit status ~130). A second signal restores default fatal handling and kills the process immediately. |
There was a problem hiding this comment.
Correct, and the exemption is in fact wider than this comment says. Rewritten in 6291751.
The old sentence asserted the general behavior and then immediately carved out the real one in a parenthetical, which reads as a contradiction. Worse, the parenthetical said "for parallel apply", but applyInternalWith also goes through execute() (rule.go), so the write sequence is shielded on the sequential path too — the exemption applies to any real apply, with or without -parallel.
The section now leads with the general behavior and states the exemption separately:
The first signal stops work at API-call boundaries and the command exits through the normal error path …
One part is deliberately exempt from that first signal: a rule whose write has already begun always completes its
PutRule→PutTargets→TagResourcesequence. That sequence is detached from the signal (it runs undercontext.WithoutCancel) and bounded only by the per-rule timeout, so no rule is ever left half-written. This holds for any realapply, with or without-parallel. What the first signal does stop in a parallel apply is admission: no further rules start, and the command waits for the in-flight ones.
The behavior was verified against a real 150-rule config: one SIGINT mid-run gave 40 applied, 0 failed, 110 not started, and every applied rule was written completely.
Both surfaced in review of Songmu#306. "one block per rule" was not true as written: a rule emits a progress line when it starts and its result block when it finishes, so progress lines do appear between result blocks. Only the result — diff plus the resulting YAML — is indivisible. Say that instead; the progress lines are worth keeping, since a parallel apply is otherwise silent for its whole duration and the sequential path prints the same line. The signal paragraph claimed the first signal cancels in-progress work at API-call boundaries, then immediately carved out the one case where it does not. Lead with the general behavior, then state the exemption on its own: a rule that has started writing always finishes. That exemption is also wider than the parenthetical said — execute() shields the write sequence on the sequential path too, not just under -parallel > 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EytbJ4CKmkZZPA8wWmZQEb
Summary
-parallelcurrently only speeds updiffandapply -dry-run; a realapplyalways runs sequentially. This PR makesapply -parallel N(N > 1) run for real, through a three-phase plan → execute → summarize path that is deliberately more conservative than the sequential path.-parallel 1(the default) and-dry-runkeep their existing code paths and byte-identical output — golden-log tests in this PR pin that.A config containing two rules with the same name no longer loads, for every subcommand, at any
-parallelvalue. Configs that are valid today can start failing after this change.It is deliberate rather than incidental.
GetRuleByNameresolves a name to the first match, so duplicates already caused two bugs: later rules were silently shadowed in sequential apply, and — worse — the same*Rulepointer was handed to several workers at once in the existing paralleldiff/apply -dry-run, which is a data race. Making the new parallel apply safe means the duplicates have to be rejected rather than tolerated.The error names the offending rule. Fixing a config means renaming the duplicate or regenerating with
ecschedule dump.How it works
sequenceDiagram autonumber participant M as runParallelApply participant W as workers<br/>(at most N in flight) participant AWS Note over M,AWS: Phase 1 — plan · read-only · N rules at a time M->>W: admit a rule (blocks while N are in flight) W->>AWS: DescribeTaskDefinition (memoized per task definition) W->>AWS: ListRules / ListTargetsByRule W-->>M: plan: differs? + rendered diff Note over M: BARRIER — every rule is planned before any write.<br/>One failure ⇒ report them all, exit 1, nothing written. Note over M,AWS: Phase 2 — execute · only rules that differ · N at a time M->>W: admit a changed rule rect rgba(127, 127, 127, 0.2) Note over W,AWS: ATOMIC per rule — a sibling's failure or the<br/>first Ctrl-C cannot interrupt this sequence<br/>(context.WithoutCancel + 2 min timeout) W->>AWS: PutRule W->>AWS: PutTargets W->>AWS: TagResource end W-->>M: outcome: applied / failed (never cancels siblings) Note over M: each rule's result is one indivisible log block,<br/>so results never interleave Note over M: Phase 3 — summary.<br/>prune runs only if every rule succeeded and nothing was interrupted.Phase 1 — plan (read-only). Every rule is validated (env / tfstate / ssm placeholders, task definition existence) and diffed against the remote. No writes happen until every rule has passed. If any rule fails validation, all of them are reported and nothing is written — a bad config can no longer leave the account half-applied.
Phase 2 — execute. Only rules that actually differ are written, continue-on-error, with per-region clients (
RetryMaxAttempts=10). Each rule's result — its diff plus the resulting YAML — is emitted as a single indivisible block, so results never interleave; the shortapplying rule "..."progress lines are separate and do appear between those blocks. A rule that has started writing always finishes itsPutRule→PutTargets→TagResourcesequence: the sequence runs undercontext.WithoutCancelplus a 2-minute timeout, so a sibling's failure — or the first Ctrl-C — can never interrupt a half-written rule, while a stalled AWS call still cannot hang the run forever.Phase 3 — summary. Applied / failed / skipped counts, with the failures listed.
runParallelApplyreturns nil only on full uninterrupted success, and that is what gates the-pruneblock, so pruning never runs on a partial result.Behavior changes worth calling out
Besides the breaking change above:
-parallelvalue. Previously nothing caught them, so a Ctrl-C duringapplykilled the process outright and could leave a rule withPutRuleapplied andPutTargetsnot. Now the first signal stops work at API-call boundaries, a rule that has already begun writing runs to completion, and the command exits 1 through the normal error path instead of dying to the signal (previously ~130). A second signal restores default fatal handling as an escape hatch.apply -parallel > 1only. WhenPutRule/PutTargetssucceeded butTagResourcedid not, the failure prints a fully substitutedaws events tag-resourcerepair command, because a re-run does not heal it.-parallel 1keeps its existing error output byte for byte.Caveat introduced by this PR
-parallel > 1some rules may surface a rate-limit-token error instead of the underlying one — two error texts for a single root cause. Documented in the README.Two parallel runners, for now
executeJobsInParallelContinueOnErrorjoins the existingexecuteJobsInParallelrather than replacing it, because the two differ in exactly one way that matters: the existing one shares anerrgroup.WithContext, so the first error cancels the rest, while apply needs every rule attempted and the failures aggregated.The split is not meant to be permanent, and the doc comment says so.
executeJobsInParallelContinueOnErroris the more general of the two — fail-fast can be layered on it by cancelling a derived context on the first error, while the reverse is not possible — so it is the consolidation target. The reason the remaining callers (diffandapply -dry-run) are not moved here is that it would change what they report, from only the first error to every rule's, and that behavior change does not belong in this PR.Also fixed here: one DescribeTaskDefinition per rule
Planning validated the task definition once per rule, so a config where many rules share a task definition issued that many identical
DescribeTaskDefinitioncalls. At-parallel 10over 150 rules that is enough for ECS to answerThrottlingException, and since planning is fail-closed the whole run aborted with a message blaming a task definition that exists. The last commit memoizes the result per region + task definition (so 150 calls become 1) and gives planning the same raised retry budget as the write phase. The dry-run anddiff -validateparallel paths share the same memoization.Testing
apply's exact log output and call sequence, so "the-parallel 1path is unchanged" is enforced rather than asserted.-prunedeleting exactly the orphans.The commits are ordered so each one builds and tests green on its own.
Pre-existing issues this PR does not address
Neither is introduced or changed by this PR, and neither is fixed here. They are listed only because applying many rules at once makes them easier to run into, and both look worth separate fixes.
-pruneorphan detection only searches the region of the ambient AWS configuration, so rules under a per-ruleregionoverride are outside its view.targetIdleaves the old target attached remotely: the rule then fires both targets and subsequently diffs as a new rule.