Skip to content

[fix](ci) Protect refreshed Codex auth updates - #66349

Open
shuke987 wants to merge 7 commits into
apache:masterfrom
shuke987:codex/auth-sync-guard
Open

[fix](ci) Protect refreshed Codex auth updates#66349
shuke987 wants to merge 7 commits into
apache:masterfrom
shuke987:codex/auth-sync-guard

Conversation

@shuke987

@shuke987 shuke987 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

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.

The hash comparison also has no generation ordering. If concurrent jobs start from the same auth and both refresh it, one job may observe the other job's upload and skip its own potentially newer refresh. This rare lost-refresh case is explicitly accepted: the immediate priority is protecting operational reseeds, while distinguishing refresh generations would require authoritative version/ownership metadata or serialization and would expand this PR beyond its intended scope.

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.
  • Verified that digest-command failures propagate through all SHA-256 pipelines with pipefail.
  • Budgeted the 173-minute job for 153 minutes of pre-finalization work, 8 minutes of auth sync, and 12 minutes of runner/post-job overhead.
  • Ran git diff --check.

### What problem does this PR solve?

Issue Number: None

Related PR: apache#66319

Problem Summary: Code review jobs always uploaded their local auth.json snapshot at the end of a run. A job that started before another job or an operator updated the OSS auth object could therefore overwrite the newer credentials. Record the original auth hash, sync after successful auth configuration so refreshes survive non-auth review failures, and upload only when the local file changed while the current OSS object still matches the original snapshot.

### Release note

None

### Check List (For Author)

- Test: Manual test
    - Parsed the workflow YAML, syntax-checked the modified Bash blocks, and validated unchanged, refreshed, manual-reseed, and remote-read-failure scenarios
- Behavior changed: Yes; unchanged or stale auth snapshots are no longer uploaded
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
Contributor

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

shuke987 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/workflows/code-review-runner.yml Outdated
@shuke987

shuke987 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

shuke987 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/workflows/code-review-runner.yml Outdated
@shuke987

shuke987 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/workflows/code-review-runner.yml Outdated
@shuke987

shuke987 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/workflows/code-review-runner.yml Outdated
@shuke987

shuke987 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review summary

Overall: Request changes. The current head resolves the earlier retry-count, temporary-file privacy, OSS-output, and invalid-auth diagnostic threads, and its unchanged/visible-conflict/fatal-transfer branches are otherwise focused. Three P2 concerns remain: two new inline correctness issues and one job-budget follow-up in the existing retry-policy thread.

Critical checkpoints:

  • Goal and proof: Capturing the validated original digest, skipping unchanged auth, rereading OSS, and writing only when that digest still matches implements the intended decision table on ordinary paths. It is not sufficient to prove safe persistence: a digest-command failure is converted to success by the implicit shell, and a remote mismatch contains no ordering information and can suppress the newest refresh.
  • Scope, clarity, and reuse: The diff is limited to the existing auth setup/finalization lifecycle in one workflow and reuses the selected object and pinned OSS client. The remediation needs an explicit shell failure contract and an authoritative per-object ordering/ownership mechanism rather than additional hash heuristics.
  • Concurrency and atomicity: There are no in-process threads or locks. The explicitly disclosed case in which two jobs both verify the original before either writes already existed in the base path and is not repeated as a finding. Distinctly, when A uploads O1 before later-refreshed B performs its verification read, the new mismatch branch discards B's O2 even though the base finalization order would have landed O2; M4 covers that introduced state-loss ordering.
  • Lifecycle, conditions, and error handling: Auth-step success establishes the selected object, validated local JSON, and original digest; review failure or timeout can still reach persistence, while failed auth cannot. Unchanged, invalid-local, visible-conflict, remote-read failure, write failure, and successful upload paths were traced. M5 shows that all three new digest pipelines can mask their left-hand failure under the workflow's implicit bash -e shell and route a failed check through a successful ordinary branch.
  • Configuration, compatibility, and propagation: No Doris runtime configuration, dynamic reload, FE/BE variable propagation, function symbol, 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, bash -n, successful decision-path mocks, and pinned-client validation; no build or live workflow run was attempted under the static-review contract. The evidence does not cover digest-command failure or concurrent token-generation ordering, and those cases should be added to the workflow mocks.
  • Observability and security: Native OSS errors, invalid refreshed JSON, conflict, unchanged, and success paths now have credential-safe diagnostics, and umask 077 plus trap cleanup protects both temporary paths. A masked digest failure instead produces a misleading successful conflict/unchanged path. CI is outside the Doris runtime threat model, so these are operational correctness findings, not Doris vulnerability conclusions.
  • Persistence, crash behavior, and performance: M4 can leave an older or revoked credential generation in OSS; failed transfers otherwise make the Actions run red. The eight-minute step contains the pinned client's approximately 374-second native HEAD+GET+PUT envelope, but the enclosing 165-minute job remains budgeted for 153 minutes before this finalizer and a documented 12-minute runner/post-job reserve. The existing retry thread has the non-duplicate follow-up explaining that the new cap consumes eight minutes of that reserve.
  • Remaining checkpoints: No static-initialization, Doris memory ownership/nullability, transaction/EditLog, delete-bitmap, storage visibility, data-result, or function-system issue applies. The custom code-review status intentionally reports whether the review landed; later auth-persistence failure remains visible through the separate Actions result.

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

Review completion: Complete after three rounds converged on the frozen current head, ledger, two-inline-comment set, and one existing-thread follow-up. Every other candidate was verified, dismissed with concrete evidence, or deduplicated.

Comment thread .github/workflows/code-review-runner.yml
Comment thread .github/workflows/code-review-runner.yml
@shuke987

shuke987 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: Review step was failure (possibly timeout or cancelled)
Workflow run: https://github.com/apache/doris/actions/runs/30798797972

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

@shuke987

shuke987 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review summary

Overall: Request changes remains appropriate. The current head fixes the earlier digest-failure masking, declared job budget, temporary-file privacy, invalid-auth diagnostic, and suppressed OSS-output concerns. No new inline thread is appropriate, but two correctness limits remain in existing discussions: generation-less conflict arbitration in r3700996217, and the retry/final-diagnostic contract in r3695278189, now updated with the pinned SDK's reset-on-each-I/O timeout behavior.

Critical checkpoints:

  • Goal and proof: The original/local/remote digest gates implement the stated unchanged, refreshed, visible-conflict, read-failure, and write-failure decision table on ordinary paths. Safe persistence is not fully proved: hash inequality cannot order concurrent refresh generations, and the native read timeout is an idle timeout rather than a whole-transfer deadline, so a slow-progressing GET can outlive the eight-minute finalizer before all intended attempts or its explicit error.
  • Scope, clarity, and reuse: The diff is small and confined to the existing auth setup/finalization lifecycle in one workflow. It reuses the selected OSS object and pinned client; the remaining fixes need an authoritative ordering/ownership mechanism and a wall-clock-bounded retry layer rather than additional hash or step-cap arithmetic.
  • Concurrency and atomicity: There are no in-process threads or locks. Multiple jobs and operator reseeds share one OSS object. Updates visible before the verification read are conservatively preserved, but a mismatch has no generation ordering and the read-to-write pair is not atomic. Those cases are already covered by r3700996217 and the PR's disclosed post-read limitation, so they are not duplicated inline.
  • Lifecycle and error handling: Successful auth setup establishes CODEX_HOME, the selected object, validated original auth, and its digest. Review failure or usage-limit handling can still reach persistence; failed auth cannot. Invalid local auth, digest failure, remote read failure, and write failure fail loudly. umask 077 and the two-path EXIT trap protect the SDK destination and .temp file. The remaining timeout gap can still let GitHub terminate the step before the shell emits its terminal diagnostic.
  • Conditions and parallel paths: Direct dispatch, reusable calls, review success/failure, usage limit, cancellation best effort, unchanged local auth, valid refresh, remote conflict, deleted/unreadable remote state, and failed upload were traced. The custom code-review status intentionally records whether the review landed; later auth-persistence failure remains represented by the Actions conclusion.
  • Configuration, compatibility, and propagation: The updated workflow timeout is static and internally sums to 153 + 8 + 12 = 173. No Doris runtime configuration, dynamic reload, FE/BE variable propagation, function symbol, storage/protocol format, or rolling-upgrade contract changes apply.
  • Tests and results: No Doris regression, BE, FE, or .out result applies to this workflow-only diff. Under the static-review contract, no build or live workflow execution was attempted. The PR reports YAML parsing, shell checks, and mocked decision paths; the outstanding coverage gap is a slow-progress transfer demonstrating that the SDK's per-I/O timeout is not a wall-clock bound.
  • Observability and security: Invalid auth, unchanged auth, visible conflict, OSS read/write failure, and successful upload have credential-safe diagnostics, and native OSS output is preserved. CI is OUT-OF-MODEL: unsupported-component for Doris vulnerability triage under threat-model.md; the credential findings here are workflow correctness and operational hardening concerns.
  • Persistence, crash behavior, and performance: Hashing and successful small transfers are negligible, and failed transfers make the Actions run fail. Generation ambiguity can retain an older/revoked refresh, while slow progress can consume the entire finalizer without completing the advertised retries or diagnostic. No Doris transaction/EditLog, storage visibility, delete-bitmap, database write atomicity, memory ownership, nullability, static initialization, or function-system checkpoint applies.

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

Review completion: The code/comment candidate set converged in Round 2 on the unchanged head/base, nine-comment context, shared ledger, zero-new-inline set, and one existing-thread reply. The exact payload audit in Round 3 found material event and completion-language corrections. Those corrections are applied to this submission, but the exact final payload could not reconverge within the three-round cap, so the review cycle is incomplete under that cap. No new code finding emerged in Round 3, and every code candidate was independently verified, dismissed with concrete evidence, or deduplicated against an existing discussion.

@shuke987
shuke987 dismissed stale reviews from github-actions[bot], github-actions[bot], github-actions[bot], github-actions[bot], github-actions[bot], and github-actions[bot] August 4, 2026 04:31

Superseded by fixes on the current head; the remaining low-probability concurrency and slow-progress transfer limitations are explicitly accepted in the PR scope.

@shuke987
shuke987 dismissed github-actions[bot]’s stale review August 4, 2026 04:31

Superseded by fixes on the current head; the remaining low-probability concurrency and slow-progress transfer limitations are explicitly accepted in the PR scope.

@shuke987
shuke987 marked this pull request as ready for review August 4, 2026 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants