Skip to content

poll: return closed when the review ended before the poll started - #20

Open
BioInfo wants to merge 1 commit into
petergyang:mainfrom
BioInfo:fix/poll-after-session-end
Open

poll: return closed when the review ended before the poll started#20
BioInfo wants to merge 1 commit into
petergyang:mainfrom
BioInfo:fix/poll-after-session-end

Conversation

@BioInfo

@BioInfo BioInfo commented Aug 8, 2026

Copy link
Copy Markdown

The bug

endSession hands {"status":"closed"} only to pollers already registered on the entry key. But the loop SKILL.md prescribes is apply-then-poll, so the poll that follows a delivered batch normally starts after the human has clicked End review. That poll registers on a target nothing will ever close and blocks for its whole --timeout.

Hit it live today reviewing a Markdown draft: poll returned a batch, I applied it, the reviewer clicked End review, and the next poll --ack had nothing to end it.

Reproduction

open a session on a file
POST /api/session/<id>/end      # no poll in flight
human-review poll <file> --timeout 20

Before: exits after the full 20s with {"status":"timeout"}.
After: exits in under 100ms with {"status":"closed"}.

The new test in test/poll-handoff.test.js asserts exactly this. Run against main it fails at 20,125ms; with the change it passes in ~2.1s. Full suite 91/91.

The change

endSession leaves an end mark in the store when the last session for a target goes away, and /api/poll consumes it before registering a poller. Three ordering details:

  • The mark is checked after the pending batch, so feedback the human already sent is never hidden by it.
  • Opening the target again clears it, so a resumed review waits for real feedback rather than reporting closed twice. The test covers this.
  • endSession consumes the mark for each poller it drains directly, so the two delivery paths cannot both fire.

It is persisted rather than held in memory because the server idles out after 45 minutes (IDLE_SHUTDOWN_MS), and a poll arriving after that would otherwise hang against a fresh process for the same reason. Verified by killing the server between the end and the poll: the fresh server answers closed in 0s.

No response vocabulary changed, so no SERVER_PROTOCOL bump and agents need no update. The closed payload moved to one SESSION_CLOSED constant since two paths now send it.

Notes

  • No reformatting; the diff is +61/-12 in src/ plus the test.
  • Written with Claude Opus 5. Tests were run locally on Node 26 (91 passing), and the before/after timings above are measured, not estimated. Happy to re-run or restructure anything.

endSession only hands {"status":"closed"} to pollers already registered on
the entry key. The documented agent loop is apply-then-poll, so the poll that
follows a delivered batch usually starts after the human has already clicked
End review. That poll registers on a target nothing will ever close and blocks
for its whole --timeout.

The server now leaves an end mark in the store when the last session for a
target goes away, and /api/poll consumes it before registering a poller. The
mark is checked after the pending batch, so feedback the human already sent is
never hidden by it, and opening the target again clears it.

It is persisted rather than held in memory because the server idles out after
45 minutes, and a poll arriving after that would otherwise hang against a fresh
process for the same reason.
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