mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-08 17:56:14 -10:00
Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
aa7ce57a39 | ||
|
|
20032342e5 |
No files matched your search
@@ -1 +1 @@
|
||||
{".":"0.6.0"}
|
||||
{".":"0.5.0"}
|
||||
@@ -6,37 +6,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
|
||||
aims to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html) once the first release is
|
||||
tagged. Until then, source builds report the version `dev`.
|
||||
|
||||
## [0.6.0](https://github.com/Gitlawb/zero/compare/v0.5.0...v0.6.0) (2026-07-29)
|
||||
|
||||
|
||||
### Features
|
||||
|
||||
* add persistent session goals ([#803](https://github.com/Gitlawb/zero/issues/803)) ([5d1869e](https://github.com/Gitlawb/zero/commit/5d1869e7bc193c97e07418b57b823905c1151d5d))
|
||||
* **providers:** add Fireworks AI provider preset ([#772](https://github.com/Gitlawb/zero/issues/772)) ([58daea7](https://github.com/Gitlawb/zero/commit/58daea7a482927e5ab35afaf97761c60bb35e113))
|
||||
* **skills:** report hash drift in skill info ([#795](https://github.com/Gitlawb/zero/issues/795)) ([9fbdc34](https://github.com/Gitlawb/zero/commit/9fbdc341551601c13434cf7b9b6f5e4ea4c483b1))
|
||||
|
||||
|
||||
### Bug Fixes
|
||||
|
||||
* **acp:** interrupt idle reads on cancellation ([#782](https://github.com/Gitlawb/zero/issues/782)) ([15b7a54](https://github.com/Gitlawb/zero/commit/15b7a5475cfeefddbc59634551451a8872b996b7))
|
||||
* **cli:** include plugins info in shell completions ([#794](https://github.com/Gitlawb/zero/issues/794)) ([fa8734f](https://github.com/Gitlawb/zero/commit/fa8734f17927de8832dfe041e043cfb86f93d12a))
|
||||
* **config:** kill provider-command process tree via job object on Windows ([#690](https://github.com/Gitlawb/zero/issues/690)) ([5c58256](https://github.com/Gitlawb/zero/commit/5c58256abc29c185b449f8ce79647eb3fae61882))
|
||||
* **config:** make the provider-command timeout an upper bound ([#811](https://github.com/Gitlawb/zero/issues/811)) ([#813](https://github.com/Gitlawb/zero/issues/813)) ([1c38e8c](https://github.com/Gitlawb/zero/commit/1c38e8cba50db255f3cda8b7cf4298712b2933e1))
|
||||
* **config:** stop billing provider-command fixtures to the 5s timeout ([#810](https://github.com/Gitlawb/zero/issues/810)) ([f5fa4ff](https://github.com/Gitlawb/zero/commit/f5fa4ffc64537897aa78a56edca36d8f1dacd021))
|
||||
* **cron:** reserve job IDs atomically ([#686](https://github.com/Gitlawb/zero/issues/686)) ([d9b882e](https://github.com/Gitlawb/zero/commit/d9b882eb0a94f38de8447cb54b57860ccd8ed053))
|
||||
* **lint:** resolve static analysis baseline ([#769](https://github.com/Gitlawb/zero/issues/769)) ([583e653](https://github.com/Gitlawb/zero/commit/583e6530bd849a8efcb2b88bbe8c297e8592e62c))
|
||||
* **providers:** retry provably pre-send transport failures ([#447](https://github.com/Gitlawb/zero/issues/447)) ([#750](https://github.com/Gitlawb/zero/issues/750)) ([8acd42a](https://github.com/Gitlawb/zero/commit/8acd42a3bd743a35807d762f23a33fb61c171839))
|
||||
* **sandbox:** add platform-specific write lock around grant state file ([#752](https://github.com/Gitlawb/zero/issues/752)) ([#755](https://github.com/Gitlawb/zero/issues/755)) ([88fb7b6](https://github.com/Gitlawb/zero/commit/88fb7b6192c1ac8173089967d5e9119f92d7a101))
|
||||
* **sandbox:** clarify scoped permission prompts ([#798](https://github.com/Gitlawb/zero/issues/798)) ([9fe02ae](https://github.com/Gitlawb/zero/commit/9fe02ae52e3234cb9dae76e0c5594a287ccbb52a))
|
||||
* **sandbox:** deny reads of git's credential stores ([#815](https://github.com/Gitlawb/zero/issues/815)) ([#816](https://github.com/Gitlawb/zero/issues/816)) ([33c94ec](https://github.com/Gitlawb/zero/commit/33c94ec7a3e7987e6e36b5fd5e037ff4e6e84c26))
|
||||
* **sandbox:** preserve user config and retry denied writes ([#801](https://github.com/Gitlawb/zero/issues/801)) ([81a5e6c](https://github.com/Gitlawb/zero/commit/81a5e6cb716ba4b7ecf7ff3ff261dfa51fd01098))
|
||||
* **sandbox:** scrub the daemon token file pointer ([#677](https://github.com/Gitlawb/zero/issues/677)) ([#818](https://github.com/Gitlawb/zero/issues/818)) ([3e7692d](https://github.com/Gitlawb/zero/commit/3e7692d8f0885d346809869935c81a9cf055d89e))
|
||||
* **swarm:** close lifecycle admission on shutdown ([#776](https://github.com/Gitlawb/zero/issues/776)) ([bdbda89](https://github.com/Gitlawb/zero/commit/bdbda89f3e42d93e8058ab3e1e9ef402659530cd))
|
||||
* **tui:** fix rendering corruption over multipass + Windows Terminal ([#709](https://github.com/Gitlawb/zero/issues/709)) ([5bb1bc1](https://github.com/Gitlawb/zero/commit/5bb1bc11c3188dcea7709ff07c8b25dda3541de8))
|
||||
* **tui:** paste clipboard images with Ctrl+V ([#534](https://github.com/Gitlawb/zero/issues/534)) ([#807](https://github.com/Gitlawb/zero/issues/807)) ([ec3b822](https://github.com/Gitlawb/zero/commit/ec3b8225050eb62f0f41a9582ed2881d2f9dcc8e))
|
||||
* **tui:** reset quiet clock for new runs ([#799](https://github.com/Gitlawb/zero/issues/799)) ([a4a805a](https://github.com/Gitlawb/zero/commit/a4a805a6c82c51cb255e909bd87ca183c8805cf2))
|
||||
* **windows:** use PowerShell for command execution ([#804](https://github.com/Gitlawb/zero/issues/804)) ([518d2e5](https://github.com/Gitlawb/zero/commit/518d2e53ba3fa073090a9b41c278face85b59b64))
|
||||
|
||||
## [0.5.0](https://github.com/Gitlawb/zero/compare/v0.4.0...v0.5.0) (2026-07-22)
|
||||
|
||||
|
||||
|
||||
@@ -131,7 +131,6 @@ export ANTHROPIC_API_KEY=...
|
||||
export GEMINI_API_KEY=...
|
||||
export AIMLAPI_API_KEY=...
|
||||
export LONGCAT_API_KEY=...
|
||||
export FIREWORKS_API_KEY=...
|
||||
export MINIMAX_API_KEY=...
|
||||
export MINIMAXI_API_KEY=...
|
||||
```
|
||||
@@ -148,12 +147,6 @@ To configure Meituan LongCat (LongCat-2.0) directly, run:
|
||||
zero providers setup longcat --set-active
|
||||
```
|
||||
|
||||
To configure Fireworks AI directly, run:
|
||||
|
||||
```bash
|
||||
zero providers setup fireworks --set-active
|
||||
```
|
||||
|
||||
MiniMax presets use the Anthropic-compatible endpoints for the global and China
|
||||
regions:
|
||||
|
||||
|
||||
+6
-138
@@ -17,7 +17,6 @@ import (
|
||||
"fmt"
|
||||
"io"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
)
|
||||
|
||||
// JSON-RPC 2.0 standard error codes.
|
||||
@@ -81,9 +80,8 @@ type NotifyFunc func(ctx context.Context, params json.RawMessage)
|
||||
// requests/notifications — needed because ACP is bidirectional (the agent calls
|
||||
// the client for session/request_permission, fs/*, terminal/*).
|
||||
type Conn struct {
|
||||
rawReader io.Reader // wrapped lazily in Serve, once ctx is known — see interruptibleReader
|
||||
readerCloser io.Closer
|
||||
w io.Writer
|
||||
reader *bufio.Reader
|
||||
w io.Writer
|
||||
|
||||
writeMu sync.Mutex // serializes all writes to w
|
||||
|
||||
@@ -98,15 +96,10 @@ type Conn struct {
|
||||
wg sync.WaitGroup // tracks in-flight inbound handlers
|
||||
}
|
||||
|
||||
// NewConn builds a peer reading ndjson from r and writing ndjson to w. This
|
||||
// Conn does not own r: cancelling Serve's context cannot forcibly close it, so
|
||||
// a Read on r that's genuinely idle (never returns on its own) leaves Serve
|
||||
// blocked until r itself produces something — e.g. a test that closes r's own
|
||||
// write side to unblock it. Use NewOwnedConn when this Conn should own r's
|
||||
// lifetime.
|
||||
// NewConn builds a peer reading ndjson from r and writing ndjson to w.
|
||||
func NewConn(r io.Reader, w io.Writer) *Conn {
|
||||
return &Conn{
|
||||
rawReader: r,
|
||||
reader: bufio.NewReader(r),
|
||||
w: w,
|
||||
handlers: make(map[string]HandlerFunc),
|
||||
notifiers: make(map[string]NotifyFunc),
|
||||
@@ -114,18 +107,6 @@ func NewConn(r io.Reader, w io.Writer) *Conn {
|
||||
}
|
||||
}
|
||||
|
||||
// NewOwnedConn is like NewConn but declares that this Conn exclusively owns r:
|
||||
// if r implements io.Closer, Serve closes it on context cancellation to
|
||||
// interrupt an otherwise-idle blocking Read. Only pass a reader whose lifetime
|
||||
// this Conn should control — e.g. the process's own stdin — never a reader a
|
||||
// caller retains or shares elsewhere, since cancellation tears it down
|
||||
// permanently for every consumer, not just this Conn.
|
||||
func NewOwnedConn(r io.Reader, w io.Writer) *Conn {
|
||||
c := NewConn(r, w)
|
||||
c.readerCloser, _ = r.(io.Closer)
|
||||
return c
|
||||
}
|
||||
|
||||
// Handle registers a request handler for method.
|
||||
func (c *Conn) Handle(method string, fn HandlerFunc) { c.handlers[method] = fn }
|
||||
|
||||
@@ -147,36 +128,17 @@ func (c *Conn) Serve(ctx context.Context) error {
|
||||
c.wg.Wait()
|
||||
}()
|
||||
|
||||
interruptible := newInterruptibleReader(ctx, c.rawReader, c.readerCloser)
|
||||
reader := bufio.NewReader(interruptible)
|
||||
|
||||
for {
|
||||
// One ndjson value per line. Framing per line (rather than streaming the
|
||||
// decoder) keeps a single malformed line from making the whole connection
|
||||
// unrecoverable — we report -32700 and continue.
|
||||
//
|
||||
// generation brackets this SPECIFIC ReadBytes call: bufio may invoke
|
||||
// interruptible.Read zero or more times underneath it (zero when the
|
||||
// answer is already sitting in bufio's own buffer — including a
|
||||
// previously-buffered, not-yet-surfaced error: bufio can return a
|
||||
// complete line with a nil error while quietly caching an error it
|
||||
// received ALONGSIDE that line's bytes, which then surfaces on the VERY
|
||||
// NEXT call with no further contact with interruptible.Read at all). The
|
||||
// generation only advances inside interruptible.Read's own cancellation
|
||||
// branch, so "unchanged across this call" is proof this call's outcome
|
||||
// didn't pass through that branch — whether because it was answered from
|
||||
// the buffer or because it raced ctx.Done() and won. Only that proof
|
||||
// makes it safe to call the outcome genuine.
|
||||
before := interruptible.generation()
|
||||
line, err := reader.ReadBytes('\n')
|
||||
interrupted := interruptible.generation() != before
|
||||
|
||||
line, err := c.reader.ReadBytes('\n')
|
||||
if len(bytes.TrimSpace(line)) > 0 {
|
||||
c.handleLine(ctx, line)
|
||||
}
|
||||
if err != nil {
|
||||
c.failAllPending(err)
|
||||
if errors.Is(err, io.EOF) || interrupted {
|
||||
if errors.Is(err, io.EOF) || ctx.Err() != nil {
|
||||
return nil
|
||||
}
|
||||
return err
|
||||
@@ -184,100 +146,6 @@ func (c *Conn) Serve(ctx context.Context) error {
|
||||
}
|
||||
}
|
||||
|
||||
// interruptibleReader wraps a reader so a context cancellation can unblock a
|
||||
// currently in-flight Read without ever discarding — or misattributing — the
|
||||
// real outcome of a call that wasn't actually blocked.
|
||||
//
|
||||
// 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
|
||||
// exactly as received; ctx being cancelled a moment later has no bearing on a
|
||||
// call that already had its answer. If ctx.Done() fires first, closer (when
|
||||
// non-nil) is closed to nudge a genuinely blocked read, the generation counter
|
||||
// is bumped, and the call STILL waits for and returns the underlying call's own
|
||||
// result — never a synthesized one — since closing a reader's outcome is
|
||||
// whatever the reader itself reports for it, not ours to invent.
|
||||
//
|
||||
// The counter is what lets Serve tell a call that was actually raced against
|
||||
// cancellation apart from one merely answered while cancellation happened to be
|
||||
// pending — the distinction the read loop's own doc comment explains.
|
||||
type interruptibleReader struct {
|
||||
inner io.Reader
|
||||
closer io.Closer
|
||||
ctx context.Context
|
||||
|
||||
closeOnce sync.Once
|
||||
gen atomic.Int64
|
||||
}
|
||||
|
||||
func newInterruptibleReader(ctx context.Context, inner io.Reader, closer io.Closer) *interruptibleReader {
|
||||
return &interruptibleReader{inner: inner, closer: closer, ctx: ctx}
|
||||
}
|
||||
|
||||
func (r *interruptibleReader) generation() int64 { return r.gen.Load() }
|
||||
|
||||
type interruptibleReadResult struct {
|
||||
n int
|
||||
err error
|
||||
}
|
||||
|
||||
// interruptibleReadDecision arbitrates whether an underlying Read completed or
|
||||
// cancellation claimed it first. Completion is recorded before the result is
|
||||
// published so cancellation cannot misclassify a finished read merely because
|
||||
// its goroutine has not sent the result yet.
|
||||
type interruptibleReadDecision struct {
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (d *interruptibleReadDecision) complete() { d.once.Do(func() {}) }
|
||||
|
||||
func (d *interruptibleReadDecision) interrupt() (claimed bool) {
|
||||
d.once.Do(func() { claimed = true })
|
||||
return claimed
|
||||
}
|
||||
|
||||
// readInner records completion in the Read call's deferred epilogue. It also
|
||||
// converts a reader panic into a terminal error so the helper goroutine always
|
||||
// publishes a result for Read to join.
|
||||
func (r *interruptibleReader) readInner(p []byte, decision *interruptibleReadDecision) (res interruptibleReadResult) {
|
||||
defer func() {
|
||||
if recovered := recover(); recovered != nil {
|
||||
res = interruptibleReadResult{err: fmt.Errorf("acp: reader panicked: %v", recovered)}
|
||||
}
|
||||
decision.complete()
|
||||
}()
|
||||
res.n, res.err = r.inner.Read(p)
|
||||
return res
|
||||
}
|
||||
|
||||
func (r *interruptibleReader) Read(p []byte) (int, error) {
|
||||
resultCh := make(chan interruptibleReadResult, 1)
|
||||
var decision interruptibleReadDecision
|
||||
go func() {
|
||||
resultCh <- r.readInner(p, &decision)
|
||||
}()
|
||||
select {
|
||||
case res := <-resultCh:
|
||||
return res.n, res.err
|
||||
case <-r.ctx.Done():
|
||||
// resultCh may not be readable yet even when inner.Read has returned: its
|
||||
// goroutine can be descheduled between recording completion and publishing
|
||||
// the result. Only interrupt a call that cancellation actually claims.
|
||||
if decision.interrupt() {
|
||||
r.gen.Add(1)
|
||||
if r.closer != nil {
|
||||
r.closeOnce.Do(func() { _ = r.closer.Close() })
|
||||
}
|
||||
}
|
||||
// Wait for the real result rather than returning synthetically: p is
|
||||
// still being written by the goroutine above until it does, so returning
|
||||
// early would race that write, and the actual error (whatever closing
|
||||
// the reader produced, or whatever the reader was already about to
|
||||
// report) is what the caller should see.
|
||||
res := <-resultCh
|
||||
return res.n, res.err
|
||||
}
|
||||
}
|
||||
|
||||
// handleLine decodes and dispatches one ndjson frame. A parse failure replies
|
||||
// with -32700 (id null) and keeps the connection alive.
|
||||
func (c *Conn) handleLine(ctx context.Context, line []byte) {
|
||||
|
||||
@@ -3,30 +3,11 @@ package acp
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"io"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
type testReadCloser struct {
|
||||
read func([]byte) (int, error)
|
||||
close func() error
|
||||
}
|
||||
|
||||
func (r testReadCloser) Read(p []byte) (int, error) { return r.read(p) }
|
||||
func (r testReadCloser) Close() error { return r.close() }
|
||||
|
||||
type testReader func([]byte) (int, error)
|
||||
|
||||
func (r testReader) Read(p []byte) (int, error) { return r(p) }
|
||||
|
||||
type testWriter func([]byte) (int, error)
|
||||
|
||||
func (w testWriter) Write(p []byte) (int, error) { return w(p) }
|
||||
|
||||
// connPair wires two Conns together over in-memory pipes and serves both.
|
||||
func connPair(t *testing.T) (a, b *Conn, stop func()) {
|
||||
t.Helper()
|
||||
@@ -44,275 +25,6 @@ func connPair(t *testing.T) (a, b *Conn, stop func()) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestConnServeCancellationInterruptsIdleRead(t *testing.T) {
|
||||
pipeReader, writer := io.Pipe()
|
||||
t.Cleanup(func() { _ = writer.Close() })
|
||||
// Buffered + non-blocking send, not close: interruptibleReader retries each
|
||||
// underlying Read on its own goroutine, so if bufio ever calls this more than
|
||||
// once, a close() here would panic on the second call.
|
||||
readStarted := make(chan struct{}, 1)
|
||||
closeCalls := make(chan struct{}, 2)
|
||||
reader := testReadCloser{
|
||||
read: func(p []byte) (int, error) {
|
||||
select {
|
||||
case readStarted <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
return pipeReader.Read(p)
|
||||
},
|
||||
close: func() error {
|
||||
closeCalls <- struct{}{}
|
||||
return pipeReader.Close()
|
||||
},
|
||||
}
|
||||
conn := NewOwnedConn(reader, io.Discard)
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- conn.Serve(ctx) }()
|
||||
|
||||
<-readStarted
|
||||
cancel()
|
||||
select {
|
||||
case err := <-done:
|
||||
if err != nil {
|
||||
t.Fatalf("Serve returned %v after cancellation, want nil", err)
|
||||
}
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("Serve did not return after cancelling an idle connection")
|
||||
}
|
||||
if got := len(closeCalls); got != 1 {
|
||||
t.Fatalf("reader Close calls = %d, want 1", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestConnServePreservesTerminalReadErrorDuringCancellation(t *testing.T) {
|
||||
for _, closable := range []bool{false, true} {
|
||||
name := "non-closable"
|
||||
if closable {
|
||||
name = "closable"
|
||||
}
|
||||
t.Run(name, func(t *testing.T) {
|
||||
wantErr := errors.New("read failed")
|
||||
read := testReader(func(p []byte) (int, error) {
|
||||
return copy(p, "not json"), wantErr
|
||||
})
|
||||
closeCalls := make(chan struct{}, 1)
|
||||
var reader io.Reader = read
|
||||
if closable {
|
||||
reader = testReadCloser{
|
||||
read: read,
|
||||
close: func() error {
|
||||
closeCalls <- struct{}{}
|
||||
return nil
|
||||
},
|
||||
}
|
||||
}
|
||||
// Buffered + non-blocking send, not close: a later change adding a
|
||||
// second write (e.g. an additional error frame) must not panic on a
|
||||
// repeated close of an already-closed channel.
|
||||
writeStarted := make(chan struct{}, 1)
|
||||
releaseWrite := make(chan struct{})
|
||||
writer := testWriter(func(p []byte) (int, error) {
|
||||
select {
|
||||
case writeStarted <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
<-releaseWrite
|
||||
return len(p), nil
|
||||
})
|
||||
newConn := NewConn
|
||||
if closable {
|
||||
// Ownership matters here: this case exists to prove that even a
|
||||
// Conn entitled to close its reader on cancellation still
|
||||
// preserves a terminal error that was already decided.
|
||||
newConn = NewOwnedConn
|
||||
}
|
||||
conn := newConn(reader, writer)
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- conn.Serve(ctx) }()
|
||||
|
||||
<-writeStarted
|
||||
cancel()
|
||||
close(releaseWrite)
|
||||
select {
|
||||
case err := <-done:
|
||||
if !errors.Is(err, wantErr) {
|
||||
t.Fatalf("Serve returned %v, want terminal read error %v", err, wantErr)
|
||||
}
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("Serve did not return after terminal read error")
|
||||
}
|
||||
if got := len(closeCalls); got != 0 {
|
||||
t.Fatalf("reader Close calls = %d, want 0 after terminal read", got)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestConnServePreservesReadErrorBufferedAlongsideAPriorLine is the deterministic
|
||||
// regression test for jatmn's #782 finding: a real, non-EOF ReadBytes failure can
|
||||
// be DECIDED before cancellation ever happens, yet only SURFACE afterward, and
|
||||
// must still be reported rather than swallowed as a clean shutdown.
|
||||
//
|
||||
// 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 delimiter is
|
||||
// found within that data, ReadBytes returns the line with a nil error for THIS
|
||||
// call, but bufio caches the error internally and returns it — WITHOUT calling the
|
||||
// underlying reader again — on the very next ReadBytes call. This test forces that
|
||||
// exact sequence: the underlying reader hands back one complete, validly-framed
|
||||
// line together with wantErr in a SINGLE call (asserted below to be its ONLY
|
||||
// call), the loop dispatches that line through a synchronous path that blocks the
|
||||
// read loop, cancellation is asserted to have happened before the loop attempts
|
||||
// its next read, and only then is the block released. By the time the read loop
|
||||
// asks for more input, cancellation is already in effect and the underlying
|
||||
// reader is never touched again — so if Serve attributed the resulting error to
|
||||
// cancellation merely because ctx was already done, it would incorrectly return
|
||||
// nil. It must instead recognize that this read was answered entirely from
|
||||
// bufio's own cache, never raced against ctx.Done(), and report wantErr.
|
||||
func TestConnServePreservesReadErrorBufferedAlongsideAPriorLine(t *testing.T) {
|
||||
wantErr := errors.New("read failed")
|
||||
var readCalls atomic.Int32
|
||||
read := testReader(func(p []byte) (int, error) {
|
||||
readCalls.Add(1)
|
||||
// An unsupported jsonrpc version with an id takes the SYNCHRONOUS
|
||||
// writeError path in handleLine (not a dispatched goroutine), giving this
|
||||
// test a reliable blocking point between processing this line and the
|
||||
// loop's next ReadBytes call.
|
||||
return copy(p, `{"jsonrpc":"1.0","id":1}`+"\n"), wantErr
|
||||
})
|
||||
writeStarted := make(chan struct{}, 1)
|
||||
releaseWrite := make(chan struct{})
|
||||
var releaseWriteOnce sync.Once
|
||||
release := func() { releaseWriteOnce.Do(func() { close(releaseWrite) }) }
|
||||
t.Cleanup(release)
|
||||
writer := testWriter(func(p []byte) (int, error) {
|
||||
select {
|
||||
case writeStarted <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
<-releaseWrite
|
||||
return len(p), nil
|
||||
})
|
||||
conn := NewConn(read, writer)
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- conn.Serve(ctx) }()
|
||||
|
||||
// The synchronous write confirms the first (and, per the panic guard above,
|
||||
// only) read has already returned and is being processed — cancelling now
|
||||
// lands squarely in the gap between that read and the loop's next one, never
|
||||
// racing an in-flight interruptibleReader.Read call.
|
||||
select {
|
||||
case <-writeStarted:
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("Serve did not reach the synchronous write")
|
||||
}
|
||||
cancel()
|
||||
release()
|
||||
|
||||
select {
|
||||
case err := <-done:
|
||||
if !errors.Is(err, wantErr) {
|
||||
t.Fatalf("Serve returned %v, want the buffered terminal read error %v", err, wantErr)
|
||||
}
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("Serve did not return after the buffered terminal read error")
|
||||
}
|
||||
if got := readCalls.Load(); got != 1 {
|
||||
t.Fatalf("underlying Read calls = %d, want exactly 1 (bufio must have served the second ReadBytes from its cache)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestInterruptibleReaderPrefersDecidedResultOverCancellation is the regression
|
||||
// test for jatmn's second #782 finding: interruptibleReader.Read's select
|
||||
// between resultCh and ctx.Done() chooses pseudo-randomly when both are ready,
|
||||
// so a real, already-decided outcome (e.g. a terminal transport error) could be
|
||||
// misattributed to cancellation purely because ctx also happened to be done by
|
||||
// the time the select ran. Each iteration lets the underlying Read return (so
|
||||
// resultCh already holds the real outcome) before cancelling, maximizing the
|
||||
// odds of landing exactly in that ambiguous window; the generation counter must
|
||||
// never advance when the real, already-decided result is what gets returned.
|
||||
func TestInterruptibleReaderPrefersDecidedResultOverCancellation(t *testing.T) {
|
||||
wantErr := errors.New("read failed")
|
||||
for i := 0; i < 100; i++ {
|
||||
resultReady := make(chan struct{})
|
||||
read := testReader(func(p []byte) (int, error) {
|
||||
defer close(resultReady)
|
||||
return copy(p, "x"), wantErr
|
||||
})
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
r := newInterruptibleReader(ctx, read, nil)
|
||||
|
||||
readDone := make(chan interruptibleReadResult, 1)
|
||||
go func() {
|
||||
n, err := r.Read(make([]byte, 8))
|
||||
readDone <- interruptibleReadResult{n, err}
|
||||
}()
|
||||
|
||||
<-resultReady
|
||||
time.Sleep(time.Millisecond) // bias resultCh's send ahead of cancel
|
||||
cancel()
|
||||
|
||||
res := <-readDone
|
||||
if !errors.Is(res.err, wantErr) {
|
||||
t.Fatalf("iteration %d: Read returned %v, want %v", i, res.err, wantErr)
|
||||
}
|
||||
if got := r.generation(); got != 0 {
|
||||
t.Fatalf("iteration %d: generation = %d, want 0 (a decided result must never be attributed to cancellation)", i, got)
|
||||
}
|
||||
cancel()
|
||||
}
|
||||
}
|
||||
|
||||
// TestInterruptibleReadCompletionWinsBeforeResultPublication exercises the
|
||||
// actual inner-read wrapper at the handoff between the underlying Read and the
|
||||
// helper goroutine's result publication. Cancellation must not reclassify a
|
||||
// result returned by this wrapper as interrupted.
|
||||
func TestInterruptibleReadCompletionWinsBeforeResultPublication(t *testing.T) {
|
||||
wantErr := errors.New("read failed")
|
||||
read := testReader(func(p []byte) (int, error) {
|
||||
return copy(p, "x"), wantErr
|
||||
})
|
||||
r := newInterruptibleReader(context.Background(), read, nil)
|
||||
var decision interruptibleReadDecision
|
||||
readComplete := make(chan struct{})
|
||||
resultCh := make(chan interruptibleReadResult)
|
||||
go func() {
|
||||
res := r.readInner(make([]byte, 8), &decision)
|
||||
close(readComplete)
|
||||
resultCh <- res
|
||||
}()
|
||||
|
||||
<-readComplete
|
||||
if decision.interrupt() {
|
||||
t.Fatal("cancellation claimed a read that completed before result publication")
|
||||
}
|
||||
res := <-resultCh
|
||||
if res.n != 1 || !errors.Is(res.err, wantErr) {
|
||||
t.Fatalf("inner read result = (%d, %v), want (1, %v)", res.n, res.err, wantErr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestConnServeReportsReaderPanicAsTerminalError(t *testing.T) {
|
||||
read := testReader(func([]byte) (int, error) {
|
||||
panic("boom")
|
||||
})
|
||||
conn := NewConn(read, io.Discard)
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- conn.Serve(context.Background()) }()
|
||||
|
||||
select {
|
||||
case err := <-done:
|
||||
const want = "acp: reader panicked: boom"
|
||||
if err == nil || err.Error() != want {
|
||||
t.Fatalf("Serve returned %v, want %q", err, want)
|
||||
}
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("Serve did not return after reader panic")
|
||||
}
|
||||
}
|
||||
|
||||
func TestConnRequestResponse(t *testing.T) {
|
||||
a, b, stop := connPair(t)
|
||||
defer stop()
|
||||
|
||||
+3
-7
@@ -27,8 +27,6 @@ Usage:
|
||||
Not meant to be run interactively — point your editor's ACP / external-agent
|
||||
setting at "zero acp".`
|
||||
|
||||
var acpSignalContext = signalContext
|
||||
|
||||
// runACP serves ACP over stdio so an editor can drive ZERO's agent core. It
|
||||
// speaks JSON-RPC 2.0 (newline-delimited JSON) on stdin/stdout; stderr stays free
|
||||
// for human-readable diagnostics. The session lifecycle maps onto ZERO's own
|
||||
@@ -44,9 +42,7 @@ func runACP(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int
|
||||
}
|
||||
}
|
||||
|
||||
// The process's own stdin belongs exclusively to this Conn for the life of
|
||||
// the command, so cancellation may close it to interrupt an idle read.
|
||||
conn := acp.NewOwnedConn(deps.stdin, stdout)
|
||||
conn := acp.NewConn(deps.stdin, stdout)
|
||||
acp.NewAgent(conn, acp.Deps{
|
||||
ResolveConfig: deps.resolveConfig,
|
||||
DiscoverModels: func(ctx context.Context, profile config.ProviderProfile) ([]providermodeldiscovery.Model, error) {
|
||||
@@ -77,9 +73,9 @@ func runACP(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int
|
||||
AgentInfo: acp.Implementation{Name: "zero", Version: version},
|
||||
})
|
||||
|
||||
ctx, stop := acpSignalContext()
|
||||
ctx, stop := signalContext()
|
||||
defer stop()
|
||||
if err := conn.Serve(ctx); err != nil {
|
||||
if err := conn.Serve(ctx); err != nil && ctx.Err() == nil {
|
||||
return writeAppError(stderr, "acp: "+err.Error(), exitCrash)
|
||||
}
|
||||
return exitSuccess
|
||||
|
||||
@@ -1,120 +0,0 @@
|
||||
package cli
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"io"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
type acpTestReader func([]byte) (int, error)
|
||||
|
||||
func (r acpTestReader) Read(p []byte) (int, error) { return r(p) }
|
||||
|
||||
type acpTestWriter func([]byte) (int, error)
|
||||
|
||||
func (w acpTestWriter) Write(p []byte) (int, error) { return w(p) }
|
||||
|
||||
func installACPTestContext(t *testing.T) context.CancelFunc {
|
||||
t.Helper()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
previous := acpSignalContext
|
||||
acpSignalContext = func() (context.Context, context.CancelFunc) {
|
||||
return ctx, cancel
|
||||
}
|
||||
t.Cleanup(func() {
|
||||
cancel()
|
||||
acpSignalContext = previous
|
||||
})
|
||||
return cancel
|
||||
}
|
||||
|
||||
func TestRunACPCancellationPreservesTerminalReadError(t *testing.T) {
|
||||
cancel := installACPTestContext(t)
|
||||
wantErr := errors.New("transport read failed")
|
||||
reader := acpTestReader(func(p []byte) (int, error) {
|
||||
return copy(p, `{"jsonrpc":"1.0","id":1}`+"\n"), wantErr
|
||||
})
|
||||
writeStarted := make(chan struct{}, 1)
|
||||
releaseWrite := make(chan struct{})
|
||||
var stdout bytes.Buffer
|
||||
writer := acpTestWriter(func(p []byte) (int, error) {
|
||||
writeStarted <- struct{}{}
|
||||
<-releaseWrite
|
||||
return stdout.Write(p)
|
||||
})
|
||||
var stderr bytes.Buffer
|
||||
done := make(chan int, 1)
|
||||
go func() {
|
||||
done <- runACP(nil, writer, &stderr, fillAppDeps(appDeps{stdin: reader}))
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-writeStarted:
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("ACP did not reach the response write")
|
||||
}
|
||||
// Keep Serve in the synchronous write until cancellation is observable. The
|
||||
// regression requires the genuine read error to win over that cancellation.
|
||||
cancel()
|
||||
close(releaseWrite)
|
||||
|
||||
select {
|
||||
case code := <-done:
|
||||
if code != exitCrash {
|
||||
t.Fatalf("exit code = %d, want crash %d", code, exitCrash)
|
||||
}
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("ACP did not exit after terminal read error")
|
||||
}
|
||||
if got := stderr.String(); !strings.Contains(got, "acp: "+wantErr.Error()) {
|
||||
t.Fatalf("stderr = %q, want terminal read error", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunACPIdleCancellationExitsCleanly(t *testing.T) {
|
||||
cancel := installACPTestContext(t)
|
||||
pipeReader, pipeWriter := io.Pipe()
|
||||
t.Cleanup(func() { _ = pipeWriter.Close() })
|
||||
readStarted := make(chan struct{}, 1)
|
||||
reader := &acpNotifyingReadCloser{ReadCloser: pipeReader, readStarted: readStarted}
|
||||
var stdout, stderr bytes.Buffer
|
||||
done := make(chan int, 1)
|
||||
go func() {
|
||||
done <- runACP(nil, &stdout, &stderr, fillAppDeps(appDeps{stdin: reader}))
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-readStarted:
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("ACP did not begin reading")
|
||||
}
|
||||
cancel()
|
||||
|
||||
select {
|
||||
case code := <-done:
|
||||
if code != exitSuccess {
|
||||
t.Fatalf("exit code = %d, want success %d; stderr: %s", code, exitSuccess, stderr.String())
|
||||
}
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("ACP did not exit after cancellation")
|
||||
}
|
||||
if stderr.Len() != 0 {
|
||||
t.Fatalf("stderr = %q, want empty", stderr.String())
|
||||
}
|
||||
}
|
||||
|
||||
type acpNotifyingReadCloser struct {
|
||||
io.ReadCloser
|
||||
readStarted chan<- struct{}
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (r *acpNotifyingReadCloser) Read(p []byte) (int, error) {
|
||||
r.once.Do(func() { r.readStarted <- struct{}{} })
|
||||
return r.ReadCloser.Read(p)
|
||||
}
|
||||
@@ -19,7 +19,6 @@ func TestProviderCatalogRuntimeProvidersShowOnboardingSetupHints(t *testing.T) {
|
||||
{id: "openai", name: "OpenAI", transport: "openai", defaultModel: "gpt-4.1"},
|
||||
{id: "groq", name: "Groq", transport: "openai-compatible", defaultModel: "llama-3.3-70b-versatile"},
|
||||
{id: "longcat", name: "LongCat", transport: "openai-compatible", defaultModel: "LongCat-2.0"},
|
||||
{id: "fireworks", name: "Fireworks AI", transport: "openai-compatible", defaultModel: "accounts/fireworks/models/kimi-k2p7-code"},
|
||||
{id: "ollama-cloud", name: "Ollama Cloud", transport: "openai-compatible", defaultModel: "qwen3-coder:480b"},
|
||||
{id: "ollama", name: "Ollama Local", transport: "openai-compatible", defaultModel: "llama3.1"},
|
||||
}
|
||||
|
||||
+16
-115
@@ -8,7 +8,6 @@ import (
|
||||
"os/exec"
|
||||
"runtime"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
)
|
||||
|
||||
@@ -18,11 +17,7 @@ func LoadProviderCommand(command string) (FileConfig, error) {
|
||||
stdout, stderr, err := runProviderCommand(command, providerCommandTimeout)
|
||||
if err != nil {
|
||||
if errors.Is(err, errProviderCommandTimeout) {
|
||||
// Report which phase ran out of time, and anything the command
|
||||
// managed to say. This used to be a bare string with the underlying
|
||||
// error and stderr both dropped, which left a user whose command was
|
||||
// slow on a cold machine with nothing to act on.
|
||||
return FileConfig{}, fmt.Errorf("provider command timed out after %s: %w%s", providerCommandTimeout, err, commandOutput(stderr))
|
||||
return FileConfig{}, fmt.Errorf("provider command timed out after 5s")
|
||||
}
|
||||
return FileConfig{}, fmt.Errorf("provider command failed: %w%s", err, commandOutput(stderr))
|
||||
}
|
||||
@@ -45,111 +40,20 @@ func LoadProviderCommand(command string) (FileConfig, error) {
|
||||
|
||||
var errProviderCommandTimeout = errors.New("provider command timeout")
|
||||
|
||||
// providerCommandDrainGrace bounds how long the timeout path waits for cmd.Wait
|
||||
// to return after the process tree has been terminated. Terminate normally makes
|
||||
// that immediate; this exists so a descendant that refuses to die cannot extend
|
||||
// a call whose deadline has already expired.
|
||||
const providerCommandDrainGrace = 2 * time.Second
|
||||
|
||||
// syncBuffer serialises the command's I/O pump against reads from this package.
|
||||
// On the timeout path runProviderCommand can return while cmd.Wait is still
|
||||
// draining, so the buffers outlive the call and a plain bytes.Buffer would be
|
||||
// read and written concurrently.
|
||||
type syncBuffer struct {
|
||||
mu sync.Mutex
|
||||
buf bytes.Buffer
|
||||
}
|
||||
|
||||
func (b *syncBuffer) Write(p []byte) (int, error) {
|
||||
b.mu.Lock()
|
||||
defer b.mu.Unlock()
|
||||
return b.buf.Write(p)
|
||||
}
|
||||
|
||||
// snapshot copies out what has been written so far. A copy rather than the
|
||||
// buffer's own slice, which the pump may still append to.
|
||||
func (b *syncBuffer) snapshot() []byte {
|
||||
b.mu.Lock()
|
||||
defer b.mu.Unlock()
|
||||
return append([]byte(nil), b.buf.Bytes()...)
|
||||
}
|
||||
|
||||
// awaitWithGrace waits for done and gives up after grace, reporting whether the
|
||||
// wait completed rather than expired.
|
||||
//
|
||||
// Split out from the timeout path so the bound can be asserted on its own. In
|
||||
// place it is effectively untestable: Terminate kills the tree, so Wait returns
|
||||
// promptly and an unbounded receive would behave identically in every test that
|
||||
// is not pathological, which is exactly how the old unbounded receive survived.
|
||||
func awaitWithGrace(done <-chan error, grace time.Duration) bool {
|
||||
timer := time.NewTimer(grace)
|
||||
defer timer.Stop()
|
||||
select {
|
||||
case <-done:
|
||||
return true
|
||||
case <-timer.C:
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
// runProviderCommand runs a provider command under a real deadline.
|
||||
//
|
||||
// "Real" is the point: timeout used to bound only the wait, so the call could
|
||||
// take arbitrarily longer than it. Two phases sat outside it. Starting the
|
||||
// process ran before the clock began, and on Windows that is CREATE_SUSPENDED,
|
||||
// job-object creation and assignment, and a system-wide thread snapshot, none of
|
||||
// which is fast on a cold or contended machine. Draining after termination was
|
||||
// unbounded as well. Both are inside the budget now, so the deadline is an upper
|
||||
// bound rather than a floor.
|
||||
func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte, error) {
|
||||
cmd := shellCommand(command)
|
||||
// Bound the time Wait spends draining I/O pipes after the process exits,
|
||||
// in case an orphaned descendant still holds the write ends open.
|
||||
cmd.WaitDelay = time.Second
|
||||
|
||||
stdout := &syncBuffer{}
|
||||
stderr := &syncBuffer{}
|
||||
cmd.Stdout = stdout
|
||||
cmd.Stderr = stderr
|
||||
var stdout bytes.Buffer
|
||||
var stderr bytes.Buffer
|
||||
cmd.Stdout = &stdout
|
||||
cmd.Stderr = &stderr
|
||||
|
||||
// Armed before the process is started, so process creation is inside the
|
||||
// budget rather than ahead of it.
|
||||
deadline := time.NewTimer(timeout)
|
||||
defer deadline.Stop()
|
||||
|
||||
type startResult struct {
|
||||
proc *commandProcess
|
||||
err error
|
||||
}
|
||||
// Buffered, so the goroutine can finish and exit even when nobody is left
|
||||
// waiting on it because the deadline already fired.
|
||||
started := make(chan startResult, 1)
|
||||
go func() {
|
||||
proc, err := startCommandProcess(cmd)
|
||||
started <- startResult{proc: proc, err: err}
|
||||
}()
|
||||
|
||||
var proc *commandProcess
|
||||
select {
|
||||
case result := <-started:
|
||||
if result.err != nil {
|
||||
return stdout.snapshot(), stderr.snapshot(), result.err
|
||||
}
|
||||
proc = result.proc
|
||||
case <-deadline.C:
|
||||
// The start is still in flight and will eventually hand back whatever it
|
||||
// created, so cleanup is handed off rather than abandoned. Wait reaps the
|
||||
// process; without it a started-but-orphaned command becomes a zombie.
|
||||
go func() {
|
||||
result := <-started
|
||||
if result.err != nil {
|
||||
return
|
||||
}
|
||||
result.proc.Terminate()
|
||||
_ = cmd.Wait()
|
||||
result.proc.Close()
|
||||
}()
|
||||
return stdout.snapshot(), stderr.snapshot(), fmt.Errorf("%w: starting the command exceeded %s", errProviderCommandTimeout, timeout)
|
||||
proc, err := startCommandProcess(cmd)
|
||||
if err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
defer proc.Close()
|
||||
|
||||
@@ -158,6 +62,9 @@ func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte,
|
||||
done <- cmd.Wait()
|
||||
}()
|
||||
|
||||
timer := time.NewTimer(timeout)
|
||||
defer timer.Stop()
|
||||
|
||||
select {
|
||||
case err := <-done:
|
||||
if err != nil {
|
||||
@@ -176,19 +83,13 @@ func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte,
|
||||
// and tests unable to tell which one happened. Callers that only ask
|
||||
// errors.Is(err, errProviderCommandTimeout) are unaffected, so the
|
||||
// message LoadProviderCommand reports is unchanged.
|
||||
return stdout.snapshot(), stderr.snapshot(), fmt.Errorf("%w: %w", errProviderCommandTimeout, err)
|
||||
return stdout.Bytes(), stderr.Bytes(), fmt.Errorf("%w: %w", errProviderCommandTimeout, err)
|
||||
}
|
||||
return stdout.snapshot(), stderr.snapshot(), err
|
||||
case <-deadline.C:
|
||||
return stdout.Bytes(), stderr.Bytes(), err
|
||||
case <-timer.C:
|
||||
proc.Terminate()
|
||||
// Bounded, unlike the bare receive this replaces. Terminate kills the
|
||||
// tree so Wait almost always returns at once, but "almost always" was
|
||||
// doing real work here: a tree that resists termination used to hold the
|
||||
// call open indefinitely, long past the deadline that had just expired.
|
||||
// Returning early leaves cmd.Wait running, which is why the buffers are
|
||||
// synchronised.
|
||||
awaitWithGrace(done, providerCommandDrainGrace)
|
||||
return stdout.snapshot(), stderr.snapshot(), fmt.Errorf("%w: the command did not finish within %s", errProviderCommandTimeout, timeout)
|
||||
<-done
|
||||
return stdout.Bytes(), stderr.Bytes(), errProviderCommandTimeout
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -414,133 +414,3 @@ func itoa(value int) string {
|
||||
}
|
||||
return string(digits[index:])
|
||||
}
|
||||
|
||||
// The whole point of #811: providerCommandTimeout has to be an upper bound on
|
||||
// how long the call takes, not a floor. Process creation used to run before the
|
||||
// clock started and the post-termination drain was unbounded, so a slow or
|
||||
// contended machine could block startup for far longer than the error text
|
||||
// promised.
|
||||
//
|
||||
// This one guards the end-to-end contract against gross regressions. It is NOT
|
||||
// the assertion that pins the drain bound: Terminate kills the tree, so Wait
|
||||
// returns promptly and this passes even with the drain unbounded. That property
|
||||
// is pinned by TestAwaitWithGraceGivesUp, and the start-phase bound by
|
||||
// TestRunProviderCommandCountsProcessStartAgainstTheDeadline.
|
||||
func TestRunProviderCommandBoundsTotalDuration(t *testing.T) {
|
||||
pidFile := filepath.Join(t.TempDir(), "bound.pid")
|
||||
command := writeCommand(t, commandScript{SleepSeconds: 60, PidFile: pidFile})
|
||||
|
||||
const budget = 2 * time.Second
|
||||
start := time.Now()
|
||||
_, _, err := runProviderCommand(command, budget)
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if !errors.Is(err, errProviderCommandTimeout) {
|
||||
t.Fatalf("error = %v, want the timeout sentinel", err)
|
||||
}
|
||||
// Budget, plus the bounded drain, plus room for a loaded runner. A failure
|
||||
// here means some phase is outside the deadline again, which is the bug.
|
||||
ceiling := budget + providerCommandDrainGrace + 8*time.Second
|
||||
if elapsed > ceiling {
|
||||
t.Fatalf("returned after %s with a %s budget; the timeout is not bounding the call (ceiling %s)", elapsed, budget, ceiling)
|
||||
}
|
||||
assertProcessTerminatedIfStarted(t, pidFile)
|
||||
}
|
||||
|
||||
// The timeout error has to carry something an operator can act on. It used to be
|
||||
// a bare string with the underlying error and the command's stderr both dropped,
|
||||
// so a user whose provider command was slow got no clue which phase ran out or
|
||||
// what the command had said.
|
||||
func TestLoadProviderCommandTimeoutReportsDiagnostics(t *testing.T) {
|
||||
command := writeCommand(t, commandScript{
|
||||
Stderr: "provider is warming up",
|
||||
SleepSeconds: 30,
|
||||
PidFile: filepath.Join(t.TempDir(), "diag.pid"),
|
||||
})
|
||||
|
||||
_, err := LoadProviderCommand(command)
|
||||
if err == nil {
|
||||
t.Fatal("LoadProviderCommand() error = nil, want timeout")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "timed out") {
|
||||
t.Fatalf("error = %q, want a timeout message", err.Error())
|
||||
}
|
||||
// Wrapped, so callers can still classify it.
|
||||
if !errors.Is(err, errProviderCommandTimeout) {
|
||||
t.Fatalf("error = %v, want errors.Is against the timeout sentinel", err)
|
||||
}
|
||||
// And it must say which phase, rather than only that time ran out.
|
||||
if !strings.Contains(err.Error(), "did not finish") && !strings.Contains(err.Error(), "starting the command") {
|
||||
t.Fatalf("error = %q, want it to name the phase that exceeded the deadline", err.Error())
|
||||
}
|
||||
}
|
||||
|
||||
// The output buffers are written by cmd.Wait's I/O pump, which keeps running
|
||||
// after the timeout path has returned them to the caller, so a plain
|
||||
// bytes.Buffer would be read and written concurrently. Asserted on the buffer
|
||||
// directly: driving it end to end does not reliably produce the overlap, because
|
||||
// a terminated command usually stops writing before the snapshot is taken, so
|
||||
// such a test passes with the locking removed and proves nothing.
|
||||
//
|
||||
// Only meaningful under -race, which CI runs.
|
||||
func TestSyncBufferAllowsConcurrentWriteAndSnapshot(t *testing.T) {
|
||||
buffer := &syncBuffer{}
|
||||
writing := make(chan struct{})
|
||||
go func() {
|
||||
defer close(writing)
|
||||
for i := 0; i < 2000; i++ {
|
||||
_, _ = buffer.Write([]byte("x"))
|
||||
}
|
||||
}()
|
||||
for i := 0; i < 2000; i++ {
|
||||
_ = buffer.snapshot()
|
||||
}
|
||||
<-writing
|
||||
if len(buffer.snapshot()) != 2000 {
|
||||
t.Fatalf("buffer holds %d bytes, want 2000", len(buffer.snapshot()))
|
||||
}
|
||||
}
|
||||
|
||||
// The drain after termination must be bounded. Asserted directly on the helper,
|
||||
// because in place it is untestable without a tree that refuses to die: Wait
|
||||
// normally returns at once, which is precisely why an unbounded receive sat
|
||||
// there unnoticed. A channel that never fires is the pathological case made
|
||||
// cheap.
|
||||
func TestAwaitWithGraceGivesUp(t *testing.T) {
|
||||
never := make(chan error)
|
||||
start := time.Now()
|
||||
if awaitWithGrace(never, 50*time.Millisecond) {
|
||||
t.Fatal("reported completion for a wait that never finished")
|
||||
}
|
||||
if elapsed := time.Since(start); elapsed > 5*time.Second {
|
||||
t.Fatalf("gave up after %s, want roughly the grace period", elapsed)
|
||||
}
|
||||
// Still reports completion when the wait does finish, or the timeout path
|
||||
// would always claim the tree hung.
|
||||
done := make(chan error, 1)
|
||||
done <- nil
|
||||
if !awaitWithGrace(done, time.Second) {
|
||||
t.Fatal("reported expiry for a wait that had already finished")
|
||||
}
|
||||
}
|
||||
|
||||
// Process creation has to be inside the budget. It used to run before the clock
|
||||
// started, and on Windows that phase is CREATE_SUSPENDED plus job-object setup
|
||||
// plus a system-wide thread snapshot, none of it fast on a contended machine.
|
||||
//
|
||||
// A budget that is already spent when the call begins can only be exceeded
|
||||
// during the start, so the phase named in the error is what distinguishes the
|
||||
// two orderings: with the clock started afterwards, the start would always
|
||||
// complete first and the failure would come from the wait instead.
|
||||
func TestRunProviderCommandCountsProcessStartAgainstTheDeadline(t *testing.T) {
|
||||
command := writeCommand(t, commandScript{Stdout: `{"name":"cmd","provider":"openai","apiKey":"sk-x","model":"m"}`})
|
||||
|
||||
_, _, err := runProviderCommand(command, time.Nanosecond)
|
||||
if !errors.Is(err, errProviderCommandTimeout) {
|
||||
t.Fatalf("error = %v, want the timeout sentinel", err)
|
||||
}
|
||||
if !strings.Contains(err.Error(), "starting the command") {
|
||||
t.Fatalf("error = %q, want the start phase to be the one that exceeded the deadline; "+
|
||||
"anything else means process creation is outside the budget again", err.Error())
|
||||
}
|
||||
}
|
||||
@@ -2,7 +2,6 @@ package imageinput
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"os"
|
||||
@@ -17,13 +16,6 @@ import (
|
||||
// clipboard, or (nil, "", nil) when the clipboard has no image. Called when
|
||||
// text clipboard is empty (the user pasted a screenshot). The media type is
|
||||
// sniffed from the bytes, not trusted from the clipboard.
|
||||
// ErrClipboardImageUnreadable reports that the clipboard holds an image the host
|
||||
// has no way to extract. It is deliberately distinct from "no image on the
|
||||
// clipboard", which is an ordinary no-op: this one is actionable, and silence is
|
||||
// the wrong response to it.
|
||||
var ErrClipboardImageUnreadable = errors.New(
|
||||
"the clipboard holds an image but no helper on this host can read it; on macOS install pngpaste (brew install pngpaste) or a Python with pyobjc")
|
||||
|
||||
func ReadClipboardImage() ([]byte, string, error) {
|
||||
data, err := readClipboardImageBytes()
|
||||
if err != nil {
|
||||
@@ -103,71 +95,35 @@ func readClipboardImageWindows() ([]byte, error) {
|
||||
|
||||
// readClipboardImageDarwin uses osascript to check for and read a clipboard
|
||||
// image. Returns (nil, nil) when no image is present.
|
||||
// darwinClipboardInfo reports what the pasteboard is currently offering, and
|
||||
// darwinClipboardExtract pulls the PNG bytes out of it.
|
||||
//
|
||||
// Both are vars so the two-step logic below can be driven in a test. The step
|
||||
// that matters, "the clipboard holds an image and nothing here can extract it",
|
||||
// only happens when the first succeeds and the second fails, which cannot be
|
||||
// arranged by stubbing the caller: a test that substitutes ReadClipboardImage
|
||||
// asserts the plumbing around this function rather than the decision inside it,
|
||||
// and stays green with that decision reverted.
|
||||
var (
|
||||
darwinClipboardInfo = func() (string, error) {
|
||||
out, err := exec.Command("sh", "-c", `osascript -e 'clipboard info'`).Output()
|
||||
return string(out), err
|
||||
func readClipboardImageDarwin() ([]byte, error) {
|
||||
// Check clipboard info for image classes.
|
||||
check := `osascript -e 'clipboard info'`
|
||||
out, err := exec.Command("sh", "-c", check).Output()
|
||||
if err != nil {
|
||||
return nil, nil
|
||||
}
|
||||
darwinClipboardExtract = func() ([]byte, error) {
|
||||
// pngpaste if available, falling back to a Python one-liner. Neither
|
||||
// ships with macOS: pngpaste is Homebrew-only and Apple dropped the
|
||||
// bundled PyObjC, so a stock Mac fails here every time.
|
||||
cmd := exec.Command("sh", "-c", `pngpaste - 2>/dev/null || python3 -c "
|
||||
info := string(out)
|
||||
if !strings.Contains(info, "PNG") && !strings.Contains(info, "JPEG") && !strings.Contains(info, "TIFF") && !strings.Contains(info, "GIF") {
|
||||
return nil, nil
|
||||
}
|
||||
// Write clipboard image to a temp file via AppleScript, then read it.
|
||||
// Using pngpaste if available, falling back to a Python one-liner.
|
||||
cmd := exec.Command("sh", "-c", `pngpaste - 2>/dev/null || python3 -c "
|
||||
import AppKit, sys
|
||||
pb = AppKit.NSPasteboard.generalPasteboard()
|
||||
data = pb.dataForType_(AppKit.NSPasteboardTypePNG)
|
||||
if data:
|
||||
sys.stdout.buffer.write(data.bytes())
|
||||
"`)
|
||||
var stdout bytes.Buffer
|
||||
cmd.Stdout = &stdout
|
||||
if err := cmd.Run(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return stdout.Bytes(), nil
|
||||
}
|
||||
)
|
||||
|
||||
// darwinClipboardOffersImage reports whether the pasteboard info line names an
|
||||
// image class.
|
||||
func darwinClipboardOffersImage(info string) bool {
|
||||
for _, class := range []string{"PNG", "JPEG", "TIFF", "GIF"} {
|
||||
if strings.Contains(info, class) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func readClipboardImageDarwin() ([]byte, error) {
|
||||
info, err := darwinClipboardInfo()
|
||||
if err != nil {
|
||||
var stdout bytes.Buffer
|
||||
cmd.Stdout = &stdout
|
||||
if err := cmd.Run(); err != nil {
|
||||
return nil, nil
|
||||
}
|
||||
if !darwinClipboardOffersImage(info) {
|
||||
if stdout.Len() == 0 {
|
||||
return nil, nil
|
||||
}
|
||||
// Past this point the clipboard is KNOWN to hold an image, because the probe
|
||||
// above said so. A failure here is therefore not "no image", it is "there is
|
||||
// an image and nothing on this host can extract it". Reporting that as
|
||||
// absence is what left users staring at a paste that silently did nothing.
|
||||
data, err := darwinClipboardExtract()
|
||||
if err != nil {
|
||||
return nil, ErrClipboardImageUnreadable
|
||||
}
|
||||
if len(data) == 0 {
|
||||
return nil, ErrClipboardImageUnreadable
|
||||
}
|
||||
return data, nil
|
||||
return stdout.Bytes(), nil
|
||||
}
|
||||
|
||||
// readClipboardImageLinux tries wl-paste (Wayland) then xclip (X11) to read
|
||||
|
||||
@@ -1,84 +0,0 @@
|
||||
package imageinput
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// Drives readClipboardImageDarwin's own decision rather than the plumbing around
|
||||
// it. Both shell steps are substituted, so this runs on any platform and, more
|
||||
// importantly, actually reaches the branch under test: a test that stubs
|
||||
// ReadClipboardImage instead stays green with the production change reverted,
|
||||
// which is how the first version of this coverage slipped through.
|
||||
func TestReadClipboardImageDarwinDistinguishesUnreadableFromAbsent(t *testing.T) {
|
||||
restore := func() func() {
|
||||
info, extract := darwinClipboardInfo, darwinClipboardExtract
|
||||
return func() { darwinClipboardInfo, darwinClipboardExtract = info, extract }
|
||||
}()
|
||||
t.Cleanup(restore)
|
||||
|
||||
const pngInfo = `«class PNGf», «class 8BPS»`
|
||||
|
||||
t.Run("image present but no extractor is actionable", func(t *testing.T) {
|
||||
darwinClipboardInfo = func() (string, error) { return pngInfo, nil }
|
||||
darwinClipboardExtract = func() ([]byte, error) { return nil, errors.New("exit status 127") }
|
||||
|
||||
data, err := readClipboardImageDarwin()
|
||||
if !errors.Is(err, ErrClipboardImageUnreadable) {
|
||||
t.Fatalf("err = %v, want ErrClipboardImageUnreadable; a stock Mac gets silence instead of a remedy", err)
|
||||
}
|
||||
if data != nil {
|
||||
t.Fatalf("data = %q, want none", data)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("extractor producing nothing is also actionable", func(t *testing.T) {
|
||||
darwinClipboardInfo = func() (string, error) { return pngInfo, nil }
|
||||
darwinClipboardExtract = func() ([]byte, error) { return nil, nil }
|
||||
|
||||
if _, err := readClipboardImageDarwin(); !errors.Is(err, ErrClipboardImageUnreadable) {
|
||||
t.Fatalf("err = %v, want ErrClipboardImageUnreadable for an extractor that exits 0 with no bytes", err)
|
||||
}
|
||||
})
|
||||
|
||||
// The other half of the distinction, and the reason this cannot simply always
|
||||
// report an error: an empty clipboard is an ordinary no-op, and turning that
|
||||
// into a notice would make every stray Ctrl+V nag the user.
|
||||
t.Run("no image on the clipboard stays silent", func(t *testing.T) {
|
||||
darwinClipboardInfo = func() (string, error) { return `«class utf8»`, nil }
|
||||
darwinClipboardExtract = func() ([]byte, error) {
|
||||
t.Fatal("extractor ran despite the clipboard offering no image")
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
data, err := readClipboardImageDarwin()
|
||||
if err != nil || data != nil {
|
||||
t.Fatalf("data=%q err=%v, want a silent no-op", data, err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("probe failure stays silent", func(t *testing.T) {
|
||||
darwinClipboardInfo = func() (string, error) { return "", errors.New("osascript missing") }
|
||||
darwinClipboardExtract = func() ([]byte, error) {
|
||||
t.Fatal("extractor ran despite the info probe failing")
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
if data, err := readClipboardImageDarwin(); err != nil || data != nil {
|
||||
t.Fatalf("data=%q err=%v, want a silent no-op", data, err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("working extractor returns the bytes", func(t *testing.T) {
|
||||
darwinClipboardInfo = func() (string, error) { return pngInfo, nil }
|
||||
darwinClipboardExtract = func() ([]byte, error) { return []byte("png-bytes"), nil }
|
||||
|
||||
data, err := readClipboardImageDarwin()
|
||||
if err != nil {
|
||||
t.Fatalf("err = %v, want none", err)
|
||||
}
|
||||
if string(data) != "png-bytes" {
|
||||
t.Fatalf("data = %q, want the extractor's bytes", data)
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -138,7 +138,6 @@ var descriptors = []Descriptor{
|
||||
openAICompat("groq", "Groq", "https://api.groq.com/openai/v1", "llama-3.3-70b-versatile", []string{"GROQ_API_KEY"}),
|
||||
openAICompat("deepseek", "DeepSeek", "https://api.deepseek.com/v1", "deepseek-chat", []string{"DEEPSEEK_API_KEY"}),
|
||||
openAICompat("together", "Together AI", "https://api.together.xyz/v1", "meta-llama/Llama-3.3-70B-Instruct-Turbo", []string{"TOGETHER_API_KEY"}),
|
||||
openAICompat("fireworks", "Fireworks AI", "https://api.fireworks.ai/inference/v1", "accounts/fireworks/models/kimi-k2p7-code", []string{"FIREWORKS_API_KEY"}, "fireworks-ai"),
|
||||
openAICompat("dashscope", "DashScope", "https://dashscope-intl.aliyuncs.com/compatible-mode/v1", "qwen-plus", []string{"DASHSCOPE_API_KEY", "QWEN_API_KEY"}, "qwen"),
|
||||
openAICompat("moonshot", "Moonshot AI", "https://api.moonshot.ai/v1", "kimi-k2-0905-preview", []string{"MOONSHOT_API_KEY"}, "kimi"),
|
||||
openAICompat("atlascloud", "Atlas Cloud", "https://api.atlascloud.ai/v1", "qwen/qwen3.5-flash", []string{"ATLASCLOUD_API_KEY"}, "atlas cloud", "atlas"),
|
||||
|
||||
@@ -22,7 +22,6 @@ var expectedCatalogIDs = []string{
|
||||
"groq",
|
||||
"deepseek",
|
||||
"together",
|
||||
"fireworks",
|
||||
"dashscope",
|
||||
"moonshot",
|
||||
"atlascloud",
|
||||
@@ -156,28 +155,6 @@ func TestLongCatDescriptor(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestFireworksDescriptor(t *testing.T) {
|
||||
descriptor, err := Require("fireworks")
|
||||
if err != nil {
|
||||
t.Fatalf("Require(fireworks) error = %v", err)
|
||||
}
|
||||
if descriptor.Name != "Fireworks AI" {
|
||||
t.Fatalf("Name = %q, want Fireworks AI", descriptor.Name)
|
||||
}
|
||||
if descriptor.DefaultBaseURL != "https://api.fireworks.ai/inference/v1" {
|
||||
t.Fatalf("DefaultBaseURL = %q, want Fireworks OpenAI-compatible endpoint", descriptor.DefaultBaseURL)
|
||||
}
|
||||
if descriptor.DefaultModel != "accounts/fireworks/models/kimi-k2p7-code" {
|
||||
t.Fatalf("DefaultModel = %q, want accounts/fireworks/models/kimi-k2p7-code", descriptor.DefaultModel)
|
||||
}
|
||||
if descriptor.Transport != TransportOpenAICompatible {
|
||||
t.Fatalf("Transport = %q, want %q", descriptor.Transport, TransportOpenAICompatible)
|
||||
}
|
||||
if !reflect.DeepEqual(descriptor.AuthEnvVars, []string{"FIREWORKS_API_KEY"}) {
|
||||
t.Fatalf("AuthEnvVars = %#v, want FIREWORKS_API_KEY", descriptor.AuthEnvVars)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAtlasCloudDescriptor(t *testing.T) {
|
||||
descriptor, err := Require("atlascloud")
|
||||
if err != nil {
|
||||
@@ -369,7 +346,7 @@ func TestListByTransportPreservesCatalogOrder(t *testing.T) {
|
||||
TransportBedrock: {"bedrock"},
|
||||
TransportVertex: {"vertex"},
|
||||
TransportAnthropicCompat: {"minimax", "minimaxi-cn", "opencode-go-anthropic-compatible", "custom-anthropic-compatible"},
|
||||
TransportOpenAICompat: {"gitlawb-opengateway", "aimlapi", "ollama-cloud", "ollama", "lmstudio", "openrouter", "huggingface", "chatgpt", "groq", "deepseek", "together", "fireworks", "dashscope", "moonshot", "atlascloud", "longcat", "nvidia-nim", "mistral", "github", "xai", "venice", "xiaomi-mimo", "bankr", "zai", "zai-cn", "kilocode", "opencode", "opencode-go", "atomic-chat", "chatgpt-proxy", "custom-openai-compatible"},
|
||||
TransportOpenAICompat: {"gitlawb-opengateway", "aimlapi", "ollama-cloud", "ollama", "lmstudio", "openrouter", "huggingface", "chatgpt", "groq", "deepseek", "together", "dashscope", "moonshot", "atlascloud", "longcat", "nvidia-nim", "mistral", "github", "xai", "venice", "xiaomi-mimo", "bankr", "zai", "zai-cn", "kilocode", "opencode", "opencode-go", "atomic-chat", "chatgpt-proxy", "custom-openai-compatible"},
|
||||
}
|
||||
|
||||
for transport, wantIDs := range cases {
|
||||
|
||||
@@ -122,14 +122,6 @@ var curatedModels = map[string][]Model{
|
||||
{ID: "deepseek-ai/DeepSeek-R1", Description: "reasoning model"},
|
||||
{ID: "meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8", Description: "multimodal model"},
|
||||
},
|
||||
"fireworks": {
|
||||
// Serverless-only curated set: ordinary API-key setups hit /inference/v1
|
||||
// and cannot use on-demand deployment IDs.
|
||||
{ID: "accounts/fireworks/models/kimi-k2p7-code", Description: "catalog default"},
|
||||
{ID: "accounts/fireworks/models/deepseek-v4-flash", Description: "fast coding model"},
|
||||
{ID: "accounts/fireworks/models/gpt-oss-120b", Description: "general model"},
|
||||
{ID: "accounts/fireworks/models/deepseek-v4-pro", Description: "reasoning model"},
|
||||
},
|
||||
"dashscope": {
|
||||
{ID: "qwen-plus", Description: "catalog default"},
|
||||
{ID: "qwen-max", Description: "strong general model"},
|
||||
|
||||
@@ -28,11 +28,6 @@ func TestModelsAreProviderScoped(t *testing.T) {
|
||||
want: []string{"llama-3.3-70b-versatile", "openai/gpt-oss-120b"},
|
||||
notWant: []string{"gpt-4.1", "claude-sonnet-4.5"},
|
||||
},
|
||||
{
|
||||
provider: "fireworks",
|
||||
want: []string{"accounts/fireworks/models/kimi-k2p7-code", "accounts/fireworks/models/deepseek-v4-flash"},
|
||||
notWant: []string{"gpt-4.1", "llama-3.3-70b-versatile"},
|
||||
},
|
||||
{
|
||||
provider: "chatgpt",
|
||||
want: []string{"gpt-5.5", "gpt-5.4", "gpt-5.4-mini", "gpt-5.3-codex-spark"},
|
||||
|
||||
@@ -132,8 +132,6 @@ func ModelsDevProviderID(provider providercatalog.Descriptor) string {
|
||||
return "zai"
|
||||
case "minimaxi-cn":
|
||||
return "minimax"
|
||||
case "fireworks":
|
||||
return "fireworks-ai"
|
||||
default:
|
||||
return strings.TrimSpace(provider.ID)
|
||||
}
|
||||
|
||||
@@ -131,7 +131,6 @@ func TestModelsDevProviderIDMapsZeroAliases(t *testing.T) {
|
||||
"ollama-cloud": "ollama-cloud",
|
||||
"zai-cn": "zai",
|
||||
"minimaxi-cn": "minimax",
|
||||
"fireworks": "fireworks-ai",
|
||||
}
|
||||
for zeroID, want := range tests {
|
||||
provider, ok := providercatalog.Get(zeroID)
|
||||
|
||||
@@ -25,9 +25,7 @@ type TokenResolver func(ctx context.Context, forceRefresh bool) (header string,
|
||||
//
|
||||
// base carries the API-key auth config (key + default header/scheme). setExtra
|
||||
// (optional) sets the provider's non-auth headers on every attempt. Transient
|
||||
// retries are handled by the inner SendWithRetry: 429/503/529 on their own
|
||||
// budget, and provably pre-send transport failures (refused or timed-out
|
||||
// connect, DNS, TLS handshake) on a separate, shorter one.
|
||||
// retries (429/503/529) are handled by the inner SendWithRetry.
|
||||
func SendWithAuthRetry(
|
||||
ctx context.Context,
|
||||
client *http.Client,
|
||||
|
||||
@@ -3,9 +3,6 @@ package providerio
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"io"
|
||||
"net"
|
||||
"net/http"
|
||||
"strconv"
|
||||
"strings"
|
||||
@@ -20,22 +17,13 @@ import (
|
||||
// (previously only the OpenAI provider retried; Anthropic and Gemini surfaced the
|
||||
// first failure).
|
||||
//
|
||||
// What is retried: 429 (rate limit) and 503 (service unavailable) — the statuses
|
||||
// where the server explicitly did NOT accept the request — plus PROVABLY
|
||||
// pre-send transport failures (DNS resolution, a refused/unreachable dial, a TLS
|
||||
// handshake timeout) where no request bytes ever left this host. In every one of
|
||||
// those cases we KNOW the server did not receive and process the request, so a
|
||||
// replay cannot duplicate work. Other 5xx (500/502/504) and every ambiguous or
|
||||
// post-send transport error (connection reset, broken pipe, EOF, a generic i/o
|
||||
// timeout, context deadline) are NOT retried: a completion POST is
|
||||
// non-idempotent and may already have reached and been processed by the server,
|
||||
// so replaying it could duplicate (billable) work. Only the INITIAL request is
|
||||
// ever in scope; once the response body starts streaming it is never re-issued.
|
||||
// Pre-send classification is by syscall errno (portable across POSIX and
|
||||
// Windows), scoped to an Op=="dial" *net.OpError so the same errno raised on a
|
||||
// post-send read/write can't be misread as pre-send. NXDOMAIN is treated as
|
||||
// permanent (not retried), and the pre-send path fails fast on its own
|
||||
// sub-second schedule rather than the seconds-long 429 backoff.
|
||||
// What is retried: ONLY 429 (rate limit) and 503 (service unavailable) — the
|
||||
// statuses where the server explicitly did NOT accept the request, so replaying
|
||||
// it cannot duplicate work. Other 5xx (500/502/504) and transport/network errors
|
||||
// are NOT retried: a completion POST is non-idempotent and may have reached and
|
||||
// been processed by the server, so replaying it could duplicate
|
||||
// (billable) work. Only the INITIAL request is ever in scope; once the response
|
||||
// body starts streaming it is never re-issued.
|
||||
|
||||
const defaultMaxRetryAttempts = 6
|
||||
|
||||
@@ -49,32 +37,12 @@ const maxBackoff = 30 * time.Second
|
||||
// schedule is 2s, 4s, 8s, 16s, then maxBackoff. A var so tests can shrink it.
|
||||
var retryBackoffBase = 2 * time.Second
|
||||
|
||||
// preSendMaxAttempts bounds retries of a provably pre-send transport failure.
|
||||
// Unlike a 429 (whose window is measured in seconds), a refused/unreachable dial
|
||||
// either recovers within a few hundred ms or is a deterministic misconfiguration
|
||||
// (wrong host/port, dead local daemon) that will never recover — so a short,
|
||||
// low-count schedule fails fast instead of stalling the agent ~60s on the
|
||||
// rate-limit schedule. This bounds the pre-send path independently of the caller's
|
||||
// maxAttempts, which is tuned for 429/503.
|
||||
const preSendMaxAttempts = 3
|
||||
|
||||
// preSendBackoffBase is the first wait before replaying a pre-send transport
|
||||
// failure. Modeled on the mid-stream reconnect base (internal/agent/reconnect.go),
|
||||
// it is sub-second so a transient dial blip recovers quickly and a permanent one
|
||||
// fails fast. A var so tests can shrink it.
|
||||
var preSendBackoffBase = 500 * time.Millisecond
|
||||
|
||||
// SendWithRetry issues an HTTP request, retrying ONLY the safe-to-replay server
|
||||
// responses (429 and 503, see ShouldRetryStatus) up to maxAttempts — backing off
|
||||
// between tries and honoring a server Retry-After header and context
|
||||
// cancellation. The one transport-error exception is a PROVABLY pre-send failure
|
||||
// (see isPreSendTransportError): a refused/unreachable dial or DNS failure where
|
||||
// no request bytes left this host, retried on its own short schedule
|
||||
// (preSendMaxAttempts / preSendBackoff). Every other 5xx and every ambiguous or
|
||||
// post-send transport error is returned immediately, never replayed, because a
|
||||
// non-idempotent completion POST may already have been received. The request is
|
||||
// rebuilt from body each attempt; setHeader (if non-nil) sets headers on every
|
||||
// attempt.
|
||||
// cancellation. Other 5xx and transport/network errors are returned immediately,
|
||||
// never replayed (see the package note). The request is rebuilt from body each
|
||||
// attempt; setHeader (if non-nil) sets headers on every attempt.
|
||||
//
|
||||
// It returns the final *http.Response (which the caller inspects for a non-2xx
|
||||
// status, exactly as before) or an error for a network failure / context
|
||||
@@ -92,47 +60,7 @@ func SendWithRetry(
|
||||
if maxAttempts <= 0 {
|
||||
maxAttempts = defaultMaxRetryAttempts
|
||||
}
|
||||
if client == nil {
|
||||
client = http.DefaultClient
|
||||
}
|
||||
// Redirects are still FOLLOWED, so a redirecting gateway keeps working, but
|
||||
// entering one makes that attempt non-retryable. The hazard is that client.Do
|
||||
// can transmit the original POST to the first host, follow a 307/308, and then
|
||||
// fail the dial to the redirect target: that error is an Op=="dial" error, so
|
||||
// isPreSendTransportError would read it as "nothing was sent" and replay a
|
||||
// completion the first host already received. Observing the redirect rather
|
||||
// than blocking it keeps both properties, since a request that was redirected
|
||||
// is never replayed while redirecting endpoints stay usable. The caller's own
|
||||
// CheckRedirect still runs and still decides whether to proceed.
|
||||
redirected := false
|
||||
callerCheckRedirect := client.CheckRedirect
|
||||
redirectAwareClient := *client
|
||||
redirectAwareClient.CheckRedirect = func(request *http.Request, via []*http.Request) error {
|
||||
redirected = true
|
||||
if callerCheckRedirect != nil {
|
||||
return callerCheckRedirect(request, via)
|
||||
}
|
||||
// Mirror net/http's default policy, which a nil CheckRedirect would have
|
||||
// applied: stop after 10 hops.
|
||||
if len(via) >= 10 {
|
||||
return errors.New("stopped after 10 redirects")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
// The two retry budgets are fully independent, each with its own counter and
|
||||
// its own backoff ordinal, because they answer different questions: a
|
||||
// dial/DNS/TLS failure is a fast local blip (preSendMaxAttempts, sub-second
|
||||
// schedule) while a 429/503 is a server-side window measured in seconds
|
||||
// (maxAttempts, retryBackoffBase). A single shared loop counter coupled them
|
||||
// in both directions: status retries starved the pre-send budget, and
|
||||
// pre-send retries shortened the status window and started its backoff at a
|
||||
// later rung. Neither kind of failure now consumes or advances the other.
|
||||
preSendAttempts := 0
|
||||
statusAttempts := 0
|
||||
for {
|
||||
// Per attempt: only a redirect entered by THIS attempt can make its own
|
||||
// transport error unsafe to replay.
|
||||
redirected = false
|
||||
for attempt := 1; ; attempt++ {
|
||||
request, err := http.NewRequestWithContext(ctx, method, url, bytes.NewReader(body))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -142,78 +70,32 @@ func SendWithRetry(
|
||||
}
|
||||
|
||||
connectSpan := trace.FromContext(ctx).Span(trace.SpanProviderConnect)
|
||||
response, err := redirectAwareClient.Do(request)
|
||||
response, err := client.Do(request)
|
||||
connectSpan.End()
|
||||
if err != nil {
|
||||
// Context cancellation always surfaces as cancellation, never a retry.
|
||||
// A transport failure on a POST does NOT mean the server didn't receive
|
||||
// it — the request may have arrived and be generating a (billable,
|
||||
// non-idempotent) completion while only the response/connection failed.
|
||||
// Replaying it could duplicate that work, so surface the error instead
|
||||
// of retrying. Only 429/503 responses (below), where we KNOW the request
|
||||
// was not accepted, are safe to retry.
|
||||
if ctx.Err() != nil {
|
||||
return nil, ctx.Err()
|
||||
}
|
||||
// A transport failure on a POST does NOT usually mean the server didn't
|
||||
// receive it — the request may have arrived and be generating a
|
||||
// (billable, non-idempotent) completion while only the response or
|
||||
// connection failed. Replaying that could duplicate work, so it is
|
||||
// surfaced immediately. The one safe exception is a PROVABLY pre-send
|
||||
// failure (see isPreSendTransportError): the request never left this
|
||||
// host, so a replay cannot duplicate anything — exactly like a 429/503
|
||||
// where we KNOW the request was not accepted. The pre-send path uses
|
||||
// its own short bound and sub-second schedule (preSendMaxAttempts /
|
||||
// preSendBackoff), not the seconds-long 429 schedule, so a permanent
|
||||
// dial failure fails fast instead of stalling the agent for ~60s.
|
||||
// A redirect means the original POST already left this host, so the
|
||||
// dial that failed belongs to a later hop and replaying would duplicate
|
||||
// a completion the first host may already have processed.
|
||||
if isPreSendTransportError(err) && !redirected {
|
||||
preSendAttempts++
|
||||
if preSendAttempts < preSendMaxAttempts {
|
||||
if r := trace.FromContext(ctx); r != nil {
|
||||
r.Counter(trace.CounterRetryCount, 1)
|
||||
}
|
||||
if preSendBackoff(ctx, preSendAttempts) {
|
||||
continue
|
||||
}
|
||||
return nil, ctx.Err()
|
||||
}
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Deliberately NOT guarded on `redirected`, unlike the transport-error
|
||||
// branch above. The two differ in what is known about the request. A
|
||||
// transport failure after a redirect leaves it unknown whether the
|
||||
// redirect target received and began processing the POST, so replaying
|
||||
// could duplicate billable work. Here every hop answered: a 307/308 is the
|
||||
// origin declining and delegating, and a retryable status is the target
|
||||
// declining as well, so nothing accepted the request and a replay
|
||||
// duplicates nothing.
|
||||
//
|
||||
// The exposure is also identical to a retryable status with no redirect in
|
||||
// play, which this package has always retried. Guarding on redirects here
|
||||
// would not close a gap, it would make a rate-limited completion behind a
|
||||
// redirecting gateway fail outright instead of backing off.
|
||||
// The transport hop succeeded, so the connect phase is healthy again and
|
||||
// the pre-send budget starts over. It bounds CONSECUTIVE connect failures,
|
||||
// not the lifetime of the call: without this reset, two early dial retries
|
||||
// followed by a 429 and then one more dial blip would surface a transport
|
||||
// error while the status budget still had room, which is the coupling
|
||||
// these two counters were split apart to remove. Total work stays bounded,
|
||||
// since every reset costs a status attempt and those are capped.
|
||||
preSendAttempts = 0
|
||||
|
||||
if ShouldRetryStatus(response.StatusCode) {
|
||||
statusAttempts++
|
||||
if statusAttempts < maxAttempts {
|
||||
if r := trace.FromContext(ctx); r != nil {
|
||||
r.Counter(trace.CounterRetryCount, 1)
|
||||
}
|
||||
wait := RetryAfter(response)
|
||||
_ = response.Body.Close()
|
||||
if Backoff(ctx, statusAttempts, wait) {
|
||||
continue
|
||||
}
|
||||
// Backoff aborted: the only reason it returns false is ctx cancellation.
|
||||
return nil, ctx.Err()
|
||||
if ShouldRetryStatus(response.StatusCode) && attempt < maxAttempts {
|
||||
if r := trace.FromContext(ctx); r != nil {
|
||||
r.Counter(trace.CounterRetryCount, 1)
|
||||
}
|
||||
wait := RetryAfter(response)
|
||||
_ = response.Body.Close()
|
||||
if Backoff(ctx, attempt, wait) {
|
||||
continue
|
||||
}
|
||||
// Backoff aborted: the only reason it returns false is ctx cancellation.
|
||||
return nil, ctx.Err()
|
||||
}
|
||||
|
||||
// Success, a non-retryable status, or retries exhausted on a retryable
|
||||
@@ -227,119 +109,6 @@ func SendWithRetry(
|
||||
}
|
||||
}
|
||||
|
||||
// isPreSendTransportError reports whether a transport error PROVES no request
|
||||
// bytes reached the server, so replaying the request cannot duplicate a
|
||||
// billable completion. Only narrow, unambiguous pre-connect failures qualify: a
|
||||
// recoverable DNS lookup failure, a refused/unreachable dial, and a TLS
|
||||
// handshake timeout (the handshake completes before any HTTP request bytes are
|
||||
// written).
|
||||
//
|
||||
// The refused/unreachable errnos (ECONNREFUSED/ENETUNREACH/EHOSTUNREACH) are
|
||||
// matched by syscall errno via errors.Is — portable across platforms, since Go
|
||||
// maps the WSA* codes on Windows too — but ONLY when carried by an Op=="dial"
|
||||
// *net.OpError. errors.Is matches an errno anywhere in the chain regardless of
|
||||
// which socket operation raised it, and the kernel raises these same errnos on
|
||||
// an established connection when a route drops mid-generation; scoping to the
|
||||
// dial phase is what makes "no request bytes were sent" structural rather than
|
||||
// argued, so a post-send read/write can never replay a non-idempotent POST.
|
||||
// NXDOMAIN is treated as permanent (deterministic) and is NOT retried.
|
||||
//
|
||||
// Ambiguous or post-send failures are deliberately excluded and checked FIRST,
|
||||
// so an error that is both is never treated as pre-send: a reset or broken pipe
|
||||
// can follow a request that was already sent, and a bare "i/o timeout" covers
|
||||
// read timeouts after send as well as dial timeouts, so none of these prove the
|
||||
// request was not received and a non-idempotent POST must not be replayed on
|
||||
// them.
|
||||
func isPreSendTransportError(err error) bool {
|
||||
if err == nil {
|
||||
return false
|
||||
}
|
||||
// DNS resolution precedes any connection, so a lookup failure proves nothing
|
||||
// was sent — but only retry a lookup that could plausibly succeed on a replay.
|
||||
// NXDOMAIN (IsNotFound) is authoritative and deterministic (a mistyped or
|
||||
// non-existent host), so retrying it just stalls the agent through the whole
|
||||
// backoff schedule before failing anyway; a timeout or temporary SERVFAIL is
|
||||
// worth another try. errors.As unwraps url.Error/OpError to reach the DNSError.
|
||||
var dnsErr *net.DNSError
|
||||
if errors.As(err, &dnsErr) {
|
||||
return !dnsErr.IsNotFound
|
||||
}
|
||||
// Post-send / ambiguous failures, excluded first. EOF and reset are matched
|
||||
// by identity so wording (or a hostname containing "eof") can't fool them.
|
||||
if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) || isConnResetErrno(err) {
|
||||
return false
|
||||
}
|
||||
// A CONNECT-phase timeout is provably pre-send for the same structural reason
|
||||
// a refused dial is: the connection was never established, so no request
|
||||
// bytes left this host. It has to be admitted BEFORE the wording exclusions
|
||||
// below, because net surfaces it with "i/o timeout" in the message, which the
|
||||
// blanket exclusion would otherwise reject. The gate is what keeps that safe:
|
||||
// a post-send read timeout is also Timeout(), but carries Op "read", so it
|
||||
// never reaches here.
|
||||
if opErr := connectOpError(err); opErr != nil && opErr.Timeout() {
|
||||
return true
|
||||
}
|
||||
msg := strings.ToLower(err.Error())
|
||||
if strings.Contains(msg, "connection reset") ||
|
||||
strings.Contains(msg, "broken pipe") ||
|
||||
strings.Contains(msg, "i/o timeout") {
|
||||
return false
|
||||
}
|
||||
// A refused/unreachable connection is provably pre-send ONLY when it was
|
||||
// raised during the DIAL (connection-establishment) phase. The same errnos
|
||||
// are also raised by the kernel on an ESTABLISHED connection — when a route
|
||||
// drops or an ICMP unreachable arrives mid-generation — which is post-send.
|
||||
// errors.Is matches an errno anywhere in the chain regardless of the
|
||||
// operation, so we first require an Op=="dial" *net.OpError: that makes "no
|
||||
// request bytes left this host" structural, so a post-send read/write carrying
|
||||
// one of these can never replay a non-idempotent POST. dialPreSendErrnos is
|
||||
// platform-specific: on Windows a refused dial carries the raw WSA* errno
|
||||
// (WSAECONNREFUSED etc.), which does NOT satisfy errors.Is against the POSIX
|
||||
// syscall.ECONNREFUSED, so the Windows list adds those codes. The string
|
||||
// markers only catch a POSIX dial error already flattened past its errno.
|
||||
if connectOpError(err) != nil {
|
||||
for _, errno := range dialPreSendErrnos {
|
||||
if errors.Is(err, errno) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
switch {
|
||||
case strings.Contains(msg, "connection refused"),
|
||||
strings.Contains(msg, "network is unreachable"),
|
||||
strings.Contains(msg, "no route to host"):
|
||||
return true
|
||||
}
|
||||
}
|
||||
// A TLS handshake times out after the TCP dial connects but before any HTTP
|
||||
// request bytes are written, so it is structurally pre-send. net/http surfaces
|
||||
// it as its own error string, not a *net.OpError, so it is matched by wording
|
||||
// independent of the dial gate.
|
||||
if strings.Contains(msg, "tls handshake timeout") {
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// connectOpError returns the outermost *net.OpError in err's chain when it comes
|
||||
// from the connection-establishment phase, else nil. Go tags a direct dial with
|
||||
// Op "dial" and a failed CONNECT through an HTTP proxy with Op "proxyconnect";
|
||||
// a failure on an already-established connection carries Op "read"/"write".
|
||||
// This is how isPreSendTransportError scopes the pre-send match to the connect
|
||||
// phase so a post-send failure can't be misclassified as pre-send.
|
||||
//
|
||||
// "proxyconnect" belongs here for the same reason "dial" does: when HTTPS_PROXY
|
||||
// is set and the CONNECT fails, the tunnel to the model host was never built, so
|
||||
// no request bytes reached it. Omitting it meant every pre-send failure behind a
|
||||
// corporate proxy fell through to no-retry, which is the environment where
|
||||
// transient connect failures are most common.
|
||||
func connectOpError(err error) *net.OpError {
|
||||
var opErr *net.OpError
|
||||
if errors.As(err, &opErr) && (opErr.Op == "dial" || opErr.Op == "proxyconnect") {
|
||||
return opErr
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// ShouldRetryStatus reports whether an HTTP status is safe to retry for a
|
||||
// non-idempotent completion POST: 429 (Too Many Requests), 503 (Service
|
||||
// Unavailable), and 529 (Anthropic's "overloaded"). All mean the server
|
||||
@@ -368,39 +137,6 @@ func Backoff(ctx context.Context, attempt int, retryAfter time.Duration) bool {
|
||||
}
|
||||
}
|
||||
|
||||
// preSendBackoff waits before pre-send retry attempt N (1-based) on a short
|
||||
// sub-second schedule (500ms, 1s, ...), returning false if the context is
|
||||
// cancelled during the wait. Separate from Backoff's seconds-long rate-limit
|
||||
// schedule because a pre-send dial failure recovers in milliseconds or never.
|
||||
func preSendBackoff(ctx context.Context, attempt int) bool {
|
||||
timer := time.NewTimer(preSendBackoffWait(attempt))
|
||||
defer timer.Stop()
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
return false
|
||||
case <-timer.C:
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
// preSendBackoffWait computes the pre-send wait before retry attempt N (1-based):
|
||||
// exponential from preSendBackoffBase (500ms, 1s, 2s, ...), capped at maxBackoff.
|
||||
// The exponent is clamped so a large attempt count cannot overflow.
|
||||
func preSendBackoffWait(attempt int) time.Duration {
|
||||
exponent := attempt - 1
|
||||
if exponent > 5 {
|
||||
exponent = 5
|
||||
}
|
||||
if exponent < 0 {
|
||||
exponent = 0
|
||||
}
|
||||
wait := preSendBackoffBase * time.Duration(1<<exponent)
|
||||
if wait > maxBackoff {
|
||||
wait = maxBackoff
|
||||
}
|
||||
return wait
|
||||
}
|
||||
|
||||
// backoffWait computes the wait before retry attempt N (1-based): Retry-After
|
||||
// when supplied, else exponential from retryBackoffBase, both capped at
|
||||
// maxBackoff. The exponent is clamped so a large attempt count cannot overflow.
|
||||
|
||||
@@ -1,32 +0,0 @@
|
||||
//go:build !windows && !plan9
|
||||
|
||||
package providerio
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"syscall"
|
||||
)
|
||||
|
||||
// dialPreSendErrnos are the syscall errnos a refused or unreachable DIAL raises
|
||||
// on this platform. On POSIX the standard syscall constants are exactly what the
|
||||
// net package surfaces, so matching them via errors.Is is sufficient. Plan 9 is
|
||||
// excluded because it does not define these BSD-style errno constants (it uses
|
||||
// string errors), so the constants below would not compile there; Zero targets
|
||||
// linux/darwin/windows, so no Plan 9 variant is needed.
|
||||
var dialPreSendErrnos = []error{
|
||||
syscall.ECONNREFUSED,
|
||||
syscall.ENETUNREACH,
|
||||
syscall.EHOSTUNREACH,
|
||||
// A connect timeout is normally caught by the Timeout() check on the connect
|
||||
// gate; this covers the case where the errno survives but the timeout flag
|
||||
// does not, e.g. an error rebuilt from the errno alone.
|
||||
syscall.ETIMEDOUT,
|
||||
}
|
||||
|
||||
// isConnResetErrno reports whether err carries a connection-reset errno, the
|
||||
// post-send exclusion in isPreSendTransportError. It lives here (per platform)
|
||||
// so retry.go references no syscall constants and stays buildable on Plan 9,
|
||||
// which defines none of them.
|
||||
func isConnResetErrno(err error) bool {
|
||||
return errors.Is(err, syscall.ECONNRESET)
|
||||
}
|
||||
@@ -1,15 +0,0 @@
|
||||
//go:build plan9
|
||||
|
||||
package providerio
|
||||
|
||||
// Plan 9 has no BSD-style errno constants (it uses string errors), so the
|
||||
// errno-based classification is unavailable here: isConnResetErrno returns false
|
||||
// and dialPreSendErrnos is empty. retry.go still classifies via its string
|
||||
// markers on this platform, the pre-send inclusions (connection refused /
|
||||
// network unreachable / no route to host) and the separate post-send
|
||||
// "connection reset" exclusion each keep working through the wording checks.
|
||||
// Zero targets linux/darwin/windows, so this variant only keeps the package
|
||||
// building on Plan 9 rather than adding real support.
|
||||
var dialPreSendErrnos = []error{}
|
||||
|
||||
func isConnResetErrno(error) bool { return false }
|
||||
@@ -1,40 +0,0 @@
|
||||
//go:build windows
|
||||
|
||||
package providerio
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"syscall"
|
||||
|
||||
"golang.org/x/sys/windows"
|
||||
)
|
||||
|
||||
// dialPreSendErrnos are the errnos a refused or unreachable DIAL raises on
|
||||
// Windows. A real Windows dial failure carries the raw Winsock error
|
||||
// (WSAECONNREFUSED = 10061, WSAENETUNREACH = 10051, WSAEHOSTUNREACH = 10065), and
|
||||
// errors.Is against the POSIX syscall.ECONNREFUSED returns FALSE there because
|
||||
// Go's Windows syscall.ECONNREFUSED is a distinct value the net package never
|
||||
// produces. So the WSA codes must be matched explicitly, or a refused dial would
|
||||
// silently never be retried on Windows. The POSIX constants stay in the list too
|
||||
// so an error built with them (e.g. a test fixture) still classifies.
|
||||
var dialPreSendErrnos = []error{
|
||||
syscall.ECONNREFUSED,
|
||||
syscall.ENETUNREACH,
|
||||
syscall.EHOSTUNREACH,
|
||||
syscall.ETIMEDOUT,
|
||||
windows.WSAECONNREFUSED,
|
||||
windows.WSAENETUNREACH,
|
||||
windows.WSAEHOSTUNREACH,
|
||||
// Same reasoning as the POSIX ETIMEDOUT entry, with the Winsock code a real
|
||||
// Windows connect timeout actually carries.
|
||||
windows.WSAETIMEDOUT,
|
||||
}
|
||||
|
||||
// isConnResetErrno reports whether err carries a connection-reset errno, the
|
||||
// post-send exclusion in isPreSendTransportError. It matches the same
|
||||
// syscall.ECONNRESET the shared path used before the errno classification was
|
||||
// split out per platform, so behavior on Windows is unchanged; it lives here
|
||||
// so retry.go references no syscall constants and stays buildable on Plan 9.
|
||||
func isConnResetErrno(err error) bool {
|
||||
return errors.Is(err, syscall.ECONNRESET)
|
||||
}
|
||||
@@ -1,34 +1,15 @@
|
||||
//go:build !plan9
|
||||
|
||||
package providerio
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"io"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"sync/atomic"
|
||||
"syscall"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// wrapDialErrno builds the error shape a dial failure has (a *net.OpError
|
||||
// wrapping an *os.SyscallError wrapping the errno) so errors.Is reaches the
|
||||
// errno exactly as in production. These fixtures use the POSIX syscall.Exxx
|
||||
// constants, which do NOT reproduce a real Windows dial: that carries distinct
|
||||
// windows.WSA* errnos (Go's syscall.ECONNREFUSED is a value the net package
|
||||
// never produces on Windows). They still classify on Windows only because
|
||||
// dialPreSendErrnos there also lists the POSIX constants for exactly this
|
||||
// fixture reason; the real Windows dial branch is covered separately by
|
||||
// TestIsPreSendTransportErrorRealRefusedDial, which makes an actual refused dial.
|
||||
func wrapDialErrno(op string, errno syscall.Errno) error {
|
||||
return &net.OpError{Op: op, Net: "tcp", Err: os.NewSyscallError("connectex", errno)}
|
||||
}
|
||||
|
||||
type roundTripperFunc func(*http.Request) (*http.Response, error)
|
||||
|
||||
func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
|
||||
@@ -54,152 +35,6 @@ func TestSendWithRetryDoesNotReplayTransportErrors(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// A PROVABLY pre-send transport failure (no request bytes left this host) is
|
||||
// safe to replay and must be retried, bounded by preSendMaxAttempts (its own
|
||||
// short schedule), unlike an ambiguous post-send failure. Real dial failures
|
||||
// arrive as an Op=="dial" *net.OpError, so the errno cases are the production
|
||||
// shape on every platform (Windows included); the string case exercises the
|
||||
// wording fallback for a dial error already flattened past its errno.
|
||||
func TestSendWithRetryReplaysProvablyPreSendErrors(t *testing.T) {
|
||||
shrinkBackoff(t)
|
||||
cases := map[string]error{
|
||||
"errno refused (dial)": wrapDialErrno("dial", syscall.ECONNREFUSED),
|
||||
"errno network unreachable": wrapDialErrno("dial", syscall.ENETUNREACH),
|
||||
"errno host unreachable": wrapDialErrno("dial", syscall.EHOSTUNREACH),
|
||||
"dial string fallback": &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connect: connection refused")},
|
||||
"tls handshake timeout": errors.New("net/http: TLS handshake timeout"),
|
||||
"dns timeout": &net.DNSError{Err: "server misbehaving", Name: "nope.invalid", IsTimeout: true},
|
||||
}
|
||||
for name, transportErr := range cases {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) {
|
||||
atomic.AddInt32(&calls, 1)
|
||||
return nil, transportErr
|
||||
})}
|
||||
// maxAttempts=6 (the default) proves the pre-send path is bounded by
|
||||
// preSendMaxAttempts, not the caller's 429-tuned maxAttempts.
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "http://example.invalid", []byte("{}"), nil, 6)
|
||||
if resp != nil {
|
||||
_ = resp.Body.Close()
|
||||
}
|
||||
if err == nil {
|
||||
t.Fatal("expected the transport error to surface after retries are exhausted")
|
||||
}
|
||||
if got := atomic.LoadInt32(&calls); got != preSendMaxAttempts {
|
||||
t.Fatalf("pre-send error tried %d times, want %d (preSendMaxAttempts)", got, preSendMaxAttempts)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// An ambiguous transport failure that could have followed a sent request must
|
||||
// NOT be replayed: a non-idempotent completion POST could duplicate billable
|
||||
// work. This is the safety line the fix must not cross.
|
||||
func TestSendWithRetryDoesNotReplayAmbiguousTransportErrors(t *testing.T) {
|
||||
for name, transportErr := range map[string]error{
|
||||
"generic i/o timeout": errors.New("dial tcp 1.2.3.4:443: i/o timeout"),
|
||||
"broken pipe": errors.New("write tcp: broken pipe"),
|
||||
"unexpected eof": io.ErrUnexpectedEOF,
|
||||
// The pre-send errnos are also raised on an ESTABLISHED connection: a route
|
||||
// dropping mid-generation surfaces on the pending read/write, which is
|
||||
// post-send. Scoping to Op=="dial" must keep these from replaying the POST.
|
||||
"host unreachable on read is post-send": wrapDialErrno("read", syscall.EHOSTUNREACH),
|
||||
"net unreachable on write is post-send": wrapDialErrno("write", syscall.ENETUNREACH),
|
||||
// NXDOMAIN is authoritative and deterministic, so retrying it would only
|
||||
// stall the agent before failing anyway.
|
||||
"dns nxdomain": &net.DNSError{Err: "no such host", Name: "nope.invalid", IsNotFound: true},
|
||||
} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) {
|
||||
atomic.AddInt32(&calls, 1)
|
||||
return nil, transportErr
|
||||
})}
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "http://example.invalid", []byte("{}"), nil, 3)
|
||||
if resp != nil {
|
||||
_ = resp.Body.Close()
|
||||
}
|
||||
if err == nil {
|
||||
t.Fatal("expected the transport error to surface")
|
||||
}
|
||||
if got := atomic.LoadInt32(&calls); got != 1 {
|
||||
t.Fatalf("ambiguous transport error replayed %d times, want 1 (no retry)", got)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A REAL refused dial (nothing listening on the target) must classify as
|
||||
// pre-send on every platform. This is the regression the errno-constant fixtures
|
||||
// miss: on Windows the kernel raises WSAECONNREFUSED, which errors.Is does NOT
|
||||
// match against syscall.ECONNREFUSED, so before dialPreSendErrnos carried the WSA
|
||||
// codes a refused dial returned false here and Windows never retried it. Using a
|
||||
// real Dial exercises the platform's true error shape.
|
||||
func TestIsPreSendTransportErrorRealRefusedDial(t *testing.T) {
|
||||
// Reserve an ephemeral port, then close it, so a dial there is refused.
|
||||
ln, err := net.Listen("tcp", "127.0.0.1:0")
|
||||
if err != nil {
|
||||
t.Fatalf("listen: %v", err)
|
||||
}
|
||||
addr := ln.Addr().String()
|
||||
if cerr := ln.Close(); cerr != nil {
|
||||
t.Fatalf("close listener: %v", cerr)
|
||||
}
|
||||
conn, dialErr := (&net.Dialer{Timeout: 2 * time.Second}).Dial("tcp", addr)
|
||||
if dialErr == nil {
|
||||
_ = conn.Close()
|
||||
t.Skip("expected a refused dial but the port accepted a connection")
|
||||
}
|
||||
if !isPreSendTransportError(dialErr) {
|
||||
t.Fatalf("real refused dial not classified pre-send: %v", dialErr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsPreSendTransportError(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
err error
|
||||
want bool
|
||||
}{
|
||||
{"nil", nil, false},
|
||||
// DNS: a lookup that could recover is pre-send; NXDOMAIN is permanent.
|
||||
{"dns timeout", &net.DNSError{Err: "server misbehaving", Name: "x.invalid", IsTimeout: true}, true},
|
||||
{"dns temporary", &net.DNSError{Err: "server misbehaving", Name: "x.invalid", IsTemporary: true}, true},
|
||||
{"dns nxdomain is permanent", &net.DNSError{Err: "no such host", Name: "x.invalid", IsNotFound: true}, false},
|
||||
{"tls handshake timeout", errors.New("net/http: TLS handshake timeout"), true},
|
||||
// Errno-wrapped DIAL failures: how a real refused/unreachable dial arrives
|
||||
// on EVERY platform, including Windows where the wording differs entirely.
|
||||
{"errno refused (dial)", wrapDialErrno("dial", syscall.ECONNREFUSED), true},
|
||||
{"errno network unreachable (dial)", wrapDialErrno("dial", syscall.ENETUNREACH), true},
|
||||
{"errno host unreachable (dial)", wrapDialErrno("dial", syscall.EHOSTUNREACH), true},
|
||||
{"dial operror string fallback", &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connect: connection refused")}, true},
|
||||
// The SAME errnos raised on a post-send read/write are NOT pre-send — the
|
||||
// kernel reports them on an established connection when a route drops.
|
||||
{"host unreachable on read is post-send", wrapDialErrno("read", syscall.EHOSTUNREACH), false},
|
||||
{"net unreachable on write is post-send", wrapDialErrno("write", syscall.ENETUNREACH), false},
|
||||
{"errno reset is post-send", wrapDialErrno("read", syscall.ECONNRESET), false},
|
||||
// A refused/unreachable error already flattened past its *net.OpError can't
|
||||
// be proven pre-send, so it is NOT retried (conservative direction).
|
||||
{"flattened refused, no opError", errors.New("dial tcp 127.0.0.1:1: connect: connection refused"), false},
|
||||
{"connection reset", errors.New("read tcp: connection reset by peer"), false},
|
||||
{"broken pipe", errors.New("write tcp: broken pipe"), false},
|
||||
{"unexpected eof", io.ErrUnexpectedEOF, false},
|
||||
{"eof", io.EOF, false},
|
||||
{"generic io timeout", errors.New("dial tcp: i/o timeout"), false},
|
||||
{"context deadline", context.DeadlineExceeded, false},
|
||||
// Exclusion is checked before inclusion, even for a dial OpError: an "i/o
|
||||
// timeout" in the message wins over the refused wording.
|
||||
{"exclusion wins over inclusion", &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connection refused / i/o timeout")}, false},
|
||||
{"unrelated", errors.New("some other error"), false},
|
||||
}
|
||||
for _, c := range cases {
|
||||
if got := isPreSendTransportError(c.err); got != c.want {
|
||||
t.Errorf("%s: isPreSendTransportError(%v) = %v, want %v", c.name, c.err, got, c.want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestShouldRetryStatus(t *testing.T) {
|
||||
cases := map[int]bool{
|
||||
http.StatusOK: false,
|
||||
@@ -259,17 +94,12 @@ func TestBackoffWaitsThenReturnsTrue(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// shrinkBackoff makes both retry schedules (429 and pre-send) negligible for the
|
||||
// duration of a test.
|
||||
// shrinkBackoff makes retry waits negligible for the duration of a test.
|
||||
func shrinkBackoff(t *testing.T) {
|
||||
t.Helper()
|
||||
savedRetry, savedPreSend := retryBackoffBase, preSendBackoffBase
|
||||
saved := retryBackoffBase
|
||||
retryBackoffBase = time.Millisecond
|
||||
preSendBackoffBase = time.Millisecond
|
||||
t.Cleanup(func() {
|
||||
retryBackoffBase = savedRetry
|
||||
preSendBackoffBase = savedPreSend
|
||||
})
|
||||
t.Cleanup(func() { retryBackoffBase = saved })
|
||||
}
|
||||
|
||||
func TestBackoffWaitSchedule(t *testing.T) {
|
||||
@@ -296,30 +126,6 @@ func TestBackoffWaitSchedule(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// The pre-send schedule is sub-second and doubles, far shorter than the 429
|
||||
// schedule: a permanent dial failure fails in ~1.5s across preSendMaxAttempts
|
||||
// (500ms + 1s) instead of stalling the agent ~60s on 2/4/8/16/30s. This is the
|
||||
// second half of the fix for a mistyped host / dead local daemon hanging a turn.
|
||||
func TestPreSendBackoffWaitSchedule(t *testing.T) {
|
||||
cases := []struct {
|
||||
attempt int
|
||||
want time.Duration
|
||||
}{
|
||||
{1, 500 * time.Millisecond},
|
||||
{2, 1 * time.Second},
|
||||
{3, 2 * time.Second},
|
||||
// The exponent clamps at 5 (500ms<<5 = 16s) so a large attempt count can't
|
||||
// overflow; in practice preSendMaxAttempts caps retries at attempt 2, so
|
||||
// only the 500ms/1s rungs are ever reached.
|
||||
{50, 16 * time.Second},
|
||||
}
|
||||
for _, c := range cases {
|
||||
if got := preSendBackoffWait(c.attempt); got != c.want {
|
||||
t.Errorf("preSendBackoffWait(%d) = %v, want %v", c.attempt, got, c.want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestSendWithRetryRetriesThenSucceeds(t *testing.T) {
|
||||
shrinkBackoff(t)
|
||||
var hits int32
|
||||
@@ -387,316 +193,3 @@ func TestSendWithRetryReturnsLastResponseAfterMaxAttempts(t *testing.T) {
|
||||
t.Fatalf("server hit %d times, want 2 (maxAttempts)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Redirects are still followed, so a gateway that answers the completion endpoint
|
||||
// with a 307/308 keeps working: the caller gets the final response, not the 3xx.
|
||||
func TestSendWithRetryFollowsRedirects(t *testing.T) {
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
if atomic.AddInt32(&calls, 1) == 1 {
|
||||
return &http.Response{
|
||||
StatusCode: http.StatusTemporaryRedirect,
|
||||
Header: http.Header{"Location": {"https://redirect-target.invalid/v1"}},
|
||||
Body: http.NoBody,
|
||||
Request: r,
|
||||
}, nil
|
||||
}
|
||||
return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: r}, nil
|
||||
})}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://origin.invalid/v1", []byte("{}"), nil, 3)
|
||||
if err != nil {
|
||||
t.Fatalf("a redirected completion must succeed: %v", err)
|
||||
}
|
||||
_ = resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200 (the redirect should have been followed)", resp.StatusCode)
|
||||
}
|
||||
if got := atomic.LoadInt32(&calls); got != 2 {
|
||||
t.Fatalf("transport called %d times, want 2 (origin then redirect target)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Once an attempt has entered a redirect, the original POST has already left this
|
||||
// host, so a dial failure on the redirect hop must NOT be replayed even though it
|
||||
// arrives as an Op=="dial" error that isPreSendTransportError would otherwise
|
||||
// treat as provably pre-send. Replaying it would re-bill a completion the first
|
||||
// host may already have processed.
|
||||
func TestSendWithRetryDoesNotReplayAfterRedirect(t *testing.T) {
|
||||
shrinkBackoff(t)
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
if atomic.AddInt32(&calls, 1) == 1 {
|
||||
return &http.Response{
|
||||
StatusCode: http.StatusTemporaryRedirect,
|
||||
Header: http.Header{"Location": {"https://redirect-target.invalid/v1"}},
|
||||
Body: http.NoBody,
|
||||
Request: r,
|
||||
}, nil
|
||||
}
|
||||
// The dial to the redirect target fails: pre-send in shape, but post-send
|
||||
// in fact, because the origin already received the POST.
|
||||
return nil, wrapDialErrno("dial", syscall.ECONNREFUSED)
|
||||
})}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://origin.invalid/v1", []byte("{}"), nil, 6)
|
||||
if resp != nil {
|
||||
_ = resp.Body.Close()
|
||||
}
|
||||
if err == nil {
|
||||
t.Fatal("expected the redirect-hop dial failure to surface as an error")
|
||||
}
|
||||
// Exactly two: the origin request and the single redirect hop. A third would
|
||||
// mean the redirected POST was replayed.
|
||||
if got := atomic.LoadInt32(&calls); got != 2 {
|
||||
t.Fatalf("redirected POST was replayed: transport called %d times, want 2", got)
|
||||
}
|
||||
}
|
||||
|
||||
// A caller that supplies its own CheckRedirect keeps it: the retry path observes
|
||||
// redirects, it does not take the decision away from the caller.
|
||||
func TestSendWithRetryPreservesCallerRedirectPolicy(t *testing.T) {
|
||||
var policyCalls int32
|
||||
client := &http.Client{
|
||||
CheckRedirect: func(*http.Request, []*http.Request) error {
|
||||
atomic.AddInt32(&policyCalls, 1)
|
||||
return http.ErrUseLastResponse
|
||||
},
|
||||
Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
return &http.Response{
|
||||
StatusCode: http.StatusTemporaryRedirect,
|
||||
Header: http.Header{"Location": {"https://redirect-target.invalid/v1"}},
|
||||
Body: http.NoBody,
|
||||
Request: r,
|
||||
}, nil
|
||||
}),
|
||||
}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://origin.invalid/v1", []byte("{}"), nil, 3)
|
||||
if err != nil {
|
||||
t.Fatalf("caller policy asked to stop at the 3xx, so it must surface: %v", err)
|
||||
}
|
||||
_ = resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusTemporaryRedirect {
|
||||
t.Fatalf("status = %d, want the 307 surfaced per the caller's policy", resp.StatusCode)
|
||||
}
|
||||
if got := atomic.LoadInt32(&policyCalls); got != 1 {
|
||||
t.Fatalf("caller CheckRedirect called %d times, want 1 (it must not be discarded)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The pre-send retry budget is independent of the 429/503 status retries: two
|
||||
// rate-limit responses must not consume the pre-send allowance, so the first
|
||||
// refused dial after them still gets its own preSendMaxAttempts tries.
|
||||
func TestSendWithRetryPreSendBudgetSurvivesStatusRetries(t *testing.T) {
|
||||
shrinkBackoff(t)
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
if atomic.AddInt32(&calls, 1) <= 2 {
|
||||
return &http.Response{StatusCode: http.StatusTooManyRequests, Body: http.NoBody, Request: r}, nil
|
||||
}
|
||||
return nil, wrapDialErrno("dial", syscall.ECONNREFUSED)
|
||||
})}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://x.invalid/v1", []byte("{}"), nil, 6)
|
||||
if resp != nil {
|
||||
_ = resp.Body.Close()
|
||||
}
|
||||
if err == nil {
|
||||
t.Fatal("expected the exhausted pre-send failure to surface as an error")
|
||||
}
|
||||
// Two status responses (calls 1-2), then the pre-send failure gets its own
|
||||
// budget: preSendMaxAttempts=3 means it retries twice more before returning.
|
||||
if got := atomic.LoadInt32(&calls); got != 2+preSendMaxAttempts {
|
||||
t.Fatalf("pre-send budget eaten by status retries: transport called %d times, want %d (2 status + %d pre-send)", got, 2+preSendMaxAttempts, preSendMaxAttempts)
|
||||
}
|
||||
}
|
||||
|
||||
// The inverse sequence: pre-send retries must not consume the 429/503 budget or
|
||||
// advance its backoff rung either. Two safely replayed refused dials followed by
|
||||
// rate limiting must still leave the status path its full maxAttempts window,
|
||||
// otherwise a couple of dial blips silently shorten every later rate-limit wait.
|
||||
func TestSendWithRetryStatusBudgetSurvivesPreSendRetries(t *testing.T) {
|
||||
shrinkBackoff(t)
|
||||
const maxAttempts = 3
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
if atomic.AddInt32(&calls, 1) <= 2 {
|
||||
return nil, wrapDialErrno("dial", syscall.ECONNREFUSED)
|
||||
}
|
||||
return &http.Response{StatusCode: http.StatusTooManyRequests, Body: http.NoBody, Request: r}, nil
|
||||
})}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://x.invalid/v1", []byte("{}"), nil, maxAttempts)
|
||||
if err != nil {
|
||||
t.Fatalf("exhausted status retries surface the response, not an error: %v", err)
|
||||
}
|
||||
_ = resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusTooManyRequests {
|
||||
t.Fatalf("status = %d, want 429", resp.StatusCode)
|
||||
}
|
||||
// Calls 1-2 are the retried dial failures; the status path then gets its own
|
||||
// full window (maxAttempts requests) before the 429 is surfaced.
|
||||
if got := atomic.LoadInt32(&calls); got != 2+maxAttempts {
|
||||
t.Fatalf("status budget eaten by pre-send retries: transport called %d times, want %d (2 pre-send + %d status)", got, 2+maxAttempts, maxAttempts)
|
||||
}
|
||||
}
|
||||
|
||||
// A redirect followed by a RETRYABLE STATUS is still retried, and deliberately
|
||||
// so. This is the case a reviewer flagged as a possible replay of billable work,
|
||||
// and the asymmetry with the transport-error guard above is intentional rather
|
||||
// than an oversight, so it is pinned here.
|
||||
//
|
||||
// The two situations differ in what is known about the request:
|
||||
//
|
||||
// - Transport error after a redirect: the original POST reached the origin and
|
||||
// the dial to the redirect target then failed, so whether the target received
|
||||
// and began processing it is UNKNOWN. Replaying could duplicate work, which
|
||||
// is why that path refuses to retry.
|
||||
// - Retryable status after a redirect: every hop answered. A 307/308 is the
|
||||
// origin declining to process and delegating, and a 429/503/529 is the target
|
||||
// declining as well. Nothing accepted the request, so replaying duplicates
|
||||
// nothing.
|
||||
//
|
||||
// The risk here is also exactly the risk of a 429 without any redirect, which
|
||||
// this package has always retried; the redirect adds no new ambiguity. Refusing
|
||||
// to retry would instead make a rate-limited completion behind a redirecting
|
||||
// gateway fail outright rather than back off.
|
||||
func TestSendWithRetryRetriesRedirectedRetryableStatus(t *testing.T) {
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
switch atomic.AddInt32(&calls, 1) {
|
||||
case 1, 3:
|
||||
// The origin declines and delegates, both times.
|
||||
return &http.Response{
|
||||
StatusCode: http.StatusTemporaryRedirect,
|
||||
Header: http.Header{"Location": {"https://redirect-target.invalid/v1"}},
|
||||
Body: http.NoBody,
|
||||
Request: r,
|
||||
}, nil
|
||||
case 2:
|
||||
// The target declines too: rate limited, so nothing was accepted.
|
||||
return &http.Response{StatusCode: http.StatusTooManyRequests, Body: http.NoBody, Request: r}, nil
|
||||
default:
|
||||
return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: r}, nil
|
||||
}
|
||||
})}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://origin.invalid/v1", []byte("{}"), nil, 3)
|
||||
if err != nil {
|
||||
t.Fatalf("a rate-limited completion behind a redirect must still retry: %v", err)
|
||||
}
|
||||
_ = resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200 after the retry succeeded", resp.StatusCode)
|
||||
}
|
||||
// origin, target(429), origin, target(200).
|
||||
if got := atomic.LoadInt32(&calls); got != 4 {
|
||||
t.Fatalf("transport called %d times, want 4 (redirect, 429, redirect, 200)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// A CONNECT-phase timeout is provably pre-send: the connection was never
|
||||
// established, so no request bytes left this host. It reaches the classifier
|
||||
// carrying "i/o timeout", which the blanket wording exclusion rejects, so
|
||||
// admitting it depends on the connect gate running first.
|
||||
//
|
||||
// The post-send counterpart is the point of the table: a read timeout is also
|
||||
// Timeout(), and must stay excluded, or a reply that timed out mid-generation
|
||||
// would replay a completion the server already billed.
|
||||
func TestPreSendClassifiesConnectTimeouts(t *testing.T) {
|
||||
timeoutErr := &timeoutError{}
|
||||
for name, testCase := range map[string]struct {
|
||||
err error
|
||||
want bool
|
||||
}{
|
||||
"dial timeout": {
|
||||
err: &net.OpError{Op: "dial", Net: "tcp", Err: timeoutErr},
|
||||
want: true,
|
||||
},
|
||||
"proxy connect timeout": {
|
||||
err: &net.OpError{Op: "proxyconnect", Net: "tcp", Err: timeoutErr},
|
||||
want: true,
|
||||
},
|
||||
"read timeout stays post-send": {
|
||||
err: &net.OpError{Op: "read", Net: "tcp", Err: timeoutErr},
|
||||
want: false,
|
||||
},
|
||||
"write timeout stays post-send": {
|
||||
err: &net.OpError{Op: "write", Net: "tcp", Err: timeoutErr},
|
||||
want: false,
|
||||
},
|
||||
} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
if got := isPreSendTransportError(testCase.err); got != testCase.want {
|
||||
t.Fatalf("isPreSendTransportError(%v) = %v, want %v", testCase.err, got, testCase.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A failed CONNECT through an HTTP proxy is pre-send for the same structural
|
||||
// reason a refused dial is, but net tags it Op "proxyconnect", so a gate
|
||||
// matching only "dial" left every pre-send failure behind a corporate proxy
|
||||
// unretried.
|
||||
func TestPreSendClassifiesProxyConnectRefusal(t *testing.T) {
|
||||
err := &net.OpError{Op: "proxyconnect", Net: "tcp", Err: syscall.ECONNREFUSED}
|
||||
if !isPreSendTransportError(err) {
|
||||
t.Fatal("a refused proxy CONNECT was not classified pre-send; no request bytes reached the model host")
|
||||
}
|
||||
// The op still has to be a connect phase. The same errno on an established
|
||||
// connection is post-send and must not replay a POST.
|
||||
post := &net.OpError{Op: "read", Net: "tcp", Err: syscall.ECONNREFUSED}
|
||||
if isPreSendTransportError(post) {
|
||||
t.Fatal("a read-phase error was classified pre-send")
|
||||
}
|
||||
}
|
||||
|
||||
// timeoutError is a net.Error whose Timeout() is true, matching the shape
|
||||
// net/http surfaces for a connect deadline.
|
||||
type timeoutError struct{}
|
||||
|
||||
func (timeoutError) Error() string { return "i/o timeout" }
|
||||
func (timeoutError) Timeout() bool { return true }
|
||||
func (timeoutError) Temporary() bool { return true }
|
||||
|
||||
// The pre-send budget bounds CONSECUTIVE connect failures, not the lifetime of
|
||||
// the call. Without a reset on a successful hop, an early run of dial blips
|
||||
// permanently spends the budget, so a later blip surfaces as a transport error
|
||||
// while the status budget still has room. That is the coupling the two counters
|
||||
// were split apart to remove, in the one ordering the earlier tests did not
|
||||
// cover: pre-send, then status, then pre-send again.
|
||||
func TestPreSendBudgetResetsAfterSuccessfulHop(t *testing.T) {
|
||||
refused := &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}
|
||||
var calls int32
|
||||
client := &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
|
||||
switch atomic.AddInt32(&calls, 1) {
|
||||
case 1, 2:
|
||||
// Two connect failures, spending the pre-send budget under the old
|
||||
// lifetime accounting.
|
||||
return nil, refused
|
||||
case 3:
|
||||
// A hop that reaches the server. It declines, so the request was not
|
||||
// accepted and retrying stays safe.
|
||||
return &http.Response{StatusCode: http.StatusTooManyRequests, Body: http.NoBody, Request: r}, nil
|
||||
case 4:
|
||||
// One more connect blip. With a lifetime cap this surfaces as an
|
||||
// error; the call should still recover.
|
||||
return nil, refused
|
||||
default:
|
||||
return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: r}, nil
|
||||
}
|
||||
})}
|
||||
|
||||
resp, err := SendWithRetry(context.Background(), client, http.MethodPost, "https://origin.invalid/v1", []byte("{}"), nil, 3)
|
||||
if err != nil {
|
||||
t.Fatalf("interleaved connect and status failures returned %v; the pre-send budget did not reset after the hop that reached the server", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200", resp.StatusCode)
|
||||
}
|
||||
if got := atomic.LoadInt32(&calls); got != 5 {
|
||||
t.Fatalf("transport called %d times, want 5 (dial, dial, 429, dial, 200)", got)
|
||||
}
|
||||
}
|
||||
@@ -212,14 +212,6 @@ func credentialDenyReadPathsForEnvironment(env credentialPathEnvironment, allowR
|
||||
candidates = append(candidates,
|
||||
filepath.Join(home, ".aws"),
|
||||
filepath.Join(home, ".azure"),
|
||||
// git's credential store backend, which holds host passwords and
|
||||
// personal access tokens in cleartext. Denied rather than the whole
|
||||
// of ~/.ssh, because these cost nothing functionally: git reads them
|
||||
// through a credential helper for authentication, not for identity,
|
||||
// so a sandboxed git still works and simply cannot authenticate as
|
||||
// the user. SSH key material is a harder trade and is tracked
|
||||
// separately (#815).
|
||||
filepath.Join(home, ".git-credentials"),
|
||||
)
|
||||
}
|
||||
npmUserConfig := strings.TrimSpace(env.NPMUserConfig)
|
||||
@@ -241,12 +233,6 @@ func credentialDenyReadPathsForEnvironment(env credentialPathEnvironment, allowR
|
||||
if configHome != "" {
|
||||
candidates = append(candidates,
|
||||
filepath.Join(configHome, "zero"),
|
||||
// The XDG location of the same git credential store. Denying the file
|
||||
// rather than the directory is deliberate: ~/.config/git also holds
|
||||
// the global git config, which userGitConfigReadPaths grants on
|
||||
// purpose so a sandboxed git can read user.name and aliases instead
|
||||
// of failing outright.
|
||||
filepath.Join(configHome, "git", "credentials"),
|
||||
)
|
||||
}
|
||||
cloudSDKConfig := strings.TrimSpace(env.CloudSDKConfig)
|
||||
|
||||
@@ -1,93 +0,0 @@
|
||||
package sandbox
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// git's credential store holds host passwords and personal access tokens in
|
||||
// cleartext, in one of two locations depending on whether the user is on the
|
||||
// XDG layout. Neither was denied, so a sandboxed command could read them
|
||||
// (#815).
|
||||
//
|
||||
// Scoped to the credential files on purpose. Denying ~/.ssh as well would stop
|
||||
// a sandboxed git push over SSH from working, which is a functional trade that
|
||||
// issue tracks separately; these two cost nothing, because git reads them for
|
||||
// authentication rather than identity.
|
||||
func TestCredentialDenyReadPathsCoversGitCredentialStores(t *testing.T) {
|
||||
home := t.TempDir()
|
||||
configHome := filepath.Join(home, ".config")
|
||||
if err := os.MkdirAll(filepath.Join(configHome, "git"), 0o700); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// The builder only emits paths that exist, so these have to be real.
|
||||
gitCredentials := filepath.Join(home, ".git-credentials")
|
||||
xdgCredentials := filepath.Join(configHome, "git", "credentials")
|
||||
gitConfig := filepath.Join(configHome, "git", "config")
|
||||
for _, path := range []string{gitCredentials, xdgCredentials, gitConfig} {
|
||||
if err := os.WriteFile(path, []byte("x"), 0o600); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
|
||||
denied := credentialDenyReadPathsForEnvironment(credentialPathEnvironment{
|
||||
Home: home,
|
||||
ConfigHome: configHome,
|
||||
}, nil)
|
||||
covered := func(target string) bool {
|
||||
norm := normalizeProfilePath(target)
|
||||
for _, entry := range denied {
|
||||
if entry == norm || pathWithinRoot(entry, norm) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
if !covered(gitCredentials) {
|
||||
t.Fatalf("~/.git-credentials is readable; deny list = %v", denied)
|
||||
}
|
||||
if !covered(xdgCredentials) {
|
||||
t.Fatalf("~/.config/git/credentials is readable; deny list = %v", denied)
|
||||
}
|
||||
// The global git config must stay readable. userGitConfigReadPaths grants it
|
||||
// deliberately so a sandboxed git can read user.name and aliases rather than
|
||||
// failing with "unable to access", so denying the directory instead of the
|
||||
// credential file would break that on purpose-built setups.
|
||||
if covered(gitConfig) {
|
||||
t.Fatalf("~/.config/git/config was denied; a sandboxed git loses its identity config")
|
||||
}
|
||||
}
|
||||
|
||||
// An explicit read grant still wins, matching how every other entry behaves: a
|
||||
// user who deliberately scopes a workspace over one of these paths is not
|
||||
// overridden by the default deny.
|
||||
//
|
||||
// Asserted from both sides on purpose. Checking only that the path is absent
|
||||
// under an allowRead is satisfied just as well by a deny rule that was never
|
||||
// added, so the run without allowRead has to establish that there is something
|
||||
// there for the grant to override.
|
||||
func TestGitCredentialDenyYieldsToExplicitAllowRead(t *testing.T) {
|
||||
home := t.TempDir()
|
||||
gitCredentials := filepath.Join(home, ".git-credentials")
|
||||
if err := os.WriteFile(gitCredentials, []byte("x"), 0o600); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
target := normalizeProfilePath(gitCredentials)
|
||||
listed := func(entries []string) bool {
|
||||
for _, entry := range entries {
|
||||
if entry == target {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
if !listed(credentialDenyReadPathsForEnvironment(credentialPathEnvironment{Home: home}, nil)) {
|
||||
t.Fatalf("~/.git-credentials is not denied without an allowRead; nothing for the grant to override")
|
||||
}
|
||||
if listed(credentialDenyReadPathsForEnvironment(credentialPathEnvironment{Home: home}, []string{home})) {
|
||||
t.Fatalf("explicit allowRead %q did not re-include the credential store", home)
|
||||
}
|
||||
}
|
||||
@@ -1027,13 +1027,6 @@ func scrubSensitiveEnv(env []string, additionalKeys ...string) []string {
|
||||
"GH_TOKEN",
|
||||
"ZERO_WEBSEARCH_API_KEY",
|
||||
"ZERO_DAEMON_REMOTE_TOKEN",
|
||||
// The file form of the same bridge token. TokenFromEnv accepts either,
|
||||
// so scrubbing only the inline variable left the pointer readable, and
|
||||
// the default sandbox posture is read-all. There is no fallback
|
||||
// location to guess at, the path comes from this variable alone, so
|
||||
// removing it closes the leak rather than half of it. Same reasoning as
|
||||
// GOOGLE_APPLICATION_CREDENTIALS below.
|
||||
"ZERO_DAEMON_REMOTE_TOKEN_FILE",
|
||||
}
|
||||
for _, descriptor := range providercatalog.All() {
|
||||
for _, key := range descriptor.AuthEnvVars {
|
||||
|
||||
@@ -699,11 +699,6 @@ func TestScrubSensitiveEnv(t *testing.T) {
|
||||
"XAI_API_KEY=xai-12345",
|
||||
"HUGGINGFACE_API_KEY=hf_12345",
|
||||
"GOOGLE_APPLICATION_CREDENTIALS=/home/user/sa-key.json",
|
||||
// Both forms of the daemon bridge token. The inline one was already
|
||||
// scrubbed; the file form was not, which left a sandboxed command the
|
||||
// path to read it from (#677).
|
||||
"ZERO_DAEMON_REMOTE_TOKEN=bridge-token-inline",
|
||||
"ZERO_DAEMON_REMOTE_TOKEN_FILE=/home/user/.zero/remote-token",
|
||||
"COMPANY_LLM_SECRET=custom-secret",
|
||||
"ZERO_OAUTH_MY_SVC_CLIENT_SECRET=oauth-secret",
|
||||
"zero_oauth_second_client_secret=case-insensitive-secret",
|
||||
|
||||
+27
-180
@@ -12,12 +12,6 @@ import (
|
||||
// team is at its slot cap the member is queued and launches when a slot frees
|
||||
// (it stays pending in the coordinator until then).
|
||||
func (s *Swarm) Spawn(pol Policy, teamName, agentType, task, cwd string) (string, error) {
|
||||
release, err := s.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
defer release()
|
||||
|
||||
def, err := s.registry.Lookup(agentType)
|
||||
if err != nil {
|
||||
return "", err
|
||||
@@ -29,44 +23,15 @@ func (s *Swarm) Spawn(pol Policy, teamName, agentType, task, cwd string) (string
|
||||
}
|
||||
s.rememberCwd(id, cwd)
|
||||
spec := s.buildSpec(pol, id, id, team, def, task, cwd)
|
||||
s.dispatchAdmitted(spec)
|
||||
s.dispatch(spec)
|
||||
return id, nil
|
||||
}
|
||||
|
||||
// beginLifecycleAdmission admits one unit of lifecycle work if shutdown has not
|
||||
// begun. On success it returns a release func the caller MUST call exactly once
|
||||
// (a deferred call is the norm) — Close blocks until every admitted unit has
|
||||
// released. On shutdown it returns a nil func and ErrSwarmClosed, and there is
|
||||
// nothing to release.
|
||||
//
|
||||
// It deliberately does NOT keep lifecycleMu held for the caller's duration, which
|
||||
// an earlier revision did. Close needs the write lock before it can cancel, so a
|
||||
// launcher that blocks in Launch until its context is cancelled would deadlock
|
||||
// against every Close caller: the launch holds the read lock while Close waits
|
||||
// for the write lock to issue the cancellation neither side can reach. Holding the
|
||||
// lock only across the flag check and the counter increment keeps the ordering
|
||||
// guarantee (no admission once closed is set) without letting external launcher
|
||||
// code sit inside the critical section.
|
||||
//
|
||||
// The read lock is enough to make the WaitGroup safe: Close flips closed under
|
||||
// the write lock, so an Add here either completes before Close acquires it (and is
|
||||
// therefore visible to the later Wait) or observes closed and is refused.
|
||||
func (s *Swarm) beginLifecycleAdmission() (func(), error) {
|
||||
s.lifecycleMu.RLock()
|
||||
defer s.lifecycleMu.RUnlock()
|
||||
if s.closed {
|
||||
return nil, ErrSwarmClosed
|
||||
}
|
||||
s.lifecycleWork.Add(1)
|
||||
return s.lifecycleWork.Done, nil
|
||||
}
|
||||
|
||||
// dispatch admits a spec to its team (launching now or queuing for a slot).
|
||||
// The caller must hold an admission ticket from beginLifecycleAdmission.
|
||||
func (s *Swarm) dispatchAdmitted(spec MemberSpec) {
|
||||
func (s *Swarm) dispatch(spec MemberSpec) {
|
||||
t := s.team(spec.Team)
|
||||
if t.admit(spec) {
|
||||
s.launchAdmitted(t, spec)
|
||||
s.launch(t, spec)
|
||||
}
|
||||
// Otherwise the spec is queued; the coordinator task stays pending until a
|
||||
// slot frees and afterExit launches it.
|
||||
@@ -74,73 +39,17 @@ func (s *Swarm) dispatchAdmitted(spec MemberSpec) {
|
||||
|
||||
// launch starts a member for spec and supervises it. A synchronous launch
|
||||
// failure fails the task and frees the slot.
|
||||
// The caller must hold an admission ticket from beginLifecycleAdmission.
|
||||
func (s *Swarm) launchAdmitted(t *Team, spec MemberSpec) {
|
||||
handle, err := s.launchMemberAdmitted(spec)
|
||||
func (s *Swarm) launch(t *Team, spec MemberSpec) {
|
||||
handle, err := s.launcher.Launch(s.baseCtx, spec)
|
||||
if err != nil {
|
||||
_ = s.coord.Fail(spec.TaskID, "launch: "+err.Error())
|
||||
if err == ErrSwarmClosed {
|
||||
t.releaseSlot()
|
||||
return
|
||||
}
|
||||
s.afterExitAdmitted(t)
|
||||
s.afterExit(t)
|
||||
return
|
||||
}
|
||||
|
||||
committed := s.commitLaunch(handle, func() {
|
||||
m := &Member{ID: spec.ID, AgentType: spec.AgentType, TaskID: spec.TaskID, handle: handle}
|
||||
t.addMember(m)
|
||||
_ = s.coord.SetStatus(spec.TaskID, StatusRunning)
|
||||
s.watchers.Add(1)
|
||||
go func() {
|
||||
defer s.watchers.Done()
|
||||
s.watch(t, m, spec)
|
||||
}()
|
||||
})
|
||||
if !committed {
|
||||
_ = s.coord.Fail(spec.TaskID, "launch: "+ErrSwarmClosed.Error())
|
||||
t.releaseSlot()
|
||||
}
|
||||
}
|
||||
|
||||
// commitLaunch closes the window between MemberLauncher.Launch returning and the
|
||||
// swarm taking ownership of the handle it produced. Launch is external code and
|
||||
// therefore runs without lifecycleMu held, so Close may have won after
|
||||
// launchMemberAdmitted's pre-check but before Launch returned. adopt runs under
|
||||
// the read lock so the shutdown check and everything the caller does to take
|
||||
// ownership (registration, status transition, watcher Add, handle swap) are
|
||||
// atomic with respect to Close setting closed and beginning its waits.
|
||||
//
|
||||
// It reports whether the handle was adopted. When shutdown won, the handle is
|
||||
// reaped before returning false: its launch context was already cancelled by
|
||||
// Close, and MemberHandle has no separate cancellation operation — Launch's
|
||||
// context is its cancellation contract — so waiting is how a
|
||||
// successfully-created process/goroutine avoids being abandoned. The caller
|
||||
// records the terminal outcome for the task it was launching.
|
||||
func (s *Swarm) commitLaunch(handle MemberHandle, adopt func()) bool {
|
||||
s.lifecycleMu.RLock()
|
||||
if s.closed {
|
||||
s.lifecycleMu.RUnlock()
|
||||
_, _ = handle.Wait()
|
||||
return false
|
||||
}
|
||||
defer s.lifecycleMu.RUnlock()
|
||||
adopt()
|
||||
return true
|
||||
}
|
||||
|
||||
// launchMemberAdmitted re-checks shutdown at the last internal boundary before
|
||||
// invoking the external launcher. A lifecycle ticket proves the operation won
|
||||
// admission before Close, but the caller may not reach its launch until after
|
||||
// Close has set closed; in that case no new member should be started.
|
||||
func (s *Swarm) launchMemberAdmitted(spec MemberSpec) (MemberHandle, error) {
|
||||
s.lifecycleMu.RLock()
|
||||
closed := s.closed
|
||||
s.lifecycleMu.RUnlock()
|
||||
if closed {
|
||||
return nil, ErrSwarmClosed
|
||||
}
|
||||
return s.launcher.Launch(s.baseCtx, spec)
|
||||
m := &Member{ID: spec.ID, AgentType: spec.AgentType, TaskID: spec.TaskID, handle: handle}
|
||||
t.addMember(m)
|
||||
_ = s.coord.SetStatus(spec.TaskID, StatusRunning)
|
||||
go s.watch(t, m, spec)
|
||||
}
|
||||
|
||||
// watch awaits a member, applies bounded relaunch on temporary failures, records
|
||||
@@ -151,98 +60,42 @@ func (s *Swarm) launchMemberAdmitted(spec MemberSpec) (MemberHandle, error) {
|
||||
// life (Member.ID == MemberSpec.ID); a retry never reuses the struct for a
|
||||
// different spec.
|
||||
func (s *Swarm) watch(t *Team, m *Member, spec MemberSpec) {
|
||||
for {
|
||||
res, err := m.handle.Wait()
|
||||
if err != nil {
|
||||
if isRetryable(err) && m.restarts < maxMemberRestarts && s.relaunchAdmitted(m, spec) {
|
||||
continue
|
||||
res, err := m.handle.Wait()
|
||||
if err != nil {
|
||||
if isRetryable(err) && m.restarts < maxMemberRestarts {
|
||||
if nh, relErr := s.launcher.Launch(s.baseCtx, spec); relErr == nil {
|
||||
m.restarts++
|
||||
m.handle = nh
|
||||
go s.watch(t, m, spec)
|
||||
return
|
||||
}
|
||||
// Fall through if shutdown started or the relaunch failed.
|
||||
// res.SessionID is preserved by the handle even on error, so a member that
|
||||
// ran then failed stays drillable; a pure launch error carries an empty id.
|
||||
_ = s.coord.FailWithSession(m.TaskID, memberError(err), res.SessionID)
|
||||
} else {
|
||||
_ = s.coord.CompleteWithSession(m.TaskID, res.Result, res.SessionID)
|
||||
// fall through to record the original error if relaunch fails
|
||||
}
|
||||
break
|
||||
// res.SessionID is preserved by the handle even on error, so a member that
|
||||
// ran then failed stays drillable; a pure launch error carries an empty id.
|
||||
_ = s.coord.FailWithSession(m.TaskID, memberError(err), res.SessionID)
|
||||
} else {
|
||||
_ = s.coord.CompleteWithSession(m.TaskID, res.Result, res.SessionID)
|
||||
}
|
||||
t.removeMember(m.ID)
|
||||
s.afterExit(t)
|
||||
}
|
||||
|
||||
// relaunchAdmitted performs one bounded relaunch of an existing member's spec,
|
||||
// reporting whether the member was relaunched and its new handle adopted. A
|
||||
// relaunch is lifecycle work like any other, so it runs the same three gates an
|
||||
// initial launch does: admission (refused outright once shutdown began), the
|
||||
// pre-Launch re-check, and the post-Launch commit — the last of which matters
|
||||
// because the production FuncLauncher returns a live handle immediately without
|
||||
// consulting its context, so a Close landing between the pre-check and the return
|
||||
// would otherwise resume supervising a member started after shutdown. When any
|
||||
// gate refuses, the caller records the member's terminal failure instead.
|
||||
func (s *Swarm) relaunchAdmitted(m *Member, spec MemberSpec) bool {
|
||||
release, err := s.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
defer release()
|
||||
handle, err := s.launchMemberAdmitted(spec)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
return s.commitLaunch(handle, func() {
|
||||
m.restarts++
|
||||
m.handle = handle
|
||||
})
|
||||
}
|
||||
|
||||
// afterExit releases the just-vacated slot and launches the next queued spec, if
|
||||
// any. Each exit drains at most one queued member; that member's own exit drains
|
||||
// the next, so the queue empties one-per-slot without unbounded recursion.
|
||||
func (s *Swarm) afterExit(t *Team) {
|
||||
release, err := s.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
t.releaseSlot()
|
||||
return
|
||||
}
|
||||
defer release()
|
||||
s.afterExitAdmitted(t)
|
||||
}
|
||||
|
||||
// afterExitAdmitted drains at most one queued spec while lifecycle admission is
|
||||
// held open. The caller must hold an admission ticket from beginLifecycleAdmission.
|
||||
func (s *Swarm) afterExitAdmitted(t *Team) {
|
||||
next, ok := t.onExit()
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
// This call's own ticket only proves shutdown hadn't begun when it was
|
||||
// granted; Close may have set closed in the meantime, and t.onExit above
|
||||
// already removed next from t.queue, so Close's clearQueue sweep can no
|
||||
// longer see it (nor will it wait for one, since this ticket is what Close's
|
||||
// lifecycleWork.Wait() is blocked on). Re-check before launching so a dequeue
|
||||
// that straddles the shutdown boundary fails the task in place of a launch a
|
||||
// shutting-down swarm never wanted started.
|
||||
s.lifecycleMu.RLock()
|
||||
closed := s.closed
|
||||
s.lifecycleMu.RUnlock()
|
||||
if closed {
|
||||
t.releaseSlot()
|
||||
_ = s.coord.Fail(next.TaskID, ErrSwarmClosed.Error())
|
||||
return
|
||||
}
|
||||
s.launchAdmitted(t, next)
|
||||
s.launch(t, next)
|
||||
}
|
||||
|
||||
// Handoff transfers a task to a fresh member of toAgentType, delivering a note to
|
||||
// the new member's inbox and marking the original task handed-off. It returns the
|
||||
// new task id. A handoff of an already-terminal task is rejected (fail closed).
|
||||
func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) (string, error) {
|
||||
release, err := s.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
defer release()
|
||||
|
||||
task, ok := s.coord.Get(taskID)
|
||||
if !ok {
|
||||
return "", fmt.Errorf("%w: %s", ErrUnknownTask, taskID)
|
||||
@@ -278,7 +131,7 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string)
|
||||
// Retire the original task (it has been re-delegated).
|
||||
_ = s.coord.SetStatus(taskID, StatusHandedOff)
|
||||
spec := s.buildSpec(pol, newID, newID, team, def, handoffTask, cwd)
|
||||
s.dispatchAdmitted(spec)
|
||||
s.dispatch(spec)
|
||||
return newID, nil
|
||||
}
|
||||
|
||||
@@ -286,12 +139,6 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string)
|
||||
// (e.g. a crashed worker) onto fresh members of toAgentType, returning the
|
||||
// adopted task ids. Terminal tasks and tasks with a live owner are left alone.
|
||||
func (s *Swarm) AdoptOrphans(pol Policy, teamName, toAgentType string) ([]string, error) {
|
||||
release, err := s.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer release()
|
||||
|
||||
def, err := s.registry.Lookup(toAgentType)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -314,7 +161,7 @@ func (s *Swarm) AdoptOrphans(pol Policy, teamName, toAgentType string) ([]string
|
||||
cwd := s.cwdFor(task.ID)
|
||||
s.rememberCwd(task.ID, cwd)
|
||||
spec := s.buildSpec(pol, newAgent, task.ID, team, def, task.Description, cwd)
|
||||
s.dispatchAdmitted(spec)
|
||||
s.dispatch(spec)
|
||||
adopted = append(adopted, task.ID)
|
||||
}
|
||||
return adopted, nil
|
||||
|
||||
@@ -2,7 +2,6 @@ package swarm
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
@@ -218,141 +217,6 @@ func TestPermanentErrorNoRetry(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestLifecycleAdmissionRejectedAfterClose(t *testing.T) {
|
||||
l := newLauncher(okFor)
|
||||
sw := newSwarmFor(t, l)
|
||||
sw.Close()
|
||||
|
||||
if _, err := sw.Spawn(Policy{}, "team", "teammate", "task", ""); !errors.Is(err, ErrSwarmClosed) {
|
||||
t.Fatalf("Spawn after Close error = %v, want ErrSwarmClosed", err)
|
||||
}
|
||||
if _, err := sw.Handoff(Policy{}, "team", "task", "teammate", "note"); !errors.Is(err, ErrSwarmClosed) {
|
||||
t.Fatalf("Handoff after Close error = %v, want ErrSwarmClosed", err)
|
||||
}
|
||||
if _, err := sw.AdoptOrphans(Policy{}, "team", "teammate"); !errors.Is(err, ErrSwarmClosed) {
|
||||
t.Fatalf("AdoptOrphans after Close error = %v, want ErrSwarmClosed", err)
|
||||
}
|
||||
if got := len(l.recorded()); got != 0 {
|
||||
t.Fatalf("launches after Close = %d, want 0", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCloseDoesNotLaunchQueuedMembers(t *testing.T) {
|
||||
gate := make(chan struct{})
|
||||
l := newLauncher(okFor)
|
||||
l.gate = gate
|
||||
sw := newSwarmFor(t, l)
|
||||
|
||||
var ids []string
|
||||
for i := 0; i < 3; i++ {
|
||||
id, err := sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
if err != nil {
|
||||
t.Fatalf("Spawn %d: %v", i, err)
|
||||
}
|
||||
ids = append(ids, id)
|
||||
}
|
||||
if got := len(l.recorded()); got != 2 {
|
||||
t.Fatalf("initial launches = %d, want 2 with one queued", got)
|
||||
}
|
||||
|
||||
sw.Close()
|
||||
if got := len(l.recorded()); got != 2 {
|
||||
t.Fatalf("launches after Close = %d, want queued member not launched", got)
|
||||
}
|
||||
for _, id := range ids {
|
||||
task, ok := sw.Coordinator().Get(id)
|
||||
if !ok || !task.Status.terminal() {
|
||||
t.Fatalf("task %s after Close = %+v, want terminal", id, task)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestClosePreventsMemberRetry(t *testing.T) {
|
||||
started := make(chan struct{}, maxMemberRestarts+1)
|
||||
release := make(chan struct{})
|
||||
l := FuncLauncher{Run: func(context.Context, MemberSpec) (MemberResult, error) {
|
||||
started <- struct{}{}
|
||||
<-release
|
||||
return MemberResult{}, ErrMemberTemporary
|
||||
}}
|
||||
sw := newSwarmFor(t, l)
|
||||
_, err := sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
if err != nil {
|
||||
t.Fatalf("Spawn: %v", err)
|
||||
}
|
||||
select {
|
||||
case <-started:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("initial member did not start")
|
||||
}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
waitFor(t, "swarm closed state", func() bool {
|
||||
sw.lifecycleMu.RLock()
|
||||
defer sw.lifecycleMu.RUnlock()
|
||||
return sw.closed
|
||||
})
|
||||
close(release)
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Close did not return after retryable member exit")
|
||||
}
|
||||
select {
|
||||
case <-started:
|
||||
t.Fatal("member retried after Close")
|
||||
default:
|
||||
}
|
||||
}
|
||||
|
||||
func TestCloseWaitsForMemberWatchers(t *testing.T) {
|
||||
release := make(chan struct{})
|
||||
l := FuncLauncher{Run: func(context.Context, MemberSpec) (MemberResult, error) {
|
||||
<-release
|
||||
return MemberResult{Result: "done"}, nil
|
||||
}}
|
||||
sw := newSwarmFor(t, l)
|
||||
id, err := sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
if err != nil {
|
||||
t.Fatalf("Spawn: %v", err)
|
||||
}
|
||||
waitFor(t, "task running", func() bool {
|
||||
task, ok := sw.Coordinator().Get(id)
|
||||
return ok && task.Status == StatusRunning
|
||||
})
|
||||
|
||||
const callers = 3
|
||||
closing := make(chan struct{}, callers)
|
||||
closed := make(chan struct{}, callers)
|
||||
for i := 0; i < callers; i++ {
|
||||
go func() {
|
||||
closing <- struct{}{}
|
||||
sw.Close()
|
||||
closed <- struct{}{}
|
||||
}()
|
||||
}
|
||||
for i := 0; i < callers; i++ {
|
||||
<-closing
|
||||
}
|
||||
select {
|
||||
case <-closed:
|
||||
t.Fatal("a Close caller returned before the member watcher exited")
|
||||
case <-time.After(50 * time.Millisecond):
|
||||
}
|
||||
close(release)
|
||||
for i := 0; i < callers; i++ {
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("all Close callers did not return after the member watcher exited")
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandoffDeliversNoteAndRetiresOriginal(t *testing.T) {
|
||||
gate := make(chan struct{})
|
||||
l := newLauncher(okFor)
|
||||
@@ -500,587 +364,3 @@ func TestSpawnUnknownAgentType(t *testing.T) {
|
||||
t.Fatal("Spawn with unknown agent type must error")
|
||||
}
|
||||
}
|
||||
|
||||
// blockingLauncher blocks inside Launch until its context is cancelled — the
|
||||
// shape of a real launcher that waits on a slot, a daemon connection, or a
|
||||
// sandbox handshake before it can return a handle.
|
||||
type blockingLauncher struct {
|
||||
entered chan struct{}
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (l *blockingLauncher) Launch(ctx context.Context, spec MemberSpec) (MemberHandle, error) {
|
||||
l.once.Do(func() { close(l.entered) })
|
||||
<-ctx.Done()
|
||||
return nil, ctx.Err()
|
||||
}
|
||||
|
||||
// TestCloseCancelsLauncherBlockedInLaunch is the regression test for holding
|
||||
// lifecycle admission across MemberLauncher.Launch. When admission was a read lock
|
||||
// held for the caller's whole duration, this deadlocked: the spawn sat inside
|
||||
// Launch waiting for its context, while Close waited for the write lock it needed
|
||||
// before it could cancel that very context. Neither side could progress, so both
|
||||
// the spawn and every Close caller hung.
|
||||
//
|
||||
// Admission is now a counted ticket, so Close can flip the flag, cancel, and then
|
||||
// wait the ticket out.
|
||||
func TestCloseCancelsLauncherBlockedInLaunch(t *testing.T) {
|
||||
launcher := &blockingLauncher{entered: make(chan struct{})}
|
||||
sw := newSwarmFor(t, launcher)
|
||||
|
||||
spawned := make(chan error, 1)
|
||||
go func() {
|
||||
_, err := sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
spawned <- err
|
||||
}()
|
||||
select {
|
||||
case <-launcher.entered:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("launcher never entered Launch")
|
||||
}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("Close deadlocked against a launcher blocked in Launch")
|
||||
}
|
||||
select {
|
||||
case <-spawned:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Spawn never returned after Close cancelled the launch context")
|
||||
}
|
||||
}
|
||||
|
||||
// TestCloseWaitsForAdmittedSpawn pins the other half of the contract: Close is a
|
||||
// barrier, so it must not return while an admitted spawn is still running. Without
|
||||
// the lifecycleWork wait, Close could finish while this launch was mid-flight.
|
||||
func TestCloseWaitsForAdmittedSpawn(t *testing.T) {
|
||||
entered := make(chan struct{})
|
||||
finish := make(chan struct{})
|
||||
launcher := &gatedLaunchLauncher{entered: entered, finish: finish}
|
||||
sw := newSwarmFor(t, launcher)
|
||||
|
||||
go func() {
|
||||
_, _ = sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
}()
|
||||
select {
|
||||
case <-entered:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("launcher never entered Launch")
|
||||
}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
select {
|
||||
case <-closed:
|
||||
t.Fatal("Close returned while an admitted spawn was still in flight")
|
||||
case <-time.After(200 * time.Millisecond):
|
||||
}
|
||||
close(finish)
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("Close did not return after the admitted spawn finished")
|
||||
}
|
||||
}
|
||||
|
||||
// TestAdmittedDispatchDoesNotLaunchAfterClose covers an operation that won a
|
||||
// lifecycle ticket before shutdown but did not reach dispatch until after Close
|
||||
// set closed. The ticket keeps Close from returning early, but it must not permit
|
||||
// a new external launch with the already-cancelled swarm context.
|
||||
func TestAdmittedDispatchDoesNotLaunchAfterClose(t *testing.T) {
|
||||
l := newLauncher(okFor)
|
||||
sw := newSwarmFor(t, l)
|
||||
sw.maxTeamSize = 1
|
||||
|
||||
if _, err := sw.coord.Register("late-task", "late-agent", "team", "late work"); err != nil {
|
||||
t.Fatalf("Register late task: %v", err)
|
||||
}
|
||||
release, err := sw.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
t.Fatalf("beginLifecycleAdmission: %v", err)
|
||||
}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
waitFor(t, "swarm closed state", func() bool {
|
||||
sw.lifecycleMu.RLock()
|
||||
defer sw.lifecycleMu.RUnlock()
|
||||
return sw.closed
|
||||
})
|
||||
|
||||
sw.dispatchAdmitted(MemberSpec{
|
||||
ID: "late-agent", TaskID: "late-task", AgentType: "teammate", Team: "team",
|
||||
})
|
||||
release()
|
||||
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Close did not return after the admitted dispatch finished")
|
||||
}
|
||||
if got := len(l.recorded()); got != 0 {
|
||||
t.Fatalf("launches after closed was set = %d, want 0", got)
|
||||
}
|
||||
team := sw.team("team")
|
||||
if got := team.Running(); got != 0 {
|
||||
t.Fatalf("team running after rejected launch = %d, want 0", got)
|
||||
}
|
||||
task, ok := sw.Coordinator().Get("late-task")
|
||||
if !ok {
|
||||
t.Fatal("late task vanished")
|
||||
}
|
||||
if task.Status != StatusFailed || !strings.Contains(task.Err, ErrSwarmClosed.Error()) {
|
||||
t.Fatalf("late task after Close = %+v, want failed with ErrSwarmClosed", task)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCloseBetweenLaunchPrecheckAndReturn rejects and reaps a handle created by
|
||||
// a Launch that straddles shutdown. The launch has passed the pre-check when it
|
||||
// enters the gate; Close then sets closed and cancels its context before the
|
||||
// launcher is allowed to return successfully.
|
||||
func TestCloseBetweenLaunchPrecheckAndReturn(t *testing.T) {
|
||||
launcher := &closeRaceLauncher{
|
||||
entered: make(chan struct{}),
|
||||
finish: make(chan struct{}),
|
||||
waited: make(chan struct{}),
|
||||
}
|
||||
sw := newSwarmFor(t, launcher)
|
||||
|
||||
spawned := make(chan string, 1)
|
||||
go func() {
|
||||
id, _ := sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
spawned <- id
|
||||
}()
|
||||
select {
|
||||
case <-launcher.entered:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("launcher never entered Launch")
|
||||
}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
waitFor(t, "swarm closed state", func() bool {
|
||||
sw.lifecycleMu.RLock()
|
||||
defer sw.lifecycleMu.RUnlock()
|
||||
return sw.closed
|
||||
})
|
||||
close(launcher.finish)
|
||||
|
||||
var id string
|
||||
select {
|
||||
case id = <-spawned:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Spawn did not return after Launch")
|
||||
}
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Close did not return after late handle was reaped")
|
||||
}
|
||||
select {
|
||||
case <-launcher.waited:
|
||||
default:
|
||||
t.Fatal("late handle was not reaped")
|
||||
}
|
||||
|
||||
team := sw.team("team")
|
||||
if got := team.Running(); got != 0 {
|
||||
t.Fatalf("team running after close-raced Launch = %d, want 0", got)
|
||||
}
|
||||
if got := len(team.liveAgents()); got != 0 {
|
||||
t.Fatalf("registered members after close-raced Launch = %d, want 0", got)
|
||||
}
|
||||
task, ok := sw.Coordinator().Get(id)
|
||||
if !ok {
|
||||
t.Fatal("task vanished")
|
||||
}
|
||||
if task.Status != StatusFailed || !strings.Contains(task.Err, ErrSwarmClosed.Error()) {
|
||||
t.Fatalf("task after close-raced Launch = %+v, want failed with ErrSwarmClosed", task)
|
||||
}
|
||||
}
|
||||
|
||||
type closeRaceLauncher struct {
|
||||
entered chan struct{}
|
||||
finish chan struct{}
|
||||
waited chan struct{}
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (l *closeRaceLauncher) Launch(ctx context.Context, spec MemberSpec) (MemberHandle, error) {
|
||||
l.once.Do(func() { close(l.entered) })
|
||||
<-l.finish
|
||||
return &closeRaceHandle{id: spec.ID, ctx: ctx, waited: l.waited}, nil
|
||||
}
|
||||
|
||||
type closeRaceHandle struct {
|
||||
id string
|
||||
ctx context.Context
|
||||
waited chan struct{}
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (h *closeRaceHandle) ID() string { return h.id }
|
||||
|
||||
func (h *closeRaceHandle) Wait() (MemberResult, error) {
|
||||
<-h.ctx.Done()
|
||||
h.once.Do(func() { close(h.waited) })
|
||||
return MemberResult{}, h.ctx.Err()
|
||||
}
|
||||
|
||||
// TestCloseBetweenRetryLaunchPrecheckAndReturn is the retry counterpart of
|
||||
// TestCloseBetweenLaunchPrecheckAndReturn: a bounded relaunch wins admission and
|
||||
// passes the pre-check, then Close lands while the launcher is inside Launch. The
|
||||
// relaunched handle must be reaped rather than adopted, so the watcher never
|
||||
// resumes supervising a member started after shutdown began — a relaunch whose
|
||||
// member goes on to succeed must not be recorded as the task's outcome.
|
||||
func TestCloseBetweenRetryLaunchPrecheckAndReturn(t *testing.T) {
|
||||
launcher := &retryCloseRaceLauncher{
|
||||
entered: make(chan struct{}),
|
||||
finish: make(chan struct{}),
|
||||
late: &retryLateHandle{id: "teammate-1"},
|
||||
}
|
||||
sw := newSwarmFor(t, launcher)
|
||||
|
||||
id, err := sw.Spawn(Policy{}, "team", "teammate", "task", "")
|
||||
if err != nil {
|
||||
t.Fatalf("Spawn: %v", err)
|
||||
}
|
||||
// The first member exits retryable, so the watcher is now inside the relaunch.
|
||||
select {
|
||||
case <-launcher.entered:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("launcher never entered the relaunch Launch")
|
||||
}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
waitFor(t, "swarm closed state", func() bool {
|
||||
sw.lifecycleMu.RLock()
|
||||
defer sw.lifecycleMu.RUnlock()
|
||||
return sw.closed
|
||||
})
|
||||
close(launcher.finish)
|
||||
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Close did not return after the late relaunch handle was reaped")
|
||||
}
|
||||
if got := launcher.launchCount(); got != 2 {
|
||||
t.Fatalf("launch attempts = %d, want 2 (initial + one relaunch)", got)
|
||||
}
|
||||
if got := launcher.late.waitCount(); got != 1 {
|
||||
t.Fatalf("late relaunch handle waits = %d, want 1 (reaped, not supervised)", got)
|
||||
}
|
||||
|
||||
team := sw.team("team")
|
||||
if got := team.Running(); got != 0 {
|
||||
t.Fatalf("team running after close-raced relaunch = %d, want 0", got)
|
||||
}
|
||||
if got := len(team.liveAgents()); got != 0 {
|
||||
t.Fatalf("registered members after close-raced relaunch = %d, want 0", got)
|
||||
}
|
||||
task, ok := sw.Coordinator().Get(id)
|
||||
if !ok {
|
||||
t.Fatal("task vanished")
|
||||
}
|
||||
if task.Status != StatusFailed {
|
||||
t.Fatalf("task after close-raced relaunch = %+v, want failed, not the late member's outcome", task)
|
||||
}
|
||||
if task.Result != "" {
|
||||
t.Fatalf("task result after close-raced relaunch = %q, want the late member's result discarded", task.Result)
|
||||
}
|
||||
}
|
||||
|
||||
// retryCloseRaceLauncher fails its first member with a retryable error, then
|
||||
// blocks inside the relaunch's Launch until the test releases it.
|
||||
type retryCloseRaceLauncher struct {
|
||||
mu sync.Mutex
|
||||
calls int
|
||||
entered chan struct{}
|
||||
finish chan struct{}
|
||||
late *retryLateHandle
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (l *retryCloseRaceLauncher) Launch(_ context.Context, spec MemberSpec) (MemberHandle, error) {
|
||||
l.mu.Lock()
|
||||
l.calls++
|
||||
first := l.calls == 1
|
||||
l.mu.Unlock()
|
||||
if first {
|
||||
return &temporaryHandle{id: spec.ID}, nil
|
||||
}
|
||||
l.once.Do(func() { close(l.entered) })
|
||||
<-l.finish
|
||||
return l.late, nil
|
||||
}
|
||||
|
||||
func (l *retryCloseRaceLauncher) launchCount() int {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
return l.calls
|
||||
}
|
||||
|
||||
// temporaryHandle exits immediately with a retryable error, driving the watcher
|
||||
// into its bounded relaunch path.
|
||||
type temporaryHandle struct{ id string }
|
||||
|
||||
func (h *temporaryHandle) ID() string { return h.id }
|
||||
|
||||
func (h *temporaryHandle) Wait() (MemberResult, error) {
|
||||
return MemberResult{}, ErrMemberTemporary
|
||||
}
|
||||
|
||||
// retryLateHandle models a member whose work does not consult the cancelled
|
||||
// launch context and reports success. Adopting it after shutdown would record a
|
||||
// post-close result on the task, so a correct shutdown reaps it exactly once and
|
||||
// discards what it returns.
|
||||
type retryLateHandle struct {
|
||||
id string
|
||||
mu sync.Mutex
|
||||
waits int
|
||||
}
|
||||
|
||||
func (h *retryLateHandle) ID() string { return h.id }
|
||||
|
||||
func (h *retryLateHandle) Wait() (MemberResult, error) {
|
||||
h.mu.Lock()
|
||||
h.waits++
|
||||
h.mu.Unlock()
|
||||
return MemberResult{Result: "late result", SessionID: "late-session"}, nil
|
||||
}
|
||||
|
||||
func (h *retryLateHandle) waitCount() int {
|
||||
h.mu.Lock()
|
||||
defer h.mu.Unlock()
|
||||
return h.waits
|
||||
}
|
||||
|
||||
func TestCloseRacesLifecycleAdmission(t *testing.T) {
|
||||
gate := make(chan struct{})
|
||||
l := newLauncher(okFor)
|
||||
l.gate = gate
|
||||
sw := newSwarmFor(t, l)
|
||||
|
||||
const spawns = 64
|
||||
start := make(chan struct{})
|
||||
results := make(chan struct {
|
||||
id string
|
||||
err error
|
||||
}, spawns)
|
||||
var callers sync.WaitGroup
|
||||
callers.Add(spawns)
|
||||
for i := 0; i < spawns; i++ {
|
||||
go func() {
|
||||
defer callers.Done()
|
||||
<-start
|
||||
id, err := sw.Spawn(Policy{}, "team", "teammate", "racing task", "")
|
||||
results <- struct {
|
||||
id string
|
||||
err error
|
||||
}{id: id, err: err}
|
||||
}()
|
||||
}
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
<-start
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
|
||||
close(start)
|
||||
callers.Wait()
|
||||
<-closed
|
||||
close(results)
|
||||
|
||||
for result := range results {
|
||||
if result.err != nil {
|
||||
if !errors.Is(result.err, ErrSwarmClosed) {
|
||||
t.Fatalf("Spawn racing Close error = %v, want ErrSwarmClosed", result.err)
|
||||
}
|
||||
continue
|
||||
}
|
||||
task, ok := sw.Coordinator().Get(result.id)
|
||||
if !ok || !task.Status.terminal() {
|
||||
t.Fatalf("admitted task %q after Close = %+v, want terminal", result.id, task)
|
||||
}
|
||||
}
|
||||
for _, task := range sw.Coordinator().List() {
|
||||
if !task.Status.terminal() {
|
||||
t.Fatalf("task %q remained %s after Close", task.ID, task.Status)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestCloseFailsQueuedSpecOnLateCreatedTeam is the regression test for queue
|
||||
// sweeping after shutdown begins. Spawn/Handoff/AdoptOrphans calls that obtained
|
||||
// tickets before closed flipped can still create a team and append to its queue.
|
||||
// Close must wait for those calls before both snapshotting teams and sweeping
|
||||
// their queues, or a late-created team's queued task remains pending forever.
|
||||
//
|
||||
// This drives that interleaving directly: two operations obtain tickets before
|
||||
// Close, then the first creates and fills a previously unseen one-slot team and
|
||||
// the second queues behind it after Close has flipped closed.
|
||||
func TestCloseFailsQueuedSpecOnLateCreatedTeam(t *testing.T) {
|
||||
sw := newSwarmFor(t, newLauncher(okFor))
|
||||
sw.maxTeamSize = 1
|
||||
|
||||
if _, err := sw.coord.Register("late-task", "late-agent", "team", "late work"); err != nil {
|
||||
t.Fatalf("Register late task: %v", err)
|
||||
}
|
||||
firstRelease, err := sw.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
t.Fatalf("first beginLifecycleAdmission: %v", err)
|
||||
}
|
||||
secondRelease, err := sw.beginLifecycleAdmission()
|
||||
if err != nil {
|
||||
firstRelease()
|
||||
t.Fatalf("second beginLifecycleAdmission: %v", err)
|
||||
}
|
||||
runningSpec := MemberSpec{ID: "running-agent", TaskID: "running-task", AgentType: "teammate", Team: "team"}
|
||||
queuedSpec := MemberSpec{ID: "late-agent", TaskID: "late-task", AgentType: "teammate", Team: "team"}
|
||||
|
||||
closed := make(chan struct{})
|
||||
go func() {
|
||||
sw.Close()
|
||||
close(closed)
|
||||
}()
|
||||
waitFor(t, "swarm closed state", func() bool {
|
||||
sw.lifecycleMu.RLock()
|
||||
defer sw.lifecycleMu.RUnlock()
|
||||
return sw.closed
|
||||
})
|
||||
select {
|
||||
case <-closed:
|
||||
t.Fatal("Close returned while an admitted ticket was still open")
|
||||
default:
|
||||
}
|
||||
|
||||
// Neither operation touched a team before Close took its old, buggy snapshot.
|
||||
// Finish the team-admission portion of both operations while their tickets are
|
||||
// still held: the first creates the team and fills its only slot; the second
|
||||
// appends to that late-created team's queue.
|
||||
team := sw.team("team")
|
||||
if !team.admit(runningSpec) {
|
||||
t.Fatal("first spec should take the late-created team's only slot")
|
||||
}
|
||||
firstRelease()
|
||||
if team.admit(queuedSpec) {
|
||||
t.Fatal("second spec should queue on the late-created team")
|
||||
}
|
||||
secondRelease()
|
||||
|
||||
select {
|
||||
case <-closed:
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("Close did not return after the admitted ticket released")
|
||||
}
|
||||
|
||||
if got := team.QueueDepth(); got != 0 {
|
||||
t.Fatalf("queue depth after Close = %d, want 0 (late spec swept)", got)
|
||||
}
|
||||
task, ok := sw.Coordinator().Get("late-task")
|
||||
if !ok {
|
||||
t.Fatal("late-queued task vanished")
|
||||
}
|
||||
if task.Status != StatusFailed {
|
||||
t.Fatalf("late-queued task status = %v, want %v (failed, not stranded)", task.Status, StatusFailed)
|
||||
}
|
||||
if !strings.Contains(task.Err, ErrSwarmClosed.Error()) {
|
||||
t.Fatalf("late-queued task err = %q, want it to mention %q", task.Err, ErrSwarmClosed.Error())
|
||||
}
|
||||
}
|
||||
|
||||
// TestAfterExitAdmittedFailsDequeuedSpecAfterClose is the regression test for
|
||||
// jatmn's second #776 finding: afterExitAdmitted used to dequeue a queued spec
|
||||
// (t.onExit) and launch it unconditionally once its own ticket was granted,
|
||||
// even though Close can flip closed in the gap between that dequeue — which
|
||||
// makes the spec invisible to Close's clearQueue sweep — and the launch. A
|
||||
// launcher that performs work before honoring its cancelled context could then
|
||||
// start a brand new worker after shutdown had already begun.
|
||||
func TestAfterExitAdmittedFailsDequeuedSpecAfterClose(t *testing.T) {
|
||||
l := newLauncher(okFor)
|
||||
sw := newSwarmFor(t, l)
|
||||
|
||||
if _, err := sw.coord.Register("queued-task", "queued-agent", "team", "queued work"); err != nil {
|
||||
t.Fatalf("Register: %v", err)
|
||||
}
|
||||
spec := MemberSpec{ID: "queued-agent", TaskID: "queued-task", AgentType: "teammate", Team: "team"}
|
||||
|
||||
// Occupy the only slot, then queue spec behind it.
|
||||
team := &Team{Name: "team", members: map[string]*Member{}, maxSize: 1}
|
||||
if !team.admit(MemberSpec{ID: "running-agent", TaskID: "running-task"}) {
|
||||
t.Fatal("first spec should take the only slot")
|
||||
}
|
||||
if team.admit(spec) {
|
||||
t.Fatal("second spec should queue, not launch, over the cap")
|
||||
}
|
||||
|
||||
// Simulate shutdown racing in between this call's ticket admission (already
|
||||
// granted, hence no beginLifecycleAdmission call here) and its dequeue+launch,
|
||||
// exactly like the finding describes: closed is already set by the time
|
||||
// afterExitAdmitted's post-dequeue check runs.
|
||||
sw.lifecycleMu.Lock()
|
||||
sw.closed = true
|
||||
sw.lifecycleMu.Unlock()
|
||||
|
||||
sw.afterExitAdmitted(team) // running-agent "exits" -> dequeues spec
|
||||
|
||||
if got := team.Running(); got != 0 {
|
||||
t.Fatalf("team running after close-raced drain = %d, want 0 (slot released, not held for a launch)", got)
|
||||
}
|
||||
task, ok := sw.Coordinator().Get("queued-task")
|
||||
if !ok {
|
||||
t.Fatal("dequeued task vanished")
|
||||
}
|
||||
if task.Status != StatusFailed {
|
||||
t.Fatalf("dequeued task status = %v, want %v (failed, not launched)", task.Status, StatusFailed)
|
||||
}
|
||||
if !strings.Contains(task.Err, ErrSwarmClosed.Error()) {
|
||||
t.Fatalf("dequeued task err = %q, want it to mention %q", task.Err, ErrSwarmClosed.Error())
|
||||
}
|
||||
for _, s := range l.recorded() {
|
||||
if s.ID == "queued-agent" {
|
||||
t.Fatal("a spec dequeued after shutdown began must never launch")
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// gatedLaunchLauncher blocks in Launch until the test releases it, ignoring
|
||||
// context cancellation so the test controls exactly when the admitted work ends.
|
||||
type gatedLaunchLauncher struct {
|
||||
entered chan struct{}
|
||||
finish chan struct{}
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (l *gatedLaunchLauncher) Launch(_ context.Context, spec MemberSpec) (MemberHandle, error) {
|
||||
l.once.Do(func() { close(l.entered) })
|
||||
<-l.finish
|
||||
return &funcHandle{id: spec.ID, done: closedChan()}, nil
|
||||
}
|
||||
|
||||
func closedChan() chan struct{} {
|
||||
ch := make(chan struct{})
|
||||
close(ch)
|
||||
return ch
|
||||
}
|
||||
+13
-12
@@ -121,12 +121,11 @@ type Scheduler struct {
|
||||
ctx context.Context
|
||||
cancel context.CancelFunc
|
||||
|
||||
mu sync.Mutex
|
||||
jobs map[string]*scheduledJob
|
||||
closed bool
|
||||
seq atomic.Uint64
|
||||
wg sync.WaitGroup
|
||||
closeOnce sync.Once
|
||||
mu sync.Mutex
|
||||
jobs map[string]*scheduledJob
|
||||
closed bool
|
||||
seq atomic.Uint64
|
||||
wg sync.WaitGroup
|
||||
}
|
||||
|
||||
// newScheduler builds a Scheduler bound to sw. Its context derives from the
|
||||
@@ -230,13 +229,15 @@ func (s *Scheduler) List() []JobStatus {
|
||||
// Close cancels every job and waits for their loops to exit. Safe to call more
|
||||
// than once.
|
||||
func (s *Scheduler) Close() {
|
||||
s.closeOnce.Do(func() {
|
||||
s.mu.Lock()
|
||||
s.closed = true
|
||||
s.cancel()
|
||||
s.mu.Lock()
|
||||
if s.closed {
|
||||
s.mu.Unlock()
|
||||
s.wg.Wait()
|
||||
})
|
||||
return
|
||||
}
|
||||
s.closed = true
|
||||
s.cancel()
|
||||
s.mu.Unlock()
|
||||
s.wg.Wait()
|
||||
}
|
||||
|
||||
// run is one job's timing loop. It requests a fresh one-shot ticker per interval
|
||||
|
||||
@@ -164,25 +164,6 @@ func TestSchedulerCloseStopsJobs(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestSchedulerCreatedAfterSwarmCloseIsClosed(t *testing.T) {
|
||||
sw := newSwarmFor(t, newLauncher(okFor))
|
||||
sw.Close()
|
||||
|
||||
sched := sw.Scheduler()
|
||||
sched.mu.Lock()
|
||||
closed := sched.closed
|
||||
sched.mu.Unlock()
|
||||
if !closed {
|
||||
t.Fatal("scheduler created after Swarm.Close was not closed")
|
||||
}
|
||||
if _, err := sched.Add(Policy{}, "team", "teammate", "t", "", Schedule{Every: time.Hour}); err == nil {
|
||||
t.Fatal("Add on a scheduler created after Swarm.Close should fail")
|
||||
}
|
||||
if got := len(sched.List()); got != 0 {
|
||||
t.Fatalf("scheduler created after Swarm.Close has %d jobs, want 0", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSchedulerAddRejectedAfterContextCancel(t *testing.T) {
|
||||
sw := newSwarmFor(t, newLauncher(okFor))
|
||||
sched := sw.Scheduler()
|
||||
|
||||
+13
-103
@@ -64,18 +64,6 @@ type Swarm struct {
|
||||
baseCtx context.Context
|
||||
cancel context.CancelFunc
|
||||
|
||||
// lifecycleMu guards closed and orders admission against shutdown. It is held
|
||||
// only for the flag check plus the lifecycleWork increment — never across a
|
||||
// launcher call, which would deadlock Close (see beginLifecycleAdmission).
|
||||
lifecycleMu sync.RWMutex
|
||||
closed bool
|
||||
// lifecycleWork counts admitted-but-unfinished lifecycle operations (spawn,
|
||||
// handoff, adoption, retry, queue drain). Close waits it out so no admitted
|
||||
// operation is still touching the swarm when Close returns.
|
||||
lifecycleWork sync.WaitGroup
|
||||
watchers sync.WaitGroup
|
||||
closeOnce sync.Once
|
||||
|
||||
mu sync.Mutex
|
||||
teams map[string]*Team
|
||||
taskCwd map[string]string // taskID -> cwd, for handoff/adoption relaunch
|
||||
@@ -149,93 +137,31 @@ func New(opts Options) (*Swarm, error) {
|
||||
}
|
||||
|
||||
// Close cancels every running member's context and releases resources. It is
|
||||
// safe to call more than once. It closes lifecycle admission before canceling
|
||||
// members, fails queued tasks, and waits for every member watcher to exit.
|
||||
// safe to call more than once. The scheduler is closed first so no new spawn
|
||||
// fires after shutdown begins.
|
||||
func (s *Swarm) Close() {
|
||||
s.closeOnce.Do(func() {
|
||||
// Closing admission and cancelling are separate steps on purpose. The flag
|
||||
// flips under lifecycleMu so no further work is admitted, and the lock is
|
||||
// released BEFORE cancelling: a member already being launched holds no lock
|
||||
// here, but it may be blocked inside MemberLauncher.Launch waiting for the
|
||||
// context this cancel provides, and it can only release its admission ticket
|
||||
// once that cancellation lands.
|
||||
s.lifecycleMu.Lock()
|
||||
s.closed = true
|
||||
s.mu.Lock()
|
||||
sched := s.scheduler
|
||||
s.mu.Unlock()
|
||||
s.lifecycleMu.Unlock()
|
||||
|
||||
if s.cancel != nil {
|
||||
s.cancel()
|
||||
}
|
||||
if sched != nil {
|
||||
sched.Close()
|
||||
}
|
||||
// Wait for every already-admitted lifecycle operation (spawn, handoff,
|
||||
// adoption, retry, queue drain) to finish BEFORE sweeping queues below.
|
||||
// Such work can create teams as well as append to their queues, so snapshot
|
||||
// the teams only after every ticket is back. At that point no team or queue
|
||||
// can gain a new entry, and the sweep cannot miss late-admitted work.
|
||||
s.lifecycleWork.Wait()
|
||||
s.mu.Lock()
|
||||
teams := make([]*Team, 0, len(s.teams))
|
||||
for _, team := range s.teams {
|
||||
teams = append(teams, team)
|
||||
}
|
||||
s.mu.Unlock()
|
||||
for _, team := range teams {
|
||||
for _, spec := range team.clearQueue() {
|
||||
_ = s.coord.Fail(spec.TaskID, ErrSwarmClosed.Error())
|
||||
}
|
||||
}
|
||||
// Every watcher is added while its admitting lifecycle ticket is still held,
|
||||
// so lifecycleWork.Wait above also orders every Add before this Wait. The
|
||||
// watcher itself can outlive that ticket, which is why both waits are needed.
|
||||
s.watchers.Wait()
|
||||
})
|
||||
s.mu.Lock()
|
||||
sched := s.scheduler
|
||||
s.mu.Unlock()
|
||||
if sched != nil {
|
||||
sched.Close()
|
||||
}
|
||||
if s.cancel != nil {
|
||||
s.cancel()
|
||||
}
|
||||
}
|
||||
|
||||
// Scheduler returns the swarm's recurring-spawn scheduler, creating it on first
|
||||
// use. Scheduling is opt-in: until a job is added the scheduler does nothing.
|
||||
//
|
||||
// The post-close handoff between this method and Close relies on the ORDER in
|
||||
// which each reads/writes s.scheduler and s.closed, not on holding one lock across
|
||||
// both: Close takes lifecycleMu.Lock() (excluding this RLock), sets closed, reads
|
||||
// whatever s.scheduler currently is, and only THEN releases lifecycleMu and calls
|
||||
// sched.Close() on what it captured. So a call here either:
|
||||
// - runs entirely before Close's write lock — sees closed == false, and the
|
||||
// scheduler it creates/returns is exactly the one Close's later snapshot will
|
||||
// capture and close; or
|
||||
// - runs entirely after Close's write lock — sees closed == true (memory model:
|
||||
// the RLock/Lock pair orders the read after Close's write) and closes the
|
||||
// scheduler itself here, because Close already captured its own snapshot
|
||||
// (possibly nil, if this call is what first creates one) and will not look
|
||||
// again.
|
||||
//
|
||||
// Either way exactly one of the two call sites closes the scheduler that ends up
|
||||
// installed, and neither can observe a "created after close but never closed"
|
||||
// scheduler — that would require reading closed == false here while also reading
|
||||
// a scheduler Close had already passed over, which the lock ordering rules out.
|
||||
func (s *Swarm) Scheduler() *Scheduler {
|
||||
s.lifecycleMu.RLock()
|
||||
closed := s.closed
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
if s.scheduler == nil {
|
||||
s.scheduler = newScheduler(s)
|
||||
}
|
||||
sched := s.scheduler
|
||||
s.mu.Unlock()
|
||||
s.lifecycleMu.RUnlock()
|
||||
if closed {
|
||||
sched.Close()
|
||||
}
|
||||
return sched
|
||||
return s.scheduler
|
||||
}
|
||||
|
||||
// ErrSwarmClosed reports lifecycle work submitted after shutdown begins.
|
||||
var ErrSwarmClosed = errors.New("swarm: closed")
|
||||
|
||||
// rememberCwd records a task's working dir so a handoff/adoption relaunch keeps it.
|
||||
func (s *Swarm) rememberCwd(taskID, cwd string) {
|
||||
s.mu.Lock()
|
||||
@@ -362,22 +288,6 @@ func (t *Team) onExit() (MemberSpec, bool) {
|
||||
return MemberSpec{}, false
|
||||
}
|
||||
|
||||
func (t *Team) releaseSlot() {
|
||||
t.mu.Lock()
|
||||
if t.running > 0 {
|
||||
t.running--
|
||||
}
|
||||
t.mu.Unlock()
|
||||
}
|
||||
|
||||
func (t *Team) clearQueue() []MemberSpec {
|
||||
t.mu.Lock()
|
||||
defer t.mu.Unlock()
|
||||
queued := t.queue
|
||||
t.queue = nil
|
||||
return queued
|
||||
}
|
||||
|
||||
func (t *Team) addMember(m *Member) {
|
||||
t.mu.Lock()
|
||||
t.members[m.ID] = m
|
||||
|
||||
@@ -23,16 +23,12 @@ type clipboardImageMsg struct {
|
||||
err error
|
||||
}
|
||||
|
||||
// readClipboardImage is the OS clipboard image reader. A var so tests can
|
||||
// substitute a stub instead of depending on the developer's real clipboard.
|
||||
var readClipboardImage = imageinput.ReadClipboardImage
|
||||
|
||||
// readClipboardImageCmd reads the OS clipboard for image content off the
|
||||
// Update goroutine. Returns a clipboardImageMsg with the bytes, or nil (no
|
||||
// command) if there is no image — the caller treats nil as a silent no-op.
|
||||
func readClipboardImageCmd() tea.Cmd {
|
||||
return func() tea.Msg {
|
||||
data, mediaType, err := readClipboardImage()
|
||||
data, mediaType, err := imageinput.ReadClipboardImage()
|
||||
if err != nil {
|
||||
return clipboardImageMsg{err: err}
|
||||
}
|
||||
|
||||
@@ -6,8 +6,6 @@ import (
|
||||
"testing"
|
||||
|
||||
tea "charm.land/bubbletea/v2"
|
||||
|
||||
"github.com/Gitlawb/zero/internal/imageinput"
|
||||
)
|
||||
|
||||
func TestComposerInsertNewlineAtCursor(t *testing.T) {
|
||||
@@ -131,77 +129,22 @@ func TestSanitizeComposerPastePreservesNewlines(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Ctrl+V must never insert TEXT: bracketed paste is the only text path, so
|
||||
// letting Bubble's own Ctrl+V binding also read the clipboard would double-paste.
|
||||
// It may return a command, but that command is the image probe below, never a
|
||||
// text paste, so the composer contents are unchanged either way.
|
||||
func TestCtrlVDoesNotPasteTextIntoComposer(t *testing.T) {
|
||||
func TestCtrlVDoesNotPasteIntoComposer(t *testing.T) {
|
||||
m := newModel(context.Background(), Options{})
|
||||
m.input.SetValue("hello")
|
||||
m.input.CursorEnd()
|
||||
|
||||
updated, _ := m.Update(testKeyCtrl('v'))
|
||||
updated, cmd := m.Update(testKeyCtrl('v'))
|
||||
next := updated.(model)
|
||||
|
||||
if cmd != nil {
|
||||
t.Fatal("ctrl+v should not run the textinput clipboard paste command")
|
||||
}
|
||||
if got := next.composerValue(); got != "hello" {
|
||||
t.Fatalf("composer value after ctrl+v = %q, want unchanged", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Ctrl+V probes the clipboard for an image (#534). A clipboard holding a
|
||||
// screenshot produces no bracketed paste at all, so without this the empty-paste
|
||||
// image probe in routePaste never runs and Ctrl+V does nothing, which is exactly
|
||||
// what users reported: right-click paste attached the image, Ctrl+V did not.
|
||||
func TestCtrlVProbesClipboardForImage(t *testing.T) {
|
||||
// Stubbed BEFORE Update, so the command Update returns is the one that runs.
|
||||
// Asserting only that some command came back, then running a freshly built
|
||||
// probe, would pass even if the key never reached the image route at all.
|
||||
original := readClipboardImage
|
||||
t.Cleanup(func() { readClipboardImage = original })
|
||||
readClipboardImage = func() ([]byte, string, error) {
|
||||
return []byte("png-bytes"), "image/png", nil
|
||||
}
|
||||
|
||||
// Both modifiers: macOS reports Command as ModSuper, so a handler matching
|
||||
// only ModCtrl leaves Cmd+V doing nothing on the platform where screenshots
|
||||
// are most often on the clipboard.
|
||||
for name, key := range map[string]tea.KeyPressMsg{
|
||||
"ctrl+v": testKeyCtrl('v'),
|
||||
"cmd+v": testKeyPressMod('v', tea.ModSuper),
|
||||
} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
m := newModel(context.Background(), Options{})
|
||||
_, cmd := m.Update(key)
|
||||
if cmd == nil {
|
||||
t.Fatal("no command issued; the clipboard image probe never ran")
|
||||
}
|
||||
image, ok := execCmd(cmd).(clipboardImageMsg)
|
||||
if !ok {
|
||||
t.Fatal("the command issued was not the clipboard image probe")
|
||||
}
|
||||
if string(image.data) != "png-bytes" || image.mediaType != "image/png" {
|
||||
t.Fatalf("unexpected image message: %+v", image)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// With no image on the clipboard the probe stays silent: Ctrl+V while copying
|
||||
// text must not emit a notice or disturb the composer. Driven through Update
|
||||
// rather than by calling the probe directly, so this covers the real key route
|
||||
// and not just the command in isolation.
|
||||
func TestClipboardImageProbeSilentWithoutImage(t *testing.T) {
|
||||
original := readClipboardImage
|
||||
t.Cleanup(func() { readClipboardImage = original })
|
||||
readClipboardImage = func() ([]byte, string, error) { return nil, "", nil }
|
||||
|
||||
m := newModel(context.Background(), Options{})
|
||||
_, cmd := m.Update(testKeyCtrl('v'))
|
||||
if msg := execCmd(cmd); msg != nil {
|
||||
t.Fatalf("probe emitted %T with no image on the clipboard, want no message", msg)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPastedMultilineComposerContentRendersAsPreview(t *testing.T) {
|
||||
paste := strings.Join([]string{
|
||||
"Create a book library dashboard page with the Bootstrap theme.",
|
||||
@@ -522,53 +465,3 @@ func TestComposerTerminalWordKeybindings(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A clipboard that holds an image the host cannot extract must say so. This is
|
||||
// the macOS case: neither pngpaste nor PyObjC ships with the OS, so the reader
|
||||
// fails on a stock Mac, and reporting that as "no image" meant Ctrl+V did
|
||||
// nothing at all with no explanation. Silence is the wrong answer to a
|
||||
// condition the user can act on.
|
||||
func TestClipboardImageUnreadableSurfacesToUser(t *testing.T) {
|
||||
original := readClipboardImage
|
||||
t.Cleanup(func() { readClipboardImage = original })
|
||||
readClipboardImage = func() ([]byte, string, error) {
|
||||
return nil, "", imageinput.ErrClipboardImageUnreadable
|
||||
}
|
||||
|
||||
m := newModel(context.Background(), Options{})
|
||||
_, cmd := m.Update(testKeyCtrl('v'))
|
||||
if cmd == nil {
|
||||
t.Fatal("no command issued; the clipboard image probe never ran")
|
||||
}
|
||||
msg := execCmd(cmd)
|
||||
image, ok := msg.(clipboardImageMsg)
|
||||
if !ok {
|
||||
t.Fatalf("probe returned %T, want clipboardImageMsg carrying the failure", msg)
|
||||
}
|
||||
if image.err == nil {
|
||||
t.Fatal("the unreadable-clipboard failure was swallowed; the user is told nothing")
|
||||
}
|
||||
// The message has to name the remedy, since the whole point is that the user
|
||||
// can fix this by installing something.
|
||||
if !strings.Contains(image.err.Error(), "pngpaste") {
|
||||
t.Fatalf("error = %q, want it to name what to install", image.err.Error())
|
||||
}
|
||||
|
||||
// And it must reach the transcript as an error row rather than stopping at
|
||||
// the message, which is where the old silent no-op ended.
|
||||
updated, _ := m.Update(image)
|
||||
next, ok := updated.(model)
|
||||
if !ok {
|
||||
t.Fatalf("Update returned %T, want model", updated)
|
||||
}
|
||||
found := false
|
||||
for _, row := range next.transcript {
|
||||
if row.kind == rowError && strings.Contains(row.text, "Clipboard image read failed") {
|
||||
found = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
t.Fatal("the failure never reached the transcript; the user still sees nothing")
|
||||
}
|
||||
}
|
||||
@@ -34,14 +34,6 @@ func keyCtrl(msg tea.KeyMsg, code rune) bool {
|
||||
return keyCode(msg) == code && keyHasMod(msg, tea.ModCtrl)
|
||||
}
|
||||
|
||||
// keySuper matches the Command key on macOS, which the rest of the TUI models as
|
||||
// ModSuper (see keybindings.go). Kept separate from keyCtrl because a binding
|
||||
// that means Ctrl on Linux and Windows usually means Command on macOS, and the
|
||||
// two arrive as different modifiers.
|
||||
func keySuper(msg tea.KeyMsg, code rune) bool {
|
||||
return keyCode(msg) == code && keyHasMod(msg, tea.ModSuper)
|
||||
}
|
||||
|
||||
func keyCtrlArrow(msg tea.KeyMsg, code rune) bool {
|
||||
return keyIs(msg, code) && keyHasMod(msg, tea.ModCtrl)
|
||||
}
|
||||
|
||||
@@ -1703,27 +1703,6 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) {
|
||||
m.input.SetWidth(maxInt(20, m.chatColumnWidth()-14))
|
||||
return m, nil
|
||||
}
|
||||
case keyCtrl(msg, 'v'), keySuper(msg, 'v'):
|
||||
// Ctrl+V probes the clipboard for an IMAGE only. Text pasting stays
|
||||
// exclusively on the terminal's bracketed-paste path (Bubble's own
|
||||
// Ctrl+V binding is disabled in newModel for exactly that reason), so
|
||||
// this cannot double-insert text. It is needed because a clipboard
|
||||
// holding a screenshot produces no bracketed paste at all: the terminal
|
||||
// has no text to send, so routePaste never runs and its empty-content
|
||||
// image probe never fires. Right-click paste reached that probe only
|
||||
// because it always delivers a clipboardReadMsg, empty or not.
|
||||
// readClipboardImageCmd yields no message when the clipboard holds no
|
||||
// image, so Ctrl+V with text on the clipboard stays a no-op here and is
|
||||
// handled by the bracketed paste exactly as before.
|
||||
//
|
||||
// Cmd+V is matched too, since macOS reports Command as ModSuper rather
|
||||
// than ModCtrl. That only helps on terminals that deliver the key to the
|
||||
// application: one that handles Cmd+V itself pastes the clipboard TEXT
|
||||
// and sends no key event, so an image-only clipboard still produces
|
||||
// nothing for this to react to.
|
||||
if m.noBlockingModal() {
|
||||
return m, readClipboardImageCmd()
|
||||
}
|
||||
case keyCtrl(msg, 'f'):
|
||||
if m.picker != nil && m.picker.kind == pickerModel {
|
||||
if m.modelPickerIsLoading() {
|
||||
|
||||
Generated
+2
-2
@@ -1,12 +1,12 @@
|
||||
{
|
||||
"name": "@gitlawb/zero",
|
||||
"version": "0.6.0",
|
||||
"version": "0.5.0",
|
||||
"lockfileVersion": 3,
|
||||
"requires": true,
|
||||
"packages": {
|
||||
"": {
|
||||
"name": "@gitlawb/zero",
|
||||
"version": "0.6.0",
|
||||
"version": "0.5.0",
|
||||
"cpu": [
|
||||
"x64",
|
||||
"arm64"
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "@gitlawb/zero",
|
||||
"version": "0.6.0",
|
||||
"version": "0.5.0",
|
||||
"description": "Zero — a fast, multi-provider terminal coding agent.",
|
||||
"type": "module",
|
||||
"module": "bin/zero.js",
|
||||
|
||||
Reference in new issue
Block a user