Repository navigation
fix(ibkr): contain superseded connect attempts - #1681
mehdijamshidian90 wants to merge 2 commits into
Conversation
|
@mehdijamshidian90 is attempting to deploy a commit to the luokerenx4's Team Team on Vercel. A member of the Team first needs to authorize it. |
When IB Gateway restarts during IBKR auto-recovery, a new waitForConnect() can start on the shared EClient before the previous attempt has opened its socket. The superseded attempt then tore down its successor's connection, and once its own TCP connect completed it resumed with a null connection and left a rejected handshake promise unobserved. Node treats that as fatal, so the UTA process exited with "Connection closed before handshake started". EClient.connect() now owns the Connection it opens: after each await it checks that the client still points at it, and if not, it closes only that socket with the wrapper detached. The handshake promise is observed as soon as it exists. RequestBridge.waitForConnect() lets only the latest attempt disconnect the shared client or clear the nextValidId waiter.
…rapper A superseded connect attempt's socket can still answer, close, or time out after its successor connects. Connection reported a late close straight to the shared wrapper, so RequestBridge marked the healthy successor dead. EClient now hands each Connection a wrapper that forwards error() and connectionClosed() until a newer connect() opens a replacement. Teardown of the current connection still reports, including handleReaderError(), which resets the client before closing the socket. Adds RequestBridge cases for a stale attempt whose handshake succeeds, closes, or times out after the successor connected, an EClient case for a reader failure on the current connection, and moves the EClient replaced-attempt spec next to base.ts per the current test layout.
6ca1944 to
5901ab3
Compare
|
Updated per the review on #1679. The branch is rebased on current Test layout
Late success / reject / timeout
Each case asserts that:
Against Fix: Where overlapping connects come from (observation only, not changed here)
So during a Gateway outage, a read while an attempt is in flight starts a second concurrent Local verification on the updated head (Windows 11, Node 24.19.0)
Live paper check on the updated head (connection only, no orders)
The restart run's attempts were sequential, so it does not by itself prove that the overlapping-connect race was hit live. That race is covered by the deterministic specs above and by the read-only overlapping-connect check from the PR description, re-run on this head: the first Automatic UTA respawn is untouched, as scoped. I also updated the PR description to match the current head. |
Problem
When IB Gateway restarts while a UTA is already in IBKR auto-recovery, recovery can start a new
waitForConnect()on the sharedEClientbefore the previous attempt has finished opening its socket. The two attempts then interfere through shared state.client.disconnect()inRequestBridge.waitForConnect) runs after its successor has already replacedEClient.conn. It destroys the successor's socket, and itsconnectionClosed()rejects the successor withConnection to TWS/Gateway closed during handshake. The successor'sConnection.connect()never settles, so that attempt hangs.EClient.connect()resumes withthis.conn === null,waitForHandshake()rejects immediately, andthis.conn.sendMsg()throws beforeawait handshake. The rejected handshake promise is never observed, and Node terminates the UTA process:In a real session (Windows 11, IB Gateway paper,
pnpm dev), the UTA log showed a run ofsuperseded/closed during handshakerecovery failures, then this crash. Guardian loggedexited (code=1) — optional service offline, continuing, and the account stayed offline until the UTA was restarted by hand. IB Gateway's daily auto-restart can set up the same sequence without anyone at the machine.Closes #1679
Approach
Make every connect attempt own the resources it created, in both layers:
EClient.connect()keeps theConnectionit opened in a local variable and checks, after eachawait, that the client still points at it. If a newerconnect()or adisconnect()/reset()replaced it, the attempt closes only its own socket, with the wrapper detached, and returns.waitForHandshake()now takes that connection explicitly. Its promise is observed as soon as it is created, so no later synchronous throw can leave a rejection unobserved.EClientwrapper ownership. EachConnectionreports through a wrapper that forwardserror()/connectionClosed()until a newerconnect()opens a replacement. So a superseded socket that answers, closes, or times out late cannot mark its successor dead. Teardown of the current connection still reports, includinghandleReaderError(), which resets the client before closing the socket.RequestBridge.waitForConnect()numbers its attempts. Only the latest attempt maydisconnect()the shared client or clear thenextValidIdwaiter. A superseded attempt still rejects withPrevious TWS/Gateway connection attempt was superseded, as before.Tradeoffs:
CONNECT_FAILthrough the shared wrapper. That error used to be reported against the attempt that replaced it, which was misleading.docs/ibkr-wire-protocol.mdrecords the ownership rule next to the existing handshake containment rule.Out of scope:
optional service offline, continuingand, unlike the Connector (armConnectorRecovery), does not respawn it. With this fix the crash above no longer happens, but automatic UTA respawn could still be worth a separate change.UnifiedTradingAccount.nudgeRecovery()can start a second concurrentbroker.init()while a recovery attempt is in flight (details in the review thread). This PR contains the effect at the IBKR boundary and leaves that behaviour alone.Evidence
Regression tests
tests/integration/ibkr-request-bridge/request-bridge.spec.ts:waitForConnect()calls against a local TCP server. Without the fix, an unhandledConnection closed before handshake startedis raised and the successor never settles. With the fix:nextValidId700);connectionDead, and the wrapper must receive noconnectionClosed()/error(). Ondevall three fail, because the overlap itself breaks there. The gateway-side close case also fails without the wrapper-ownership change.packages/ibkr/src/client/base.connect-replaced-attempt.spec.ts:error/connectionClosed, and its socket is closed.Live checks (read-only, IB Gateway paper on port 4002, separate client ids, no orders)
Overlapping connects. The
devcolumn is from the original submission; the branch column is re-run on the updated head.devwaitForConnect()callsConnection closed before handshake startedGateway restart on the updated head. A read-only
UnifiedTradingAccountaroundIbkrBrokerwas watched while IB Gateway was shut down and started again as a new process, with a manual login.offline, then four recovery attempts reacheddownwhile the Gateway was down.readable, and an account read succeeded.Commands run on the updated head (Windows 11, Node 24.19.0, pnpm 11.7.0)
packages/ibkr,services/uta, roottsc --noEmit, andtests/tsconfig.json.pnpm exec vitest run packages/ibkr/src/client tests/integration/ibkr-request-bridge tests/integration/ibkr-reader-recovery: 30 passed.pnpm test:integration:uta: pass.pnpm test:select --package @traderalice/ibkr: 98 passed.pnpm test: 7552 passed, 17 failed in 14 files. None of the failures are inpackages/ibkror the UTA broker code. The same 14 files also fail on a cleanorigin/devworktree on this machine (17 failed there too).