[fix](ci) Protect refreshed Codex auth updates

[shuke987]

What this PR changes

This PR makes the Codex auth.json write-back in the automated review workflow conditional and conflict-aware.

• Record the SHA-256 of the selected and validated OSS auth file before Codex starts.
• After the review finishes, upload auth.json only when Codex actually changed the local file.
• Re-download the current OSS object immediately before write-back and upload only when it still matches the originally downloaded version.
• Bound OSS reads and writes with the client's native retry policy: three total attempts, a 10-second connection timeout, and a 30-second read timeout. If a refreshed credential cannot be verified or persisted, fail this non-required workflow instead of silently losing the update.
• Keep the downloaded verification copy and the OSS SDK's intermediate .temp file private with umask 077, and clean both paths on every exit.
• Preserve the pinned OSS client's native output so fatal reads and writes retain the OSS error code, request ID, or concrete network cause.
• Run the sync whenever auth setup succeeded, even if the review later failed for an unrelated reason, because Codex may already have refreshed the credential.

The resulting decision is:

Local auth after Codex	Current OSS auth	Result
Unchanged from the downloaded version	Not read	Skip write-back
Changed	Still the downloaded version	Upload the refreshed auth
Changed	Changed by another job or operator	Skip the stale write-back
Changed	OSS cannot be read or written after retries	Fail the workflow

Why

The previous step uploaded auth.json unconditionally. An in-flight job could therefore overwrite a credential that an operator had just reseeded, or overwrite another newer OSS copy even when Codex had not refreshed anything in that job.

This change protects the normal operational reseed path without requiring jobs to drain first. It also keeps the implementation local to the existing workflow and does not introduce a lease or reduce review concurrency.

Scope and accepted limitations

This PR intentionally addresses only safe persistence of a Codex-refreshed auth.json.

It does not change:

• account selection, randomization, or usage-limit cooldown handling;
• revoked-account handling;
• the fact that multiple jobs may select the same account;
• the shared-auth model into an officially supported serialized model;
• token refresh behavior inside Codex.

The OSS comparison and final upload are not an atomic compare-and-swap. A small race remains if the same OSS object changes between the final read and upload. Eliminating that race would require conditional object writes, versioning, or locking and is deliberately left out to keep this PR small. The workflow continues to accept rare concurrent-refresh failures rather than limiting review concurrency.

Validation

• Parsed the workflow as YAML.
• Ran bash -n on the updated shell step.
• Exercised the unchanged, refreshed, and remote-conflict decision paths with mocked OSS operations.
• Verified the native retry and timeout arguments against the pinned ossutil version's option semantics.
• Verified the pinned OSS SDK's temporary-file behavior and restrictive-umask cleanup path.
• Verified that the pinned client reports terminal transfer failures through stdout and that the workflow no longer discards it.
• Ran git diff --check.

[hello-stephen]

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?

[shuke987]

/review

[github-actions]

Automated review summary

Overall: Request changes. The content-hash conflict check is focused and the disclosed non-atomic read-to-upload limitation is handled consistently, but the new retry wrapper does not control the retry and timeout policy of the pinned OSS client. One inline issue identifies the resulting mismatch between the promised three bounded attempts and the actual execution path.

Critical checkpoints:

• Goal and proof: The hash snapshot, local-change gate, remote re-read, and conflict skip implement the stated conditional write-back policy. However, the promised three-attempt retry behavior is not implemented by the real pinned client: each wrapper invocation retains ossutil's 10 native retries and timeouts longer than the step. In this review the workflow parsed as YAML and both modified shell blocks passed bash -n; per the automated-review contract, no build or live workflow execution was attempted.
• Scope and clarity: The diff is limited to the existing auth configuration and write-back lifecycle in one workflow and reuses the selected object and existing OSS tooling.
• Concurrency: There are no in-process threads or locks. Cross-job/operator concurrency is handled by comparing the originally downloaded content with a fresh OSS read. Updates visible before that read are preserved; simultaneous changes after it remain the documented non-atomic limitation.
• Lifecycle and error handling: The selected object, validated local auth, and original hash are established by the same successful setup step. Final local JSON is validated again, the comparison file is mode 0600 and trap-cleaned, and invalid or unavailable state fails loudly rather than being uploaded.
• Conditions and parallel paths: The auth-success outcome correctly fences all required state, and unchanged local content, remote conflict, failed review, and cancellation have distinct paths. The OSS failure path is incomplete because a single native call can consume the five-minute step before the wrapper reaches later attempts or its terminal diagnostic. Review completion status remains separate from later credential-persistence job failure.
• Configuration and compatibility: No Doris configuration, storage or protocol format, function symbol, rolling-upgrade behavior, or FE/BE variable propagation changes.
• Tests and results: No Doris regression, BE, FE, or .out result is applicable to this workflow-only change. Static YAML and shell syntax checks passed. The PR's mocked outer-loop retry cases do not cover ossutil's native retry/timeout layer, which is the blocking gap.
• Observability and security: The explicit warnings and success log avoid credential content and print only fixed descriptions or the selected object basename. On a native client stall, however, the step timeout can prevent the wrapper's terminal ::error:: from running. CI infrastructure is outside the Doris runtime threat model, while its operational credential handling was still reviewed for correctness.
• Persistence, crash behavior, and performance: Remote mismatch conservatively wins and the Actions step ultimately fails when persistence cannot complete. The unchanged path adds only local hashing, but changed auth can invoke roughly 30 client-level retries or spend the whole step in its first wrapper attempt until the retry policy is explicitly bounded.

User focus: No additional review focus was provided; the entire PR was reviewed.

All other review candidates were independently checked and resolved. One inline comment is proposed for the retry-policy issue.

[github-actions]

[P2] Bound the actual OSS retry policy

The workflow pins ossutil 1.7.19, whose cp --help defaults to 10 internal retries, a 120-second connect timeout, and a 1,200-second read timeout. Each pass through this new loop can therefore retry many times or outlive the entire five-minute step; a stalled first invocation is killed before attempts 2/3 and the terminal ::error::, while fast retryable failures can produce roughly 30 client-level retries instead of the advertised three. The mocked outer-loop retry cases do not exercise this nested policy. Please use one bounded retry layer, or pass explicit native retry/connect/read timeouts that leave room for all intended outer attempts and final diagnostics.

[github-actions]

The native timeout flags fix the earlier multiplicative/unbounded behavior, but one follow-up remains in this retry-policy thread. In pinned ossutil v1.7.19, both ossUploadFileRetry and ossDownloadFileRetry start with i = 1 and return once i >= retryTimes, so --retry-times=2 permits two total attempts, not an initial attempt plus two retries. That contradicts the new "after 3 attempts" diagnostics and the PR description. Please use 3 if three total attempts are intended, or align the policy text, diagnostics, and transfer-count test to two attempts.

[github-actions]

One timeout-budget follow-up remains in this existing thread. In pinned ossutil v1.7.19, a non-recursive download is not a single retried request: downloadSingleFile first runs the independently retried ossGetObjectStatRetry HEAD, then the independently retried ossDownloadFileRetry GET; the later upload has its own independently retried PUT. With three attempts, 10-second connection and 30-second read limits, and the native sleeps, the foreground envelope can be about 122 + 126 + 126 = 374 seconds, exceeding this step's 300-second limit. GitHub can therefore kill the step during PUT before all intended attempts or the final ::error::. Please bound the whole HEAD + GET + PUT sequence, or adjust the per-phase limits and step budget so the advertised policy and terminal diagnostic fit. Sources: https://github.com/aliyun/ossutil/blob/v1.7.19/lib/cp.go#L2498-L2504, https://github.com/aliyun/ossutil/blob/v1.7.19/lib/cp.go#L2535-L2538, https://github.com/aliyun/ossutil/blob/v1.7.19/lib/command.go#L658-L675, https://github.com/aliyun/ossutil/blob/v1.7.19/lib/cp.go#L2219-L2246, and https://github.com/aliyun/ossutil/blob/v1.7.19/lib/cp.go#L2606-L2633.

[shuke987]

/review

[github-actions]

Automated review summary

Overall: Request changes. The content-hash guard is focused and protects every remote update visible before the verification read; the current head also removes the previous multiplicative retry loop and long native timeouts. One issue remains in the existing retry-policy thread: pinned ossutil v1.7.19 treats --retry-times=2 as two total attempts, while both new diagnostics and the PR description promise an initial attempt plus two retries. No duplicate inline thread is proposed; the precise follow-up is attached to the existing P2 discussion.

Critical checkpoints:

• Goal and proof: Capturing the validated download hash, skipping unchanged local auth, re-reading the selected OSS object, and comparing before upload implement the stated conflict-aware write-back goal. A sequence where the first two native attempts fail but a third would succeed disproves the stated three-attempt policy. The workflow parsed as YAML and the changed shell step passed bash -n; no build or live workflow execution was attempted under the review-only contract.
• Scope and clarity: The code change stays within the existing auth setup/finalization lifecycle in one workflow and reuses the selected object and existing OSS client. The only clarity mismatch is the retry count exposed by the diagnostics and PR text.
• Concurrency: There are no in-process threads or locks. For concurrent jobs or operator reseeds, every completed update visible to the verification GET changes the hash and prevents the stale upload. A change after that GET and before the unconditional PUT can still be overwritten, exactly the disclosed non-atomic limitation.
• Lifecycle and error handling: A successful auth step establishes CODEX_HOME, the selected object, a validated local credential, and the original digest. Unchanged auth skips remote I/O; malformed local auth and failed remote reads/writes fail before an unsafe upload; the temporary remote file is mode 0600 and trap-cleaned. Later review failure or timeout intentionally still permits persistence.
• Conditions and parallel paths: Auth setup failure, unchanged auth, changed auth, visible remote conflict, review failure, cancellation best effort, remote deletion/read failure, and write failure were traced. The custom code-review commit status records whether review landed, while a later required persistence failure separately makes the Actions run red. No functionally parallel auth write-back path was missed.
• Configuration, compatibility, and propagation: No Doris configuration, dynamic reload behavior, function/storage/protocol format, rolling-upgrade contract, or FE/BE variable propagation changes apply.
• Tests and results: No Doris regression, BE, FE, or .out result is applicable to this workflow-only change. The PR reports YAML parsing, shell syntax checks, mocked decision paths, and pinned-client option validation, but that validation missed the retryTimes total-attempt semantics; a transfer-count mock would cover the gap.
• Observability: Conflict, unchanged, read-failure, write-failure, and successful-upload paths have credential-safe diagnostics. The current "after 3 attempts" errors are inaccurate with value 2 and should be aligned with the chosen policy.
• Persistence, crash behavior, and performance: Visible remote conflicts conservatively win; failed verification never writes; failed persistence turns the workflow red. Hashing and transfers are constant-size work, and the explicit native timeouts bound the prior pathological behavior. The remaining issue is one fewer retry than promised, not excessive work.
• Remaining checkpoints: No static-initialization, memory ownership, Doris transaction/EditLog, data-write atomicity, storage visibility, or runtime security-review conclusion applies to this CI-only diff. No additional correctness or coverage issue survived the final sweep.

User focus: No additional review focus was provided; the entire PR was reviewed.

Review completion: Complete after convergence on the frozen ledger and comment set. Zero new inline comments are submitted because the only accepted point substantially continues the existing retry-policy thread.

[shuke987]

/review

[github-actions]

Automated review summary

Overall: Request changes. The content-hash conflict check is focused and the current head now uses one bounded native retry layer with three total attempts. One P2 remains: the supposedly private remote verification copy is replaced by ossutil's 0664 SDK temp file and may be left behind on failed streaming copies.

Critical checkpoints:

• Goal and proof: Recording the validated original hash, skipping unchanged local auth, re-reading the selected object, and uploading only when the remote still equals the original implement the stated conflict-aware write-back table. The remaining final-read-through-upload race is the PR's explicit non-atomic limitation. The implementation still mishandles the downloaded credential's filesystem mode and cleanup, as described inline.
• Scope and clarity: The change stays focused on auth setup and write-back in one workflow and reuses the existing selected object and OSS client.
• Concurrency: There are no in-process threads or locks. Same-original jobs, staggered jobs, operator reseeds/deletes, byte-identical ABA, and native upload retries were traced. Every completed remote change visible to the verification read is preserved; only changes after that read can be overwritten, matching the disclosed limitation.
• Lifecycle and error handling: A successful auth step establishes CODEX_HOME, the selected object, validated auth bytes, and the original digest. Unchanged auth skips remote I/O; malformed local auth and failed reads/writes fail loudly; visible conflicts skip safely. The remaining lifecycle defect is that chmod protects only the empty placeholder, not the SDK temp file that contains the credential or can survive a streaming failure.
• Conditions and parallel paths: Auth setup failure, review success/failure, cancellation best effort, unchanged local state, remote conflict, remote deletion/unreadability, and upload failure were checked. The custom code-review status intentionally records whether the review landed, while later persistence failure separately makes this non-required Actions run red. No functionally parallel auth write-back path was missed.
• Configuration and compatibility: No Doris configuration, dynamic reload behavior, storage/protocol format, function symbol, rolling-upgrade contract, or FE/BE variable propagation changes apply. Pinned ossutil v1.7.19 now uses three total native attempts with explicit 10/30-second timeouts, resolving the existing retry-policy thread without a duplicate comment.
• Tests and results: No Doris regression, BE, FE, or .out result applies to this workflow-only diff. I independently parsed the YAML and ran bash -n on the changed shell block; both passed. The PR reports mocked decision/retry paths, but no committed test exercises the pinned SDK's actual destination mode and partial-file cleanup. No build or live workflow mutation was attempted under the review-only contract.
• Observability and security: Existing messages cover unchanged, conflict, read failure, write failure, and successful upload without logging credential contents. CI is outside the Doris runtime vulnerability model, but this workflow still handles live access and refresh tokens and should not leave them mode 0644.
• Persistence, crash behavior, and performance: Remote mismatch conservatively wins and bounded transfer failures make the run red. Hashing and two small transfers are constant-size work. Failed streaming currently can leave the SDK temp credential behind; the inline remediation covers both successful and failed downloads.
• Remaining checkpoints: No static initialization, Doris memory ownership, transaction/EditLog, database write atomicity, storage visibility, data result, or runtime function-system issue applies.

User focus: No additional review focus was provided; the entire PR was reviewed.

Review completion: Complete after three convergence rounds on the frozen ledger and payload. All other candidates were verified, dismissed, or deduplicated. One inline comment is submitted; the resolved retry-policy thread is not repeated.

[github-actions]

[P2] Keep the downloaded credential copy private

This chmod only applies to the empty placeholder. Pinned ossutil v1.7.19 uses aliyun-oss-go-sdk v3.0.2, whose small-file GetObjectToFile writes to filePath + ".temp" with FilePermMode=0664 and then renames that file over the destination; on a streaming error it can leave the temp file behind. With the runner's default 0022 umask, the current OSS auth (including access and refresh tokens) therefore becomes 0644, while this trap removes only the placeholder/final path. Please download inside a 0700 temporary directory, or set umask 077 and clean both the destination and SDK temp path; a chmod only after success would still leave the failure path exposed.

[shuke987]

/review

[github-actions]

Automated review summary

Overall: Request changes. The current head implements the focused conflict-aware auth write-back and resolves the existing retry-policy and temporary-file-permission threads. One P2 remains: both now-fatal ossutil transfers discard stdout, which is where the pinned client emits its terminal failure reason.

Critical checkpoints:

• Goal and proof: Recording the validated original hash, skipping unchanged local auth, re-reading the selected OSS object, and uploading only when the remote still equals the original implement the stated decision table. The disclosed final-GET-to-PUT race remains the only concurrency limitation. The workflow parsed as YAML and both changed Bash blocks passed bash -n; no build or live workflow mutation was attempted under the review-only contract.
• Scope and clarity: The diff stays within the existing auth setup/finalization lifecycle in one workflow and reuses the selected object and pinned client. The hash and state gates are small and readable.
• Concurrency: There are no in-process threads or locks. Same-original jobs, operator reseeds/deletes, byte-identical ABA, and ambiguous PUT retries were traced. Every completed change observed by the verification GET is preserved; only changes after that GET can be overwritten, matching the PR's explicit non-atomic limitation.
• Lifecycle and error handling: Auth-step success establishes CODEX_HOME, the selected object, validated auth bytes, and the original digest. Unchanged auth skips remote I/O; malformed local auth and transfer failures fail; visible remote conflicts skip safely. umask 077 and the two-path EXIT trap now protect and remove the SDK destination and .temp file. The remaining error-handling gap is the lost client cause described inline.
• Conditions and parallel paths: Ordinary review failure and the review-step timeout still reach persistence when auth setup succeeded; failed/skipped auth cannot enter it. Forced cancellation remains best effort and is unchanged from the base last-position finalizer. The usage-limit .context write is separate, and no parallel auth write-back path was missed.
• Configuration, compatibility, and propagation: No Doris configuration, dynamic reload behavior, FE/BE variable, function/storage/protocol format, or rolling-upgrade contract changes apply.
• Tests and results: No Doris regression, BE, FE, or .out test applies to this workflow-only change. The PR reports mocked unchanged/refreshed/conflict paths and pinned-client/SDK checks; static YAML and shell syntax checks passed. No checked-in workflow test covers the fatal diagnostic path identified inline.
• Observability and security: Unchanged, conflict, and success paths are credential-safe and clear, but fatal GET/PUT paths erase the OSS error code/request ID or network cause. CI is outside the Doris runtime threat model, so this is an operational correctness/diagnosis issue rather than a Doris vulnerability.
• Persistence, crash behavior, and performance: Remote mismatch conservatively wins, transfer errors make the Actions job fail, and the existing custom review status remains scoped to whether the review landed. Three total native attempts with 10/30-second limits fit the five-minute step; hashing and one conditional GET of a small auth file are negligible.
• Remaining checkpoints: No static initialization, Doris memory ownership/nullability, transaction/EditLog, storage visibility/delete bitmap, database write atomicity, or function-system issue applies.

User focus: No additional review focus was provided; the entire PR was reviewed.

Review completion: Complete after two convergence rounds on the frozen head, ledger, and one-comment set. All other candidates were verified, dismissed with evidence, or deduplicated against existing threads.

[github-actions]

[P2] Preserve the OSS failure reason

Both guarded transfers now redirect stdout, but pinned ossutil v1.7.19 prints its terminal Error: ... through stdout (source), including the OSS error code/request ID or the concrete network cause. On a failed verification or upload, the log therefore retains only this generic annotation, so an operator cannot distinguish a missing object, rejected credentials, a timeout, or an OSS service failure even though this PR makes the step fatal. Please retain the client output on failure, or capture it privately and emit a credential-safe diagnostic only when the command fails; successful copies can remain quiet.

[shuke987]

/review

[github-actions]

Automated review summary

Overall: Request changes. The content-hash guard is focused and the current head resolves the temporary-file-permission and suppressed-client-output threads. Two P2 issues remain: the five-minute budget still cannot cover the pinned client's independently retried HEAD, GET, and PUT phases, and structurally invalid refreshed local auth fails the newly fatal finalizer without any diagnostic.

Critical checkpoints:

• Goal and proof: Capturing the validated original hash, skipping unchanged local auth, rereading the selected OSS object, and uploading only when the remote still matches implement the stated decision table. The disclosed final-read-to-upload race remains the only cross-job overwrite window. The current retry budget and silent local-validation path prevent the failure behavior from being fully proved. I parsed the workflow as YAML and ran bash -n on the changed sync block; both passed. No build or live workflow mutation was attempted under the review-only contract.
• Scope and clarity: The diff remains small and focused on the existing auth setup/finalization lifecycle in one workflow, reusing the selected object and pinned client.
• Concurrency: There are no in-process threads or locks in the changed shell. For concurrent jobs or operator reseeds, every completed remote update visible to the guarded read is preserved; only updates after that read can be overwritten, matching the PR's accepted non-atomic limitation.
• Lifecycle and error handling: A successful auth step establishes CODEX_HOME, the selected object, validated original bytes, and the original digest. Unchanged auth skips remote I/O; malformed files and failed transfers refuse unsafe upload; visible conflicts conservatively win. The bare final jq predicate is now fatal but silent for syntactically valid schema failures, as described inline.
• Conditions and parallel paths: Auth failure cannot enter write-back, while review failure or timeout can still persist a refresh. The custom code-review status intentionally records whether a review landed; a later persistence failure separately makes this non-required Actions run red. The verification cp internally performs retried metadata and data reads before the separately retried upload, so the five-minute finalizer can still be killed before its final diagnostic; the existing retry thread has the detailed follow-up.
• Configuration, compatibility, and propagation: No Doris configuration, dynamic reload behavior, FE/BE variable, function/storage/protocol format, or rolling-upgrade contract changes apply.
• Tests and results: No Doris regression, BE, FE, or .out test applies to this workflow-only diff. The PR reports YAML parsing, shell checks, mocked decision paths, and pinned-client validation, but the available evidence does not cover the silent false jq predicate or the client's sequential HEAD + GET + PUT retry envelope.
• Observability and security: Native ossutil output is now retained, and umask 077 plus the two-path trap keeps the SDK destination/temp files private and cleans ordinary exits. The remaining local-validation failure has no credential-safe reason. Under threat-model.md, CI is OUT-OF-MODEL: unsupported-component for Doris vulnerability triage; these are operational correctness findings, not Doris runtime vulnerabilities.
• Persistence, crash behavior, and performance: Remote mismatch wins and transfer errors fail the Actions run. Hashing and successful small transfers are negligible, but timeout-heavy HEAD, GET, and PUT phases can exceed 300 seconds and be terminated before retry completion. No Doris transaction, EditLog, storage visibility, delete-bitmap, data-write atomicity, static-initialization, memory-ownership, or function-system checkpoint applies.

User focus: No additional review focus was provided; the entire PR was reviewed.

Review completion: Complete after two convergence rounds on the frozen head, ledger, one new inline comment, and one existing-thread follow-up. All other candidates were dismissed with concrete evidence or deduplicated.

[github-actions]

[P2] Report invalid refreshed auth before failing

This predicate exits 1 without printing anything when Codex leaves syntactically valid JSON but changes the auth mode or leaves either required token empty. Since this sync is now fatal, the Actions log then contains only the generic process exit; neither OSS diagnostic below runs, and the ephemeral credential file cannot be inspected afterward. Please wrap this validation in an explicit conditional and emit a credential-safe error identifying invalid refreshed auth before exiting.