Repository navigation
Reject nested steps and missing cursor targets - #1534
Conversation
e55befb to
88abef3
Compare
24e18d3 to
a79aaa9
Compare
88abef3 to
f45a4df
Compare
a79aaa9 to
a379437
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: a379437228
ℹ️ 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".
|
@brandur PR copy was bad here, but this is not actually mergeable. There are some key design issues with allowing nested resumable jobs. So far this PR mainly illustrates their failure with test cases. What do you think about just blocking them? Is there even a use case for it? |
|
@bgentry The only thing that's coming to mind is if you had kind of a messy codebase happening and there was a variety of checkpoints across job implementation / controller / model helpers, then it might be easier for you to refactor code if we just allowed nesting and and approximately the right thing would happen. Especially if it doesn't work though, I'm good with banning it. It didn't occur to me as something that people would even try to do when I wrote the first implementation. |
Nested resumable steps can persist unreachable resume points and replay rolled-back parent checkpoints when a child updates its cursor. Reject starting a step while another step callback is active in Go, JavaScript, and Rust, matching Java and Ruby. Preserve nesting errors so an unsuccessful parent cannot be marked complete or lose its cursor. Remove the enclosing-step state that nesting requires, and document the restriction in the public APIs and guides. Replace the draft reproductions with coverage for plain and cursor nesting, committed and rolled-back parent checkpoints, and caught nesting errors. Verify all parent and child step combinations across runtimes.
a379437 to
2ba8a0d
Compare
bgentry
changed the title
bgentry
changed the title
bgentry
changed the title
A first-step cursor can be saved without a completed-step checkpoint. When a retry omits that step, the job can silently complete or persist a later checkpoint that makes the original cursor unreachable. Track cursor-only targets independently of the skip boundary across all five runtimes. Require each target to be reached through a cursor step, and preserve the original checkpoint while targets remain unmatched. Reject transactional checkpoints that would advance past those targets. Keep the existing metadata format and preserve behavior for jobs with a step marker, including legacy stale cursor entries. Add shared compatibility cases and database recovery coverage, including zero and null cursors, renamed steps, plain-step replacements, earlier failures, and checkpoints inside a restored target. Document the cursor-only restriction and the behavior change for nested steps.
1f3da46 to
9d94fc6
Compare
|
@brandur updated to reject nested steps and missing cursor targets |
Nested resumable steps can save unreachable resume points or replay rolled-back parent checkpoints. Reject nesting before invoking a child callback in Go, JavaScript, and Rust, matching Java and Ruby, and keep a nesting failure from completing the parent or clearing its cursor.
A retry can also silently complete when it omits a saved cursor-only step. Track those targets independently of the existing skip boundary in all five runtimes. Require the cursor API when reaching a target, preserve the original checkpoint while targets remain unmatched, and reject transactional checkpoint writes that would advance past them. Restoring an omitted step can then recover and checkpoint normally.
The metadata format stays unchanged. Jobs with an existing step marker keep their behavior, including legacy stale cursor entries. Shared fixtures and database regressions cover compatibility and recovery, and release notes explicitly call out the nesting restriction.