Skip to content

revert: websocket close timeout collector fix - #16

Merged
proerror77 merged 1 commit into
mainfrom
codex/revert-collector-close-timeout
Jul 14, 2026
Merged

revert: websocket close timeout collector fix#16
proerror77 merged 1 commit into
mainfrom
codex/revert-collector-close-timeout

Conversation

@proerror77

@proerror77 proerror77 commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Why

Production rollout disproved the PR #15 hypothesis: USD-M still hit TaskCancellationStuck pending=1 and restarted after 11 minutes with close_timeout=1.

Action

Evidence

  • python3 -m unittest deployment/aliyun/test_binance_lob_archiver.py (29 passed)
  • python3 -m py_compile deployment/aliyun/binance_lob_archiver.py deployment/aliyun/test_binance_lob_archiver.py
  • production Spot: 1357 synced, gaps=0, pending=0
  • production USD-M: 573 synced, gaps=0, pending=0

Summary by CodeRabbit

  • Bug Fixes
    • Improved WebSocket connection shutdown behavior by removing the fixed close-timeout setting.
    • Retained existing connection timeout, keepalive, and message-size limits.

…ket-close-timeout"

This reverts commit 16251f8, reversing
changes made to 207de07.

@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: 1a3989cfb7

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

close_timeout=1,
ping_interval=20,
max_size=8 * 1024 * 1024,
url, open_timeout=20, ping_interval=20, max_size=8 * 1024 * 1024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep websocket close under cancellation budget

When a receiver is cancelled during shutdown or reconnect, exiting this async with connect(...) must complete before cancel_tasks_bounded()'s 5-second TASK_CANCEL_TIMEOUT_SECONDS budget; without an explicit close_timeout, websockets uses a 10-second close timeout, so a peer that doesn't complete the close handshake quickly can leave the receiver pending long enough to raise TaskCancellationStuck and force the collector down the fatal restart path instead of rotating cleanly.

Useful? React with 👍 / 👎.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 16821be8-9110-4644-a543-5ced4a8f901e

📥 Commits

Reviewing files that changed from the base of the PR and between 16251f8 and 1a3989c.

📒 Files selected for processing (2)
  • deployment/aliyun/binance_lob_archiver.py
  • deployment/aliyun/test_binance_lob_archiver.py
💤 Files with no reviewable changes (1)
  • deployment/aliyun/test_binance_lob_archiver.py

📝 Walkthrough

Walkthrough

The Binance LOB archiver removes the explicit websocket close_timeout=1 connection option and deletes the unit test that enforced it. Existing open timeout, ping interval, and maximum message size settings remain unchanged.

Changes

WebSocket shutdown configuration

Layer / File(s) Summary
Remove close timeout contract
deployment/aliyun/binance_lob_archiver.py, deployment/aliyun/test_binance_lob_archiver.py
receive_url no longer passes close_timeout=1; the associated cancellation-budget test is removed while other websocket options remain unchanged.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

  • proerror77/monday#6: Addresses related receiver shutdown and websocket close-timeout cancellation behavior.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly indicates this PR reverts the websocket close timeout collector fix, matching the main change set.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/revert-collector-close-timeout

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proerror77
proerror77 merged commit c15589a into main Jul 14, 2026
24 of 25 checks passed
@proerror77
proerror77 deleted the codex/revert-collector-close-timeout branch July 15, 2026 20:23
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