Harden machine staging cleanup - #4
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
![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: a6adba7d5c
ℹ️ 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".
| completed = subprocess.run( | ||
| ["fuser", "-s", str(path)], |
There was a problem hiding this comment.
When any staging candidate exists, this unconditionally executes fuser, but neither the default Nix dev shell nor the packaged runtime in flake.nix includes psmisc (which provides that executable). In a clean supported environment the cleanup therefore raises FileNotFoundError before deleting or returning its JSON receipt; the tests conceal this by replacing _path_is_open. Add the runtime dependency or perform the probe without an undeclared external command.
AGENTS.md reference: AGENTS.md:L123-L131
Useful? React with 👍 / 👎.
| pool = "normal" | ||
| scratch = "nvme" | ||
| cache = "none" | ||
| exclusive_keys = ["lynchpin:machine-carrier-staging"] |
There was a problem hiding this comment.
When apply-mode cleanup uses a grace period shorter than an in-progress materialization, this key does not conflict with the lynchpin:canonical-products key used by promote_incremental and promote_full, so AgentCTL may run them concurrently. _manifest.atomic_text_writer closes its temporary file immediately before os.replace; cleanup can probe during that closed-but-still-active interval, see no open descriptor, unlink the file, and make the promotion fail. Use a shared exclusive key or an actual writer/cleanup lock rather than relying solely on fuser rechecks.
Useful? React with 👍 / 👎.
| if grace_period_s < 0: | ||
| raise ValueError("machine staging grace period must be non-negative") |
There was a problem hiding this comment.
If the CLI receives --grace-period-s nan, argparse accepts it as a float and this validation does not reject it because comparisons with NaN are false. The later age_seconds < grace_period_s check is also false for every candidate, so --apply treats even newly created closed staging files as stale and deletes them. Require a finite, non-negative value before scanning.
Useful? React with 👍 / 👎.
| [operations.cleanup_machine_staging] | ||
| description = "Preview or remove abandoned machine-carrier staging files through the owning materializer" | ||
| exec = ["python", "-m", "lynchpin.ingest.machine_materialize", "--cleanup-staging"] |
There was a problem hiding this comment.
This introduces a new operator-facing AgentCTL maintenance contract, including destructive apply mode and grace-period semantics, but the commit does not update the Development Workflow operation list or another owning document. The supported cleanup route is therefore absent from the repository's canonical operational guidance; document the command, its dry-run default, apply behavior, and receipt output alongside the other declared operations.
AGENTS.md reference: AGENTS.md:L192-L193
Useful? React with 👍 / 👎.
1 participant
Recheck open-file ownership at the deletion boundary and cover the race with a regression test. Exact-head full check: 678a19dd-e230-4931-ac12-9b84e8cf0a7f.