Publish waiting state after pending input exists - #1055
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56c8b7ee1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 963e0c80ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cdff23ae3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eb6690d6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 similar comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf695c8b5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case <-waitCtx.Done(): | ||
| b.clearPendingIfMatch(req.RunID, entry) | ||
| if err := ctx.Err(); err != nil { | ||
| return nil, time.Time{}, err | ||
| } |
There was a problem hiding this comment.
At exact head cf695c8b, if OnPending remains blocked while Submit succeeds before the deadline, Submit deletes the pending entry and buffers the answer, but this deadline branch returns a timeout without checking entry.answerC. The checkpoint broker now recovers an answer that won this race, but the in-memory broker still discards the successfully accepted answer; check the buffered submission before reporting the timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8e216c4. Both in-memory deadline branches now use bounded cleanup that returns a buffered answer accepted before the deadline; the regression uses bounded gating/receives and passed 30 normal plus 10 race repetitions.
| if record.Status != StatusPending { | ||
| s.resolveMu.Unlock() | ||
| return nil |
There was a problem hiding this comment.
When an answer, approval, or HTTP resume races deadline expiry, the caller can read a pending checkpoint before ExpirePending wins resolveMu; resolve then reaches this branch and returns nil. Consequently Submit, Approve, or POST /resume acknowledges success even though the checkpoint remains expired and the submitted payload is discarded. Return a distinguishable already-resolved result so callers do not falsely report accepted input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed end to end in 8e216c4. ErrAlreadyResolved remains distinct in the checkpoint service, maps to existing no-pending semantics for AskUser and approval/deny callers, maps to 409 no_pending_input through /v1/runs/{id}/input, and maps to stable 409 already_resolved for generic checkpoint resume without changing durable status, payload, or updated_at.
|
Exact tested head: 8e216c4. Sixth review repair adds shared checkpoint deadline recovery, stable no-pending/conflict error mapping, immutable repeated-resume behavior, and bounded deterministic race tests. Evidence: focused count=30; focused race count=10; complete affected packages normal/race; unchanged ./scripts/test-regression.sh PASS with coverage 85.6% and zero uncovered functions. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e216c4b64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| select { | ||
| case <-notified: | ||
| case <-waitCtx.Done(): | ||
| return b.finishAskWait(ctx, req, entry) |
There was a problem hiding this comment.
When OnPending has entered the runner's setStatus and is stalled in Store.UpdateRun, a submission can be accepted and buffered before the deadline. Fresh evidence at the final head is that this deadline arm now calls finishAskWait, which returns that answer without waiting for notified to close; the step engine can therefore emit run.resumed before the notifier later emits run.waiting_for_user, or the wait event can be dropped if the run becomes terminal first. The checkpoint-backed branch has the same early recovery behavior, so accepted-answer recovery must preserve wait-event publication ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on exact head 331bc7c44d3d8302ed21f1843a48f79372cdba7c. Accepted-answer recovery now waits for pending publication; notifier persistence uses the supplied deadline context; started observer publication drains; transient status/event failures retry once-on-success. Deterministic ordering, deadline, observer, retry, race, and full regression gates pass.
8e216c4 to
97665cc
Compare
|
Rebased exact tested head: 97665cc, based on origin/main 8906149. Reviewed checkpoint race and HTTP semantics are preserved. Evidence rerun after rebase: focused normal count=30 PASS; focused race count=10 PASS; complete internal/checkpoints, internal/harness, and internal/server normal/race PASS; unchanged foreground non-TTY ./scripts/test-regression.sh PASS with coverage 85.7% and zero uncovered functions. @codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
97665cc to
331bc7c
Compare
|
Exact-head verification for
The rebased head also includes once-on-success pending publication, observer drain, atomic cross-Service checkpoint CAS, transient poll tolerance, monotonic serialized status persistence, strict waiting-event retry with contiguous SSE cursors, deliberate redaction-drop completion, and preserved ordinary-event best-effort behavior. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 331bc7c44d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| persisted := j.runner.storeAppendEventContext(ctx, delivery.event, delivery.eventSeq) | ||
| if (!persisted && requirePersistence) || ctx.Err() != nil { | ||
| j.discardPreparedEvent(delivery) | ||
| return false |
There was a problem hiding this comment.
When a strict waiting-event append commits just as the notifier deadline expires, persisted is true but the independent ctx.Err() check still calls discardPreparedEvent. The durable store retains that sequence while the in-memory ledger rolls it back; the next event then reuses the same (run_id, seq), causing SQLite and MemoryStore to reject its append and making durable replay diverge from live history.
Useful? React with 👍 / 👎.
| if !started { | ||
| stopped = true | ||
| cancel() | ||
| } |
There was a problem hiding this comment.
When the notifier's first waiting-state persistence attempt fails quickly and a user submits before the polling observer has seen the pending record, submission removes that record and the tool returns successfully. This stop path then cancels the never-started observer, so the transient failure is never retried and run.resumed can be emitted without a preceding run.waiting_for_user; unlike the existing retry test, this scenario does not wait for the observer's second attempt before submitting.
Useful? React with 👍 / 👎.
| if record.Status != StatusPending && !s.resolutionActive(id) { | ||
| s.unregister(id, ch) | ||
| return waitResultFromRecord(record) |
There was a problem hiding this comment.
When an approval's ResolvePending has durably committed before the timeout but its store call has not yet returned, this local-resolution suppression prevents Wait from observing the approved row. checkpointApprovalBroker.Ask therefore reaches its deadline and reports ApprovalTimeoutError, while the subsequent expiration loses to the already-approved checkpoint and the operator's approval call later succeeds. Fresh evidence at this head is that only the ask-user timeout path re-reads the winning resolution; approval waits still unconditionally report timeout.
Useful? React with 👍 / 👎.
331bc7c to
3a911af
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a911af9ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ticker := time.NewTicker(time.Millisecond) | ||
| defer ticker.Stop() | ||
| for { | ||
| if pending, ok := broker.Pending(runID); ok { |
There was a problem hiding this comment.
When broker.Pending(runID) blocks—for example, while the checkpoint store is stalled—the observer cannot react to observerCtx cancellation, and stopPendingObserver subsequently waits on done before tool execution can finish. This can delay or indefinitely hang an otherwise answered, timed-out, or cancelled AskUserQuestion run; use a context-aware/bounded pending lookup or avoid synchronously draining an uncancellable read.
Useful? React with 👍 / 👎.
3a911af to
e6e255d
Compare
1 participant
Closes #1054
Summary
waiting_for_useronly after AskUserQuestion pending input is readable, with deadline-bound notifier work and accepted-answer orderingPendingobserver fallback for third-party brokers that omitOnPending; callback and observer share serialized once-on-success publicationExact-head verification
Rebased candidate
e6e255d8ba60d1ae3035031f1342ae22b0c5d74dontomainfa2f7c8ed6e576006d14ec6ec989ac2d6f1a5e91.go test ./internal/checkpoints ./internal/harness ./internal/server -count=1: PASSgo test -race ./internal/checkpoints ./internal/harness ./internal/server -count=1: PASS./scripts/test-regression.sh: PASStest-fastandtest-race: PASS on this exact SHAThe prior hosted results applied to the obsolete pre-rebase head and are not being used as promotion evidence.
Dependency placement
This slice must merge before #1067/#1070 so terminal transition atomicity is composed on top of the accepted waiting-state lifecycle. It also unblocks the #1000 cron/callback promotion chain.