Skip to content

fix: bound gateway recovery epochs - #103

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/ibkr-gateway-recovery-epoch-reslice
Closed

fix: bound gateway recovery epochs#103
Pigbibi wants to merge 2 commits into
mainfrom
codex/ibkr-gateway-recovery-epoch-reslice

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Fresh production-offline reslice from e001c6d.

  • Initial lower bound is max(StartedAt, fixed assessment lower bound).
  • Revalidates container ID + StartedAt before and after each snapshot; drift starts a bounded new assessment.
  • Terminal returns immediately within the shell orchestrator; progress cannot clear it.
  • Removes GNU date -d and makes CI print each shell test name.

No deploy, restart, trigger, scheduler, trading, account, secret, or live operation was performed.

Validation: all shell tests individually printed and passed; py_compile; compileall; bash -n; actionlint; focused shellcheck; diff-check.

Review policy: one closure maximum; do not merge or rerun.

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

🚫 Merge blocked: 1 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: block is warranted because the cumulative diff shows a real behavior regression in scripts/recover_ib_gateway_ready.sh. The old implementation classified Found Gateway main window, Getting config dialog, Getting main window, and Dismissing post-login dialog as acceptable in-flight progress, but the new snapshot_decision() progress regex no longer matches them. In the new flow, if those are the only recent milestone lines, seen_progress stays false, snapshot_decision() returns none, and assess_stage() treats none the same as terminal/invalid (return 1), which immediately advances to restart or recreate. That directly supports the current finding’s flapping risk. The prior nanosecond-timestamp blocker is unrelated and appears addressed by the new Python timestamp parsers that accept 1-9 fractional digits, so there is no contract conflict.

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in scripts/recover_ib_gateway_ready.sh

The new progress regex dropped several log lines that the previous implementation explicitly treated as successful in-flight startup progress (Found Gateway main window, Getting config dialog, Getting main window, Dismissing post-login dialog). When a slow first-run or post-login flow only emits those messages, snapshot_decision() now returns none and immediately escalates to restart/recreate even though the gateway is still advancing normally. In production this can create recovery flapping and prevent the login/config sequence from ever finishing. (line 63)

Suggestion: Restore the previously accepted non-terminal progress markers in the progress classifier, or otherwise preserve the old behavior for known good login/config milestones so bounded assessments do not treat them as lack of progress.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in scripts/recover_ib_gateway_ready.sh

assess_stage() pins wait_ready() to a specific container ID (assessed_id), but epoch drift is only checked before and after each snapshot, not while the blocking readiness wait is running. If the container is restarted or recreated during wait_ready, the script can sit on the dead epoch for the full stage timeout and then trigger another recovery step against a newer healthy epoch that was already available. (line 80)

Suggestion: Revalidate container ID/StartedAt around or inside the readiness polling loop as well, or keep wait_for_ib_gateway_ready.sh following the logical container name and abort/reassess as soon as the observed epoch changes.


Review by Codex PR Review bot • PR

Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b5afce3beb

ℹ️ 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".

from datetime import datetime, timezone
lower = datetime.fromisoformat(sys.argv[1].replace("Z", "+00:00"))
terminal = re.compile(r"IBC closing because login has not completed|(?:authentication|login).*(?:timeout|timed out|failed)", re.I)
progress = re.compile(r"IBC: (Starting Gateway|Login attempt|Second Factor Authentication|Login has completed|Configuration tasks completed)|Authentication window found|Auto-fill submitted|Passed token authentication|Authentication completed|Security code:", re.I)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve IBC config-flow progress markers

When a first-run Gateway is still moving through IBC configuration, the launcher can emit progress such as IBC: Getting config dialog, IBC: Getting main window, or IBC: Found Gateway main window; the previous recovery guard treated those as reasons to extend instead of restarting. This new snapshot regex no longer matches any of those config/main-window markers, so after the readiness timeout snapshot_decision returns none and the script can restart/recreate the container while IBC is actively completing the setup flow the guard is meant to protect.

Useful? React with 👍 / 👎.

if stage == "initial":
lower = max(start, datetime.now(timezone.utc) - timedelta(seconds=int(window)))
else:
lower = start

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Require recent progress when extending retries

For restart and recreate stages this lower bound is always the container's StartedAt, so each later extension re-scans the entire epoch. A normal early line like IBC: Starting Gateway can keep snapshot_decision returning progress for every configured extension even when no progress occurred during the last wait, delaying recreate/failure by the full extension budget for a stuck gateway; the reassessment should use a recent window bounded by StartedAt, not the full epoch.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Closed under absolute-final permanent freeze. Preserved review evidence: accepted IBC config/main-window progress markers were dropped, risking recovery flapping. No further closure, merge, rerun, or live action.

@Pigbibi Pigbibi closed this Jul 15, 2026
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.

1 participant