Repository navigation
Release/0.7.10 - #108
Merged
Merged
Release/0.7.10#108
Conversation
* Add snowman-tests job to cd.yml per the Snowman CI Hook Guide
Dispatches the Snowman QA framework after a tag release, waiting on the tests
only (~60-90 min); any rollout runs asynchronously on Snowman's side, so this
job is red only when tests fail.
Three deliberate deviations from the guide's example, each forced by something
specific to this repo:
- `needs: release` rather than `needs: deploy_to_docker`. dataflow-runner
builds and publishes its image in one job.
- The gate is `github.repository == 'snowplow/dataflow-runner'` rather than
`github.event.repository.private == true`. That condition exists to skip
the public mirror of a private release repo; dataflow-runner is public-only
with no private mirror, so it would be permanently false and the job would
never run. Gating on the canonical repository preserves the intent — forks
must not dispatch into Snowman — while letting the job fire here.
- `#pipeline-rollouts` rather than `#automated-rollouts`. The latter is
deprecated since QA-1114; pipeline and data-processing components post to
#pipeline-rollouts, which is what the guide's own summary table says even
though its prose still names the old channel.
The matrix is component tests only. dataflow-runner is a CLI binary invoked by
a single stack, so there is no multi-component interaction a composite would
add. The other two suites that stand up the shredder — components/rdb_loader
and composites/collector_warehouse_pipelines — do not pin
dataflow_runner_version, so dispatching a release at them would exercise the
DS4 default rather than the released version: green, and meaningless.
actionlint reports the same 6 findings before and after this change, all in the
pre-existing release job.
* Drop the release-notes step from the Snowman CI hook
The guide's example appends the Snowman run link to the GitHub release body via
softprops/action-gh-release@v2 with append_body. Removed: the same link is
already published to the Actions run summary by the preceding step, so the
release-notes copy is duplication that also gives the job write access to
releases it does not otherwise need.
The tag-type detection step stays — both summary steps still branch on it.
* Do not fail a transient run on a throttled DescribeCluster Production runs were reported as failed after a single ThrottlingException. None had failed: clusters had come up, or terminated cleanly, and we exited anyway — in the launch cases leaving a cluster running its steps unwatched. Live testing since reproduced this exactly: the old binary abandoned a cluster that went on to reach ALL_STEPS_COMPLETED four minutes later. The waiters swallow API errors and keep polling, so the throttled call was never a poll. It was the describe around one: the existence check before it, or the re-describe after it, since waiter.Wait discards its response and the status had to be fetched again. WaitForOutput hands back the response that satisfied the acceptor, so that second call is gone from the happy path entirely; what remains covers only a waiter that errored or timed out, and a TERMINATED that EMR has not yet attached a reason to. Both transient waits lose their existence check as well. Their callers pass a jobflow ID RunJobFlow has just returned, so it guards against nothing, and the waiter's first poll is issued with no delay — it asks the same question and resolves the same terminal states through its own acceptors. Dropping it removes a duplicate call from the path where call volume is the problem, and turns a throttled first poll into something the waiter absorbs rather than something a retry budget has to outlast. Only `down`'s wait keeps one, since only `down` takes a cluster ID from the operator. Removing the job phase's check also closes a second false failure, one no throttling was needed to reach. That check returned an already-TERMINATED cluster with a nil error whether or not EMR had attached its reason yet, and a reasonless TERMINATED reaches the caller's "cannot be confirmed complete" branch. The waiter's output is now only trusted when the reason is present, and recovered by a retried describe when it is not — and a status recovered that way never downgrades one the launch already classified, since a bootstrap failure often presents as plain TERMINATED and losing that code loses the relaunch. A launch that cannot be read is left alone rather than terminated: not observing RUNNING says nothing about whether the cluster reached it, and killing one mid-step leaves half-written output, which is worse than an unwatched cluster that finishes and self-terminates. Both paths that give up without a status say which cluster they gave up on — and a wait that did see the cluster terminate hands that status back, so nobody is sent after a cluster that has already gone. Waiting is bounded at both ends by what could still change the answer. A throttle, a dropped connection, a timeout or a 5xx gets the whole wait; an answer AWS actually gave us — a role without DescribeCluster, an ID that does not exist — ends it after three polls rather than holding the waiter for its full bound. Waiters warn after five consecutive failed polls and every five after that, counted in polls so the threshold means the same on a 30-90s launch wait as on a 60-300s job one. Six hours of unbroken blindness ends the wait outright, so the job phase cannot spend its fortnight-long bound on a cluster that finished long ago. The same line decides how long a call outside the waiters retries. Failures that might clear are waited out for minutes, because these calls are how a run learns whether it succeeded and each is issued while a cluster is already up or once it has gone. The rest get the few seconds they always had — enough that EMR briefly not recognising a jobflow ID it just issued is survivable, while a bad --emr-cluster is still reported in seconds. That covers ListSteps and DescribeStep, which sit on the transient success path and could equally turn a completed run into a failed one, and TerminateJobFlows, which is idempotent and whose throttling would otherwise fail `down` in three seconds. To reduce the pressure in the first place, waiters poll at 30-90s while a cluster launches and 60-300s during a transient job, rather than a fixed 30s: smithy only jitters when the bounds differ, so every runner had been polling on the same beat against a budget measured at ~18 requests of burst and ~0.8/sec sustained for the whole account. That reduction, not the retries, is what removes load, and the note by the budgets now points at the step-status loop as the larger remaining source. Both EMR clients move to adaptive retry mode, which damps a single process's own burst rather than coordinating anything. Neither raises its attempt count, which is passed per call instead, and only on calls that are safe to repeat: RunJobFlow and AddJobFlowSteps share those clients and no EMR write has an idempotency token, so retrying one whose response was merely lost would launch a second cluster or submit the playbook's steps twice. Three fixes to what `up` tells the operator. A successful launch no longer logs at error level, nor fails when the waiter resolves on RUNNING rather than WAITING. A bootstrap failure that is about to be retried is a warning, matching the transient path, and no longer sleeps before the attempt that gives up. And an `up` that fails after launching names the cluster it left behind — it sets KeepJobFlowAliveWhenNoSteps, so that cluster will not terminate itself, and its ID reached the operator nowhere else. Also drops fetchStateChangeReason, which has had no callers since before e9939d3 and was the last unretried DescribeCluster in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Own the poll-again decision rather than borrowing it from the SDK pollThroughBlindness now decides for itself that a failed poll means "ask again", instead of deferring to the generated waiter acceptor — which returns (true, nil) on an API error in service/emr v1.47.0 but (false, err) from v1.47.5 onward. The acceptors are consulted only when there is a response to inspect. No behaviour change on the pinned version. Also reworks the unreadable-launch test to reach its branch via a cancelled context, and corrects the blind-poll threshold comment, which implied a healthy run never warns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
Oguzhan Unlu (oguzhanunlu)
approved these changes
Aug 11, 2026
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
3 participants
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.
Jira ref: PDP-2827