Repository navigation
fix(pending-questions): pollers reuse one room read instead of a socket per poll - #5644
qingyun-wu wants to merge 1 commit into
Conversation
…et per poll Every pending-questions gather runs the adapter's `serve`, which opens the owner room's `db` socket, syncs it, settles 1 s and closes. Since the store moved into a room database, the web client's 3 s /tasks/active poll and the dashboard's 15 s reload each did that on every poll: about 24 db socket opens a minute per agent with either page open. The reader now offers gather_recent / count_recent: a read no older than POLL_REUSE_SEC (60 s) is reused, concurrent callers wait on the one in-flight read, and resolve / reconcile_pass through the reader drop what it held (a write that lands during a read keeps that read from being stored). /tasks/active and the dashboard count use it; the triage queue, /answer and the one-shot readers stay fresh. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
liususan091219
left a comment
There was a problem hiding this comment.
No change-request condition fires at 32387051c. What's missing is a review: the PR has none (reviewDecision: REVIEW_REQUIRED) and no unresolved threads. An expert on the pending-questions reader or agent-api should give a substantive approval.
What I checked
- Read the whole diff in
src/pending_questions_reader.py,src/agent-api.pyandsrc/dashboard.py./tasks/activeand the dashboard count now use the cached read./questions/queue,/answerand the one-shot readers still callgatherdirectly (src/agent-api.py:480,502,541;src/obsidian-mirror.py:175;src/morning-briefing.py:673;src/friction-detector.py:53).answer_questioncloses throughpending_questions_reader.resolve, so in agent-api a resolved question leaves the next/tasks/activepoll straight away.
- The guard against a write landing during a read looks right. The counter is read under the per-key lock before the gather, and
forget_recentraises it. A write either during the read, or after the adapter returns but beforeforget_recent, stops the result being stored. That errs on the side of a fresh read. - Ran the PR's tests with
SUTANDO_WORKSPACE_DIRandSUTANDO_TASKS_DIRunset:tests/pending-questions-poll-reuse.test.py: parentdbb0d45source givesFAILED (failures=2, errors=10); HEAD givesOK(12 tests). This matches the PR body.- All 22
tests/pending-questions-*.test.py, plustests/agent-api-pending-questions.test.pyandtests/dashboard-usecase-empty-state.test.py, pass at HEAD.
- Changed the code six ways to see which changes the new suite catches. Five fail it as they should:
- always storing the read (in-flight write guard removed);
- no
forget_recentinreconcile_pass; - no
forget_recentinresolve; - no per-key lock;
- no expiry.
- The new tests exercise the real module through a counting adapter. None of them grep source or reimplement the function under test.
- Looked for other pollers the PR might have missed.
/questions/queueis fetched only when a person opens the queue (src/web-client.ts:3645), and the othersetIntervalpollers insrc/don't hit a gather route. No sibling site was left out. - Added lines contain no hardcoded paths. All CI checks pass.
license/clais present andsuccess(one status context). No other open PR covers the same change.
Non-blocking notes
src/pending_questions_reader.py:180: the copy made on a cache hit isn't covered by any test. Changing it toreturn hit[1]still passes everything.test_a_caller_cannot_change_what_the_next_one_gets(tests/pending-questions-poll-reuse.test.py:135) clears the first, uncached result, which was copied on the other path. A thirdgather_recentafter changing a cached result would cover it.src/pending_questions_reader.py:161: on macOS,time.monotonicstops while the machine sleeps. A read cached before sleep can still be served for up to 60 s of awake time after wake, even if hours have passed. The delay is still at most 60 s, so this is a note, not an ask.- A failed read ("room unreachable") is also kept for 60 s. The test treats this as intended, so a room that comes back can show as unknown for up to a minute. That seems fine; it just might be worth one sentence in the PR body.
tests/dashboard-usecase-empty-state.test.pyonly rewords a docstring and renames a test, which has nothing to do with this fix. It's harmless, but it is a second concern in a single-concern PR.
Not verified
- No live round trip after a restart. The PR measures socket opens with a local
poll_loop.pyharness, not a restarted agent-api and dashboard against a real room. Agent-api serves live requests, so that round trip is still owed. Per our policy it is noted here, not blocking. - I did not re-measure the prd room-doc open counts quoted in the PR body.
— Sutando (susans-MacBook-Pro)
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 32387051 (unchanged since my 2026-10-17 review). Verdict: comment. No change-request condition fires; not approving because there is no expert approval.
Read again at this head
gather_recent(src/pending_questions_reader.py) reuses a gather for up to 60 s per (workspace, adapter). Concurrent callers wait on one per-key lock. Aforget_recentduring a read bumps the generation, so that read is not stored.resolveandreconcile_passcallforget_recentinfinally.- Only the two pollers switch:
/tasks/active(_pending_gather_polled) and the dashboard count (count_recent)./questions/queue,/answerand the one-shot readers still callgather. - The tests drive the real module through a counting adapter; no source-grep and no in-file copy of the unit.
- CI green,
license/cla: success(1 context), no unresolved threads, no added hardcoded paths. StillMERGEABLEwithmain.
New since my last review (non-blocking): main has since added clicks / due_clicks / ack to the same reader (#5946). ack is a write through this module that this PR does not invalidate. Today it is only called from the CLI, a separate process, so the agent-api cache is not affected beyond the 60 s staleness the PR already accepts across processes. If agent-api ever calls ack in-process, it should call forget_recent like resolve.
Still missing before approval
- A substantive APPROVED review from a write-access expert on the pending-questions reader or agent-api. The PR has no reviews other than mine.
Owed, not blocking: a live round trip with agent-api and the dashboard restarted against a real room. The socket-count evidence comes from a local harness.
— Sutando (susans-MacBook-Pro)
What
Pollers of the owner's pending questions reused nothing: every
gatherruns thepending-questions adapter's
serve, which opens the owner room's?kind=dbsocket, syncs,settles 1 s and closes. Since #5027 moved the store into a room database (merged
2026-10-03 17:48Z), two pollers do that on every tick:
setInterval(..., 3000)onGET /tasks/active(src/web-client.ts) →_active_tasks_payload→_pending_gather()(src/agent-api.py);location.reload()→get_pending_count()(src/dashboard.py).That is ~24 db socket opens a minute per agent with either page open.
This PR adds
gather_recent/count_recenttosrc/pending_questions_reader.py, the onecore reader both edges already go through:
POLL_REUSE_SEC(60 s) is reused;resolveandreconcile_passthrough the reader drop what it held, and a write thatlands while a read is in flight keeps that read from being stored.
/tasks/activeand the dashboard count use it./questions/queue(shown by a person),/answer, and the one-shot readers (briefing, friction, obsidian-mirror, CLI) still read fresh.A question filed by another process shows up in the polled views within 60 s.
Why: prd evidence (room-doc logs, read-only)
Agent socket opens per day (
room-doc:open … (agent)):The step starts the evening of 10-03, a few hours after #5027 merged. On 10-15, sockets by
kind:
db34,506,html16,379,markdown1,700, other kinds <1k each. Each socket lives~0.2–1 s (
connection open→connection closed), which matchesserve'ssettle(1.0). Per-agent db cadence matches the two pollers: 2.4–3 s gaps(
vanessaxiaofen-sutando.agent,ruiwangwarm,selfsoothingcare,vidhu22agents) is the 3 s/tasks/activepoll; 15–21 s (sutando-sonichiat a steady ~21 s,kchen6red-m1max.agent) isthe 15 s dashboard reload plus the render it waits on.
Before / after: opens per minute in a local loop
poll_loop.pyloadsagent-api.pyanddashboard.pyfrom a checkout, injects an adapterwhose every gather opens one TCP connection to a local counting listener (standing in for
serve's socket), and polls/tasks/activeevery 3 s plus the dashboard every 15 s for areal 60 s:
Tests
New
tests/pending-questions-poll-reuse.test.py: a 3 s poll for a minute opens once; the windowexpires;
count_recentmatchescount;gatheris never reused;resolveandreconcile_passinvalidate; a write during an in-flight read is not cached; 8 concurrentpollers share one read; callers get copies; an unreachable room stays unknown;
/tasks/activex20 = 1 open while
/questions/queuestays fresh; dashboard x10 = 1 open.Existing tests updated where they stand in for the reader: the dashboard delegate test now
asserts
count_recent, and two agent-api tests that patchresolve/gathercallforget_recentas the realresolvedoes. Alltests/agent-api-*,tests/dashboard-*andtests/pending-questions-*pass at HEAD;scripts/review-checks.shpasses.Expected reduction
Per agent with the web client and dashboard open: 24 → 2 db opens a minute (agent-api and the
dashboard are separate processes, one read each per minute), about 90%. Fleet-wide, the
dbshare (~34.5k of ~57.5k opens on 10-15) should fall to a few thousand a day.
Not addressed here (not our code): the remaining
htmlvolume is two agents re-reading onepage each on a loop,
html-2rbfbqzwin!IPVrHdzViRcaITCWZXevery ~11.6 s since 10-02 andhtml-ozphai8qin!duvkIeoPZtNujenOSbevery ~6–13 s since 10-07, about 15k opens a daybetween them.
🤖 Generated with Claude Code