fix(slack): bound reconnect-path and socket-write network calls with timeouts

[howie]

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1524555735294410812

What problem does this solve?

Continuation of #1281, which was auto-closed by the stale-contributor bot for inactivity before its final round of review feedback could be addressed. All prior review rounds (Claude + Codex mob review, @dogzzdogzz's consolidated Claude+Codex review) had converged to LGTM on the core fix; this PR rebases those commits onto current main and additionally addresses the one unresolved finding from the last review round.

Original problem (from #1279): get_socket_mode_url and tokio_tungstenite::connect_async in the Slack Socket Mode reconnect loop (crates/openab-core/src/slack.rs) had no timeout, unlike the post-connect idle watchdog (IDLE_TIMEOUT_SECS) and SlackAdapter::new's Web API client. A stalled network path during reconnect parked the reconnect task in .await forever — no error, no log, process alive but the bot permanently dark on Slack until manually restarted. This caused an 11+ hour production outage on openab-viola.

Closes #1279

What's carried over from #1281 (unchanged, already LGTM'd)

1. Wrap get_socket_mode_url(...) in tokio::time::timeout(35s) (RECONNECT_HTTP_TIMEOUT_SECS), set strictly above the adapter client's own 30s timeout so the diagnosable inner error fires first.
2. Wrap connect_async in tokio::time::timeout(30s) (RECONNECT_TIMEOUT_SECS) via connect_with_reconnect_timeout().
3. Reuse SlackAdapter's bounded reqwest::Client in get_socket_mode_url instead of constructing an unbounded one per call (folds in Slack: get_socket_mode_url creates new reqwest::Client per reconnect #386).
4. Extract wait_backoff_or_shutdown() to deduplicate the backoff+shutdown select! block that was copy-pasted at three retry points.
5. Regression test connect_async_timeout_fires_on_stalled_handshake (start_paused = true against a local TcpListener that never completes the handshake).

New in this PR (addresses #1281's last review round)

@dogzzdogzz's consolidated review on #1281 flagged that the reconnect handshake was bounded, but the live socket loop still had unbounded writes — the same bug class as #1279, just on the write side:

• slack.rs: envelope ack send, pong send, keepalive ping send, shutdown close send were all unbounded write.send(...).await. If any of these stalls on the same flaky/half-open socket that motivated bug(slack): get_socket_mode_url / connect_async in the reconnect loop have no timeout, unlike the post-connect idle watchdog #1279, the task parks inside that select! branch — and since it can never return to ping_interval.tick(), IDLE_TIMEOUT_SECS can't fire either.

Fix: extracted send_with_timeout() (bounded by a new WRITE_TIMEOUT_SECS = 10) and used it at all four call sites; each now breaks to force a reconnect on timeout, matching the existing keepalive-ping error-handling pattern.

Intentional behaviour change — ack timeout now skips routing: on an envelope ack-send timeout (or send error) the loop breaks before the event is routed/dispatched, whereas previously the ack result was ignored and the event was routed regardless. This is deliberate and safer: Slack redelivers an un-acked envelope on the next connection, so skipping dispatch avoids double-processing an envelope Slack still considers pending. Called out explicitly so it isn't later mistaken for an oversight.

Also addressed from the same review round:

• Added a regression test for the previously-untested HTTP timeout leg (get_socket_mode_url_with_timeout), using a stalled local listener with reqwest::Client::builder().resolve(...) to redirect the slack.com host, mirroring the existing WS handshake test.
• Added ConnectResult / SocketModeUrlResult type aliases for the nested Result return types (readability nit).
• Aligned the ack and pong send sites to the keepalive-ping site's 3-arm match (Ok(Err(e)) / Err(_) / Ok(Ok(()))) so a completed-but-erroring send (a real tungstenite::Error, e.g. broken pipe) forces a reconnect instead of being silently swallowed by an outer-.is_err()-only check.

Validation

• cargo build -p openab-core passes
• cargo test -p openab-core --lib slack — 66/66 tests pass (including the 2 new regression tests)
• cargo clippy -p openab-core --all-targets — no new warnings (4 pre-existing warnings in cron.rs/discord.rs are unrelated)
• cargo fmt -p openab-core -- --check — no new formatting diffs introduced by this change (pre-existing unformatted regions elsewhere in the file are untouched)

Supersedes

Closes/supersedes #1281 (auto-closed for inactivity, all review rounds there had converged to LGTM on the core fix).

[dogzzdogzz]

🟡 Non-blocking suggestions

1 · Ack/pong send sites swallow non-timeout send errors (both reviewers)
send_with_timeout returns Result<Result<(), tungstenite::Error>, Elapsed>. The ack (slack.rs:884) and pong (slack.rs:1220) sites check only .is_err() on the outer result, so a completed-but-erroring send (a real tungstenite::Error, e.g. broken pipe) is silently ignored and the loop proceeds as if it succeeded. The keepalive-ping site (slack.rs:1253) does it right — it 3-way matches Ok(Err(e)) / Err(_) / Ok(Ok(())). Not a regression (matches the old let _ = write.send()), but since this PR is specifically about tightening these sends, consider aligning ack/pong to the ping site's match pattern (or a one-line "best-effort; read/ping side catches a dead socket" comment).

2 · Undocumented behaviour change on ack timeout (infrabot)
On an ack-send timeout (slack.rs:882-897) the code now breaks before event routing/dispatch — everything from slack.rs:899 on is skipped. Previously the ack result was ignored and the event was routed regardless. This is likely the safer behaviour (Slack redelivers an un-acked envelope on reconnect, avoiding a double-process), but it's an intentional change that isn't called out in the PR description — worth a line so it isn't later mistaken for an oversight.

🟢 Info / follow-ups (out of scope for this PR)

• Write-side timeout is untested (both) — send_with_timeout (4 sites) is the PR's primary new behaviour; the 2 new regression tests cover only the reconnect legs. Low risk (structurally symmetric to the tested connect_with_reconnect_timeout); deterministically forcing TCP write-backpressure is hard. A stalled-sink test would close the gap. Fine to merge as-is.
• Pre-existing WSS-URL logging (infra-Patrick) — the unchanged info!(url = %ws_url) line logs the full Socket Mode URL, which embeds a short-lived auth ticket. Out of scope here; follow-up only.
• Rollout watch (infrabot) — first production exercise of these bounds after the 11h outage; worth a glance at reconnect-frequency metrics post-deploy, though 30/35/10s are generous vs. PING_INTERVAL_SECS=30 / IDLE_TIMEOUT_SECS=75.

Correctness questions — confirmed clean

1. 35s > 30s ordering / race? No race. SlackAdapter::new sets .timeout(30s) covering the whole request cycle incl. resp.json().await; same tokio clock with a 5s margin → the inner reqwest error fires first. 35s is correctly scoped as a backstop for the fallback path where .build() fails to an unbounded Client::new(). The new HTTP test targets exactly that bare-client scenario.
2. break on write timeout / in-flight state? Safe — falls through to the same reconnect/backoff path as the pre-existing ping-failure break. Only real change is the ack-timeout-skips-routing behaviour noted above.
3. 10s shutdown-close truncating handshake? No — Message::Close(None) is a local buffer write+flush; tungstenite doesn't wait for the peer's close-ack here, before or after. Worst-case shutdown latency goes unbounded → 10s, strictly better.

Also verified: all 4 previously-unbounded write sites now route through send_with_timeout (no unbounded write remains in the live loop); Cargo.toml's new [dev-dependencies] tokio test-util is necessary (main dep uses full, which excludes test-util); wait_backoff_or_shutdown is a faithful, behaviour-preserving extraction across all 3 call sites.

[chaodu-agent]

Note

LGTM ✅ — Solid, well-tested fix for a real production outage. Addresses all prior review feedback from #1281.

What This PR Does

Bounds all previously-unbounded network calls in the Slack Socket Mode reconnect loop and live socket write path with explicit timeouts, preventing stalled connections from permanently parking the adapter task (the root cause of an 11+ hour production outage on openab-viola, #1279).

How It Works

1. Reconnect path: connect_async bounded by RECONNECT_TIMEOUT_SECS (30s), get_socket_mode_url bounded by RECONNECT_HTTP_TIMEOUT_SECS (35s — strictly above the inner client timeout for better diagnostics).
2. Live socket writes: new send_with_timeout() helper (10s) wraps all four write sites (envelope ack, pong, keepalive ping, shutdown close). Timeout or error → break to reconnect, consistent with the existing keepalive-ping error-handling pattern.
3. Refactors: wait_backoff_or_shutdown() deduplicates three copy-pasted backoff+shutdown blocks; ConnectResult/SocketModeUrlResult type aliases tame nested generics; get_socket_mode_url reuses the adapter's bounded reqwest::Client instead of constructing an unbounded one per call (folds in Slack: get_socket_mode_url creates new reqwest::Client per reconnect #386).
4. Intentional behavior change: ack-send failure now skips event dispatch (correct — Slack redelivers unacked envelopes, so this avoids double-processing).

Findings

#	Severity	Finding	Location
1	🟢	Correct timeout layering: outer HTTP timeout (35s) > inner client timeout (30s), ensuring the diagnosable inner error fires first	slack.rs:718-723
2	🟢	Consistent 3-arm match pattern at all write sites prevents silently swallowing tungstenite::Error	slack.rs (all send_with_timeout call sites)
3	🟢	Excellent regression tests using start_paused = true + stalled local listeners — pins the actual production code path	slack.rs (test module)
4	🟢	Clean extraction of wait_backoff_or_shutdown eliminates copy-paste divergence risk at three retry points	slack.rs:776-783
5	🟢	Deliberate ack-skip-on-timeout documented in PR body and code comments, preventing future misidentification as a bug	slack.rs:883-908

What's Good (🟢)

• Production-motivated: directly traces to a real 11-hour outage with clear repro scenario
• Defense in depth: bounds both the reconnect path and the live socket loop writes — same bug class, complete coverage
• Testable design: extracted helpers (connect_with_reconnect_timeout, send_with_timeout, get_socket_mode_url_with_timeout) are unit-testable in isolation
• Minimal blast radius: only touches slack.rs and its dev-dependencies; no cross-crate changes
• Well-documented: thorough PR description explains every decision, calls out the intentional behavior change, and references prior review rounds

Baseline Check

• PR opened: 2026-07-08
• Main already has: idle-timeout watchdog (IDLE_TIMEOUT_SECS), backoff loop structure, reqwest::Client with 30s timeout in SlackAdapter::new
• Net-new value: timeout coverage for connect_async, get_socket_mode_url, and all four write.send() sites — none of which had any timeout on main

5️⃣ Three Reasons We Might Not Need This PR

1. Tokio-tungstenite or reqwest could add native timeouts upstream — but they haven't in years, and a production outage already happened. Waiting is not an option.
2. The idle-timeout watchdog should catch half-open sockets — but as the PR correctly identifies, a stalled write in a select! branch prevents the tick from ever firing. The existing watchdog has a blind spot this PR closes.
3. The original fix(slack): bound reconnect-path network calls with a 30s timeout #1281 was already LGTM'd and could just be rebased — this PR is that rebase, plus it addresses the one remaining finding (unbounded writes) that the last review round identified. Strictly better than a raw rebase.

[chaodu-agent]

Note

LGTM ✅ — Well-structured timeout + write-bounding fix for a real 11-hour production outage, with proper regression tests.

What This PR Does

Fixes #1279: the Slack Socket Mode reconnect path (get_socket_mode_url, connect_async) and the live socket write calls (ack, pong, keepalive ping, shutdown close) had no timeout bounds. A stalled network path during reconnect or a half-open socket on the write side could park the adapter task forever with zero observability. This caused an 11+ hour outage on openab-viola.

How It Works

1. Reconnect-path timeouts: connect_with_reconnect_timeout() wraps connect_async in a 30s timeout; get_socket_mode_url_with_timeout() wraps the HTTP call in a 35s timeout (set above the client's own 30s so the diagnosable inner error fires first).
2. Write-path timeouts: send_with_timeout() bounds all four outbound write sites with a 10s timeout (WRITE_TIMEOUT_SECS). Each site now breaks to force reconnect on timeout or send error, matching the existing keepalive-ping error-handling pattern.
3. Client reuse: get_socket_mode_url now accepts the adapter's bounded reqwest::Client instead of constructing an unbounded one per call (folds in Slack: get_socket_mode_url creates new reqwest::Client per reconnect #386).
4. Deduplication: wait_backoff_or_shutdown() replaces three copy-pasted backoff+shutdown select! blocks.
5. Deliberate behavior change: ack-send timeout/error now skips event dispatch (breaks before routing) — correct because Slack redelivers unacked envelopes, avoiding double-processing.
6. Type aliases: ConnectResult, SocketModeUrlResult, SlackWsSink for readability.
7. Tests: 4 new regression tests covering both timeout legs, wait_backoff_or_shutdown happy path and shutdown path, all using start_paused = true for instant virtual-clock resolution.

Findings

#	Severity	Finding	Location
1	🟢	Timeout hierarchy is well-designed: HTTP 35s > client 30s > WS handshake 30s > write 10s — each layer fires before its outer backstop	constants
2	🟢	Extracting send_with_timeout / connect_with_reconnect_timeout / wait_backoff_or_shutdown into named functions both improves readability and makes the exact production code path testable	slack.rs
3	🟢	The ack-skip-dispatch behavior change is explicitly documented in the PR description and is semantically correct (Slack's at-least-once delivery guarantees safe redelivery)	slack.rs ack site
4	🟢	3-arm match pattern (Ok(Err(e)) / Err(_) / Ok(Ok(()))) at all write sites ensures both completed-but-errored sends (e.g. broken pipe) and timeouts trigger reconnect, rather than being silently swallowed	all write sites
5	🟢	dev-dependencies addition of tokio = { version = "1", features = ["test-util"] } is properly scoped and doesn't affect production builds	Cargo.toml

Baseline Check

• PR opened: 2026-07-09 (rebases fix(slack): bound reconnect-path network calls with a 30s timeout #1281, which was auto-closed by stale bot)
• Main already has: IDLE_TIMEOUT_SECS + PING_INTERVAL_SECS idle-watchdog for the connected read loop (added in fix(slack): detect half-open Socket Mode via ping + idle-timeout reconnect #1120). No timeout protection in the reconnect path or on outbound writes.
• Net-new value: Bounds all previously-unbounded network calls (2 in reconnect path, 4 in write path), reuses the adapter client (Slack: get_socket_mode_url creates new reqwest::Client per reconnect #386), deduplicates backoff logic, adds 4 regression tests. Addresses the exact production outage scenario from bug(slack): get_socket_mode_url / connect_async in the reconnect loop have no timeout, unlike the post-connect idle watchdog #1279.

What's Good (🟢)

• Complete coverage of the unbounded-call surface: both the reconnect path (HTTP + WS handshake) and the live socket write path (ack, pong, ping, close) are now bounded.
• Tests exercise the actual extracted production functions rather than duplicating the timeout logic, making them true regression guards.
• The wait_backoff_or_shutdown extraction removes a common async footgun (copy-pasting select! blocks that could drift out of sync).
• PR description is exceptionally thorough — documents the intentional behavior change, links to prior art, and explains the timeout hierarchy.
• CI: check passes, all smoke tests pass, 66/66 unit tests pass including the 2 new ones.

5️⃣ Three Reasons We Might Not Need This PR

1. The original fix(slack): bound reconnect-path network calls with a 30s timeout #1281 was already LGTM'd — This is functionally a rebase + one additional fix (write-side timeouts). However, fix(slack): bound reconnect-path network calls with a 30s timeout #1281 was auto-closed and could not be merged, so a new PR is the only path forward.
2. Write-side stalls might be rare in practice — The original incident was a reconnect-path stall, not a write-path stall. But the PR correctly identifies that write stalls are the same bug class and would produce the same symptom (task parked forever, idle watchdog unreachable), so defense-in-depth is warranted.
3. A liveness probe would catch future variants — True, but a liveness probe is a separate concern (detect + restart) vs. this fix (self-heal via reconnect). Both are needed; this PR addresses the root cause rather than papering over it with restarts.