Skip to content

fix: TCNet仕様準拠のプロトコル実装バグ修正#2

Merged
9c5s merged 10 commits intomainfrom
fix/protocol-bugs
Mar 21, 2026
Merged

fix: TCNet仕様準拠のプロトコル実装バグ修正#2
9c5s merged 10 commits intomainfrom
fix/protocol-bugs

Conversation

@9c5s
Copy link
Copy Markdown
Owner

@9c5s 9c5s commented Mar 21, 2026

概要

SPEC_ANALYSIS.mdに文書化されていた3件の確定バグに加え、実機テストで発見された3件の潜在バグを修正。

修正内容

仕様書記載の確定バグ

  • OptIn writeUInt8引数逆転 (network.ts): writeUInt8(offset, value)writeUInt8(value, offset) に修正。バージョン情報がバイト64-66に正しく書き込まれるようになった
  • Windows broadcastAddress (utils.ts): IPアドレスを返していたのをIP+netmaskからブロードキャスト計算に変更。broadcast-address 依存を削除しproduction依存ゼロに
  • デフォルトunicastPort (tcnet.ts): 65032 → 仕様準拠の65023

実機テストで発見した潜在バグ

  • receiveBroadcast OptIn検出 (tcnet.ts): MasterのブロードキャストOptInからも接続を検出するよう復元。初期コミットにあったがc2c1b7fで削除されていた。同一マシン上のBridgeはユニキャストOptIn応答を返さないため必要
  • sendServer ソケット (tcnet.ts): unicastSocket → broadcastSocket。BridgeはbroadcastSocket (ポート60000) からのパケットのみ受け付ける
  • requestData layer変換 (tcnet.ts): layer + 1 を追加。15e2ed3でAPIを0-basedに統一した際に漏れていた
  • example layerインデックス (examples/index.ts): packet.layer - 1packet.layer (APIは既に0-based)

補い合うバグの構図

修正前のWindows環境では、broadcastAddress=IPアドレスの「バグ」がローカルユニキャストとして機能し、同一マシンのBridgeに到達していた。broadcastAddress修正に伴い、receiveBroadcast OptIn検出とsendServerソケット変更が連鎖的に必要になった。

テスト

  • BRIDGE98 (Pro DJ Link Bridge) + CDJ-3000で接続確認
  • メタデータ取得 (Artist/Title) 正常動作を確認
  • lefthook pre-commit (format, lint, typecheck, build) 全パス

9c5s added 7 commits March 21, 2026 10:09
Buffer.writeUInt8(value, offset) のAPI仕様に対して引数が逆転していた。
offsetの数値(64,65,66)がvalueとして書き込まれ、バージョン情報が
正しく送信されていなかった。
TCNet仕様ではデフォルトユニキャストポートは65023だが、
65032が設定されていた。
Windows環境ではinterfaceAddress()がIPアドレスを返していたため、
OptInパケットがブロードキャストされなかった。
OS分岐を廃止しIPとサブネットマスクからブロードキャストアドレスを
自前計算するように変更。broadcast-address依存を削除。
同一マシン上のBridgeはユニキャストOptIn応答を返さない場合がある。
receiveBroadcastでもMasterのOptInを検出して接続を確立するように変更。
併せてtimestampSocketのbindアドレスをbrodcastListeningAddressに統一。
sendServerがunicastSocket(65023)を使用していたが、BridgeはbroadcastSocket(60000)
からのパケットのみ受け付ける。broadcastSocketに変更。
また、requestDataでlayer+1変換が欠落していたため、0-based APIから1-based
ワイヤフォーマットへの変換を追加。
15e2ed3でAPIが0-basedに統一されたが、exampleのpacket.layer - 1が
そのまま残っていた。
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 21, 2026

Summary by CodeRabbit

リリースノート

  • バグ修正

    • OptIn時のバージョン送信を正しく修正
    • ブロードキャスト先アドレス計算を改善(Windows含む)
    • デフォルトのユニキャストポートを仕様準拠値に更新
    • レイヤー索引の不整合を解消(要求送信と状態格納の整合)
    • サーバー検出・接続処理とソケットの開閉・タイムアウト管理を安定化
  • Chores

    • 外部ブロードキャスト依存を削減し、アドレス計算を内部実装へ移行

Walkthrough

OptInパケットのバージョン書き込み順を修正、Windows依存のブロードキャスト算出をビット演算へ置換、デフォルトunicastポートを65023に変更、requestDataのオンワイヤ層を0→1ベースへ変換、送受信ハンドリングとソケットライフサイクルを調整。

Changes

Cohort / File(s) Summary
プロトコル/クライアント制御フロー
src/tcnet.ts, .changeset/fix-protocol-bugs.md
デフォルトunicastPort65023へ変更。connect()でのバインドアドレス指定をbrodcastListeningAddressへ(綴りは現状のまま)。disconnect()でsocket閉処理を共通化しtimestampSocketも閉じる。receiveBroadcast()でMasterからのTCNetOptInPacket受信時にserver設定、connected=true、即時announceApp()呼出し、connectedHandler()実行を追加。リクエスト管理をresolve+timeoutへ変更し、受信側でタイムアウトクリアと削除を行う。sendServer()は送信ソケットをbroadcastSocketへ変更。changeset追加。
OptInパケットのシリアライゼーション
src/network.ts
TCNetOptInPacket.write()buffer.writeUInt8の引数順を修正し、バージョンバイトを固定オフセット(64/65/66)へ正しく書き込むように変更。
ブロードキャストアドレス計算再実装
src/utils.ts, package.json
broadcast-address依存を削除。os.networkInterfaces()から取得したIPv4アドレス/ネットマスクを基にcalculateBroadcastAddress(address, netmask)でビット演算によりブロードキャストを算出。interfaceAddress()を簡素化しプラットフォーム分岐を除去。package.jsonの依存からbroadcast-addressを除去。
APIレイヤーインデックス整合 & examples
src/tcnet.ts, examples/index.ts
requestData()で入力layerを整数0–7に検証し、オンワイヤでrequest.layer = layer + 1に変換(内部リクエストマップは0ベースのままキー保持)。examples/index.tsのメトリクスハンドラはpacket.layer - 1参照をpacket.layerへ変更しインデックス整合。

Sequence Diagram(s)

sequenceDiagram
    participant BroadcastSocket
    participant Client
    participant Server
    participant TimestampSocket
    BroadcastSocket->>Client: UDP OptIn (TCNetOptInPacket) + rinfo
    Client->>Client: this.server = {address: rinfo.address, port: packet.nodeListenerPort}
    Client->>Client: this.connected = true
    Client->>Server: announceApp() (immediate)
    Client->>Client: invoke connectedHandler() and clear it
    Client->>BroadcastSocket: sendServer() via broadcastSocket to Server
    Client->>TimestampSocket: bind to brodcastListeningAddress (on connect)
    Note over Client,Server: requestData(layer) encodes on-wire layer = layer + 1
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • Issue 3 — misspelled brodcastListeningAddress: このPRは同名の誤綴りプロパティをconnect()で使用しており、該当issueの目的(プロパティ名/接続バインドの確認)と関連する可能性があります。
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed タイトルはTCNetプロトコル実装バグ修正を正確に説明しており、変更セットの主要な目的と完全に一致しています。
Description check ✅ Passed 説明は修正内容の詳細(仕様書記載の確定バグ3件と実機テスト発見の潜在バグ3件)を包括的に記載しており、変更セットと密接に関連しています。

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/protocol-bugs

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.

@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

このプルリクエストは、TCNetプロトコル実装における合計6つのバグを修正します。これには、仕様書に記載されていた3つの確定バグと、実機テスト中に発見された3つの潜在バグが含まれます。特に、Windows環境でのブロードキャストアドレス計算の修正は、他の関連するバグ修正(MasterのブロードキャストOptIn検出の復元やsendServerソケットの変更)を連鎖的に引き起こし、全体的なプロトコル通信の堅牢性と仕様準拠を大幅に向上させます。これにより、異なる環境や設定下での安定した動作が期待されます。

Highlights

  • OptInパケットのバージョン情報書き込み修正: writeUInt8の引数順序が誤っていたため、バージョン情報が正しく書き込まれていなかった問題を修正しました。
  • Windows環境でのブロードキャストアドレス計算改善: broadcast-addressライブラリへの依存を削除し、IPアドレスとネットマスクからブロードキャストアドレスを計算する内部ロジックを実装しました。これにより、Windows環境でのブロードキャストアドレスが正しく機能するようになりました。
  • デフォルトユニキャストポートの仕様準拠: デフォルトのユニキャストポートを仕様に準拠した65023に変更しました(以前は65032)。
  • MasterのブロードキャストOptIn検出の復元: MasterからのブロードキャストOptInパケットからも接続を検出するロジックを復元しました。これにより、同一マシン上のBridgeがユニキャスト応答を返さない場合でも接続が確立されます。
  • sendServerでのソケット使用の修正: sendServerメソッドがunicastSocketではなくbroadcastSocketを使用するように修正しました。BridgeはbroadcastSocketからのパケットのみを受け付けるためです。
  • requestDataのレイヤー変換修正: requestDataメソッドでAPIの0-basedインデックスを仕様の1-basedインデックスに変換するため、layer + 1を追加しました。
  • exampleのレイヤーインデックス修正: examples/index.tspacket.layer - 1packet.layerに修正し、APIが既に0-basedであることを反映させました。
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

このプルリクエストは、TCNetプロトコルの仕様準拠に関する複数のバグを修正するもので、非常に価値のある変更です。特に、writeUInt8の引数順序の修正や、Windows環境でのブロードキャストアドレス計算の修正は、プロトコルの安定性と互換性を大幅に向上させるものです。依存関係を削減した点も素晴らしいです。

コードレビューの結果、2点ほど軽微な改善点を提案させていただきました。

  1. src/tcnet.ts で新たに追加されたプロパティ名にタイポが見つかりました。
  2. 同じく src/tcnet.ts で、エラーがサイレントに握りつぶされている箇所がありましたので、デバッグを容易にするためのログ出力を提案しました。

これらの修正は、コードの保守性と信頼性をさらに高めるものと期待されます。全体として、非常によく練られた修正であり、マージする価値が高いと判断します。

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

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)
src/tcnet.ts (1)

386-390: ⚠️ Potential issue | 🟠 Major

layer の境界チェックが抜けています。

API を 0-based に寄せたので、ここで -18 以上を弾かないと無効な wire 値をそのまま送れてしまいます。加えて、1-based 変換は最終的に TCNetRequestPacket.write() 側へ寄せた方が契約が一貫します。

🔧 最低限の防御策
 public requestData(dataType: number, layer: number): Promise<nw.TCNetDataPacket> {
     return new Promise((resolve, reject) => {
+        if (!Number.isInteger(layer) || layer < 0 || layer > 7) {
+            reject(new RangeError("layer must be an integer between 0 and 7"));
+            return;
+        }
+
         const request = new nw.TCNetRequestPacket();
         request.dataType = dataType;
         request.layer = layer + 1; // APIは0-based、仕様は1-based
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tcnet.ts` around lines 386 - 390, The requestData method currently
adjusts layer to 1-based and lacks bounds checking, so invalid values (e.g. -1
or >=8) can be sent; validate the incoming layer in requestData (ensure 0 <=
layer <= 7) and reject/throw for out-of-range inputs, and move the +1 1-based
conversion out of requestData into TCNetRequestPacket.write() so the conversion
contract is centralized (update references to requestData and
TCNetRequestPacket.write accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.changeset/fix-protocol-bugs.md:
- Around line 5-9: Update the changeset text to also mention the 0-based layer
fix and explicitly document that the change affects requestData() interpretation
and example/client behavior; add a concise bullet noting "0-based layer indexing
corrected" and a short note advising integrators that requestData() semantics
and example usages have changed and that they should verify/remove any prior
workarounds, so downstream users can track and adapt to the behavioral
difference.

In `@src/network.ts`:
- Around line 130-132: 追加した wire-format 書き込みの回帰テストを作成して、バッファのバイト 64–66 に
this.majorVersion/this.minorVersion/this.bugVersion
が正しく書き込まれていることを検証してください。具体的には Network クラス(または該当の write メソッドを含むクラス/関数、例:
writeUInt8 を呼んでいるメソッド)を使ってシリアライズした結果のバッファを取得し、buffer.readUInt8(64),
readUInt8(65), readUInt8(66) がそれぞれ期待値と一致するアサーションを追加してください。テスト名は「wire format
version bytes remain stable」など分かりやすくして regression テストスイートに含めてください。

---

Outside diff comments:
In `@src/tcnet.ts`:
- Around line 386-390: The requestData method currently adjusts layer to 1-based
and lacks bounds checking, so invalid values (e.g. -1 or >=8) can be sent;
validate the incoming layer in requestData (ensure 0 <= layer <= 7) and
reject/throw for out-of-range inputs, and move the +1 1-based conversion out of
requestData into TCNetRequestPacket.write() so the conversion contract is
centralized (update references to requestData and TCNetRequestPacket.write
accordingly).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 12c95e1f-2714-4b90-a424-5e308f64791c

📥 Commits

Reviewing files that changed from the base of the PR and between 95fbf3a and 0fe1ccf.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (6)
  • .changeset/fix-protocol-bugs.md
  • examples/index.ts
  • package.json
  • src/network.ts
  • src/tcnet.ts
  • src/utils.ts
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.21.0)
.changeset/fix-protocol-bugs.md

[warning] 5-5: First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

- announceApp()のcatchでエラーをログ出力するよう改善
- requestData()にlayer境界チェック (0-7) を追加
- changesetにlayer修正とexample修正を追記
Copy link
Copy Markdown
Owner Author

@9c5s 9c5s left a comment

Choose a reason for hiding this comment

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

レビュー対応を 99627bb でプッシュしました。

採用 (3件):

  • .catch(() => {}) → エラーログ出力に改善
  • changeset に layer 修正を追記
  • requestData() に layer 境界チェック (0-7) を追加

スコープ外 → issue化 (2件):

  • #3 brodcastListeningAddress タイポ修正 (破壊的変更)
  • #4 テストフレームワーク導入と回帰テスト

却下 (1件):

  • layer+1 の TCNetRequestPacket.write() への移動: layer プロパティがワイヤ値を保持するセマンティクスを変えることになり、本PRのスコープ外

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

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

Inline comments:
In `@src/tcnet.ts`:
- Around line 396-399: The caller-side 0→1 conversion (request.layer = layer + 1
in TCNetRequestPacket usage) must be removed and the responsibility moved into
the serializer: revert callers (e.g. where TCNetRequestPacket is constructed in
src/tcnet.ts) to assign the API's 0-based layer value directly, and update
TCNetDataPacket.write() in src/network.ts to perform the wire conversion when
serializing (write byte 25 as this.layer + 1), optionally adding a bounds
check/assert for valid layer range; this ensures all send paths use a single
canonical API representation and the wire format is applied only in write().
- Around line 100-101: disconnect() currently only closes broadcastSocket and
unicastSocket leaving timestampSocket bound; when timestampSocket is created via
createSocket(...) and bound with bindSocket(..., TCNET_TIMESTAMP_PORT) you must
also close it in disconnect(): add logic in the disconnect() function to check
if this.timestampSocket exists, call its close() (or equivalent) and
null/undefine the property so UDP/60001 is released and subsequent bindSocket
calls succeed; reference the timestampSocket property, the disconnect() method,
and the TCNET_TIMESTAMP_PORT usage when making the change.
- Around line 198-206: The current code calls announceApp() fire-and-forget and
then immediately sets this.connected = true and invokes this.connectedHandler(),
causing a race where requestData() may run before Master registration completes;
change the flow in the connect logic so you await this.announceApp() (e.g.,
await this.announceApp() inside a try/catch), and only after it resolves
successfully set this.connected = true and call/clear this.connectedHandler; on
failure handle/log the error and do not mark connected or call the handler until
announceApp() succeeds. Ensure you reference announceApp(), this.connected, and
this.connectedHandler when making the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7839c215-485d-464c-a52c-b10997a22e6e

📥 Commits

Reviewing files that changed from the base of the PR and between 0fe1ccf and 99627bb.

📒 Files selected for processing (2)
  • .changeset/fix-protocol-bugs.md
  • src/tcnet.ts
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.21.0)
.changeset/fix-protocol-bugs.md

[warning] 5-5: First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

timestampSocket (UDP/60001) がclose漏れしており、再接続時に
EADDRINUSEになる可能性があった。
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

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

⚠️ Outside diff range comments (1)
src/tcnet.ts (1)

401-407: ⚠️ Potential issue | 🟠 Major

成功応答後の pending request を掃除していないため、同じキーの再要求が壊れます。

Line 401-407 では timeout を登録していますが、成功パスは resolve() するだけで this.requests と timer を残します。最初の要求が即応答したあと requestTimeout 以内に同じ (dataType, layer) を再度投げると、1 本目の stale timeout が 2 本目の entry を消して、2 本目の Promise が未解決のまま残ります。

🧹 修正案
-type STORED_RESOLVE = (value?: nw.TCNetDataPacket | PromiseLike<nw.TCNetDataPacket> | undefined) => void;
+type STORED_RESOLVE = {
+    resolve: (value?: nw.TCNetDataPacket | PromiseLike<nw.TCNetDataPacket> | undefined) => void;
+    timeout: NodeJS.Timeout;
+};
-const pendingRequest = this.requests.get(`${dataPacket.dataType}-${dataPacket.layer}`);
-if (pendingRequest) {
-    pendingRequest(dataPacket);
+const key = `${dataPacket.dataType}-${dataPacket.layer}`;
+const pendingRequest = this.requests.get(key);
+if (pendingRequest) {
+    this.requests.delete(key);
+    clearTimeout(pendingRequest.timeout);
+    pendingRequest.resolve(dataPacket);
 }
-            this.requests.set(`${dataType}-${layer}`, resolve);
-
-            setTimeout(() => {
-                if (this.requests.delete(`${dataType}-${layer}`)) {
+            const key = `${dataType}-${layer}`;
+            const timeout = setTimeout(() => {
+                if (this.requests.delete(key)) {
                     reject(new Error("Timeout while requesting data"));
                 }
             }, this.config.requestTimeout);
+            this.requests.set(key, { resolve, timeout });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tcnet.ts` around lines 401 - 407, 現在の実装は成功時に this.requests
エントリとタイマーを残すため、最初のタイマーが後続リクエストのエントリを削除してしまいます。修正は、request
を登録するときにタイマーIDを一緒に保持し(例: this.requests.set(key, {resolve, reject,
timerId}))、成功パス(resolve を呼ぶ箇所)で必ず clearTimeout(timerId) を実行してから
this.requests.delete(key) でエントリを掃除するようにすること、そして setTimeout のコールバック側では保存された
timerId と一致することを確認してから reject かつ this.requests.delete(key) を行うようにしてください(参照シンボル:
this.requests, setTimeout, clearTimeout, resolve, reject,
this.config.requestTimeout)。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/tcnet.ts`:
- Around line 401-407: 現在の実装は成功時に this.requests
エントリとタイマーを残すため、最初のタイマーが後続リクエストのエントリを削除してしまいます。修正は、request
を登録するときにタイマーIDを一緒に保持し(例: this.requests.set(key, {resolve, reject,
timerId}))、成功パス(resolve を呼ぶ箇所)で必ず clearTimeout(timerId) を実行してから
this.requests.delete(key) でエントリを掃除するようにすること、そして setTimeout のコールバック側では保存された
timerId と一致することを確認してから reject かつ this.requests.delete(key) を行うようにしてください(参照シンボル:
this.requests, setTimeout, clearTimeout, resolve, reject,
this.config.requestTimeout)。

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 02142081-4f45-4a67-a186-30cfdcce8909

📥 Commits

Reviewing files that changed from the base of the PR and between 99627bb and bd03537.

📒 Files selected for processing (1)
  • src/tcnet.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:397-400
Timestamp: 2026-03-21T05:02:24.940Z
Learning: In the `node-tcnet` repository (`src/network.ts`, `src/tcnet.ts`), `TCNetDataPacket.write()` has no callers. `TCNetDataPacket` objects are only ever deserialized on receive paths (`read()`), never serialized and sent. The only outbound packet carrying a layer field is `TCNetRequestPacket`, constructed inside `requestData()` in `src/tcnet.ts`. Suggesting to move layer conversion logic into `TCNetDataPacket.write()` is inappropriate because that method is dead code.
📚 Learning: 2026-03-21T05:02:24.940Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:397-400
Timestamp: 2026-03-21T05:02:24.940Z
Learning: In the `node-tcnet` repository (`src/network.ts`, `src/tcnet.ts`), `TCNetDataPacket.write()` has no callers. `TCNetDataPacket` objects are only ever deserialized on receive paths (`read()`), never serialized and sent. The only outbound packet carrying a layer field is `TCNetRequestPacket`, constructed inside `requestData()` in `src/tcnet.ts`. Suggesting to move layer conversion logic into `TCNetDataPacket.write()` is inappropriate because that method is dead code.

Applied to files:

  • src/tcnet.ts
🔇 Additional comments (4)
src/tcnet.ts (4)

18-18: 既定の unicastPort を 65023 に合わせたのは良いです。

設定未指定時の接続先が仕様値と揃います。


193-209: broadcast OptIn での server 発見と broadcastSocket 送信をセットで直したのは良いです。

同一ホストの Bridge が unicast OptIn を返さないケースでも、発見と送信元ポート 60000 の要件を両立できています。

Also applies to: 346-346


392-400: layer の範囲チェックと 0-based API → 1-based wire 変換を requestData() に閉じたのは妥当です。

API 表現のまま受信側キーを維持しつつ、送信時だけ wire 形式へ補正できています。

Based on learnings: TCNetDataPacket.write() には呼び出し元がなく、layer を送る outbound path は requestData() 内の TCNetRequestPacket だけです。


120-123: ⚠️ Potential issue | 🔴 Critical

disconnect() がどのソケットも実際には閉じていません

promisifyBasicFunction() は Promise を返す関数を返すため、Promise.all() に渡された値は関数のままで、Promise ではありません。Line 120-123 は Promise.all([関数1, 関数2, 関数3]) を実行しているため、これらの関数は呼び出されず、broadcastSocket.close() / unicastSocket.close() / timestampSocket.close() は一度も実行されません。ソケットが開いたままになり、再接続時に EADDRINUSE エラーが発生します。

修正案
+const closeSocket = (socket: Socket): Promise<void> =>
+    new Promise((resolve, reject) => {
+        try {
+            socket.close(() => resolve());
+        } catch (err) {
+            reject(err);
+        }
+    });
+
 public disconnect(): Promise<void> {
     clearInterval(this.announcementInterval);
     this.removeAllListeners();
     this.connected = false;
     return Promise.all([
-        promisifyBasicFunction(() => this.broadcastSocket.close()),
-        promisifyBasicFunction(() => this.unicastSocket.close()),
-        promisifyBasicFunction(() => this.timestampSocket.close()),
+        closeSocket(this.broadcastSocket),
+        closeSocket(this.unicastSocket),
+        closeSocket(this.timestampSocket),
     ])
			> Likely an incorrect or invalid review comment.

promisifyBasicFunctionは関数を返すだけで呼ばないため、disconnect()で
ソケットが実際にはcloseされていなかった。closeSocketヘルパーに置き換え。
また、requestDataの成功パスでrequestsエントリとtimerが残り、同一キーの
再リクエストで壊れる問題を修正。
Copy link
Copy Markdown
Owner Author

@9c5s 9c5s left a comment

Choose a reason for hiding this comment

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

2248ebf で2件とも修正した。

  • disconnect() close漏れ: promisifyBasicFunctionは関数を返すだけで呼ばないためソケットがcloseされていなかった。closeSocketヘルパーに置き換え、promisifyBasicFunctionは削除。
  • pending requestクリーンアップ: 成功パスでthis.requests.delete()clearTimeout()を実行するよう修正。STORED_RESOLVE型をSTORED_REQUESTに変更し、timeoutを保持。

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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

Inline comments:
In `@src/tcnet.ts`:
- Around line 249-255: The response handler can't find pending requests because
requestData() stores keys using 0-based layer but TCNetDataPacket.read()
currently reads a 1-based layer; update TCNetDataPacket.read() to convert the
wire value to 0-based by subtracting 1 when assigning this.layer (i.e., set
this.layer = this.buffer.readUInt8(25) - 1) so the keys `${dataType}-${layer}`
produced for requests match the keys used when resolving responses; this mirrors
the conversion already done in TCNetRequestPacket.read().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e81e3e8a-af0c-4f24-9578-5576de0a23df

📥 Commits

Reviewing files that changed from the base of the PR and between bd03537 and 2248ebf.

📒 Files selected for processing (1)
  • src/tcnet.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:397-400
Timestamp: 2026-03-21T05:02:24.940Z
Learning: In the `node-tcnet` repository (`src/network.ts`, `src/tcnet.ts`), `TCNetDataPacket.write()` has no callers. `TCNetDataPacket` objects are only ever deserialized on receive paths (`read()`), never serialized and sent. The only outbound packet carrying a layer field is `TCNetRequestPacket`, constructed inside `requestData()` in `src/tcnet.ts`. Suggesting to move layer conversion logic into `TCNetDataPacket.write()` is inappropriate because that method is dead code.
📚 Learning: 2026-03-21T05:02:24.940Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:397-400
Timestamp: 2026-03-21T05:02:24.940Z
Learning: In the `node-tcnet` repository (`src/network.ts`, `src/tcnet.ts`), `TCNetDataPacket.write()` has no callers. `TCNetDataPacket` objects are only ever deserialized on receive paths (`read()`), never serialized and sent. The only outbound packet carrying a layer field is `TCNetRequestPacket`, constructed inside `requestData()` in `src/tcnet.ts`. Suggesting to move layer conversion logic into `TCNetDataPacket.write()` is inappropriate because that method is dead code.

Applied to files:

  • src/tcnet.ts
🔇 Additional comments (9)
src/tcnet.ts (9)

9-12: LGTM!

リクエストごとに resolvetimeout をペアで管理する型定義は、タイムアウトのクリーンアップを確実に行うための適切なアプローチです。


21-21: LGTM!

デフォルト unicastPort を仕様準拠の 65023 に変更。


33-33: LGTM!

socket.close() のコールバックを適切に Promise 化しています。promisifyBasicFunction の誤用を修正し、確実にソケットのクローズ完了を待機できるようになりました。


95-97: LGTM!

timestampSocket のバインドアドレスを broadcastSocket と同じ brodcastListeningAddress に統一。一貫性が向上しています。


115-119: LGTM!

closeSocket ヘルパーを使用して3つのソケット(broadcastSocketunicastSockettimestampSocket)を確実にクローズ。再接続時の EADDRINUSE エラーを防止します。


188-205: LGTM!

Master のブロードキャスト OptIn からも接続を検出するロジックを復元。同一マシン上の Bridge がユニキャスト応答を返さないケースに対応しています。announceApp() の fire-and-forget パターンは、1秒間隔の setInterval による再送で確実に登録されるため問題ありません。


344-344: LGTM!

送信ソケットを broadcastSocket に変更。Bridge はソースポート 60000 からのパケットのみを受け付けるため、この変更によりサーバーへのパケット送信が正しく動作します。


390-394: LGTM!

layer パラメータの検証を追加。0〜7 の整数範囲を強制することで、不正な入力による予期しない動作を防止しています。


397-413: LGTM!

タイムアウト管理が改善されています:

  • request.layer = layer + 1 で wire フォーマットの 1-based 変換(学習に基づき、ここでの変換が適切)
  • タイムアウトをリクエストと共に保存し、成功時・エラー時に確実にクリア
  • requests.delete(key) の戻り値で二重 reject を防止

@9c5s 9c5s merged commit 090bc6a into main Mar 21, 2026
1 check passed
@9c5s 9c5s deleted the fix/protocol-bugs branch March 21, 2026 05:36
9c5s added a commit that referenced this pull request Mar 24, 2026
- convergeToAdapter()でbroadcastAddress更新 (#1)
- waitConnected()の二重close防止 (#2)
- disconnectSockets()でserver=nullリセット (#3)
- connect()失敗時のソケットクリーンアップ (#4)
- waitConnected()タイムアウトをdetectionTimeoutに変更 (#5)
- detectingAdapterフラグで経路識別を明示化 (#6)
- listNetworkAdapters()の重複呼び出し削減 (#7)
- findIPv4Address()ヘルパーで述語重複を解消 (#8)
- privateフィールドの命名規則統一 (#9)
- announceApp()の検出中送信を並列化 (#10)
- announcementIntervalの型安全性改善 (#11)
- announceApp()のエラーハンドリング追加 (#12)
- selectedAdapterのセット順統一 (#13)
- closeSocketをfunction宣言に変更 (#14)
9c5s added a commit that referenced this pull request Mar 24, 2026
* feat: listNetworkAdapters()の型定義と実装を追加

* feat: TCNetConfiguration拡張と基盤インスタンス変数を追加

* refactor: disconnectSockets()分離とdisconnect()/waitConnected()リファクタリング

* feat: connect()をマルチソケット検出+即resolveに書き換え

- connect()を全non-internal IPv4アダプタにソケット作成する方式に変更
- Master検出を待たず即座にresolveするように変更
- Master OptIn検出時にアダプタ収束(convergeToAdapter)を実装
- 検出タイムアウトでdetectionTimeoutイベントを発火
- connectToAdapter()内部メソッドを追加(switchAdapter用)
- selectedAdapter/isConnectedプロパティを追加
- receiveBroadcastにadapterName引数を追加してマルチソケット対応
- announceApp()にbroadcastSocket nullガードを追加

* feat: announceApp/broadcastPacket/receiveTimestampのガード追加

- announceApp(): 検出中は全broadcastSocketsに送信するマルチソケット対応
- broadcastPacket(): エラーメッセージを "Adapter not yet selected" に統一
- sendServer(): _switchingフラグによるガード追加
- requestData(): _switchingフラグによるガード追加
- receiveTimestamp(): connected未確定時はtimeイベントを発火しないガード追加

* feat: switchAdapter()のバリデーションと切り替えロジックを実装

* test: switchAdapter()のリトライとエッジケーステストを追加

* docs: Wiki更新 - アダプタ自動検出・切り替えAPIの追加

* fix: コードレビュー指摘13件の修正

- convergeToAdapter()でbroadcastAddress更新 (#1)
- waitConnected()の二重close防止 (#2)
- disconnectSockets()でserver=nullリセット (#3)
- connect()失敗時のソケットクリーンアップ (#4)
- waitConnected()タイムアウトをdetectionTimeoutに変更 (#5)
- detectingAdapterフラグで経路識別を明示化 (#6)
- listNetworkAdapters()の重複呼び出し削減 (#7)
- findIPv4Address()ヘルパーで述語重複を解消 (#8)
- privateフィールドの命名規則統一 (#9)
- announceApp()の検出中送信を並列化 (#10)
- announcementIntervalの型安全性改善 (#11)
- announceApp()のエラーハンドリング追加 (#12)
- selectedAdapterのセット順統一 (#13)
- closeSocketをfunction宣言に変更 (#14)

* fix: CodeRabbitレビュー指摘5件の修正とchangeset追加

- waitConnected()のPromiseハング防止 (CR-3)
- switchAdapter()最終リトライ失敗時のクリーンアップ (CR-4)
- broadcastPacket()にswitchingガード追加 (CR-5)
- connect()の再入防止ガード強化 (CR-2)
- findIPv4Address()のAPI Reference記載 (CR-1)
- changeset追加 (minor)

* fix: announceApp()がswitchingガードに阻害される問題を修正

announceApp()が公開APIのbroadcastPacket()/sendServer()を経由していたため、
switchAdapter()中のconnectToAdapter()からの呼び出し時にswitchingガードに
ブロックされていた。内部メソッドのsendPacket()を直接使用するよう変更。

* fix: CodeRabbit 2回目レビュー指摘2件の修正

- requestTimeoutの説明を旧挙動から修正 (CR-new-1)
- waitConnected()にtimeoutMsパラメータ追加、detectionTimeout=0でもhangしない (CR-new-3)
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.

1 participant