A Codex RPC can deadlock the reader it waits on
This commit is contained in:
+90
-32
@@ -90,11 +90,11 @@ type rpcResult struct {
|
||||
|
||||
type Session struct {
|
||||
cmd *exec.Cmd
|
||||
stdin io.WriteCloser
|
||||
stdin *os.File
|
||||
events chan Event
|
||||
done chan struct{}
|
||||
|
||||
writeMu sync.Mutex
|
||||
writeLock chan struct{}
|
||||
recordMu sync.Mutex
|
||||
transcript io.Writer
|
||||
|
||||
@@ -160,18 +160,23 @@ func spawn(opts Options) (*Session, error) {
|
||||
cmd.Stderr = os.Stderr
|
||||
cmd.Env = buildEnv(opts)
|
||||
|
||||
stdin, err := cmd.StdinPipe()
|
||||
stdinR, stdin, err := os.Pipe()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
cmd.Stdin = stdinR
|
||||
|
||||
stdout, err := cmd.StdoutPipe()
|
||||
if err != nil {
|
||||
stdinR.Close()
|
||||
stdin.Close()
|
||||
return nil, err
|
||||
}
|
||||
|
||||
err = cmd.Start()
|
||||
stdinR.Close()
|
||||
if err != nil {
|
||||
stdin.Close()
|
||||
return nil, fmt.Errorf("start codex: %w", err)
|
||||
}
|
||||
|
||||
@@ -180,10 +185,11 @@ func spawn(opts Options) (*Session, error) {
|
||||
return s, nil
|
||||
}
|
||||
|
||||
func newSession(opts Options, stdin io.WriteCloser, stdout io.Reader) *Session {
|
||||
func newSession(opts Options, stdin *os.File, stdout io.Reader) *Session {
|
||||
s := &Session{
|
||||
stdin: stdin,
|
||||
events: make(chan Event, 256),
|
||||
writeLock: make(chan struct{}, 1),
|
||||
done: make(chan struct{}),
|
||||
transcript: opts.Transcript,
|
||||
pending: map[int64]chan rpcResult{},
|
||||
@@ -421,47 +427,81 @@ func (s *Session) Interrupt(ctx context.Context) error {
|
||||
return s.Call(ctx, MethodTurnInterrupt, params, nil)
|
||||
}
|
||||
|
||||
func (s *Session) Steer(ctx context.Context, text string) error {
|
||||
func (s *Session) Steer(text string) (*Reply, error) {
|
||||
s.stateMu.Lock()
|
||||
params := TurnSteerParams{ThreadID: s.threadID, ExpectedTurnID: s.turnID, Input: []UserInput{{Type: UserInputTypeText, Text: text}}}
|
||||
s.stateMu.Unlock()
|
||||
|
||||
if params.ExpectedTurnID == "" {
|
||||
return errors.New("no turn in progress")
|
||||
return nil, errors.New("no turn in progress")
|
||||
}
|
||||
return s.Call(ctx, MethodTurnSteer, params, nil)
|
||||
return s.request(context.Background(), MethodTurnSteer, params)
|
||||
}
|
||||
|
||||
func (s *Session) Compact(ctx context.Context) error {
|
||||
return s.Call(ctx, MethodThreadCompactStart, ThreadCompactStartParams{ThreadID: s.ID()}, nil)
|
||||
}
|
||||
|
||||
const rpcTimeout = time.Minute
|
||||
|
||||
type Reply struct {
|
||||
s *Session
|
||||
id int64
|
||||
ch chan rpcResult
|
||||
deadline time.Time
|
||||
}
|
||||
|
||||
func (s *Session) Call(ctx context.Context, method string, params any, result any) error {
|
||||
id := s.nextID.Add(1)
|
||||
ch := make(chan rpcResult, 1)
|
||||
r, err := s.request(ctx, method, params)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
return r.Wait(ctx, result)
|
||||
}
|
||||
|
||||
s.pendingMu.Lock()
|
||||
s.pending[id] = ch
|
||||
s.pendingMu.Unlock()
|
||||
|
||||
defer func() {
|
||||
s.pendingMu.Lock()
|
||||
delete(s.pending, id)
|
||||
s.pendingMu.Unlock()
|
||||
}()
|
||||
func rpcDeadline(ctx context.Context) time.Time {
|
||||
deadline := time.Now().Add(rpcTimeout)
|
||||
d, ok := ctx.Deadline()
|
||||
if ok && d.Before(deadline) {
|
||||
return d
|
||||
}
|
||||
return deadline
|
||||
}
|
||||
|
||||
func (s *Session) request(ctx context.Context, method string, params any) (*Reply, error) {
|
||||
raw, err := json.Marshal(params)
|
||||
if err != nil {
|
||||
return err
|
||||
return nil, err
|
||||
}
|
||||
|
||||
err = s.write(message{ID: json.RawMessage(strconv.FormatInt(id, 10)), Method: method, Params: raw})
|
||||
r := &Reply{s: s, id: s.nextID.Add(1), ch: make(chan rpcResult, 1), deadline: rpcDeadline(ctx)}
|
||||
|
||||
s.pendingMu.Lock()
|
||||
s.pending[r.id] = r.ch
|
||||
s.pendingMu.Unlock()
|
||||
|
||||
err = s.write(message{ID: json.RawMessage(strconv.FormatInt(r.id, 10)), Method: method, Params: raw}, r.deadline)
|
||||
if err != nil {
|
||||
return err
|
||||
r.forget()
|
||||
return nil, err
|
||||
}
|
||||
return r, nil
|
||||
}
|
||||
|
||||
func (r *Reply) forget() {
|
||||
r.s.pendingMu.Lock()
|
||||
delete(r.s.pending, r.id)
|
||||
r.s.pendingMu.Unlock()
|
||||
}
|
||||
|
||||
func (r *Reply) Wait(ctx context.Context, result any) error {
|
||||
defer r.forget()
|
||||
|
||||
ctx, cancel := context.WithDeadline(ctx, r.deadline)
|
||||
defer cancel()
|
||||
|
||||
select {
|
||||
case res := <-ch:
|
||||
case res := <-r.ch:
|
||||
if res.err != nil {
|
||||
return res.err
|
||||
}
|
||||
@@ -469,7 +509,7 @@ func (s *Session) Call(ctx context.Context, method string, params any, result an
|
||||
return nil
|
||||
}
|
||||
return json.Unmarshal(res.result, result)
|
||||
case <-s.done:
|
||||
case <-r.s.done:
|
||||
return errors.New("codex exited")
|
||||
case <-ctx.Done():
|
||||
return ctx.Err()
|
||||
@@ -485,13 +525,13 @@ func (s *Session) notify(method string, params any) error {
|
||||
}
|
||||
m.Params = raw
|
||||
}
|
||||
return s.write(m)
|
||||
return s.write(m, time.Now().Add(rpcTimeout))
|
||||
}
|
||||
|
||||
func (s *Session) Close() error {
|
||||
s.writeMu.Lock()
|
||||
s.writeLock <- struct{}{}
|
||||
s.stdin.Close()
|
||||
s.writeMu.Unlock()
|
||||
<-s.writeLock
|
||||
|
||||
select {
|
||||
case <-s.done:
|
||||
@@ -509,18 +549,36 @@ func (s *Session) Kill() {
|
||||
s.cmd.Wait()
|
||||
}
|
||||
|
||||
func (s *Session) write(m message) error {
|
||||
func (s *Session) write(m message, deadline time.Time) error {
|
||||
b, err := json.Marshal(m)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
s.writeMu.Lock()
|
||||
defer s.writeMu.Unlock()
|
||||
wait := time.NewTimer(time.Until(deadline))
|
||||
defer wait.Stop()
|
||||
select {
|
||||
case s.writeLock <- struct{}{}:
|
||||
case <-wait.C:
|
||||
return os.ErrDeadlineExceeded
|
||||
}
|
||||
defer func() { <-s.writeLock }()
|
||||
|
||||
if !time.Now().Before(deadline) {
|
||||
return os.ErrDeadlineExceeded
|
||||
}
|
||||
|
||||
s.record("send", b)
|
||||
|
||||
_, err = s.stdin.Write(append(b, '\n'))
|
||||
err = s.stdin.SetWriteDeadline(deadline)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
n, err := s.stdin.Write(append(b, '\n'))
|
||||
if n > 0 && errors.Is(err, os.ErrDeadlineExceeded) {
|
||||
slog.Error("codex stopped reading its stdin mid-line; killing it", "pid", s.cmd.Process.Pid, "written", n, "len", len(b)+1)
|
||||
s.cmd.Process.Kill()
|
||||
}
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -646,7 +704,7 @@ func (s *Session) handleServerRequest(m message) {
|
||||
result, err := s.answerServerRequest(m)
|
||||
if err != nil {
|
||||
slog.Error("codex server request failed", "method", m.Method, "err", err)
|
||||
s.write(message{ID: m.ID, Error: &RPCError{Code: -32601, Message: err.Error()}})
|
||||
s.write(message{ID: m.ID, Error: &RPCError{Code: -32601, Message: err.Error()}}, time.Now().Add(rpcTimeout))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -655,7 +713,7 @@ func (s *Session) handleServerRequest(m message) {
|
||||
slog.Error("encode codex server response", "method", m.Method, "err", err)
|
||||
return
|
||||
}
|
||||
s.write(message{ID: m.ID, Result: raw})
|
||||
s.write(message{ID: m.ID, Result: raw}, time.Now().Add(rpcTimeout))
|
||||
}
|
||||
|
||||
func (s *Session) answerServerRequest(m message) (any, error) {
|
||||
|
||||
Reference in New Issue
Block a user