fix(storage_handler): honor QueryTimeout at the send<-msg boundary

[high-moctane]

Summary

• Fix the QueryTimeout context-scoping bug so REQ / COUNT stalls can actually be aborted at the send<-msg boundary, not just during scan.
• Reimplement StorageHandler as a direct Handler (no longer wrapped by SimpleHandlerBase) so the timeout ctx can cover the exact send sites that stall.
• Adopt the iter.Seq recv-drain pattern recorded in CLAUDE.md (rule introduced by fix: decouple recv drain from response forwarding in simpleHandler #78): decouple recv drain from response forwarding so a stalled send<-msg can no longer stop recv drain entirely. Without this, the new direct-Handler form would violate the rule and re-introduce the "deadlock spring" shape for the duration of QueryTimeout every time it fires.
• Tests:
  • TestStorageHandler_QueryTimeout_ReqAbortsOnStalledConsumer — the original regression test (failing on main, passing here).
  • TestStorageHandler_DrainsRecvWhileYieldBlocks — new recv-drain test, mirrors TestSimpleHandler_DrainsRecvWhileYieldBlocks from fix: decouple recv drain from response forwarding in simpleHandler #78.

Why (QueryTimeout scope)

Production logs on salmon (over a recent 24 h window) showed REQ subscriptions with send_wait_ms > 3 h while:

• conn.Write stayed at ~5 ms per call (no write_timeout metric trips),
• ping_timeout never fired (client kept round-tripping pongs),
• the connection eventually closed with io.EOF (client-initiated TCP close).

That is the exact "live but slow client" case #73 introduced QueryTimeout for — and yet REQs were surviving 19× the configured 10 min timeout. Tracing the flow showed why.

Bug 1: QueryTimeout scope

In the prior implementation, handleReq wrapped the timeout inside its iter.Seq closure:

return func(yield func(*ServerMsg) bool) {
    if h.queryTimeout > 0 {
        ctx, cancel = context.WithTimeout(ctx, h.queryTimeout)
        defer cancel()
    }
    events, errFn, closeFn := h.storage.Query(ctx, msg.Filters)
    for event := range events {
        if ctx.Err() != nil { ... break }       // only polled here
        if !yieldTracked(NewServerEventMsg(...)) { return }
        eventsSent++
    }
}

The closure-local ctx only reaches storage.Query. The yield itself — which forwards through SimpleHandlerBase.ServeNostr's send<-msg — uses the outer connection ctx, which has no deadline. Once a receiver stalls, the inner ctx.Err check never re-runs because yield blocks before the next iteration starts. Result: QueryTimeout covered only the scan phase, not the send-wait phase it was specifically introduced to address.

Fix

Reimplement StorageHandler as a direct Handler so it can own its ServeNostr loop and attach queryCtx to the exact send sites that can stall. Key points:

• queryCtx := ctx.WithTimeout(ctx, QueryTimeout) derived once per REQ / COUNT.
• storage.Query(queryCtx, filters) — scan still respects the deadline.
• Every event emission uses select { send<-msg; <-queryCtx.Done() }.
• queryCtx.Err() polled at the top of each iteration (fast-path abort).
• Terminal CLOSED emitted on the outer ctx so it can outlive queryCtx.
• Outer ctx cancellation (connection ending) surfaces as ctx.Err() through ServeNostr, same as before.

Bug 2: iter.Seq recv-drain — surfaced by #78

While Bug 1 was on the wire as a draft, #78 landed on main with a new CLAUDE.md rule:

Handlers that implement Handler directly and consume iter.Seq responses must follow the same shape.

Bug 1's fix moves StorageHandler from SimpleHandlerBase to a direct Handler — exactly the case the rule targets. After rebasing on main I confirmed it: the new storageHandler.ServeNostr reads recv and forwards REQ responses through handleReq's yield loop on the same goroutine, so a stalled receiver halts recv drain entirely. Under an upstream MergeHandler (childRecvs cap 10) this wedges into the deadlock-spring shape recorded in the diary for 2026-04-24.

The interaction with Bug 1 is the operationally important part: without this second fix, every QueryTimeout firing would produce the spring shape for QueryTimeout ms before the abort kicks in. For the salmon configuration that is up to 10 minutes of MergeHandler broadcast wedging per slow REQ — a strict regression compared to the pre-#76 behaviour.

Fix

Port simpleHandler's pattern (#78) verbatim:

• Spawn a dedicated recv-drain goroutine fed by a cap-10 internal queue (storageHandlerMsgQueueBuffer, mirroring simpleHandlerMsgQueueBuffer and MergeHandler.childRecvs — all three layers share the same backpressure budget; only ≥1 is required for correctness).
• Main loop consumes from msgQueue, dispatches to handleEvent / handleReq / handleCount, forwards responses on send.
• drainCtx is a sub-ctx of the connection ctx with defer drainCancel() at handler scope, so the drain goroutine is reliably reaped on every main-loop return path without depending on caller-side cancellation — matches the self-contained shape the fix: decouple recv drain from response forwarding in simpleHandler #78 review feedback tightened.

Public API impact

None visible. NewStorageHandler still returns Handler. The internal wrapping via SimpleHandler(&storageHandler{...}) is gone; now it returns &storageHandler{...} directly. Anything type-asserting on the internal shape was already relying on unexported implementation.

Tests

TestStorageHandler_QueryTimeout_ReqAbortsOnStalledConsumer

Unbuffered send channel → handler delivers 1 EVENT, stalls on the 2nd send → test advances synctest's fake clock past QueryTimeout → asserts the next message is CLOSED "error: query timeout", not another EVENT.

• On main (before this change): test fails with the 2nd EVENT being delivered (handler ignores the timeout at the send boundary).
• On this branch: test passes.

TestStorageHandler_DrainsRecvWhileYieldBlocks (new)

Direct mirror of TestSimpleHandler_DrainsRecvWhileYieldBlocks from #78, applied to storageHandler. Pushes a REQ with two stored events, drains one EVENT to park the main loop on the second yield's send<-msg, then pushes a second client message from a helper goroutine and asserts the push completes — i.e. recv drain continues even while yield is stalled.

• On the post-rebase pre-fix commit: fails with the deadlock-spring message (recv drain stops the moment yield blocks).
• On this branch: passes — drain goroutine keeps recv flowing independently of the main loop's yield state.

All existing StorageHandler tests — including the existing QueryTimeout tests that exercise the scan path — continue to pass. Full suite + go test -race ./... clean.

Out of scope (but discovered during investigation)

• go test -race flags a pre-existing DATA RACE in MergeHandler / metrics tests (TestMergeHandler_Broadcast_REQTimeoutAdvances, TestMergeHandler_Broadcast_OKForcedFalseOnTimeout, TestMetrics_MergeHandler_LostChildAndBroadcastTimeout). Originally reproducible on main as of 6d25455 — not introduced by this PR. Worth a separate ticket. (Re-verifying against the current main head after the post-fix(storage_handler): honor QueryTimeout at the send<-msg boundary #76 churn is itself a follow-up.)
• The salmon logs also show cases where connections stayed alive for 3 h despite the stall, even though ping/pong should have caught a dead receiver. A separate investigation is in progress using the block profile infrastructure deployed in moctane-mocrelay 67d36ad + mocvps e04bff7. That problem is orthogonal to the QueryTimeout bug fixed here.

Follow-ups

• After merge + release, bump moctane-mocrelay's mocrelay dep to pick up both fixes.
• Observe whether new slow-REQ logs show completed=false at ~QueryTimeout (expected: yes) rather than scaling with receiver stall time, and whether MergeHandler broadcast-timeout logs no longer correlate with slow REQs (expected: no correlation, since recv drain now survives independently of yield state).

🐛 Generated with Claude Code