Skip to content

#280 ALLOWヘッダでUPDATE未サポート時に re-inviteを返すように変更 - #281

Merged
MasanoriSuda merged 7 commits into
developfrom
bugfix/280-fix-session-refresh-reinvite-fallback
Mar 10, 2026
Merged

MasanoriSuda merged 7 commits into
developfrom
bugfix/280-fix-session-refresh-reinvite-fallback

Conversation

@MasanoriSuda

@MasanoriSuda MasanoriSuda commented Mar 10, 2026 •

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • SIPセッションリフレッシュを強化:リモートのUPDATE対応有無に応じてUPDATEまたは再INVITEを選択し、必要時にローカルSDPを含める再ネゴシエーションに対応。
    • リフレッシュ失敗を通知するイベントを導入し、制御経路へ確実に伝達。
  • Bug Fixes

    • リフレッシュ失敗時の完全なクリーンアップを確実化(タイマー・RTP停止、適切な終了処理)。
  • Tests

    • 成功/失敗パス、UPDATE vs RE-INVITE選択、ACK/BYE処理などを検証するテストを追加/拡張。
  • Documentation

    • RFC準拠の設計/実装計画ドキュメントを追加。

Masanori Suda added 2 commits March 10, 2026 21:07
@coderabbitai

coderabbitai Bot commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

SIP セッション更新でリモートの UPDATE 対応可否を判定し、UPDATE と re-INVITE を切り替える RFC 準拠フローを導入。InviteContext に状態追跡を追加し、失敗時は SessionRefreshFailed を生成してセッション制御経路で完全なクリーンアップと終了を行うようにしました。

Changes

Cohort / File(s) Summary
設計・ドキュメント
virtual-voicebot-backend/docs/steering/STEER-280_fix-session-refresh-reinvite-fallback.md
RFC4028 準拠のフォールバック設計書を追加(背景、再現、ユーザーストーリー、実装計画、テスト、チェンジログ等)。
エントリポイント / イベント転送
virtual-voicebot-backend/src/main.rs
forward_refresh_failure と forward_sip_command_events を追加。SipEvent::SessionRefreshFailed を受けてセッション制御へ転送。SessionOut::SipSendUpdate → SipSendSessionRefresh { expires, local_sdp } に対応するルーティングとテストを追加。
プロトコル定義・ポート
virtual-voicebot-backend/src/shared/ports/sip.rs
SipEvent::SessionRefreshFailed { call_id } を追加。SipCommand::SendUpdate を SendSessionRefresh { expires, local_sdp } に置換し、ローカルSDPを含めうるコマンドに変更。
セッション型定義
virtual-voicebot-backend/src/protocol/session/types.rs
SessionControlIn::SipSessionRefreshFailed(call_id) を追加。SessionOut::SipSendUpdate を SipSendSessionRefresh { expires, local_sdp } に変更(公開シグネチャ変更)。
セッションハンドラ / 状態機械
virtual-voicebot-backend/src/protocol/session/handlers/mod.rs, virtual-voicebot-backend/src/protocol/session/state_machine.rs
SipSessionRefreshFailed 受信時のタイマー/RTP/再生/転送停止・レコーダ停止・アプリ通知・EndReason::Timeout による終了を実装。SessionRefreshDue で SipSendSessionRefresh を発行するよう更新。テスト追加・既存テスト修正。
SIP コア実装
virtual-voicebot-backend/src/protocol/sip/core.rs
UPDATE vs re-INVITE の選択ロジック、InviteContext に allow_update, pending_refresh, remote_target_uri, remote_cseq 等を追加。build_session_refresh_request / build_update_request / build_reinvite_request / build_ack_request とエラー型を実装し、handle_session_refresh_response で 2xx/422/408/481 等を統合処理、CSeq 同期や Min-SE 再試行、失敗時の BYE/終了および SessionRefreshFailed 生成を追加。多くの関連テストを追加・更新。

Sequence Diagram

sequenceDiagram
    participant SessionCtrl as Session Control
    participant SIPCore as SIP Core
    participant Main as Main / Router
    participant Remote as Remote Peer
    participant App as Client App

    Note over SessionCtrl,SIPCore: セッション更新発火 (SessionRefreshDue)

    SessionCtrl->>Main: SessionOut::SipSendSessionRefresh {expires, local_sdp?}
    Main->>SIPCore: 送信要求 (expires, local_sdp)
    SIPCore->>SIPCore: allow_update を確認、method 決定 (Update / re-INVITE)
    SIPCore->>Remote: 送信 UPDATE / re-INVITE
    Remote-->>SIPCore: 応答 2xx / 422 / 408 / その他
    SIPCore->>SIPCore: handle_session_refresh_response()
    alt 2xx
        SIPCore-->>Main: 成功イベント -> SessionCtrl に反映
    else 422 (Min-SE)
        SIPCore-->>Main: min-se 調整と再試行指示
    else 408/失敗
        SIPCore-->>Main: SipEvent::SessionRefreshFailed {call_id}
        Main->>SessionCtrl: SessionControlIn::SipSessionRefreshFailed -> クリーンアップ
        SessionCtrl->>App: AppEvent: EndReason::Timeout
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰🌱 ぼくは小さなウサギ、セッションの道を見てる
UPDATEか再INVITEか、相手の合図を確かめるよ
応答が帰らなければ、タイマーを優しく止めて
RTPも静かに止めて、呼びを閉じるよ
また次の波で会話は続くよ ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed PRタイトルは、ALLOWヘッダを参照してUPDATE未サポート時にre-inviteを使用するという変更内容を正確に反映しており、実装されたメイン機能を明確に説明している。
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/280-fix-session-refresh-reinvite-fallback

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 and usage tips.

@codecov-commenter

codecov-commenter commented Mar 10, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 94.19795% with 85 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
virtual-voicebot-backend/src/protocol/sip/core.rs 94.20% 70 Missing ⚠️
virtual-voicebot-backend/src/main.rs 83.90% 14 Missing ⚠️
...cebot-backend/src/protocol/session/handlers/mod.rs 99.37% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@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: 6c587939e5

ℹ️ 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 on lines +541 to +545
let uri = req
.header_value("Contact")
.map(extract_contact_uri)
.unwrap_or_else(|| req.uri.as_str())
.to_string();

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 Target ACK to response Contact after re-INVITE success

build_ack_request builds the ACK request URI from ctx.req (the stored inbound request) rather than from the refresh response, but this ACK path is used for re-INVITE 2xx handling. When the peer returns a different Contact in the 2xx (e.g., target refresh/failover), the ACK is sent to a stale target and the re-INVITE refresh can fail to complete even though 2xx was received. Use the response (or dialog-updated remote target) when forming ACK destination.

Useful? React with 👍 / 👎.

Comment on lines +551 to +555
"SIP/2.0/{} {}:{};branch={}",
transport,
cfg.advertised_ip,
cfg.sip_port,
generate_branch()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reuse INVITE transaction branch for non-2xx ACK

The ACK builder always generates a new Via branch, while handle_session_refresh_response sends this ACK for all final re-INVITE responses (>=200), including non-2xx like 408/481/500. For non-2xx INVITE responses, ACK needs to correlate with the INVITE client transaction; using a new branch can make stateful intermediaries/UAS treat it as unrelated, leading to repeated error-response retransmits and slower/unstable teardown.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@virtual-voicebot-backend/docs/steering/STEER-280_fix-session-refresh-reinvite-fallback.md`:
- Around line 38-42: The fenced code blocks containing the user story are
missing a language tag which triggers markdownlint MD040; update both
occurrences (the block at the section around the displayed snippet and the other
block at lines ~190-199) to include a language identifier such as text (e.g.,
change ``` to ```text) so the markdown linter stops warning; ensure both code
fences are updated consistently.
- Around line 7-15: The STEER entry with ID STEER-280 (タイトル: refresher=uas 時の
session refresh を Allow ヘッダに従い re-INVITE へフォールバック) should not be created as
Approved; change its Status from "Approved" to "Draft" and remove any wording
that implies review/approval already occurred. Also scan the same document for
other newly added steering entries that are marked "Approved" on creation
(including the nearby entries referenced in the review) and set them to "Draft"
so review/approval happens in subsequent updates.

In `@virtual-voicebot-backend/src/protocol/session/handlers/mod.rs`:
- Around line 687-693: The SessionRefreshDue path currently uses
self.session_out_tx.try_send(...) which may drop the Full error and lose the
only refresh trigger; replace the try_send call with awaiting the bounded
channel via self.session_out_tx.send((self.call_id.clone(),
SessionOut::SipSendSessionRefresh { expires, local_sdp: self.local_sdp.clone()
})).await and handle the Result (e.g., map_err/log and propagate) so the refresh
is never silently dropped; update the code around the SessionRefreshDue handling
in mod.rs to use send().await for the SessionOut::SipSendSessionRefresh message
and ensure proper error handling to avoid mis-triggered SessionTimerFired
disconnects.

In `@virtual-voicebot-backend/src/protocol/session/types.rs`:
- Around line 113-116: The SipSessionRefreshFailed event is not treated as a
terminal transition in next_session_state(), causing sessions to linger despite
teardown/CallEnded being executed in the handler; update next_session_state() to
treat the SipSessionRefreshFailed variant as a terminal/ended state (same
outcome as CallEnded) so the function returns the terminal SessionState for
SipSessionRefreshFailed and lets registry/task cleanup proceed; locate the
SipSessionRefreshFailed enum variant and the next_session_state() match arms and
add or adjust the arm to map SipSessionRefreshFailed -> terminal/CallEnded
state.

In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 2101-2119: In the CSeq overflow branch inside
handle_session_refresh_response (pattern SessionRefreshBuildError::CseqOverflow)
you only send BYE and remove the invite but do not notify upper layers, causing
state inconsistency; mirror the other error path by emitting the same
SessionRefreshFailed event for the given call_id (and any relevant
reason/metadata) after sending the BYE/removing from self.invites and clearing
outbound_call_id; if the codebase uses a specific emitter/collector (e.g.,
pushing into a Vec<SipEvent>, calling an event-emitting method, or invoking the
same path handle_sip_command would use), call that same mechanism so the event
appears to the session layer (ensure you reference
handle_session_refresh_response, SessionRefreshBuildError::CseqOverflow,
build_bye_request, self.invites, outbound_call_id, and SessionRefreshFailed when
adding the emission).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f2692cbc-f5c1-4c74-a4a5-10d8c3df4297

📥 Commits

Reviewing files that changed from the base of the PR and between f5ce292 and 6c58793.

📒 Files selected for processing (6)
  • virtual-voicebot-backend/docs/steering/STEER-280_fix-session-refresh-reinvite-fallback.md
  • virtual-voicebot-backend/src/main.rs
  • virtual-voicebot-backend/src/protocol/session/handlers/mod.rs
  • virtual-voicebot-backend/src/protocol/session/types.rs
  • virtual-voicebot-backend/src/protocol/sip/core.rs
  • virtual-voicebot-backend/src/shared/ports/sip.rs

Comment on lines +7 to +15
| 項目 | 値 |
|------|-----|
| ID | STEER-280 |
| タイトル | refresher=uas 時の session refresh を Allow ヘッダに従い re-INVITE へフォールバック |
| ステータス | Approved |
| 関連Issue | #280 |
| 優先度 | P0 |
| 作成日 | 2026-03-10 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

新規 STEER を承認済み状態で追加しないでください。

この内容だと、同じ PR 内で Draft → Review → Approved の手順を文書上すでに通過したことになります。新規作成時は Draft から始めて、レビュー/承認の更新は後続の人手変更に分けた方が運用と整合します。

As per coding guidelines, 「Create new steering files with Draft status before starting work」「Resolve Review phase feedback and modifications to steering documents (Status: Review)」に反しています。

Also applies to: 74-96

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@virtual-voicebot-backend/docs/steering/STEER-280_fix-session-refresh-reinvite-fallback.md`
around lines 7 - 15, The STEER entry with ID STEER-280 (タイトル: refresher=uas 時の
session refresh を Allow ヘッダに従い re-INVITE へフォールバック) should not be created as
Approved; change its Status from "Approved" to "Draft" and remove any wording
that implies review/approval already occurred. Also scan the same document for
other newly added steering entries that are marked "Approved" on creation
(including the nearby entries referenced in the review) and set them to "Draft"
so review/approval happens in subsequent updates.

Comment on lines +38 to +42
```
As a Voicebot オペレーター
I want to UPDATE 未サポートの SIP 端末・PBX との通話が session timer によって強制切断されないこと
So that 長時間通話が正常に継続できる
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

コードフェンスに言語を付けてください。

markdownlint の MD040 がこの 2 箇所を警告しています。ここは text / rust などを明示すると lint ノイズを消せます。

Also applies to: 190-199

🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 38-38: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@virtual-voicebot-backend/docs/steering/STEER-280_fix-session-refresh-reinvite-fallback.md`
around lines 38 - 42, The fenced code blocks containing the user story are
missing a language tag which triggers markdownlint MD040; update both
occurrences (the block at the section around the displayed snippet and the other
block at lines ~190-199) to include a language identifier such as text (e.g.,
change ``` to ```text) so the markdown linter stops warning; ensure both code
fences are updated consistently.

Comment thread virtual-voicebot-backend/src/protocol/session/handlers/mod.rs Outdated
Comment thread virtual-voicebot-backend/src/protocol/session/types.rs
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs Outdated
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
virtual-voicebot-backend/src/protocol/sip/core.rs (1)

1087-1090: allow_update を最新の Allow で再同期した方が安全です。

allow_update は初回 INVITE でしか更新されていないので、その後の re-INVITE / UPDATE / 2xx で相手の Allow が変わっても SendSessionRefresh の選択が古いまま残ります。今回の目的が「UPDATE 非対応なら re-INVITE に落とす」ことなら、ダイアログ中に受けた最新の Allow を ctx.allow_update に反映しておくと取りこぼしを防げます。

Also applies to: 1433-1445, 1776-1782

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@virtual-voicebot-backend/src/protocol/sip/core.rs` around lines 1087 - 1090,
Update the dialog state to resync ctx.allow_update from the latest incoming
response's Allow header so SendSessionRefresh selection isn't stale: when
handling responses (e.g. the block setting ctx.tx.peer and
ctx.remote_target_uri) extract the response Allow (if present) and set
ctx.allow_update accordingly; apply the same fix in the other handling sites
noted (around the ranges corresponding to lines 1433-1445 and 1776-1782) so
re-INVITE/UPDATE/2xx responses update ctx.allow_update with the most recent
Allow value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@virtual-voicebot-backend/src/main.rs`:
- Around line 495-504: 追加した 2 経路が必ず SessionControlIn::SipSessionRefreshFailed
に変換されることを検証するテストを追加してください;具体的には main.rs の SipEvent::SessionRefreshFailed
ハンドリング(現在 session_registry.get(&call_id).await から
control_tx.send(SessionControlIn::SipSessionRefreshFailed { .. }) している箇所)と
handle_sip_command() 側で即座に SessionRefreshFailed
を返す経路の両方をカバーするテストを用意し、必要ならルーティング→送信処理を forward_refresh_failure
のような小さなヘルパー関数に切り出して routing (SipEvent::SessionRefreshFailed) と
handle_sip_command の両方から呼び出すようにしてそのヘルパーを単体テストできるようにしてください;テストはモック/テスト用
session_registry とセッションの control_tx を使い、受信側が確実に
SessionControlIn::SipSessionRefreshFailed を受け取ることをアサートしてください。

In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 1175-1182: SessionRefreshBuildError::CseqOverflow means
ctx.local_cseq is already u32::MAX so calling build_bye_request() will produce a
duplicate CSeq; fix by not creating/sending a BYE on CSeq overflow: in the
Err(SessionRefreshBuildError::CseqOverflow) branch clear ctx.pending_refresh,
set action.bye_payload = None (do not call build_bye_request()), keep
action.events.push(SipEvent::SessionRefreshFailed { call_id: call_id.clone() })
and action.remove_invite = true; alternatively (preferred) make
build_bye_request() return Result and let next_local_cseq()/build_bye_request()
return an error that you propagate so the overflow case fails without producing
a BYE (apply same treatment to the analogous branch around next_local_cseq usage
at the other location mentioned).
- Around line 2187-2205: The CseqOverflow arm currently only sends BYE and
cleans up the dialog but must also notify the session layer of the refresh
failure like the MissingLocalSdp branch does; after performing the BYE send,
outbound_call_id clear, and self.invites.remove(call_id), invoke the same
failure notification used for SessionRefreshFailed (emit the
SessionRefreshFailed event / call the same helper the MissingLocalSdp branch
uses) with the CseqOverflow error so the session layer observes the refresh
failure (reference SessionRefreshBuildError::CseqOverflow, build_bye_request,
self.send_payload, self.outbound_call_id, self.invites and the
SessionRefreshFailed event path).

---

Nitpick comments:
In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 1087-1090: Update the dialog state to resync ctx.allow_update from
the latest incoming response's Allow header so SendSessionRefresh selection
isn't stale: when handling responses (e.g. the block setting ctx.tx.peer and
ctx.remote_target_uri) extract the response Allow (if present) and set
ctx.allow_update accordingly; apply the same fix in the other handling sites
noted (around the ranges corresponding to lines 1433-1445 and 1776-1782) so
re-INVITE/UPDATE/2xx responses update ctx.allow_update with the most recent
Allow value.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dca8e152-9394-4709-a93e-83758308e6f9

📥 Commits

Reviewing files that changed from the base of the PR and between 6c58793 and f5424c7.

📒 Files selected for processing (5)
  • virtual-voicebot-backend/src/main.rs
  • virtual-voicebot-backend/src/protocol/session/handlers/mod.rs
  • virtual-voicebot-backend/src/protocol/session/state_machine.rs
  • virtual-voicebot-backend/src/protocol/session/types.rs
  • virtual-voicebot-backend/src/protocol/sip/core.rs

Comment thread virtual-voicebot-backend/src/main.rs
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 1087-1090: The code keeps ctx.allow_update fixed from the initial
INVITE and never re-evaluates it when dialog state changes; update
ctx.allow_update whenever you replace ctx.req or update the remote target (e.g.,
in the branches that set ctx.tx.peer = response_peer and ctx.remote_target_uri =
response_remote_target_uri(resp)) by re-parsing the latest response Allow header
to set ctx.allow_update = allow_header_contains("UPDATE"). Locate the points
that modify ctx.req, response_peer, or call response_remote_target_uri(resp) and
add logic to extract the Allow header from the response and set ctx.allow_update
accordingly; also add a regression test that swaps Contact/remote target to a UA
without UPDATE support to verify the refresher falls back to re-INVITE.
- Around line 291-298: In sync_local_cseq_with_request the code incorrectly
advances ctx.local_cseq from the incoming request's CSeq (in InviteContext),
which can let a remote peer exhaust or collide our outgoing in-dialog sequence
numbers; instead stop updating ctx.local_cseq from incoming messages, add/use a
separate field like ctx.remote_cseq (or ctx.last_seen_remote_cseq) to record the
parsed remote CSeq from parse_cseq_header, and ensure ctx.local_cseq is only
advanced by our sending logic (where we build in-dialog requests such as
refresh/BYE) so that local sequencing remains under our control and
duplicates/SessionRefreshFailed are avoided.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9044d4f8-9ff0-4fbc-a01f-39fb626a6d2e

📥 Commits

Reviewing files that changed from the base of the PR and between f5424c7 and 91ebd7c.

📒 Files selected for processing (2)
  • virtual-voicebot-backend/src/main.rs
  • virtual-voicebot-backend/src/protocol/sip/core.rs

Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs Outdated
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 300-307: sync_remote_cseq_with_request は受信した CSeq を単純に上書きしているため
UDP の再順序化や遅延した古いリクエストで remote_cseq が巻き戻ってしまいます。修正方法:
sync_remote_cseq_with_request 内で parse_cseq_header(req.header_value("CSeq"))
で取得した cseq.num をそのまま代入するのではなく、ctx.remote_cseq が Some
の場合は既存値と比較して大きい方を保持し(max(existing, new))、None の場合のみ新しい値を設定するように変更してください(参照箇所: 関数
sync_remote_cseq_with_request, フィールド ctx.remote_cseq, ヘッパーパース処理
parse_cseq_header / SipRequest::header_value)。
- Around line 1097-1099: The code currently overwrites ctx.allow_update
unconditionally with response_allows_update(resp), which clears the capability
when the latest response simply omits an Allow header; change the update to only
set ctx.allow_update when the response explicitly includes an Allow header
(i.e., only update when response_allows_update(resp) indicates a
present/explicit value), otherwise leave ctx.allow_update unchanged so
SendSessionRefresh (local_sdp: None) and flows that rely on retained UPDATE
support (avoiding MissingLocalSdp → BYE → SessionRefreshFailed) keep the prior
capability; apply the same guard to the other occurrences referenced (around the
other blocks you noted).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5809cab6-ec43-436a-a5b3-25f9b65898c3

📥 Commits

Reviewing files that changed from the base of the PR and between 91ebd7c and 0c4c74d.

📒 Files selected for processing (1)
  • virtual-voicebot-backend/src/protocol/sip/core.rs

Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
virtual-voicebot-backend/src/protocol/sip/core.rs (1)

1265-1285: ⚠️ Potential issue | 🟠 Major

古い INVITE CSeq を re-INVITE として通さないでください。

いまは req_cseq == prev_cseq だけ retransmit 扱いで、それ以外はすべて handle_reinvite() に流れます。prev_cseq より小さい INVITE は stale request なので、新しい re-INVITE として session 層へ上げると、古い SDP / Allow / Contact で dialog state を巻き戻せてしまいます。> のときだけ新規 in-dialog INVITE として扱う分岐が必要です。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@virtual-voicebot-backend/src/protocol/sip/core.rs` around lines 1265 - 1285,
現在の分岐は req_cseq == prev_cseq の場合のみ retransmit として扱い、それ以外はすべて handle_reinvite
に流してしまうため、req_cseq < prev_cseq(古い CSeq)の INVITE を新しい re-INVITE
として上げてしまう問題があります。修正は invites ブロック内で req_cseq と ctx.remote_cseq を両方 Some
にしてから比較し、等しい場合は既存の retransmit 処理(ctx.tx.on_retransmit / final_ok_payload /
send_tx_action / send_payload)を行い、req_cseq < prev_cseq のときは stale request
として何もしない(または既存 retransmit 応答を再送)して vec![] を返し、req_cseq > prev_cseq のときだけ
handle_reinvite(req, headers, peer) を呼ぶように分岐を入れてください(識別子: self.invites,
headers.call_id, req.header_value("CSeq"), parse_cseq_header, req_cseq,
prev_cseq, ctx.remote_cseq, ctx.tx.on_retransmit, ctx.final_ok_payload,
ctx.tx.peer, send_tx_action, send_payload, handle_reinvite)。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 1448-1453: 現在の処理は in-dialog re-INVITE で Contact ヘッダが無い場合でも
request_remote_target_uri(&req)(フォールバックで req.uri を使う)で既存の ctx.remote_target_uri
を上書きしてしまうため、以降のリフレッシュや BYE
を誤った(自身の)URIへ組み立てる可能性があります。修正方法:self.invites.get_mut(&headers.call_id) ブロック内で
ctx.req = req.clone() はそのまま行い、ctx.remote_target_uri を更新する際は
request_remote_target_uri(&req) の結果を無条件に使わず、Contact ヘッダが存在している場合にのみ
ctx.remote_target_uri を置き換えるロジックにする(つまり request_remote_target_uri の返り値で Contact
が明示されているケースだけで更新)。request_allows_update(&req) と ctx.allow_update の処理はそのまま維持すること。
- Around line 1794-1803: The current block updates dialog bookkeeping only
inside the session_timer branch and treats any UPDATE as a SessionRefresh
without checking CSeq ordering; first locate the dialog ctx via
self.invites.get_mut(&headers.call_id) and always sync dialog state (call
sync_remote_cseq_with_request(ctx, &req), set ctx.tx.peer = peer,
ctx.remote_target_uri = request_remote_target_uri(&req), and update
ctx.allow_update if request_allows_update(&req) returns Some) so that requests
without Session-Expires still update remote_cseq/peer/target/allow_update; then
perform ordering check against ctx.remote_cseq (or use the result of
sync_remote_cseq_with_request) and only assign ctx.session_timer =
Some(cfg.clone()) and treat as SessionRefresh when the incoming request CSeq is
>= the stored remote_cseq; effectively split dialog bookkeeping from
session_timer refresh and gate the SessionRefresh update by CSeq ordering.

---

Outside diff comments:
In `@virtual-voicebot-backend/src/protocol/sip/core.rs`:
- Around line 1265-1285: 現在の分岐は req_cseq == prev_cseq の場合のみ retransmit
として扱い、それ以外はすべて handle_reinvite に流してしまうため、req_cseq < prev_cseq(古い CSeq)の INVITE
を新しい re-INVITE として上げてしまう問題があります。修正は invites ブロック内で req_cseq と ctx.remote_cseq
を両方 Some にしてから比較し、等しい場合は既存の retransmit 処理(ctx.tx.on_retransmit /
final_ok_payload / send_tx_action / send_payload)を行い、req_cseq < prev_cseq のときは
stale request として何もしない(または既存 retransmit 応答を再送)して vec![] を返し、req_cseq > prev_cseq
のときだけ handle_reinvite(req, headers, peer) を呼ぶように分岐を入れてください(識別子: self.invites,
headers.call_id, req.header_value("CSeq"), parse_cseq_header, req_cseq,
prev_cseq, ctx.remote_cseq, ctx.tx.on_retransmit, ctx.final_ok_payload,
ctx.tx.peer, send_tx_action, send_payload, handle_reinvite)。

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 12d6862f-4c82-47cc-afc8-9f74e8217921

📥 Commits

Reviewing files that changed from the base of the PR and between 0c4c74d and e90eeaf.

📒 Files selected for processing (1)
  • virtual-voicebot-backend/src/protocol/sip/core.rs

Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs
Comment thread virtual-voicebot-backend/src/protocol/sip/core.rs Outdated
@MasanoriSuda
MasanoriSuda merged commit 22a940e into develop Mar 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants