[fix](ci) deduplicate comment-triggered TeamCity builds - #66194
[fix](ci) deduplicate comment-triggered TeamCity builds#66194hello-stephen wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for six correctness issues in the new deduplication path.
- The raw admission/concurrency grammar disagrees with the established parser, so valid whitespace-normalized commands can be dropped and parser-invalid case variants can displace valid pending requests.
- Concurrency groups do not cover every workflow run that mutates the same concrete TeamCity pipeline.
- A current-revision match returns before stale revisions are cleaned up.
- Repeat count is omitted from request identity, so semantically different runs are skipped.
- TeamCity pagination is ignored, allowing later-page duplicates to be missed.
buildallis tracked only through Compile, not through the downstream fan-out it starts.
Critical checkpoints:
- Goal and tests: The exact-spelling filter removes the reported generic
runfalse positive, but the deduplication goal is not achieved for the races and lifecycle cases above, and valid command semantics regress. The PR changes no test files; the description reports actionlint, shell syntax, filter, mock, and live checks, but no reusable automated fixtures are present. In this review-only runner,bash -n regression-test/pipeline/common/teamcity-utils.shpassed; builds were prohibited andactionlintwas unavailable. - Scope and clarity: The two-file scope is focused, but raw admission, GitHub scheduling, TeamCity lookup, cancellation, and fan-out do not share one request model.
- Concurrency: Applicable and blocking. GitHub groups do not serialize all concrete pipeline mutations, default pending replacement can lose a valid request, and lookup/trigger remains a non-atomic remote sequence.
- Lifecycle and parallel paths: Applicable and blocking. Mixed old/current revisions and Compile-versus-descendant lifetimes are not handled; direct commands and
buildallare parallel paths to the same pipelines. - Error handling and observability: HTTP/JSON lookup failures correctly fail open with warnings, and duplicate logs include useful identifiers. Pagination is still an unhandled successful-response case.
- Compatibility/configuration/persistence/data correctness: No Doris runtime configuration, storage/protocol, transaction, persistence, data-write, FE/BE variable, or static-lifecycle change is involved.
- Performance: The intended duplicate suppression would reduce CI load, but the uncovered races, fan-out window, and later-page miss still allow redundant expensive builds.
- Security: The changed files are CI/dev surfaces outside the Doris runtime vulnerability model. No newly introduced runtime security finding is claimed.
- User focus: No additional user-provided focus was supplied; the whole PR was reviewed.
The review completed the required bounded multi-agent convergence against this frozen comment set, and every candidate was accepted, merged, or dismissed with evidence.
| ${{ | ||
| github.event.issue.pull_request && | ||
| ( | ||
| contains(github.event.comment.body, 'run buildall') || |
There was a problem hiding this comment.
[P1] Use one canonical command grammar before entering concurrency
This admission test is not equivalent to the parser below: it examines the raw body with case-insensitive contains, while lines 91/179 normalize whitespace and then shell-match case-sensitively. Thus run p0 (or a line break between the words) previously normalizes to valid run p0 but is now skipped; conversely RUN P0 is admitted and assigned the p0 group, then the parser no-ops. Because the default concurrency queue keeps only one pending job, that no-op can replace a legitimate pending p0 request. Please make admission, grouping, and dispatch use the same canonical command and preserve distinct pending requests.
| }} | ||
|
|
||
| # Serialize duplicate commands for one PR. The next run then sees the build | ||
| # queued by the previous run and can safely skip an identical PR revision. |
There was a problem hiding this comment.
[P1] Coordinate buildall through the complete fan-out
The helper and GitHub lock cover only Compile even though this workflow skips the downstream steps because Compile triggers them. This fails in both directions: for PR 66196, Compile 1008441 finished while same-revision P0 1008463, External 1008464, Cloud P0 1008465, and NonConcurrent 1008467 were still running, so another buildall starts a new root; while Compile is still active, a direct run p0 can queue P0 before Compile later enqueues the same P0 entirely inside TeamCity, bypassing this helper and every GitHub group. Please enforce exact-request coordination at the TeamCity fan-out boundary, or route both direct and fan-out requests through one atomic coordinator for the full descendant lifetime.
| concurrency: | ||
| group: >- | ||
| comment-to-trigger-teamcity-${{ github.event.issue.number }}-${{ | ||
| contains(github.event.comment.body, 'run buildall') && 'buildall' || |
There was a problem hiding this comment.
[P1] Serialize mutations of the same concrete pipeline
This key does not cover every run that executes the same check-then-trigger operation. run buildall is placed in the buildall group but later calls the helper for FEUT, BEUT, CloudUT, Compile, and Performance, while each direct command uses a different group. Also, run feut run beut is grouped as beut by this fixed precedence but the parser executes the first textual command, feut. Two such jobs can both observe no active FEUT build and then both enqueue it. Please lock by the concrete PR/pipeline mutation (or use a PR-wide/TeamCity-side atomic lock) and derive the key from the same canonical command used for dispatch.
| curl -sSf -X GET \ | ||
| -u OneMoreChance:OneMoreChance \ | ||
| -H "Accept: application/json" \ | ||
| "${teamcity_rest_url}/buildQueue?locator=buildType:(id:${PIPELINE})&fields=build(id,branchName,revisions(revision(version)))" |
There was a problem hiding this comment.
[P2] Follow TeamCity pagination when looking for duplicates
TeamCity collection responses may be partial and advertise the next page through nextHref, but this projection does not request that field and the function performs only one request. Since the queue locator can filter by build type but not by PR branch/revision, a busy pipeline can place the exact duplicate on a later page; this returns “none”, the single-page cancel path misses it too, and line 378 queues another copy. Please follow nextHref (for queued and running collections) until a match is found or the collection is exhausted, with a multi-page fixture.
| jq -s -r \ | ||
| --arg branch "pull/${PULL_REQUEST_NUM}" \ | ||
| --arg revision "${COMMIT_ID_FROM_TRIGGER}" \ | ||
| '.[] | .build[]? | |
There was a problem hiding this comment.
[P1] Include repeat count in the duplicate identity
Repeat count changes the request: the parser preserves it and trigger_build sends it as env.repeat_times_from_trigger, which the regression pipelines use for their iteration count and timeout. This selector compares only PR branch and revision, so while run cloud_p0 is active, a subsequent run cloud_p0 3 on the same head is reported as a duplicate and the requested three passes are never scheduled. Please normalize the empty count to 1 and compare the active build's repeat property as part of duplicate equivalence, while keeping all requests for one PR/pipeline in the same serialization domain.
| ) || duplicate_lookup_status=$? | ||
| if [[ ${duplicate_lookup_status} -eq 0 ]]; then | ||
| echo "INFO: active build(s) ${duplicate_build_ids//$'\n'/,} already exist for PR ${PULL_REQUEST_NUM}, pipeline ${COMMENT_TRIGGER_TYPE}, revision ${COMMIT_ID_FROM_TRIGGER}; skip duplicate trigger" | ||
| return 0 |
There was a problem hiding this comment.
[P1] Still clean up stale revisions before returning
get_active_builds_of_revision reports only IDs matching the requested revision, so this return says nothing about other active builds for the same PR/pipeline. If a current-revision build is queued while an older revision is still running or queued (for example after an external trigger or a failed/asynchronous cancellation), this branch bypasses both cleanup calls below and leaves the stale build alive. That contradicts the stated preservation of cancel-old-revision behavior. Please classify the full active set, cancel every non-current build, and skip only the new trigger when a current build remains.
1 participant
What changed
issue_commentjob filter to supported TeamCity commands instead of matching every comment containingrun.Why
Performance result comments such as
Total hot run timematched the genericcontains(..., 'run')condition and unnecessarily consumed GitHub-hosted runners. Repeatedrun buildallcomments for the same PR revision could also cancel and recreate identical TeamCity builds.This reduces false GitHub Actions queue pressure and avoids duplicate TeamCity fan-out without changing valid command semantics.
Validation
actionlint .github/workflows/comment-to-trigger-teamcity.ymlbash -n regression-test/pipeline/common/teamcity-utils.shrun buildall, numbered pipeline runs, and skip commands1008222, BEUT1008223, CloudUT1008224, and Compile1008225all matched the same PR HEAD revision