Repository navigation
fix: preserve running status while a job starts - #17
Conversation
Signed-off-by: boulea7 <zln1905391059@163.com>
Reviewer's GuideThe PR fixes a startup race by having status() recognize a locally owned startup reservation before reconciling an absent worker, while preserving existing ownership and terminal-state behavior. Regression tests cover both startup timing windows and confirm duplicate-start orphan reconciliation still occurs after the reservation is released. Sequence diagram for preserving running status during job startupsequenceDiagram
participant Start as Supervisor.start()
participant Status as Supervisor.status()
participant Registry as Registry lock
participant Worker as Thread.start()
participant Store as Job store
Start->>Registry: Create startup reservation and running metadata
par Concurrent status check
Status->>Registry: Read _starting_jobs and fresh record
alt Local startup reservation exists
Registry-->>Status: Local owner identity, worker may be absent
Status-->>Start: Return running record
else Reservation not present
Registry-->>Status: No local reservation
Status->>Store: finalize_job(job_id)
Store-->>Status: Reconciled terminal record
end
and Worker startup
Start->>Worker: Thread.start()
end
Worker-->>Start: Worker runs and completes normally
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesStartup status handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The fix preserves startup status without adding execution authority or weakening ownership checks. No security issue introduced or materially worsened by this change was identified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If a local start remains marked in _starting_jobs after its worker fails to register or start, status() will continue reporting the persisted job as running and skip orphan reconciliation. Reverting restores the old behavior, but any stale running record created meanwhile would need a bounded cleanup or re-run.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
On the startup reservation concern: Both startup windows now preserve the running record and complete without the false crash diagnostic. CI on |
Concurrent Python calls to
Supervisor.start()andSupervisor.status()on the same instance could mark a job failed before its worker starts. The job could then complete successfully with a stale crash error. Check the existing startup reservation and fresh record ownership under the registry lock before reconciling an absent worker.Regression tests pause startup after metadata creation and before
Thread.start(), then verify normal completion without the false diagnostic. A duplicate-start test checks that orphan reconciliation resumes once the reservation is released. Foreign-owner and terminal handling are preserved. Standard synchronous stdio handling does not interleave these calls.Validation: 150 selected Supervisor, MCP and session-store tests passed on Linux/Python 3.12, plus Ruff, an offline wheel/sdist build and the release artifact audit. Local full-suite, native Windows and real-provider E2E runs were not performed.
CI for
e9da634passed all five code jobs and 48 steps, including required lint/test/build/artifact checks and Linux/macOS compatibility tests on Python 3.11 and 3.13: CI run.Follow-up to the startup race identified in #16.
Summary by Sourcery
Preserve running job state while local workers are starting and reconcile genuine orphaned jobs after startup reservations are released.
Bug Fixes:
Tests:
Summary by cubic
Fixes a race where concurrent
Supervisor.start()andSupervisor.status()calls on the same instance could mark a job failed before its worker started, leaving a stale crash error on a job that later completed successfully.status()now checks the job's local startup reservation and fresh record ownership under the registry lock before reconciling an absent worker.Written for commit e9da634. Summary will update on new commits.
Summary by CodeRabbit