feat(server): composite session state and long-poll wait endpoint

[andybons]

Summary

Closes the orchestration ambiguity that caused real monitor false-fires: sessions now surface a composite state (idle | busy | goal-running — goal-active wins over momentary worker idle; old status kept for compat), and GET /session/{id}/wait?until=idle|goal-done&timeout_s=N long-polls on the existing subscriber infrastructure — immediate return when the condition pre-holds, terminal kind (achieved/cleared) in the response, context-clean, no waiter leak on disconnect. OpenAPI updated with named schemas.

Design lineage: docker wait / kubectl wait --for=condition — sessions as orchestrated resources.

Fifth attempt at this feature; the prior four deaths yielded the goal-loop hardening (#38, #42, #43, #49) that this run certified by completing on it. Box session ses_01kx3zjvx1e6nr6vh14gt4gnx9, box-pushed, origin-verified.

🤖 Generated with Claude Code

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 4m 28s —— View job

---

Review: composite session state + long-poll wait endpoint

I read AGENTS.md first, then reviewed all six files with a focus on the concurrency/protocol priorities called out for this repo. Built (go build ./...), vetted, and ran the new tests under -race (go test -race -run 'TestWait|TestCompositeState' ./server/ → green).

• Read AGENTS.md and gather context
• Review server/wait.go (long-poll, waiter lifecycle, leak on disconnect)
• Review server/handlers.go (composite state, endpoint wiring)
• Review server/journal.go and server/server.go
• Review server/openapi.yaml schemas
• Review server/wait_test.go against AGENTS.md testing rules
• Post inline comments + summary

Verdict

This is a strong, carefully-reasoned PR. The wait machinery is correct on the hard parts, and the doc comments make the invariants auditable. No blocking issues found — one minor efficiency nit (inline).

Concurrency & missed-wakeup analysis (the part that matters most here):

• Register-before-check ordering is correct. The waiter is inserted into s.waiters before the immediate waitConditionMet check (wait.go:69-82), so an event racing the check can't be lost: it either lands before registration (immediate check sees it) or after (buffered wake is pending). The buffered-1, non-blocking, coalescing send in notifyWaitersLocked is safe precisely because the waiter never trusts the payload and always re-derives via waitSnapshot — a coalesced/dropped wake can't strand it.
• No lock-order violation. notifyWaitersLocked runs under s.mu (session.mu → server.mu path via publishGoal), does only non-blocking channel sends, and never calls back into a session. The woken handler's waitSnapshot acquires s.mu only and reads server-owned state (st.running, goalState) — no session method, so no cycle. Consistent with the leaf-lock invariant documented in server.go:116 / journal.go:216.
• State transitions are consistent under the lock. publishGoal mutates goalState and calls emitDurableLocked→notifyWaitersLocked in one s.mu critical section, so a woken waiter always observes the post-event snapshot. Verified the goal.achieved case where running is briefly still true after goal.active=false — until=goal-done correctly resolves (ignores running), until=idle correctly keeps waiting until the trailing idle event. No flake window.
• No waiter leak. Every return path (immediate, timer, wt.ch, r.Context().Done(), s.closing) is covered by the single defer unregister. Disconnect cleanup is driven by request-context cancellation, not server polling — and TestWaitDisconnectDoesNotLeakWaiter proves it.

Testing (per AGENTS.md): exemplary. Timer path is inside a synctest bubble with fake time (TestWaitTimeoutReturnsCleanly), no raw time.Sleep anywhere, production timer uses time.NewTimer + defer Stop(), and the bubble carefully unwinds the blocked goal goroutine before exit to satisfy the leak detector. The gate-provider pattern (workerStarted/workerReleased) lets the "unblock via the notify mechanism, not the fast path" claim be proven deterministically via waitForWaiterCount. Red-first intent is documented per test.

OpenAPI: thorough — named SessionState, StatusEntry, StatusResponse, WaitResponse schemas, state correctly added to Session.required, and the "200 whether met or timed out; caller distinguishes via state/goal" contract is documented on both the schema and the operation.

Notes (all non-blocking)

1. Wasted disk load on the long-poll path — handleWait uses s.lookup(id) purely as an existence check, which fully deserializes a non-resident session just to discard it. Inline suggestion left on wait.go:67.
2. Coverage gap (minor): the until=idle unblock path is exercised indirectly, but there's no direct test of until=idle waking on a plain prompt completion (only goal terminals). The mechanism is shared, so low risk — worth a one-liner if you want symmetry with the goal-done tests.
3. Shutdown vs. timeout are both 200 with unmet state — the <-s.closing branch returns the current (possibly unmet) snapshot, indistinguishable from a timeout to the client. This is defensible (client re-checks state), but the OpenAPI 200 description only mentions "met or timed out", not the drain case. Consider a sentence so orchestrators know a wait can return early on server drain.

Startup-speed check

No init-time network/disk/subprocess work introduced. The wait path's only disk touch is note (1), which is per-request, not boot. ✅
· feat/wait-api3

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 3m 12s —— View job

---

Review: composite session state + long-poll wait endpoint

Read AGENTS.md first, then reviewed all six files with the repo's concurrency/protocol priorities in mind. go build ./... ✅, go vet ./server/ ✅, and go test -race -run 'TestWait|TestCompositeState' ./server/ → green.

• Read AGENTS.md and gather context
• Review server/wait.go (long-poll, waiter lifecycle, leak on disconnect)
• Review server/handlers.go (composite state, wiring)
• Review server/journal.go and server/server.go
• Review server/openapi.yaml schemas
• Review server/wait_test.go against AGENTS.md testing rules
• Post review

Verdict

Solid PR — no blocking issues. The prior round's one inline nit (the long-poll existence check fully deserializing an evicted session) is resolved in d4ce315: handleWait now does the cheap resident || sessionOnDisk(id) check (wait.go:64-70), matching handleAbort/handleGoalDelete, and TestWaitColdSessionDoesNotLoad guards it. Good fix.

Concurrency / missed-wakeup (the part that matters here):

• Register-before-check is correct. The waiter goes into s.waiters (wait.go:72-75) before the immediate waitSnapshot/waitConditionMet check (wait.go:82). An event racing the check either lands before registration (the snapshot reflects it) or after (a buffered wake is pending on wt.ch). Coalescing buffered-1 sends are safe precisely because the waiter never trusts the payload — it re-derives via waitSnapshot on every wake.
• Every wait-relevant transition emits a durable event, so it always notifies. running false→true (handlePrompt/handleGoal emit busy), running true→false (runPrompt/runGoal emit idle), goal set/achieved/cleared (all via publishGoal) — all route through emitDurableLocked → notifyWaitersLocked (journal.go:277). I could not find a state change that mutates running/goalState without a paired durable emit. The st.running = false in runGoal is set under s.mu, released, then the idle emit notifies — so a woken waiter always observes the post-transition snapshot. No flake window.
• No lock-order violation. notifyWaitersLocked runs under s.mu, does only non-blocking sends, and never re-enters a session. waitSnapshot takes only s.mu and reads server-owned state (st.running, goalState) — no session method — so the leaf-lock invariant (server.go:116) holds.
• No waiter leak. Every exit (immediate, timer.C, wt.ch, r.Context().Done(), s.closing) is covered by the single deferred unregister; disconnect cleanup is context-driven, proven by TestWaitDisconnectDoesNotLeakWaiter. The timer is created once and not reset across spurious wakes, so the total timeout budget is preserved.
• Post-eviction terminals are still correct. goalState is never evicted, so until=goal-done on an evicted-but-previously-goal'd session still returns the right achieved/cleared terminal rather than a spurious "no goal". Nice.

Testing (per AGENTS.md): exemplary. Timer path is inside a synctest bubble on fake time (TestWaitTimeoutReturnsCleanly), which then unwinds the blocked goal goroutine before bubble end to satisfy the leak detector; production timer uses time.NewTimer + defer Stop(); no raw time.Sleep anywhere. The gateProvider workerStarted/workerReleased gating lets "unblocks via the notify path, not the fast path" be proven deterministically via waitForWaiterCount, and each test documents red-first intent.

OpenAPI: thorough — named SessionState/StatusEntry/StatusResponse/WaitResponse, state added to Session.required, and the "200 whether met or timed out; caller distinguishes via state/goal" contract documented on both schema and operation.

Non-blocking notes (optional)

1. Drain returns early as an unmet 200 — the <-s.closing branch (wait.go:95-100) returns the current, possibly-unmet snapshot, indistinguishable from a timeout. Defensible (client re-checks state), but the WaitResponse/200 docs still only mention "met or timed out" — a sentence about the server-drain early return would let orchestrators know a wait can end before timeout_s on shutdown.
2. Coverage symmetry (minor): the goal terminals have direct unblock tests (TestWaitUnblocksOnGoalCompletion/...Cleared), but there's no direct test of until=idle waking on a plain prompt completion. The mechanism is shared (both ride emitDurableLocked), so low risk — a one-liner would close the gap.
3. waitForWaiterCount spin-polls in-process state with runtime.Gosched (wait_test.go:75-87). It's not a sleep and follows the existing journalHasIdle precedent, and the real-HTTP tests can't use synctest (they need a live r.Context() for the disconnect case), so it's a reasonable deviation from the strict channel rule — noting only for awareness.

Startup-speed check

No init-time network/disk/subprocess work. The only disk touch on the wait path is sessionOnDisk's directory listing, per-request and not on boot. ✅
· feat/wait-api3

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 4m 9s —— View job

---

Review: composite session state + long-poll wait endpoint

Read AGENTS.md first, then reviewed all six files with this repo's concurrency/protocol priorities in mind. go build ./server/ ✅, go test -race -run 'TestWait|TestCompositeState' ./server/ → green.

• Read AGENTS.md and gather context
• Review server/wait.go (long-poll, waiter lifecycle, leak on disconnect)
• Review server/handlers.go (composite state, wiring)
• Review server/journal.go / server/server.go
• Review server/openapi.yaml schemas
• Review server/wait_test.go against AGENTS.md testing rules
• Post review

Verdict

Strong PR. The prior two rounds' notes are all now addressed: the cold-session existence check (d4ce315, TestWaitColdSessionDoesNotLoad), the drain early-return doc on WaitResponse/200 (c78967d), and the direct until=idle-on-plain-prompt unblock test (8335958, TestWaitUnblocksOnPromptIdle). The concurrency core is correct. I found one new minor logic bug (inline on wait.go) — not blocking.

Concurrency / missed-wakeup (the part that matters here):

• Register-before-check is correct. The waiter is inserted into s.waiters (wait.go:77-79) before the immediate waitSnapshot/waitConditionMet check (wait.go:86). An event racing the check either lands before registration (the snapshot reflects it) or after (a buffered wake is pending on wt.ch). The non-blocking, buffered-1, coalescing send in notifyWaitersLocked is safe precisely because the waiter never trusts the payload — it re-derives via waitSnapshot on every wake.
• Every wait-relevant transition emits a durable event, so it always notifies. running true→false (runPrompt/runGoal emit idle), goal set/achieved/cleared (all via publishGoal) — all route through emitDurableLocked → notifyWaitersLocked (journal.go:277). I could not find a running/goalState mutation without a paired durable emit. In runGoal, st.running=false is set under s.mu, released, then the idle emit notifies — so a woken waiter always observes the post-transition snapshot. The goal.achieved-then-idle ordering for until=idle is correct (goal inactive but still running → busy → keeps waiting until the trailing idle event; that event notifies, no flake window).
• No lock-order violation. notifyWaitersLocked runs under s.mu, does only non-blocking sends, never re-enters a session; waitSnapshot takes only s.mu and reads server-owned state — leaf-lock invariant (server.go:116) holds.
• No waiter leak. Every exit (immediate, timer.C, wt.ch, r.Context().Done(), s.closing) is covered by the single deferred unregister; disconnect cleanup is context-driven (TestWaitDisconnectDoesNotLeakWaiter). The timer is created once and not reset across spurious wakes, so the total timeout budget is preserved.

Testing (per AGENTS.md): exemplary. Timer path is inside a synctest bubble on fake time (TestWaitTimeoutReturnsCleanly) that unwinds the blocked goal goroutine before bubble end; production timer is time.NewTimer + defer Stop(); no raw time.Sleep. The gateProvider gating proves "unblocks via notify, not the fast path" deterministically, and TestWaitUnblocksOnPromptIdle's elapsed-time bound (< 5s) is what actually makes it fail on a broken notify rather than surviving to the 30s deadline. waitForWaiterCount's runtime.Gosched spin is a reasonable, documented deviation for the real-HTTP disconnect case (can't use synctest with a live r.Context()).

Finding (non-blocking)

parseWaitTimeout caps after multiplying, so a huge timeout_s overflows int64 → negative duration → immediate return (wait.go:129-137). time.Duration(n) * time.Second overflows for n ≳ 9.2e9 (timeout_s=10000000000 → 1e19 > int64 max → wraps negative); the negative value slips past the > maxWaitTimeout check and time.NewTimer(negative) fires at once. This contradicts the function's own contract ("a generous client asking for the moon still gets a bounded wait"). Fix: cap the integer before converting to a Duration. Reachable on 64-bit builds; on 32-bit Atoi rejects it (platform-divergent). Inline suggestion + a proposed overflow-sized table-test row left on wait.go.

OpenAPI

Thorough — named SessionState/StatusEntry/StatusResponse/WaitResponse, state added to Session.required, and the drain early-return case now documented on both WaitResponse and the 200.

Startup-speed check

No init-time network/disk/subprocess work. The only wait-path disk touch is sessionOnDisk's directory listing, per-request, not on boot. ✅
· feat/wait-api3

[claude]

Claude finished @andybons's task in 2m 39s —— View job

---

Review: composite session state + long-poll wait endpoint

• Read AGENTS.md and gather context
• Review server/wait.go (long-poll, waiter lifecycle, overflow fix)
• Review server/handlers.go / server/journal.go / server/server.go
• Review server/openapi.yaml schemas
• Review server/wait_test.go against AGENTS.md testing rules
• Post review

Read AGENTS.md first, then reviewed all six files with this repo's concurrency/protocol priorities in mind. go build ./server/ ✅, and go test -race -run 'TestWait|TestParseWaitTimeout|TestCompositeState' ./server/ → green.

Verdict

Strong PR, and the one real bug from the prior round — the parseWaitTimeout int64 overflow — is now fixed correctly in 4cbbe3b. parseWaitTimeout caps the integer seconds before time.Duration(n) * time.Second (wait.go:138-141), so an oversized timeout_s clamps to 300s instead of wrapping to a negative duration that fired the timer immediately. TestParseWaitTimeout's "10000000000" row (wait_test.go:261) is the red-first guard, and it asserts both the capped value and got >= 0. No blocking issues remain; I found nothing new to add.

Concurrency / missed-wakeup (the part that matters most here):

• Register-before-check is correct. The waiter goes into s.waiters (wait.go:77-79) before the immediate waitSnapshot/waitConditionMet check (wait.go:86). An event racing the check either lands before registration (the snapshot reflects it) or after (a buffered wake is pending on wt.ch). The non-blocking, buffered-1, coalescing send in notifyWaitersLocked (journal.go:287) is safe precisely because the waiter never trusts the payload — it re-derives via waitSnapshot on every wake.
• Every wait-relevant transition emits a durable event. running true→false (runPrompt/runGoal emit idle) and goal set/achieved/cleared (all via publishGoal) route through emitDurableLocked → notifyWaitersLocked. I could find no running/goalState mutation without a paired durable emit. In runGoal, st.running=false is set under s.mu, released, then the idle emit notifies — so a woken waiter always observes the post-transition snapshot.
• Composite state closes the until=idle gap cleanly. running stays true for the whole PursueGoal call, and even if it didn't, goalActive wins in compositeState → goal-running, never idle. So until=idle can't fire spuriously during the goal's between-turn gap — exactly the ambiguity this field exists to remove.
• Terminals survive eviction. goalState is never evicted, so until=goal-done on an evicted-but-previously-goal'd session still resolves to the correct achieved/cleared terminal; waitSnapshot treats a nil resident session as idle, which is correct for a non-resident (non-running) session.
• No lock-order violation, no waiter leak. notifyWaitersLocked runs under s.mu, does only non-blocking sends, never re-enters a session; waitSnapshot takes only s.mu and reads server-owned state — the leaf-lock invariant (server.go:116) holds. Every exit path (immediate, timer.C, wt.ch, r.Context().Done(), s.closing) is covered by the single deferred unregister; disconnect cleanup is context-driven (TestWaitDisconnectDoesNotLeakWaiter).

Testing (per AGENTS.md): exemplary. Timer path is inside a synctest bubble on fake time (TestWaitTimeoutReturnsCleanly) that unwinds the blocked goal goroutine before bubble end; production timer is time.NewTimer + defer Stop(); no raw time.Sleep. TestWaitUnblocksOnPromptIdle closes the prior round's coverage-symmetry note (direct until=idle-on-plain-prompt unblock), and its < 5s elapsed bound is what actually makes it fail on a broken notify rather than riding the timer to 30s. The overflow regression is red-verified per its doc comment. waitForWaiterCount's runtime.Gosched spin remains a reasonable, documented deviation for the real-HTTP disconnect case (can't use synctest with a live r.Context()).

OpenAPI: complete — named SessionState/StatusEntry/StatusResponse/WaitResponse, state added to Session.required, and the drain early-return case documented on both WaitResponse and the 200 response (openapi.yaml:807-813).

Startup-speed check

No init-time network/disk/subprocess work introduced. The only wait-path disk touch is sessionOnDisk's directory listing — per-request, not on boot. ✅

All three prior rounds' notes are now resolved (cold-session check d4ce315, drain doc c78967d, plain-prompt idle test 8335958, overflow fix 4cbbe3b). LGTM.
· feat/wait-api3