Skip to content

Fix gateway recovery epoch replacement boundary - #101

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/ibkr-replacement-identity-epoch
Closed

Fix gateway recovery epoch replacement boundary#101
Pigbibi wants to merge 2 commits into
mainfrom
codex/ibkr-replacement-identity-epoch

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • capture and remove the old gateway container before creating a replacement
  • fail closed unless the replacement has a different immutable container ID and valid StartedAt
  • classify only replacement-ID logs at or after that epoch, with sticky terminal veto and readiness-first success
  • ignore pre-epoch and untimestamped lines; remove ambiguous dialog dismissal from progress evidence

This is a fresh reslice from main; it does not reuse or reopen frozen PRs #99/#100.

Tests

  • all 7 shell test scripts
  • python3 -m py_compile 2fa_bot.py scripts/*.py
  • bash -n for scripts/tests/container override
  • actionlint
  • focused shellcheck --severity=warning
  • git diff --check

Safety

Code/CI only. No deployment, live restart/recreate, Scheduler invocation, /run, /dry-run, secrets, account, or trading changes were performed.

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: 2 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: The prior blocking issue is fixed in this diff: the initial epoch is now captured via inspect_current_container and passed into wait_for_ready_with_progress on the first readiness check. But the current blocking findings still stand. First, scripts/recover_ib_gateway_ready.sh defines default_terminal_regex='Connection reset by peer|Server disconnected|...', and classify_ib_gateway_epoch_activity.awk always returns terminal if any epoch log line matches terminal_regex, even when later lines match progress_regex; tests/test_gateway_replacement_epoch_policy.sh explicitly locks in that behavior with replacement_terminal_sticky and comma_fraction_terminal, proving a generic disconnect in the current epoch vetoes later login progress despite the comment promising terminal handling only for authentication failure. Second, replace_gateway_container restart unconditionally does docker compose stop then docker compose rm -f before up -d, so the first recovery stage is a destructive replacement, not an in-place restart; the same script comment says first-run/login flows can legitimately take minutes and should not have their progress interrupted, so deleting the container on the first recovery attempt still discards that epoch’s in-container state instead of preserving it for a true restart. The current findings do not contradict the prior finding’s required behavior; they identify separate remaining defects.

🚫 Blocking Issues

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

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

default_terminal_regex treats any Connection reset by peer or Server disconnected line in the current epoch as terminal. Those patterns are not scoped to authentication/login failure, so a transient network disconnect anywhere in startup becomes a sticky terminal classification and blocks every progress-based extension even if the replacement is still making valid login progress. (line 18)

Suggestion: Restrict terminal vetoes to unambiguous authentication/login failure messages, or split generic disconnects into a separate non-terminal signal that does not override later progress.

2. 🟠 [HIGH] Reliability in scripts/recover_ib_gateway_ready.sh

The new replace_gateway_container restart path always runs docker compose stop followed by docker compose rm -f before up -d, so the first recovery attempt is no longer a restart at all: it destroys the existing container and all of its ephemeral in-container state. That can turn a slow but recoverable first-run/login/config flow into repeated cold starts and permanent non-readiness. (line 160)

Suggestion: Keep the first retry as a true in-place docker compose restart and reserve destructive remove/recreate for the final stage, or explicitly persist any state that must survive the first retry before deleting the container.


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: 15ad6bbe39

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

}

line_timestamp = canonical_timestamp(line_timestamp)
if (line_timestamp == "" || line_timestamp < epoch_timestamp) {

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 Don't drop same-second file log events

When /home/ibgateway/2fa.log or launcher.log emits its normal YYYY-MM-DD HH:MM:SS timestamp during the same second as a replacement whose Docker StartedAt has fractional seconds, canonical_timestamp pads the log line to .000000000, so this < epoch_timestamp check skips it even if it happened after the container started. That makes post-replacement progress or terminal evidence in that first second invisible and can prevent the intended wait extension or terminal veto.

Useful? React with 👍 / 👎.

line_timestamp = ""
if ($0 ~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z[[:space:]]/) {
line_timestamp = $1
} else if ($0 ~ /^[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?[[:space:]]/) {

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 Support IBC launcher millisecond timestamps

When /home/ibgateway/Jts/launcher.log contains IBC's millisecond format YYYY-MM-DD HH:MM:SS:mmm IBC: ... as seen in upstream logs (example), this condition never captures a timestamp because it only allows a dot fraction or whitespace after seconds. Those file-log progress/terminal lines are classified as none, so replacement recovery can skip the intended extension or terminal veto whenever the relevant evidence is only in launcher.log.

Useful? React with 👍 / 👎.

@@ -0,0 +1,61 @@
function canonical_timestamp(value, fraction, digits) {
if (value ~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z$/) {

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 Avoid mawk-incompatible timestamp regexes

On hosts where plain awk resolves to mawk (common on Ubuntu/Debian images), this interval regexp with an optional group aborts the classifier with a REcompile() - panic error; because recover_ib_gateway_ready.sh invokes awk directly, replacement log classification then produces no activity instead of detecting progress or terminal events. Use a POSIX-compatible pattern or require gawk so recovery does not silently skip the new epoch policy on those hosts.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

PERMANENT_FREEZE / HUMAN_REQUIRED

Absolute-final technical adjudication: all three unresolved P2 findings are valid. No further changes, reruns, thread resolution, bypass, merge, or live operations will be performed on this PR.

Evidence retained:

  • second-only file timestamps normalize below a fractional StartedAt, so valid first-second post-start events can be dropped;
  • upstream IBC logs use YYYY-MM-DD HH:MM:SS:mmm, which the current classifier does not accept;
  • the exact interval/optional-group expression reproduces a mawk REcompile() - panic on the Debian-derived gateway image, even though the current VM host resolves awk to gawk.

Separate live recovery completed naturally with a successful systemd result. This PR was never deployed.

@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