shuke987 commented on PR #66119: URL: https://github.com/apache/doris/pull/66119#issuecomment-5101461496
Re-review of head `c3a9f5087e5bb1dbc4d01b809a33eda92eea2235`: the stable account ordering, terminal-error parsing, OSS secret isolation, shared-state serialization, and side-effect check are good improvements. The following functional issues still remain: 1. **Refresh-token failures still do not rotate accounts.** `classify_failure()` only recognizes a parsed usage-limit timestamp, HTTP status codes, and transient network text. Actual Codex errors such as `refresh token was revoked`, `refresh token has expired`, and `refresh token was already used` normally contain no HTTP 401, so they are classified as `fatal` and the retry loop exits without selecting another account. Even a failure classified through HTTP 401 is automatically made available again after one day, although a revoked/expired refresh token cannot recover by waiting. These credentials should remain disabled until the corresponding OSS auth object changes (for example, a new ETag/content hash/version). 2. **Refreshed ChatGPT credentials are discarded.** This revision removes the final auth sync to OSS. Codex persists any access/refresh token returned by a successful refresh into its local `auth.json`; when the runner exits, that update is lost and OSS still contains the old refresh token. A later review can therefore load an already-consumed refresh token and fail with `refresh token was already used`. This needs a trusted credential broker or another isolated mechanism that persists refresh results without trusting an arbitrary agent-written auth file. 3. **The repository-global concurrency group can drop review requests.** With `group: code-review-codex-auth-rotation` and the default queue behavior, GitHub keeps at most one running and one pending run; a newer queued run replaces the existing pending run even though `cancel-in-progress` is false. Because one review may run for 120 minutes, `/review` requests can be silently canceled under normal load. `queue: max` would at least preserve pending runs, but serializing the entire review also prevents multiple credentials from providing concurrent throughput. Prefer a short transactional allocation/update critical section or per-account leases instead of locking the whole workflow. 4. **Common same-day usage-limit messages are still classified as fatal.** The regex requires a full timestamp such as `Aug 2nd, 2026 1:27 AM`, but Codex formats a reset later on the current day as only `1:27 AM`, and emits `Try again later` when no reset is available. Classification should recognize the canonical usage-limit message independently of whether the reset timestamp can be parsed, then parse full-date/time-only variants and use a safe fallback when needed. 5. **Reset recovery remains lazy rather than scheduled.** `recover_expired_accounts()` only runs during `select`/`record`; there is no scheduled recovery or delayed requeue. When every account is unavailable, the current review exits instead of resuming at `next_retry_at`. If the requirement is specifically to reset accounts on schedule, that lifecycle is still missing. Because items 1 and 2 can permanently break credential rotation, and item 3 can lose review requests, I still recommend holding the PR until these are addressed. -- 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]
