github-actions[bot] commented on code in PR #66349:
URL: https://github.com/apache/doris/pull/66349#discussion_r3700995964
##########
.github/workflows/code-review-runner.yml:
##########
@@ -946,27 +949,61 @@ jobs:
OSS_ENDPOINT: oss-cn-hongkong.aliyuncs.com
OSS_CODEX_SESSION_PREFIX: oss://doris-community-ci/session
- - name: Sync Codex auth back to OSS
- if: ${{ always() }}
- continue-on-error: true
+ - name: Sync refreshed Codex auth back to OSS
+ if: ${{ always() && steps.auth.outcome == 'success' }}
timeout-minutes: 5
run: |
- if [ -z "$CODEX_AUTH_OSS_OBJECT" ]; then
- echo "No selected Codex auth object found; skipping OSS auth sync."
- exit 0
- fi
-
- if [ ! -s "$CODEX_HOME/auth.json" ]; then
- echo "No Codex auth file found; skipping OSS auth sync."
- exit 0
- fi
+ retry_oss_copy() {
+ local source="$1"
+ local destination="$2"
+ local description="$3"
+ for attempt in 1 2 3; do
Review Comment:
[P2] The eight-minute step now contains the pinned client's roughly
122-second HEAD, 126-second GET, and 126-second PUT envelopes, but the
enclosing job budget was not expanded with it. On the supported late
usage-limit path, declared caps total 153 minutes before this finalizer and 161
including it, so the new cap consumes eight of the 12 minutes that lines 58-60
reserve for runner setup and post-job cleanup, leaving four. Once accumulated
runner/inter-step overhead exceeds about 5m46s, even the approximately
374-second native envelope no longer fits under `timeout-minutes: 165`. Please
raise the job cap or rebalance the budgets so this finalizer and the documented
cleanup reserve both fit.
##########
.github/workflows/code-review-runner.yml:
##########
@@ -277,6 +277,9 @@ jobs:
and (.tokens.access_token | type == "string" and length > 0)
and (.tokens.refresh_token | type == "string" and length > 0)
' "$RUNNER_TEMP/codex-home/auth.json" >/dev/null
+ sha256sum "$RUNNER_TEMP/codex-home/auth.json" \
Review Comment:
[P2] Preserve failures from the digest command
This workflow does not set `shell: bash`, so GitHub runs these steps as
`bash -e` without `pipefail`; the pipeline status is only `awk`'s status. If
`sha256sum` fails to read the file, `awk` still exits zero and this step
records an empty original hash. A later valid local/remote digest then differs
from that empty baseline, so the finalizer takes the normal conflict-warning
path and silently drops the refreshed credential even though OSS did not
change. The two finalizer hash pipelines have the same masking problem. Enable
`pipefail` in both affected steps (or avoid the pipelines while preserving
`sha256sum`'s exit status), and cover a digest-failure path in the mock
validation.
##########
.github/workflows/code-review-runner.yml:
##########
@@ -946,27 +949,49 @@ jobs:
OSS_ENDPOINT: oss-cn-hongkong.aliyuncs.com
OSS_CODEX_SESSION_PREFIX: oss://doris-community-ci/session
- - name: Sync Codex auth back to OSS
- if: ${{ always() }}
- continue-on-error: true
- timeout-minutes: 5
+ - name: Sync refreshed Codex auth back to OSS
+ if: ${{ always() && steps.auth.outcome == 'success' }}
+ timeout-minutes: 8
run: |
- if [ -z "$CODEX_AUTH_OSS_OBJECT" ]; then
- echo "No selected Codex auth object found; skipping OSS auth sync."
+ if ! jq -e '
+ .auth_mode == "chatgpt"
+ and (.tokens.access_token | type == "string" and length > 0)
+ and (.tokens.refresh_token | type == "string" and length > 0)
+ ' "$CODEX_HOME/auth.json" >/dev/null; then
+ echo "::error::Refreshed Codex auth is invalid; refusing OSS auth
sync."
+ exit 1
+ fi
+
+ original_hash="$(<"$RUNNER_TEMP/codex-auth-original.sha256")"
+ local_hash="$(sha256sum "$CODEX_HOME/auth.json" | awk '{print $1}')"
+ if [ "$local_hash" = "$original_hash" ]; then
+ echo "Codex auth was not refreshed; skipping OSS auth sync."
exit 0
fi
- if [ ! -s "$CODEX_HOME/auth.json" ]; then
- echo "No Codex auth file found; skipping OSS auth sync."
+ umask 077
+ remote_auth="$(mktemp "$RUNNER_TEMP/codex-auth-current.XXXXXX")"
+ trap 'rm -f "$remote_auth" "${remote_auth}.temp"' EXIT
+ if ! ossutil -i "$OSS_AK" -k "$OSS_SK" -e "$OSS_ENDPOINT" \
+ --retry-times=3 --connect-timeout=10 --read-timeout=30 \
+ cp -f "$CODEX_AUTH_OSS_OBJECT" "$remote_auth"; then
+ echo "::error::Could not verify the current OSS auth after 3
attempts."
+ exit 1
+ fi
+
+ remote_hash="$(sha256sum "$remote_auth" | awk '{print $1}')"
+ if [ "$remote_hash" != "$original_hash" ]; then
Review Comment:
[P2] Do not treat every remote mismatch as proof that the local refresh is
stale
The hashes have no generation ordering. Two supported runs can both start
from O0, A can refresh to O1, and B can later refresh to O2. If A uploads O1
before B reaches this read, B sees `remote_hash != original_hash` here and
skips O2, leaving the older (and potentially revoked) generation in OSS. In the
base workflow the same finalization order uploaded O1 and then O2, so this new
conflict branch introduces a lost-newest-refresh case even though the remote
write happened before B's read, outside the disclosed post-read race. Please
prevent concurrent use of one auth object (for example with an atomic
per-object lease), or carry authoritative generation/ownership metadata; hash
inequality alone cannot decide which refresh is stale.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
