Conversation
…deck#782) フロント (Pinia store) が持っていた「メモリ → DB → ネットワーク」の stale-while-revalidate・TTL 判定・in-flight dedup を ServerInfoService に集約。 保存するのは生の検出結果 (nodeinfo software + /api/meta 生 JSON) で、 フォーク解決と feature 判定はアプリ側が読取時に行う — 判定ロジックの更新が 古いキャッシュに埋まる鮮度問題 (旧 servers テーブル) を構造的に解消する。 - V5 migration: server_detections 追加、旧 servers は削除 (24h キャッシュの ため旧データ損失は軽微) - StoredServer / load_servers / get_server / upsert_server は削除 (β方針) - plan_for (TTL 判定) は純関数、SWR 経路は wiremock + temp DB でテスト Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat: server_info SWR キャッシュ (servers → server_detections)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe release replaces the legacy server cache with raw detection storage, adds ChangesServer detection cache
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ServerInfoService
participant Database
participant MisskeyClient
Caller->>ServerInfoService: get_or_fetch(host)
ServerInfoService->>Database: get_server_detection(host)
alt Fresh cache
Database-->>ServerInfoService: fresh ServerDetection
ServerInfoService-->>Caller: cached detection
else Stale cache
Database-->>ServerInfoService: stale ServerDetection
ServerInfoService->>MisskeyClient: re-detect in background
ServerInfoService-->>Caller: stale detection
else Cache miss
Database-->>ServerInfoService: no detection
ServerInfoService->>MisskeyClient: fetch nodeinfo and /api/meta
MisskeyClient-->>ServerInfoService: detection responses
ServerInfoService->>Database: upsert_server_detection
ServerInfoService-->>Caller: detected and stored result
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/db.rs (2)
396-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared row-mapper for
ServerDetection.
load_server_detectionsandget_server_detectionboth inline the same 6-field closure. The codebase already has a convention for this (row_to_account); doing the same here avoids column-order drift if the schema changes.♻️ Suggested extraction
+ fn row_to_server_detection(row: &rusqlite::Row) -> rusqlite::Result<ServerDetection> { + Ok(ServerDetection { + host: row.get(0)?, + software_name: row.get(1)?, + software_version: row.get(2)?, + software_repository: row.get(3)?, + meta_json: row.get(4)?, + updated_at: row.get(5)?, + }) + } + pub fn load_server_detections(&self) -> Result<Vec<ServerDetection>, NoteDeckError> { let conn = self.lock_read()?; let mut stmt = conn.prepare_cached( "SELECT host, software_name, software_version, software_repository, meta_json, updated_at FROM server_detections", )?; - let rows = stmt.query_map([], |row| { - Ok(ServerDetection { - host: row.get(0)?, - software_name: row.get(1)?, - software_version: row.get(2)?, - software_repository: row.get(3)?, - meta_json: row.get(4)?, - updated_at: row.get(5)?, - }) - })?; + let rows = stmt.query_map([], Self::row_to_server_detection)?; Ok(rows.collect::<rusqlite::Result<Vec<_>>>()?) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 396 - 436, Extract the duplicated six-field `ServerDetection` row mapping into a shared helper, following the existing `row_to_account` convention. Update both `load_server_detections` and `get_server_detection` to pass that helper to `query_map`, preserving their current result and optional-row behavior.
394-436: 🚀 Performance & Scalability | 🔵 TrivialNo eviction policy for
server_detections.Unlike
notes_cache/chat_messages_cache, this table has no TTL/cap-based cleanup. Rows are small, so this is likely a non-issue today, but ifServerInfoServiceends up being invoked for many transient remote hosts (link previews, federated timelines), consider adding it tocleanup_with_eviction-style maintenance later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 394 - 436, The current server_detections additions do not include eviction maintenance. Extend the database cleanup flow, specifically cleanup_with_eviction or its existing maintenance path, to apply an appropriate TTL or capacity-based eviction policy to server_detections, while preserving the existing load_server_detections and get_server_detection behavior.src/server_info.rs (1)
56-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
inflighthost-lock map grows without bound.Entries inserted by
host_lock(via.entry(host).or_default()) are never removed, unlike the self-cleaningrevalidatingset right beside it. Over a long-running process, every distinct host that ever hits a cache miss adds a permanentString+Arc<Mutex<()>>entry that's never freed — a slow, unbounded memory leak if the service ever sees many distinct hosts (e.g. via federated content across many instances, not just logged-in accounts).♻️ Suggested cleanup after releasing the per-host guard
CachePlan::Miss => { let lock = self.host_lock(host).await; - let _guard = lock.lock().await; - // ロック待機中に先行リクエストが保存した行を拾う (dedup) - if let Some(det) = self.db.get_server_detection(host)? { - if plan_for(Some(&det), now_ms(), SERVER_DETECTION_TTL_MS) == CachePlan::Fresh { - return Ok(det); - } - } - self.detect_and_store(host).await + let result = { + let _guard = lock.lock().await; + // ロック待機中に先行リクエストが保存した行を拾う (dedup) + if let Some(det) = self.db.get_server_detection(host)? { + if plan_for(Some(&det), now_ms(), SERVER_DETECTION_TTL_MS) + == CachePlan::Fresh + { + Ok(det) + } else { + self.detect_and_store(host).await + } + } else { + self.detect_and_store(host).await + } + }; + // No other waiter holds a clone of this lock — safe to drop the entry. + let mut map = self.inflight.lock().await; + if map.get(host).is_some_and(|l| Arc::strong_count(l) == 1) { + map.remove(host); + } + result }Also applies to: 72-95, 145-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server_info.rs` at line 56, Update the inflight host-lock lifecycle in host_lock so each host entry is removed from the map after its per-host mutex guard is released, matching the self-cleaning behavior of revalidating. Ensure cleanup only removes the entry associated with the completed lock and does not race with a new waiter or lock acquisition for the same host. Keep serialization intact while preventing stale host keys and Arc<Mutex<()>> values from accumulating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server_info.rs`:
- Around line 189-197: Make the `ENV_LOCK` used by `register_insecure_host`
shared across modules, exposing or relocating it so `src/insecure.rs` uses the
same lock before mutating `NOTECLI_INSECURE_HOSTS`. Remove any module-local
duplicate lock and preserve the existing serialized environment-variable
updates.
---
Nitpick comments:
In `@src/db.rs`:
- Around line 396-436: Extract the duplicated six-field `ServerDetection` row
mapping into a shared helper, following the existing `row_to_account`
convention. Update both `load_server_detections` and `get_server_detection` to
pass that helper to `query_map`, preserving their current result and
optional-row behavior.
- Around line 394-436: The current server_detections additions do not include
eviction maintenance. Extend the database cleanup flow, specifically
cleanup_with_eviction or its existing maintenance path, to apply an appropriate
TTL or capacity-based eviction policy to server_detections, while preserving the
existing load_server_detections and get_server_detection behavior.
In `@src/server_info.rs`:
- Line 56: Update the inflight host-lock lifecycle in host_lock so each host
entry is removed from the map after its per-host mutex guard is released,
matching the self-cleaning behavior of revalidating. Ensure cleanup only removes
the entry associated with the completed lock and does not race with a new waiter
or lock acquisition for the same host. Keep serialization intact while
preventing stale host keys and Arc<Mutex<()>> values from accumulating.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd64c19-860d-40a4-ab2a-b44a6dc4191d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlmigrations/V5__server_detections.sqlsrc/db.rssrc/lib.rssrc/models.rssrc/server_info.rs
Comment on lines
+189
to
+197
| static ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); | ||
|
|
||
| fn register_insecure_host(host: &str) { | ||
| let merged = match std::env::var("NOTECLI_INSECURE_HOSTS") { | ||
| Ok(v) if !v.is_empty() => format!("{v},{host}"), | ||
| _ => host.to_string(), | ||
| }; | ||
| std::env::set_var("NOTECLI_INSECURE_HOSTS", merged); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other test modules also mutate NOTECLI_INSECURE_HOSTS and whether they
# share a lock with src/server_info.rs's ENV_LOCK.
rg -n 'NOTECLI_INSECURE_HOSTS' --type=rust -C3
rg -n 'static ENV_LOCK' --type=rust -C3Repository: notedeck-dev/notecli
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Rust files:\n'
git ls-files '*.rs' | sed -n '1,200p'
printf '\nSearch NOTECLI_INSECURE_HOSTS / ENV_LOCK / set_var occurrences:\n'
rg -n 'NOTECLI_INSECURE_HOSTS|static ENV_LOCK|set_var\(' --glob '*.rs' -C 2 || trueRepository: notedeck-dev/notecli
Length of output: 543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tree (top-level and src):\n'
find . -maxdepth 2 -type f | sed 's#^\./##' | sort | sed -n '1,200p'
printf '\nFocused search for NOTECLI_INSECURE_HOSTS in repository:\n'
rg -n 'NOTECLI_INSECURE_HOSTS' . || true
printf '\nFocused search for ENV_LOCK in repository:\n'
rg -n 'static ENV_LOCK|ENV_LOCK' . || trueRepository: notedeck-dev/notecli
Length of output: 1787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'src/server_info.rs around the referenced lines:\n'
sed -n '150,240p' src/server_info.rs
printf '\nAll Rust files containing NOTECLI_INSECURE_HOSTS or ENV_LOCK:\n'
rg -n 'NOTECLI_INSECURE_HOSTS|static ENV_LOCK|Mutex::const_new|set_var\(' src --glob '*.rs' -C 3 || trueRepository: notedeck-dev/notecli
Length of output: 6236
Share the NOTECLI_INSECURE_HOSTS test lock across modules. src/insecure.rs also mutates this env var directly, so ENV_LOCK here doesn’t stop parallel tests in the same binary from racing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server_info.rs` around lines 189 - 197, Make the `ENV_LOCK` used by
`register_insecure_host` shared across modules, exposing or relocating it so
`src/insecure.rs` uses the same lock before mutating `NOTECLI_INSECURE_HOSTS`.
Remove any module-local duplicate lock and preserve the existing serialized
environment-variable updates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
変更
ServerInfoService— SWR (fresh 即返し / stale 返却+背景再検出 / miss は per-host dedup で検出+保存)server_detections(生 nodeinfo + meta 保存、読取時解決) を追加し旧serversテーブルを削除StoredServer/load_servers/get_server/upsert_serverを削除 (β方針)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes