Repository navigation
feat(q): q_connect arms TCP keepalive, as the core's client does - #24
Conversation
A q connection whose peer vanished without closing, such as a VPN tunnel reaped without a FIN, waited forever: q_connect set no keepalive, and a subscription that only receives has no request in flight for a timeout to bound. The core solved the same problem for its own client (#589), and this follows it. q_connect_opts is q_connect with a keepalive argument, which means what keepalive_ms means to the core's ray_ipc_connect_opts. Q_KEEPALIVE_AUTO (0) arms keepalive at the core's 60 s default with no bound on unacknowledged data. Q_KEEPALIVE_OFF (-1) arms nothing. A budget in ms arms keepalive at that budget plus TCP_USER_TIMEOUT. A static assert holds the two constants equal to the core's RAY_IPC_KEEPALIVE_*. q_connect now calls it with AUTO, so every caller, .q.connect included, gets keepalive. The core made the same default change for ray_ipc_connect. The options are set through the core's ray_sock_set_keepalive once the handshake succeeds, on the blocking socket, so q_conn_attach keeps them. A failure to arm them is ignored, as in the core: the link is no worse than before. q.c now needs core v2.11.0 or later. driver --keepalive-selftest connects to driver --serve through q_connect, through q_connect_opts with a 4000 ms budget, and with Q_KEEPALIVE_OFF, and reads the options back before and after q_conn_attach against the core's mapping, as the core's test_ipc_keepalive_sockopts does. The budget connection then answers a query. run.sh runs it in the server leg, which needs no q. Tested on macOS and on Linux (Ubuntu 24.04, clang).
…ured INTEGRATING.md said a silent peer is dead about n ms after it went quiet. Measured on Linux 6.12 with its packets dropped, there are two rules. With a budget, TCP_USER_TIMEOUT decides, but the kernel only drops the connection on a probe tick, so it can be up to one interval late: a 25 s budget died at 28.3 s. Otherwise (AUTO, or a budget elsewhere) it is idle + 3 x interval in whole seconds, which can be early: a 10 s budget without the user timeout died at 8.1 s. AUTO died at 60.8 s. A failure to arm was said to leave the connection without keepalive. That holds only when SO_KEEPALIVE itself fails; a tuning option the kernel rejects keeps its system default, as in the core. q.h said the same and is corrected too. The changelog now names the trade-off: an idle connection whose network is down for 30-60 s or more is closed, where it used to resume, and Q_KEEPALIVE_OFF keeps the old behaviour.
The pinned rayforce-q commit, 2cc931c, is the head of the open RayforceDB/rayforce-q#24 and exists only on ihrfv/rayforce-q, so a build that clones the submodule from upstream cannot find it. The URL goes back to upstream, and the pin to the merge commit, once #24 merges.
|
@morsel-ray could you please review the changes? |
morsel-ray
left a comment
morsel-ray
left a comment
There was a problem hiding this comment.
Reviewed exact2cc931c6b9a7b767ad1abcb758eed956781b7b77: complete seven-file delta, connector/attach path, and the core keepalive helper. AUTO/positive/OFF dispatch matches the core contract; the new default is an intentional observable behaviour change, with the interruption-vs-detection tradeoff and best-effort option arming documented. No new hot-path validation requested.
Independently ran the q-free suite against pinned core5d5f47c2 (v2.11.5): codec/exchange/CLI,33 server assertions, AUTO/OFF/4000ms socket-option checks before/after attach, malformed/fragmented-frame controls all pass. Removing only the new keepalive calls in an external counterfactual builds but fails the required socket-option assertions. Exact clean source, core archive and original driver hashes rechecked; whole relevant logs contain no sanitizer diagnostics.
Scope: adapter/driver ASan+UBSan, preexisting uninstrumented release core archive; test-only linker wrapper confined the listener to127.0.0.1. No real-q, packet-loss/deadline/timing, macOS, minimum-v2.11.0 linking or combined#24+#25 execution claim. I used the explicit core SHA, not a floating branch. This does not address the separate trusted-evaluation/handler-dispatch boundary in#26.
Approved for this keepalive change; not merged by me.
How it looks from the user's side
A process subscribed to a q publisher through
.q.connect(or a binding'sq_connect) waits for pushes. When the link between them dies without a FIN (a VPN tunnel is reaped, a NAT drops the flow, the publisher's host is suspended), nothing reaches the subscriber again, and nothing tells it so.Before:
q_connectsets no keepalive. With no request in flight,timeout_msbounds nothing, and the connection waits forever.After: every
q_connectconnection has kernel TCP keepalive at the core's 60 s default, asray_ipc_connectalready has since #589. A silent peer is declared dead and the socket fails. A caller that wants a tighter bound passes a budget to the newq_connect_opts.What changed
Modelled on the core's
ray_ipc_connect_opts(src/core/ipc.h,src/core/ipc.c), so AUTO, OFF and a budget mean the same thing across the engine:q_connect_opts(host, port, user, password, timeout_ms, keepalive_ms)inq.h.keepalive_msfollowsray_ipc_connect_opts:Q_KEEPALIVE_AUTO(0): keepalive at the core'sRAY_IPC_KEEPALIVE_DEFAULT_MS(60 s), with no bound on unacknowledged data.Q_KEEPALIVE_OFF(-1): nothing is armed.n > 0: keepalive at a budget of n ms, plusTCP_USER_TIMEOUT.A
_Static_assertkeeps the two constants equal toRAY_IPC_KEEPALIVE_AUTOandRAY_IPC_KEEPALIVE_OFF.q_connectbecomesq_connect_opts(…, Q_KEEPALIVE_AUTO), asray_ipc_connectdefaults to AUTO. Every caller gets it,.q.connectincluded.The options are set by the core's own
ray_sock_set_keepalive, after the handshake succeeds, on the blocking socket. They surviveq_conn_attach. A failure to arm them is ignored, asray_ipc_connect_optsignores it.q.cnow includescore/ipc.handcore/sock.h, so it needs rayforce core v2.11.0 or later, the first release withray_sock_set_keepalive. On an older core it fails to compile. CI clones core master, which has it.Docs: a Keepalive section and the core requirement in
docs/INTEGRATING.md,README.md, and an[Unreleased]entry indocs/CHANGELOG.md.Tests
driver --keepalive-selftest HOST PORT, run inrun.sh's server leg, so CI covers it without a q binary. It mirrors the core'stest_ipc_keepalive_sockopts: it connects todriver --servethree times and reads the socket options back before and afterq_conn_attach, checking them against the core's mapping:SO_KEEPALIVEq_connectq_connect_opts, 4000 msQ_KEEPALIVE_OFFIdle: read as
TCP_KEEPIDLE, orTCP_KEEPALIVEon macOS.User timeout: read as
TCP_USER_TIMEOUTon Linux, orTCP_RXT_CONNDROPTIME(whole seconds) on macOS, where the core sets that instead.The budget connection then answers
(+ 1 2).make test # macOS, core master d4899d9
make test CC=clang # ubuntu:24.04, core cloned from GitHub master, as CI does
Both pass:
keepalive selftest: ok, with the server round-trip and selftests unchanged. Legs that need a q binary are skipped, as in CI. With the keepalive argument ignored, the selftest fails on every option, e.g.q_connect, connected: SO_KEEPALIVE is 0, want 1.Against a peer that really goes silent
That needs the peer's packets dropped (iptables or tc, as root), so it is not in CI. Measured by hand on Linux 6.12, with
iptablesdropping everything the peer sends:Q_KEEPALIVE_AUTOSo there are two rules, and
docs/INTEGRATING.mdnow states them:TCP_USER_TIMEOUTdecides. The kernel only drops the connection on a probe tick, so it can be up to one interval late: 25 s became 28.3 s.The same again end to end, through a binding:
rayforce-rsconnecting with this branch'sq_connect_opts, the connection attached to a poll as a subscription, andis_alive()polled every 200 ms (Linux 6.12, amd64):is_alive()false afterkeepalive,11seckeepalive,29secA peer that closes, killed 40 times, turned
is_alive()false within the first poll slice every time, as before: keepalive plays no part there.The trade-off, also in the changelog: an idle connection whose network is down for longer than its budget (60 s by default) is now closed, where it used to resume once the network came back.
Q_KEEPALIVE_OFFkeeps the old behaviour.