feat(store): record which process owns a run, and enumerate unsettled runs - #76
Conversation
Introduce a `RunExecutor` struct to record the host, pid, and process start time of the process executing a run, enabling liveness checks that distinguish active runs from tombstones left by killed processes. Also add a `cancel_requested` flag to `RunRecord` for durable cancellation that works across process boundaries, along with a builder method for setting the executor. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The `RunExecutor` type is now publicly re-exported from the store types module so that it can be used by external consumers of the crate. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a new method `unsettled_runs` to the `WorkflowStore` trait that returns every recorded run that has not settled, across all workflows. This enables a reconciliation sweep to query which records still claim to be live without re-reading the runs directory once per workflow. The default implementation returns an empty vector, allowing stores that cannot enumerate their runs to be left alone rather than failing to compile. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Extract the common logic for reading and parsing run records from the run directory into a private helper method, then reuse it in both `list_runs` and the new `unsettled_runs` to eliminate duplication and ensure consistent error handling. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a method to read all parsable run records from a scope's runs directory, returning an empty list when the directory does not exist. The implementation skips unparseable files rather than failing, so that a single corrupt record does not hide the rest of the run history. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Set executor to None and cancel_requested to false in the factory function so that newly created run records are properly unowned and not prematurely cancelled, keeping the factory usable in tests and tools that do not execute runs. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add the missing `executor` and `cancel_requested` fields to the test structs in three test functions to match the updated data model, ensuring the tests remain valid after the struct was extended. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add `RunExecutor` and `RunRecord` to the public re-exports from `crate::store::types` in the test module, so that test code can reference these types without additional imports. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Consolidated multi-line re-export statements in the store module and its types submodule into single lines, removing the unnecessary line breaks that were splitting related items across lines. This improves readability without changing any exported symbols or behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe store now persists executor and cancellation metadata, reads run records through a shared helper, and exposes ChangesRun Store Changes
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds durable run ownership, cancellation markers, and unsettled-run enumeration without changing behavior until consumed by the host-side fix; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant WorkflowStore
participant read_run_dir
participant RunsDirectory
WorkflowStore->>read_run_dir: request run records
read_run_dir->>RunsDirectory: enumerate JSON records
RunsDirectory-->>read_run_dir: return readable records
read_run_dir-->>WorkflowStore: return parsed records
WorkflowStore-->>WorkflowStore: filter unsettled and sort by start time
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Warning Your free Security trial is over. An organization admin can activate billing to continue. 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 |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows3 changed behaviours across 8 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 40 further behaviours left out to keep the diagram readable. flowchart LR
n0["...er_recorded_is_an_error_not_a_silent_none<br/>changed"]:::changed
n1["RunOrigin<br/>changed"]:::changed
n2["RunRecord<br/>changed"]:::changed
n3["WorkflowError"]:::impacted
n4["map"]:::impacted
n5["read_run_dir"]:::impacted
n6["require_run"]:::impacted
n7["validate_graph"]:::impacted
n8["WorkflowStore"]:::impacted
n0 -->|calls| n6
n0 -->|tests| n6
n2 -->|uses| n1
n5 -->|uses| n2
n5 -->|uses| n3
n5 -->|calls| n4
n6 -->|uses| n8
n7 -->|calls| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 9367c00029
ℹ️ 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".
| #[test] | ||
| fn unsettled_runs_spans_every_workflow_and_skips_finished_ones() { |
There was a problem hiding this comment.
_tests.rs file
These newly added tests extend src/store/tests/runs.rs, whose filename does not end in _tests.rs; repository guidance explicitly requires Rust tests to be kept in files using that suffix. Move this test module to, for example, runs_tests.rs and update its declaration in src/store/tests/mod.rs so the change follows the mandated test organization.
AGENTS.md reference: AGENTS.md:L6-L7
Useful? React with 👍 / 👎.
1 participant
Summary
The store half of a fix for run records left behind by a process that was killed. Medulla-side change: tinyhumansai/medulla#280 (depends on this landing first).
A host accumulated 64 run records marked
runningwith nothing executing any of them — the oldest 13 days old with zero steps.RunFinalizeron the host side reconciles on drop, which covers every exit that runs destructors, but not aSIGKILL, an OOM kill, or a reboot. Those left records claiming to be live forever, and nothing could clear them: a cancel could only reach a run in the calling process, so an orphan was refused as "not mine to cancel".Fixing that needs two things from the store, which is what this PR adds. It is inert on its own — no behaviour changes until a host uses these.
What changed
RunExecutoronRunRecord. Host, pid, and the executing process's own start time. Without an owner recorded there is no way to tell "another host is working on this right now" from "this is a tombstone", and a sweep that cannot tell the difference eventually settles live work.The start time is the pid-reuse guard, and it is the field that makes this safe rather than merely plausible: a run left by pid 4711 looks alive the moment anything unrelated is assigned 4711. With both halves, a pid that is live but started at a different time is conclusively not the process that wrote the record.
cancel_requestedonRunRecord. The durable half of cancellation. An in-memory registry only reaches runs in its own process, so a cancel aimed elsewhere is written onto the record and the owning process picks it up. Never cleared — a run that was asked to stop and then settled should still say it was asked.WorkflowStore::unsettled_runs(). What a reconciliation sweep reads. It spans every workflow, because "which records still claim to be live" is a question about the whole scope; doing it throughlist_runswould re-read the runs directory once per workflow. Defaults to empty, so a store that cannot enumerate its runs is one a sweep skips rather than one that fails to compile.FileWorkflowStoreimplements it, andlist_runsnow shares the newread_run_dirhelper rather than keeping its own copy of the directory walk.Compatibility
Both fields are additive with
#[serde(default)], so run files written by older builds still parse. A record with no executor reads as unowned — correct, and exactly the backlog the host-side sweep clears.Testing
Four new tests in
src/store/tests/runs.rs:unsettled_runsspans both workflows, orders newest first, and excludes all four settled statuses plus includespending_approvalPlus the existing
RunRecordwire tests updated for the new fields.Commands run locally:
cargo fmt --all -- --check— cleancargo clippy --all-targets— cleancargo test— 1104 lib tests green, full suite greenSummary by CodeRabbit
New Features
Bug Fixes