mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 05:01:36 -10:00
15b7a5475c
* fix(acp): interrupt idle reads on cancellation * fix(acp): make idle-read interruption per-Read, not per-Serve-call Addresses the P2 on #782. The previous design raced a single, whole-connection Once (endRead) between the read loop's own "I have a terminal result" claim and a watcher goroutine's "ctx is done, treat this as interrupted" claim. This has a real, deterministic failure mode that has nothing to do with goroutine-scheduling luck: bufio.Reader can obtain data AND a terminal error from ONE underlying Read call (io.Reader explicitly permits returning (n>0, err) together). If a line delimiter is found within that data, ReadBytes returns the line with a nil error for that call, but bufio caches the error internally and returns it — without ever touching the underlying reader again — on the very next ReadBytes call. If cancellation happens in the gap between that first, successful read and the next one (e.g. while a synchronously-dispatched handler is still writing its response), the watcher's ctx.Done() case fires and claims the Once BEFORE the read loop ever gets back to a ReadBytes call — so when the cached error then surfaces (with zero contact with the underlying reader, let alone anything closing it), it is misattributed as a cancellation-driven interruption and silently swallowed as a clean shutdown, hiding a genuine broken transport. The fix moves interruption-detection down to the Read call itself instead of bracketing the whole Serve loop. interruptibleReader wraps the connection's reader: each Read runs the underlying call on its own goroutine and races it against ctx.Done(). If the underlying call finishes first, its result is returned untouched — cancellation a moment later has no bearing on a call that already had its answer. If ctx.Done() fires first, the reader is closed (when closable) to nudge a genuinely blocked read, a generation counter is bumped, and the call still waits for and returns the underlying call's own result rather than a synthesized one. Serve then brackets each ReadBytes call (which may invoke the wrapped Read zero or more times — zero when bufio answers from its own buffer, including a previously-cached error) with a snapshot of that counter before and after. A read whose bracket shows no change was never raced against cancellation — either answered from the buffer or genuinely completed before ctx.Done() could matter — so its outcome is reported truthfully regardless of the ctx state by the time it surfaces. Only a bracket that changed proves this specific call actually took the cancellation branch. Verified the fix has teeth: reverted to the original implementation and confirmed the new regression test (which forces the exact bufio-caching sequence deterministically, with a panic guard proving the underlying reader is never called a second time) fails 100% of the time against it, then restored the fix and confirmed it passes. Also applied the two open review threads on the existing tests: readStarted and writeStarted are now buffered channels with non-blocking sends rather than close(), so a future change causing an extra read or write can't panic on a double-close. Validated with go build/vet/test on Windows, plus go vet/build and `go test -race ./internal/acp/... -count=50` (and count=100 focused on the cancellation/error tests) under WSL/Linux — this session's Windows host has no C toolchain for -race directly. * fix(acp): preserve decided read results and require explicit reader ownership Addresses jatmn's two #782 review findings: - interruptibleReader.Read's select between resultCh and ctx.Done() chooses pseudo-randomly when both are ready, so a real terminal read error decided just before cancellation could be misattributed to shutdown and silently swallowed. Re-check resultCh non-blockingly before claiming a call for cancellation. - NewConn inferred reader ownership from an io.Closer assertion and closed any reader that happened to implement it, including one a caller merely borrowed. Split into NewConn (never closes r) and NewOwnedConn (explicit ownership, closes r on cancellation), and switch the sole production call site (process stdin) to the latter. Also applies CodeRabbit's atomic.Int64 and leading-ctx-parameter nits. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(acp): synchronize read completion with cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-9e4e-7506-85ab-e151888bfd71 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): preserve read completion across cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): bound buffered error synchronization Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): report shutdown transport errors Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): inject cancellation context Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>