Service links: split the probe and reader components, give DNS lookups their own pool #677

New issue

Open

opened 2026-10-02 08:45:13 +00:00 by claude · 2 comments

claude commented

2026-10-02 08:45:13 +00:00

Member

The service-link code works but is crowded: wide positional parameters, multi-type files, a probe with too many duties. One item (DNS starvation) is a robustness defect, not only structure.

Found during IW-652 (per-user dashboard, service links, Claude session links). Follow-ups, verbatim:

WorktreeListView.render (dashboard/jvm/src/presentation/views/WorktreeListView.scala) has no production caller; only WorktreeListViewTest calls it.
Service links are threaded as a positional parameter through ~25 view signatures, and ProjectDetailsView.WorktreeCardData is an 8-tuple; make it a named case class — review-phase-02-20261001-133339.md composition W1.
ServicePortProbe holds a pool, two TTL caches and the deadline policy; extract a TTL cache and a deadline runner — review-phase-02-20261001-133339.md composition W3.
dashboard/jvm/src/domain/ServiceLink.scala holds six public types, and ServicePortProbe.scala also holds BindAddresses; split them into files — review-phase-02-20261001-133339.md style W1, W2.
ServicePortProbe runs DNS lookups and TCP connects on one 16-thread pool, and a cancelled lookup keeps its thread, so many slow Host names can starve probing; give resolution its own smaller pool — review-phase-02-20261001-133339.md security W2.
ServiceLinksReader reads declarations, de-duplicates log lines and builds Unreadable from three kinds of cause; extract the log-on-change rule as its own small component, also used by ClaudeSessionsReader — review-phase-02-20261001-135259.md composition W1.

The service-link code works but is crowded: wide positional parameters, multi-type files, a probe with too many duties. One item (DNS starvation) is a robustness defect, not only structure. Found during IW-652 (per-user dashboard, service links, Claude session links). Follow-ups, verbatim: - [ ] `WorktreeListView.render` (`dashboard/jvm/src/presentation/views/WorktreeListView.scala`) has no production caller; only `WorktreeListViewTest` calls it. - [ ] Service links are threaded as a positional parameter through ~25 view signatures, and `ProjectDetailsView.WorktreeCardData` is an 8-tuple; make it a named case class — review-phase-02-20261001-133339.md composition W1. - [ ] `ServicePortProbe` holds a pool, two TTL caches and the deadline policy; extract a TTL cache and a deadline runner — review-phase-02-20261001-133339.md composition W3. - [ ] `dashboard/jvm/src/domain/ServiceLink.scala` holds six public types, and `ServicePortProbe.scala` also holds `BindAddresses`; split them into files — review-phase-02-20261001-133339.md style W1, W2. - [ ] `ServicePortProbe` runs DNS lookups and TCP connects on one 16-thread pool, and a cancelled lookup keeps its thread, so many slow `Host` names can starve probing; give resolution its own smaller pool — review-phase-02-20261001-133339.md security W2. - [ ] `ServiceLinksReader` reads declarations, de-duplicates log lines and builds `Unreadable` from three kinds of cause; extract the log-on-change rule as its own small component, also used by `ClaudeSessionsReader` — review-phase-02-20261001-135259.md composition W1.

mph referenced this issue from a commit

2026-10-02 08:47:47 +00:00

workflow: Triage lessons for IW-652

claude added the

needs-human

label

2026-10-03 04:03:45 +00:00

claude commented

2026-10-03 04:03:45 +00:00

Author

Member

Intake: needs-human
Workflow: waterfall
Reason: G7 — the separate DNS pool only delays starvation under a sustained flood (rejected lookups after the 10 s entry expiry show NotChecked to all users); accept that residual, or add stale-on-reject / pool-free localhost and IP literals
Analysis: project-management/issues/IW-677/analysis.md on branch IW-677

Intake: needs-human Workflow: waterfall Reason: G7 — the separate DNS pool only delays starvation under a sustained flood (rejected lookups after the 10 s entry expiry show NotChecked to all users); accept that residual, or add stale-on-reject / pool-free localhost and IP literals Analysis: project-management/issues/IW-677/analysis.md on branch IW-677

claude commented

2026-10-06 18:16:57 +00:00

Author

Member

Intake: needs-input
Workflow: waterfall
Reason: G7 (1 [human] finding)
Analysis: project-management/issues/IW-677/analysis.md on branch IW-677

[G7] The analysis gives DNS lookups their own pool of 4 threads and says this "makes starvation impossible by construction" (analysis.md:19-21, :33, :179). The review found that under a sustained flood it only delays the impact: a name that hangs never enters the cache, so it holds all 4 resolver threads until the OS resolver gives up; the tailnet name's cache entry then expires after 10 s (ResolutionLifetimeMs, ServicePortProbe.scala:273), every later lookup of it is rejected at once, and every card shows NotChecked for all users again. Risk 1 rates this Low/Low (analysis.md:311-314). What should IW-677 build? Options: a) accept the residual: build the separate pool as analysed, and correct the analysis to say it delays starvation, not prevents it b) also serve the stale cache entry when the pool rejects a lookup c) also resolve localhost and IP literals without the pool, so they never wait for it d) both b) and c) Recommended: a)

Answer in a comment on this issue; intake reads it on its next iteration.

Intake: needs-input Workflow: waterfall Reason: G7 (1 [human] finding) Analysis: project-management/issues/IW-677/analysis.md on branch IW-677 1. [G7] The analysis gives DNS lookups their own pool of 4 threads and says this "makes starvation impossible by construction" (analysis.md:19-21, :33, :179). The review found that under a sustained flood it only delays the impact: a name that hangs never enters the cache, so it holds all 4 resolver threads until the OS resolver gives up; the tailnet name's cache entry then expires after 10 s (`ResolutionLifetimeMs`, `ServicePortProbe.scala:273`), every later lookup of it is rejected at once, and every card shows `NotChecked` for all users again. Risk 1 rates this Low/Low (analysis.md:311-314). What should IW-677 build? Options: a) accept the residual: build the separate pool as analysed, and correct the analysis to say it delays starvation, not prevents it b) also serve the stale cache entry when the pool rejects a lookup c) also resolve `localhost` and IP literals without the pool, so they never wait for it d) both b) and c) Recommended: a) Answer in a comment on this issue; intake reads it on its next iteration.

claude added

needs-input

and removed

needs-human

labels

2026-10-06 18:16:57 +00:00