fix: classify gateway recovery epochs in Python - #102
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: The current findings remain valid. In 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Logic in
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efea8091bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SH | ||
| } | ||
| if ! decision="$({ | ||
| if ! docker logs --timestamps --since "${epoch_started_at}" "${epoch_container_id}" 2>&1 \ |
There was a problem hiding this comment.
For the initial health-check path, this reads docker logs from the container's StartedAt, which may be days old for an already-running gateway. If that long-lived container later loses API readiness, any startup-era line such as IBC: Login attempt still makes the classifier return progress, so wait_for_ready_with_progress spends the configured progress extensions before restarting/recreating instead of treating the stale progress as irrelevant; the previous implementation bounded this with IB_GATEWAY_RECOVERY_PROGRESS_WINDOW_SECONDS.
Useful? React with 👍 / 👎.
| exit 42 | ||
| fi | ||
| if [ -f "${log_path}" ]; then | ||
| cat "${log_path}" |
There was a problem hiding this comment.
This now streams the entire launcher and 2FA log files into a classifier that rejects snapshots over 512 events or 256 KiB. On a restarted-but-not-recreated container, those files keep historical entries from prior attempts, so once either log grows past the limit the classifier returns invalid before it can consider fresh current-epoch progress, disabling the progress extension path even during an active login/config flow.
Useful? React with 👍 / 👎.
| PROGRESS_MARKER = re.compile( | ||
| r"IBC: (Starting Gateway|Login attempt|Second Factor Authentication|Login has completed|" | ||
| r"Configuration tasks completed|Found Gateway main window|Getting config dialog|Getting main window)|" | ||
| r"Authentication window found|Auto-fill submitted|Passed token authentication|" | ||
| r"Authentication completed|Security code:", | ||
| re.IGNORECASE, |
There was a problem hiding this comment.
When the only fresh evidence is the 2FA helper clearing a post-login dialog, the classifier now returns none because this marker list omits the existing Dismissing post-login dialog log emitted by dismiss_dialog in 2fa_bot.py. Those dialogs can block Gateway/API readiness after authentication, and the previous recovery regex treated this as progress, so this path can now refuse an extension and proceed to restart/recreate while the helper is actively unblocking login.
Useful? React with 👍 / 👎.
| import sys | ||
| from dataclasses import dataclass | ||
| from datetime import datetime, timezone | ||
| from enum import StrEnum |
There was a problem hiding this comment.
On deployment hosts where python3 is older than 3.11, such as Ubuntu 22.04's default Python 3.10, this import fails before the classifier can return a decision. The wrapper defaults to the host python3, and recovery treats that invocation failure as invalid, so login/config progress extensions are disabled even though the rest of these recovery scripts do not install or require Python 3.11.
Useful? React with 👍 / 👎.
| decision="$(capture_epoch_decision \ | ||
| "${epoch_container_id}" "${epoch_started_at}" "${old_container_id}" "${replacement_identity}")" |
There was a problem hiding this comment.
If the gateway process exits during wait_for_ready, Docker's restart: always policy restarts the same container ID with a new .State.StartedAt, but this still classifies logs using the pre-wait epoch captured before that restart. In that case the snapshot includes events from a dead container run, so stale terminal/progress lines can drive the extension or recovery decision for the wrong epoch instead of re-inspecting and scoping to the currently running attempt.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2179e03ba2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ "${MOCK_SCENARIO}" = terminal ] && [ "$count" -ge 2 ]; then | ||
| printf '%s\n' '2026-07-15T16:38:02.000000000Z IBC closing because login has not completed' | ||
| else | ||
| printf '%s\n' '2026-07-15T16:38:01.000000000Z IBC: Login attempt' |
There was a problem hiding this comment.
When this CI test runs more than 420 seconds after the hard-coded 2026-07-15T16:38:01Z log entry, the recovery script's default IB_GATEWAY_RECOVERY_INITIAL_SNAPSHOT_WINDOW_SECONDS=420 filters this mocked progress out during the initial stage, so the drift scenario never reaches the expected epoch-change path. I ran bash tests/test_gateway_recovery_runtime_contract.sh on July 15, 2026 at 19:29 UTC and it failed for exactly this reason; since .github/workflows/ci.yml now invokes this script, the workflow will stay red unless these timestamps are derived from the test's current time or the window is controlled.
Useful? React with 👍 / 👎.
| if ! docker logs --timestamps --since "${event_not_before}" "${epoch_container_id}" 2>&1 \ | ||
| | sed "s/^/D\\t${epoch_container_id}\\t/"; then |
There was a problem hiding this comment.
When the container emits more than 512 docker-log lines during the current wait window, this unbounded stream exceeds MAX_SNAPSHOT_EVENTS in parse_protocol_snapshot, causing the classifier to return invalid before the recovery code can use any legitimate IBC: Login attempt/auth progress in the same snapshot. The previous grep-based docker-log check could still find progress in a chatty startup, but this path now refuses the extension, so cap/tail or prefilter the docker source before feeding the bounded classifier.
Useful? React with 👍 / 👎.
| def event_lower_bound(self) -> tuple[datetime, int]: | ||
| return parse_rfc3339_nanos(self.event_not_before or self.started_at) |
There was a problem hiding this comment.
When the initial-stage container has restarted within the 420-second snapshot window, event_not_before can be earlier than the current .State.StartedAt, and this property then lets docker/file log lines from the previous process epoch count as current progress. For example, a fresh restart at 16:39 with a retained 16:38:30 IBC: Login attempt line is classified as progress, so recovery can extend instead of acting on the stalled current epoch; the lower bound should be the later of the recency cutoff and started_at.
Useful? React with 👍 / 👎.
|
Closed under main-authorized permanent freeze. Preserved CI evidence: required Run shell tests failed at head 2179e03. Any follow-up must use a fresh origin/main reslice. |
1 participant
Scope
Safety contract
Validation
tests/*.shpython3 tests/test_gateway_recovery_classifier.py(11 tests)python3 -m py_compile 2fa_bot.py scripts/*.pypython3 -m compileall -q 2fa_bot.py scripts tests/test_gateway_recovery_classifier.pybash -n scripts/*.sh tests/*.sh container_overrides/*.shactionlint .github/workflows/ci.ymlshellcheck --severity=warning ...git diff --cached --checkNo Scheduler, Cloud Run, trading, account, secret, deploy, endpoint, restart, or recreate actions were performed.
Review policy: preflight then at most one concentrated closure; no merge/rerun/poll.