Skip to content

feat(p2p): follow grants onto the Endpoint, and withdraw a listener on close - #79

Open
masa-koz wants to merge 2 commits into
mainfrom
feat/p2p-endpoint-grants
Open

feat(p2p): follow grants onto the Endpoint, and withdraw a listener on close#79
masa-koz wants to merge 2 commits into
mainfrom
feat/p2p-endpoint-grants

Conversation

@masa-koz

@masa-koz masa-koz commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

The server side of this is ISEKAI-link-server #203: a grant is now keyed on
(owner_endpoint, allowed_endpoint, protocol) rather than on a listener, so it
survives the app being restarted. This is the client half.

The proxy client moves to /v1/peer/grants and /v1/peer/pairing-codes with the
caller as the owner, Grant loses listener_id, PairingCode names the
owner Endpoint it lets someone in to, and ReachableListener carries the
owner Endpoint — which is the field that identifies a camera across a restart,
where listener_id does not. show_pairing_code sends the session's protocol
now that no listener is there to imply it.

Closing a listener session deletes the listener rather than leaving it to
lapse. Until it lapsed it kept appearing in every paired peer's list as
something that looks connectable and is not — which is exactly what was seen
on hardware. This was not safe to do before: the delete used to cascade the
grants away, so withdrawing a listener would have thrown away every pairing.

Cancelling the session only starts that withdrawal, though. The delete is an
HTTP request over the same msquic registration the process is about to drain,
so ServerHandle hands back the command loop's join handle, camera-server
parks it where main can reach it after the window closes, and main awaits
it with a bound before draining. The withdrawal has its own three-second bound
inside close, six around the whole shutdown, so a proxy that is unreachable
at exit is decided by the inner bound rather than keeping the process alive
until the transport gives up.

camera-client shows one row per camera rather than one per listener. A grant
reaches every listener its owner is running, and a camera that crashed without
withdrawing its old one runs two, only one of which connects. The row kept is
the listener with the later deadline — both were leased for the same span, so
the later deadline is the later start — with the id as a tie-break so the
selected camera does not move under the operator between refreshes. Both
things that rests on are written down at the function, because the listing
does not carry enough to check either: that the TTL is uniform, and that
expires_at keeps arriving in the one format whose text order is time order.

Verification

  • LIBCLANG_PATH=/usr/lib/llvm-18/lib RUSTFLAGS="-L /usr/lib/llvm-18/lib" cargo test --manifest-path rust/Cargo.toml --workspace — all passing, including new tests pinning the grant and pairing-code routes and request bodies, that a listing carries owner_endpoint, and the one-row-per-camera collapse
  • cargo clippy on the touched crates introduces no new warnings
  • cargo fmt on the touched files only; these crates are not rustfmt-clean on main

Release order

There is no distributed build of these apps and no other consumer of the
proxy: both sides are built from source by the same person who runs the
server, so the switch is "rebuild both, then deploy", not a staged rollout.
That is why #203 removed the old routes outright instead of keeping aliases
for a release. Between merging this and deploying #203, whichever order, an
app built from the other side's main will get 404s on pairing and grant
management — the window is however long the rebuild takes, and it affects
nobody else.

#203 is already merged. Merge this and rebuild before deploying the
server.

Not yet done on hardware

Pair, connect, close the camera-server, re-open it, and confirm the viewer's
Refresh shows the camera again with the pairing intact — the case that started
this. Also that the old listener disappears from the list at close rather than
lingering for the rest of its lease; for that one, checking that
DELETE /v1/peer-listeners/... reaches the proxy's log is what separates "did
not send" from "sent and failed". I will run both once #203 is deployed and
report what happens.

🤖 Generated with Claude Code

…n close

The server side of this is ISEKAI-link-server #203: a grant is now keyed on
(owner_endpoint, allowed_endpoint, protocol) rather than on a listener, so it
survives the app being restarted. This is the client half, and it has to ship
before that is deployed — the old routes are gone.

The proxy client moves to /v1/peer/grants and /v1/peer/pairing-codes with the
caller as the owner, `Grant` loses `listener_id`, `PairingCode` names the
owner Endpoint it lets someone in to, and `ReachableListener` carries the
owner Endpoint — which is the field that identifies a camera across a restart,
where `listener_id` does not. `show_pairing_code` sends the session's protocol
now that no listener is there to imply it.

Closing a listener session deletes the listener rather than leaving it to
lapse. Until it lapsed it kept appearing in every paired peer's list as
something that looks connectable and is not — which is exactly what was seen
on hardware. This was not safe to do before: the delete used to cascade the
grants away, so withdrawing a listener would have thrown away every pairing.
A failure is logged, not returned; the lease ends it within the hour anyway
and a caller on the way out can do nothing with the error.

camera-client shows one row per camera rather than one per listener. A grant
reaches every listener its owner is running, and a camera that crashed without
withdrawing its old one runs two, only one of which connects. The row kept is
the listener with the later deadline — both were leased for the same span, so
the later deadline is the later start — with the id as a tie-break so the
selected camera does not move under the operator between refreshes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@masa-koz

masa-koz commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

概要

ISEKAI-link-server #203(Grant を Listener から Endpoint へ移す)のクライアント側。ルート移行、Grant からの listener_id 除去、ReachableListenerowner_endpoint 追加、show_pairing_code の protocol 送出、そして「セッションを閉じるときに Listener を削除する」の 4 点。

対応関係はきれいだと思います。

  • one_per_camera の判断(Grant は端末に対するもの、クラッシュ後は同じ owner の Listener が 2 つ並ぶ、operator に見せるのは 1 行)が正しく、同着の並びを id で固定して refresh のたびに選択が動かないようにしている点まで詰められている
  • ペアリング成功後の名前解決を listener_id 一致から owner_endpoint 一致に変えている。ここを直さないと再起動後に名前が出なくなるので、変更の波及を追えている
  • ルートとボディを固定するテスト(MockTransport)が追加されていて、feat(p2p): give the apps the calls that replace the four values #77 のレビューで挙げた「新しい API が未カバー」がこの範囲では解消している
  • close() の削除失敗をログ止まりにする判断と、その理由(終了処理であり呼び出し側にできることがない)

指摘

1. アプリ終了時に DELETE が本当に飛ぶかは、プロセスが生き残るかに依存している(要確認)

経路を追うと:

MyApp::dropclose()token.cancel()command_loopbreaksession.close().awaitDELETE /v1/peer-listeners/{id}

一方 mainrun_native から戻ったあと、カメラタスクを await して shutdown_and_exit(&reg, MSQUIC_DRAIN_TIMEOUT, ..) に入ります。command_loop タスクの完了は誰も待っていません。 つまり DELETE は「msquic のドレインが終わってプロセスが落ちるまでの間に、spawn されたタスクが動いて H3 の往復を終えられたら飛ぶ」という競争になります。ドレインが早く終われば飛びませんし、そもそも DELETE 自体がその msquic 上の H3 リクエストです。

PR で「未実施」とされている確認項目のうち「閉じたときに古い Listener が一覧から消えるか」は、まさにここが効く部分です。実機で見るときは DELETE /v1/peer-listeners/... がプロキシのログに届いているかまで見ると、飛んでいないのか失敗しているのかを切り分けられます。

対処としては、ServerHandle に完了を待てるハンドルを持たせて main が短いタイムアウト付きで await する、あるいは close() の中の削除を tokio::time::timeout で包んだうえで、終了経路がそれを待つ、あたりだと思います。後者は次の指摘にも効きます。

2. close() の DELETE にタイムアウトが無い

プロキシが落ちている・到達できない状態でアプリを閉じると、delete_peer_listener は下位のトランスポートが諦めるまで戻りません。終了処理でそこに引っかかると、ウィンドウを閉じたのにプロセスが残ります。数秒の timeout で包んで、超えたら今のログ(「リースで消える」)と同じ扱いにするのが安全だと思います。

3. one_per_camera の「後の期限 = 生きている方」は TTL が一定であることに依存している

コメントのとおり「同じ長さで貸したなら、期限が後の方が後から始まった」は成り立ちますが、成り立つのはすべての Listener が同じ TTL で作られている間だけです。アプリの設定やバージョンで TTL が変わると、古いが長期リースの Listener が新しい短期リースの Listener より後の期限を持ち、死んだ方が選ばれて接続できない行が残ります

サーバの ReachableListenercreated_at を返していないので、堅くするならサーバ側に 1 フィールド足す話になります(#203 の範囲外)。今の実装のままにするなら、「この判定はアプリが常に同じ TTL で Listener を作ることを前提にする」とコメントに書いておくと、TTL を触る人が気づけます。

併せて細かい点として、比較は RFC 3339 の文字列に対する辞書順です。プロキシが常に UTC・Z・秒精度で返す現状では時刻順と一致しますが、オフセット表記や小数秒が混ざると静かに壊れます。ここも前提として一行あると安心です。

4. リリース順序は、このままだとどちらの順でも一時的に壊れる

「#203 をデプロイする前にこれを出す」と書かれていますが、新クライアント + 旧サーバの組み合わせでは POST /v1/peer/grants/v1/peer/pairing-codes が旧サーバに存在せず 404 になります(旧クライアント + 新サーバも同様に 404)。つまり、アプリ配布とサーバデプロイが同時でない限り、どちらを先に出しても切り替えの窓でペアリングと Grant 管理が止まります。

#203 のレビューでも書きましたが、サーバ側で旧ルートを 1 リリースだけ別名として残すのがいちばん安全です(ハンドラは同じで、listener_id は無視して caller を owner とする)。そうすれば「サーバ → クライアント」の順で、どの時点でも壊れない切り替えができます。この PR 側で対処するものではありませんが、リリース順の記述はそのままだと誤解を招くので、前提(アプリはまだ配布していない/同時切り替えができる)を明記しておくのが良いと思います。

5. 細かい点

  • close() の doc に「the listener lapses on its own within the hour」とありますが、Listener の TTL は作成時に指定でき(サーバ側は 60〜86,400 秒)、既定が 3,600 秒です。既定前提であることを書き添えるか、「リースが切れるまで」と一般化しておくと将来ずれません
  • close() が Listener を消すようになったことで、同じ Endpoint が複数セッションを同時に持つ場合、片方の close() は自分の listener_id しか消しません(正しい挙動です)。一方 show_pairing_code / list_grants / revoke_grant は Endpoint 単位になったので、セッションが 2 つあると同じものを 2 か所から操作できます。今のアプリは 1 セッションなので問題ありませんが、ListenerSession のメソッドとして生やしている以上、doc に「これは Endpoint に対する操作でセッションに閉じない」と一言あると誤解が減ります

総評

指摘 1 が、この PR の狙い(閉じたら一覧から消える)がそのまま実現するかどうかに直結するので、実機確認のときに DELETE が飛んでいるかまで見ていただくのが良いと思います。2 は終了時のハングを防ぐ保険、3 はコメントで前提を固定する話、4 は #203 と合わせた運用の判断です。変更そのものはサーバ側の設計変更に正しく追随できていると思います。

🤖 Generated with Claude Code

Cancelling the session only starts the withdrawal. The delete that follows is
an HTTP request over the same msquic registration the process is about to
drain, and nothing was waiting for it — so whether the listener came down was
a race between that request and the drain, which is exactly the outcome this
PR set out to guarantee.

`ServerHandle` now hands back the command loop's join handle, camera-server
parks it where `main` can reach it after the window closes, and `main` awaits
it with a bound before draining msquic. The handle is shared rather than owned
because the app is dropped inside `run_native` and what has to be awaited
outlives it.

The withdrawal itself is bounded too. Without a timeout, closing the window
while the proxy is unreachable would keep the process alive until the
transport gave up on a request whose only purpose is tidiness. Three seconds
inside `close`, six around the whole shutdown, so a slow proxy is decided by
the inner bound rather than by the outer one racing it.

Also documents what `one_per_camera` is resting on, since the listing does not
carry enough to check either assumption: that every listener of one camera is
leased for the same span — vary the TTL and a long-leased dead listener
outranks a short-leased live one — and that `expires_at` keeps arriving in the
one format whose text order is time order. And on `ListenerSession`, that the
pairing-code and grant methods act on the Endpoint rather than on the session
they hang off, while `close` is the one that is about this listener.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@masa-koz

masa-koz commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — you were right that the DELETE was a race, and it was not firing reliably.

1. The withdrawal was not actually awaited. Fixed rather than only noted. ServerHandle now returns the command loop's JoinHandle; camera-server parks it in an Arc<Mutex<Option<_>>> shared with main, because the app is dropped inside run_native and what has to be awaited outlives it; main awaits it with a bound after the window closes and before shutdown_and_exit. The field carries a doc saying that an application which exits without awaiting it races its own cleanup — the next caller should not have to rediscover this.

2. No timeout on the delete. Three seconds inside ListenerSession::close, and six around the whole shutdown in main, so a slow proxy is decided by the inner bound rather than by the outer one racing it. All three outcomes log distinctly — withdrawn, failed, timed out — which is also what will make the hardware check readable.

3. one_per_camera's assumptions. Both written down at the function: that every listener of one camera is leased for the same span (vary the TTL and a long-leased dead listener outranks a short-leased live one, leaving a row that cannot connect), and that expires_at keeps arriving as UTC/Z/whole seconds so its text order is its time order. Also noted that the durable fix is created_at on GET /v1/peer/listeners, which the proxy does not return — not something to bolt on here.

4. Release order. The premise you asked for is now explicit in the PR body: there is no distributed build of these apps and no other consumer of the proxy, so the switch is "rebuild both, then deploy" rather than a staged rollout. That is why #203 removed the old routes outright. The window is however long a rebuild takes and affects nobody else — which is the same reasoning that settled the aliases question on #203.

5.

  • close's doc no longer says "within the hour"; it says the lease ends it, 60s–24h with an hour the default.
  • ListenerSession's pairing-code and grant methods now carry a note that they act on the Endpoint and not on the session — two sessions of one Endpoint see and change the same things, and only close is about this listener.

On the hardware check: I will confirm DELETE /v1/peer-listeners/... reaches the proxy log, so "did not send" and "sent and failed" are distinguishable rather than both showing up as a listener that stayed in the list.

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