Skip to content

fix(live): disarm sideband connect watchdog on the pre-opened path - #4496

Merged
lidge-jun merged 1 commit into
devfrom
codex/260913-live-sideband-connect-timer
Sep 13, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/260913-live-sideband-connect-timer

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 13, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Fixes a regression introduced by 321b9b1 (pre-opened live sideband upstream): attachLiveSidebandUpstream arms the 10s liveConnectTimer whenever liveMaxSessionMs is set, but the only non-teardown clear site is the upstream "open" listener, which can never fire for a socket that was already OPEN at attach time.
  • Net effect on current dev: every dictation (DICTATION_SESSION_MAX_MS) and live-call (LIVE_CALL_TTL_MS) audio session is force-closed with "audio connection timed out" exactly 10 seconds after attach.
  • Fix: on the successful pre-opened takeover path, disarm the connect watchdog exactly as the open listener does. The session timer (whole-session bound) stays armed.
  • Found by the 2.53.0 release regression audit (parallel commit audit of origin/main..origin/dev). Release blocker for 2.53.0; 2.52.0 is unaffected (pre-open flow is not on main).

Verification

  • Local suite/build/typecheck: NOT RUN (maintainer rule for this task; hosted exact-head CI is the gate).
  • Root cause verified against HEAD source: src/server/index.ts:744 (arm), :813 (only non-teardown clear, unreachable for pre-opened sockets), :429/:519 (teardown-only clears).
  • Regression test added: tests/server/server-live.test.ts "disarms the connect watchdog when the upstream was pre-opened" — asserts the connect timer is cleared on the takeover path while the session timer stays armed. No new test file, so no layout.json change.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed live sessions being closed prematurely when the upstream connection was already open before attachment.
    • Ensured the connection watchdog is cleared correctly while the overall session lifetime limit remains active.
  • Tests

    • Added coverage for pre-opened upstream connections and timer behavior.

Since 321b9b1 the upgrade handler pre-opens the sideband upstream and
hands it to attachLiveSidebandUpstream already OPEN. attach arms the 10s
liveConnectTimer whenever liveMaxSessionMs is set, but the only non-teardown
clear site is the upstream "open" listener, which can never fire for a
socket that opened before the client existed. Every dictation and live-call
session was therefore force-closed with "audio connection timed out" exactly
ten seconds after attach. Disarm the watchdog on the successful takeover
path, mirroring the open listener, and cover it with a regression test.

Found by the 2.53.0 release regression audit (parallel commit audit).
Local suite NOT RUN per maintainer rule; hosted exact-head CI is the gate.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 13, 2026 09:38
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-13T09:41:41.963312Z 4b0a62b PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 13, 2026
@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 4493f2da-5536-4152-bd35-842e0f393f50

📥 Commits

Reviewing files that changed from the base of the PR and between 981b53e and 4b0a62b.

📒 Files selected for processing (2)
  • src/server/index.ts
  • tests/server/server-live.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The pre-opened upstream path now clears the connect watchdog during attachment. The session timer remains active. A server test verifies both timer states and the admission release on close.

Changes

Live sideband watchdog

Layer / File(s) Summary
Pre-opened upstream timer handling
src/server/index.ts, tests/server/server-live.test.ts
At src/server/index.ts:793-799, the pre-opened upstream branch clears and undefines liveConnectTimer. It leaves liveSessionTimer armed. The test at tests/server/server-live.test.ts:1879-1899 verifies these states and checks that closing the connection emits one admission release.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4b0a6

The fix prevents valid sessions from timing out after 10 seconds while preserving the overall session limit; no merge-blocking risk is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: disarming the live sideband connect watchdog on the pre-opened upstream path. This matches the implementation and regression test.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/260913-live-sideband-connect-timer

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.

@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: 4b0a62b215

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

Comment thread src/server/index.ts
Comment on lines +798 to +799
if (ws.data.liveConnectTimer !== undefined) clearTimeout(ws.data.liveConnectTimer);
ws.data.liveConnectTimer = undefined;

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 Update the mapped structure contract for the timer change

This changes the live-sideband timer semantics in src/server/index.ts without updating any of the structure documents mapped to src/server/; in particular, structure/runtime.md#live-sideband-handshake owns this handshake contract but does not record that a successful pre-opened takeover disarms the connect watchdog while retaining session expiry. Update the mapped structure documents in this same change so the maintained contract reflects the runtime behavior.

AGENTS.md reference: src/AGENTS.md:L11-L11

Useful? React with 👍 / 👎.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration record (MAINTAINERS.md dev exception): integrating as repository owner without a second maintainer approval. No outstanding maintainer change requests. Exact-head verification: all required checks green on head 4b0a62b (ci aggregator pass, gates/hygiene/enforce-target/docker smoke/api usage pass; windows/macos-control skipped by changes-gate design; CodeRabbit completed with comments reviewed — no correctness findings). Local suite NOT RUN per task rule; hosted exact-head CI is the gate. This fix unblocks the 2.53.0 promotion (release-audit P1).

@lidge-jun
lidge-jun merged commit 248670e into dev Sep 13, 2026
31 checks passed
@lidge-jun
lidge-jun deleted the codex/260913-live-sideband-connect-timer branch September 13, 2026 09:55
luvs01 pushed a commit to luvs01/opencodex that referenced this pull request Sep 13, 2026
Product tree is dev at eb81eaa, byte-identical (dev already carries the
2.53.0 version line).

This promotion follows a 364-commit regression audit (origin/main..981b53e)
by 15 parallel subagent lanes plus two audit-spawned fixes reviewed and merged
to dev: lidge-jun#4496 (live sideband connect watchdog) and lidge-jun#4497 (chat image part
shapes). Zero unresolved P0/P1 at promotion time. Exact-head hosted CI green
on eb81eaa (run 34750934849). Local suite NOT RUN per task rule; hosted
exact-head CI is the gate.
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
…deband-connect-timer

fix(live): disarm sideband connect watchdog on the pre-opened path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant