fix: recover deadlocked Binance receivers - #6
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe Aliyun Binance archiver now includes configurable process watchdog and bounded task-cancellation controls. It records market-data activity, exits after prolonged stalls, detects stuck shutdown tasks, and documents the recovery behavior. ChangesAliyun process recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketReceiveLoop
participant LAST_DATA_AT
participant run_process_watchdog
participant systemd
WebSocketReceiveLoop->>LAST_DATA_AT: record depth-frame timestamp
run_process_watchdog->>LAST_DATA_AT: check elapsed time
run_process_watchdog->>systemd: exit process after watchdog deadline
sequenceDiagram
participant run_session
participant cancel_tasks_bounded
participant asyncioTasks
participant collect
run_session->>cancel_tasks_bounded: cancel session tasks
cancel_tasks_bounded->>asyncioTasks: await cancellation with timeout
cancel_tasks_bounded-->>collect: raise TaskCancellationStuck
collect->>collect: write fatal health record and re-raise
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca27c24c94
ℹ️ 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".
| await asyncio.gather( | ||
| *tasks, *resync_tasks.values(), return_exceptions=True | ||
| ) | ||
| await cancel_tasks_bounded(tuple([*tasks, *resync_tasks.values()])) |
There was a problem hiding this comment.
When cancel_tasks_bounded times out, it raises here before the existing queue-drain loop runs. In the new stuck-close fatal path, any diff/snapshot frames already read from the websocket and sitting in queue are discarded on process restart instead of being written as archived-only/replayed-unsafe as the following drain logic intends; preserve the timeout error but flush the queued frames before re-raising it.
Useful? React with 👍 / 👎.
| SPOOL_DIR.mkdir(parents=True, exist_ok=True) | ||
| recover_parts() | ||
| LAST_DATA_AT = time.monotonic() | ||
| PROCESS_WATCHDOG_ARMED = True |
There was a problem hiding this comment.
Arming the process watchdog for the entire collector also leaves it active while ArchiveRuntime.rotate() is compressing a segment, even though finalize_segment() is allowed to spend up to ZSTD_TIMEOUT_SECONDS (300s) in zstd. On the full-market services, if a scheduled/shutdown rotation or compression stall applies backpressure long enough that receivers stop reading for more than PROCESS_WATCHDOG_SECONDS (180s), the watchdog takes the os._exit path and drops the in-memory queue instead of letting the normal .part recovery path preserve the segment; disarm or heartbeat the watchdog around intentional long rotations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deployment/aliyun/binance_lob_archiver.py (1)
1031-1031: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: simplify tuple construction.
tuple([*tasks, *resync_tasks.values()])allocates an intermediate list. A direct tuple literal avoids that.♻️ Proposed refactor
- await cancel_tasks_bounded(tuple([*tasks, *resync_tasks.values()])) + await cancel_tasks_bounded((*tasks, *resync_tasks.values()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployment/aliyun/binance_lob_archiver.py` at line 1031, In the task cancellation call, replace the intermediate-list tuple construction around cancel_tasks_bounded with a direct tuple construction combining tasks and resync_tasks.values(), preserving the same elements and ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deployment/aliyun/binance_lob_archiver.py`:
- Line 1031: In the task cancellation call, replace the intermediate-list tuple
construction around cancel_tasks_bounded with a direct tuple construction
combining tasks and resync_tasks.values(), preserving the same elements and
ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12db39ec-5639-4854-a012-dea90b9defae
📒 Files selected for processing (5)
deployment/aliyun/README.mddeployment/aliyun/binance-lob-archiver-spot.envdeployment/aliyun/binance-lob-archiver-usdm.envdeployment/aliyun/binance_lob_archiver.pydeployment/aliyun/test_binance_lob_archiver.py
This was referenced Jul 14, 2026
proerror77
pushed a commit
that referenced
this pull request
Aug 18, 2026
…ine tapes Gate #6 (candidate ad00c63f, invocation 1cad2263b3c74d678d12adbe14d93c0b) failed on two stacked gaps: 1. The trade-parity mode chain had no branch for a finalization-deferred shadow against a finalization-deferred baseline: with no early-closing market, neither side can emit a mature trade inside a 3600-second gate, so byte/field/dedupe parity fail by construction (dedupe requires a non-empty legacy trade set in legacy_overlap mode) and the gate died as continuous_overlap. Route a failed verifier with that emission pair to a new finalization_deferred_deferred_overlap mode: false checks must be confined to the byte/field/dedupe trio, both trade counts must be zero with no only/duplicate trade IDs, and the baseline settlement set must be fully covered with matching shared values; rust-only settlements are tolerated because the production uploader deletes baseline tapes after upload while the shadow retains its own. A fully passing verifier keeps full continuous_overlap parity. The raw verdict and the finalization progression evidence stay fail-closed, and the gate policy admits the new mode with the same shape. 2. The parity verifier reads only legacy tapes still present in the spool, but the production uploader deletes each tape right after upload: the baseline tape holding 56 settlements was deleted at 09:11, long before the 09:42 verification. The gate now hardlinks every lookback-window baseline tape into the evidence directory at observation start and again immediately before verification (hardlinks pin the inode at zero copy cost; multi-GiB copies are not acceptable), and the verifier reads that snapshot. The links are released once the distilled parity verdict is written; on failure the snapshot stays for forensics. Also print the actual false parity checks in the non-adjudicated failure message instead of a static family list. Refs #878
proerror77
added a commit
that referenced
this pull request
Aug 18, 2026
…ine tapes (#927) * polymarket: adjudicate deferred-deferred shadow gates, snapshot baseline tapes Gate #6 (candidate ad00c63f, invocation 1cad2263b3c74d678d12adbe14d93c0b) failed on two stacked gaps: 1. The trade-parity mode chain had no branch for a finalization-deferred shadow against a finalization-deferred baseline: with no early-closing market, neither side can emit a mature trade inside a 3600-second gate, so byte/field/dedupe parity fail by construction (dedupe requires a non-empty legacy trade set in legacy_overlap mode) and the gate died as continuous_overlap. Route a failed verifier with that emission pair to a new finalization_deferred_deferred_overlap mode: false checks must be confined to the byte/field/dedupe trio, both trade counts must be zero with no only/duplicate trade IDs, and the baseline settlement set must be fully covered with matching shared values; rust-only settlements are tolerated because the production uploader deletes baseline tapes after upload while the shadow retains its own. A fully passing verifier keeps full continuous_overlap parity. The raw verdict and the finalization progression evidence stay fail-closed, and the gate policy admits the new mode with the same shape. 2. The parity verifier reads only legacy tapes still present in the spool, but the production uploader deletes each tape right after upload: the baseline tape holding 56 settlements was deleted at 09:11, long before the 09:42 verification. The gate now hardlinks every lookback-window baseline tape into the evidence directory at observation start and again immediately before verification (hardlinks pin the inode at zero copy cost; multi-GiB copies are not acceptable), and the verifier reads that snapshot. The links are released once the distilled parity verdict is written; on failure the snapshot stays for forensics. Also print the actual false parity checks in the non-adjudicated failure message instead of a static family list. Refs #878 * polymarket: harden baseline tape snapshot against rotation, bad names, races Address three Codex P1 review findings on snapshot_legacy_tapes: - Refresh the active-tape hardlink on re-sweeps: when the live active tape rotated between sweeps, relocate the stale snapshot link to a synthetic strict closed-tape name and pin the new active inode, and never link the same inode twice (the verifier would read its rows twice). - Fail closed on malformed rotated baseline tape names, mirroring strict_rotation_name in the parity verifier, instead of silently skipping them. - Fail closed on any hardlink failure, including the uploader winning its deletion race against the sweep, so truncated baseline evidence cannot reach adjudication. Add contract cases for the rotation refresh, the malformed-name failure, a same-name foreign inode, and the simulated mid-link source deletion race. --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
systemd activestate.Root cause evidence
Both services had no network socket and no file growth after approximately 22:17, while systemd remained active. The Python main threads were idle in
epoll_waitwith no scheduled timeout, consistent withrun_sessionhanging while gathering cancelled receiver tasks.Verification
git diff --checkpass.status=synced,sequence_gaps=0.Test plan
python3 -m unittest deployment/aliyun/test_binance_lob_archiver.pypython3 deployment/aliyun/binance_lob_archiver.py --self-testpython3 -m py_compile deployment/aliyun/binance_lob_archiver.pygit diff --checkSummary by CodeRabbit
Bug Fixes
Documentation