Repository navigation
Hold the run lock through clean and reset with or without a store - #1716
Merged
Merged
Conversation
clean took the run lock only when store/ already existed, and decided that once, before its confirmation prompt. With no store, a run that started while the prompt waited created the store, took the lock, and then lost its staging and target to the clean. reset_workflow.py --full had the same check-then-act gap. Both also deleted the store with the held lock file in it, which lets a run lock a new file beside it, and which Windows refuses because the file is open. clean and reset --full now take the lock in every case, before asking or deleting, and hold it to the end. They empty the store around the lock file instead of deleting it (clean_directory keep=, clear_store), so the next run locks the same file. Whether there was a store is read before the lock creates the folder, and a store holding only the lock file counts as none, so it is not opened for the batch registry. Tests now read the lock state while clean asks and while each command deletes, assert the lock file is kept, assert a refused clean or reset reclaims no batch record, and assert retry --filter holds the lock for all its work and its dry run takes none. Each fails on a version that takes the lock and releases it early, or takes none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1711 (issue #1699), which merged at its earlier head. A blind review of that head found a defect that is now on main; this PR fixes it. It also adds the missing "held while it works" test for #1713.
Problem
#1711 made
cleanandreset_workflow.py --fulltake the run lock, but only whenstore/already existed, decided once before the prompt. Before #1711,agac clean --alldeletedagent_io/store/while a run was in progress. With #1696 the run lock lives in that folder, so the lock file went with it and a secondagac runthen passed the lock check.cleanalso deleted the live database and the run's staging inputs.reset_workflow.py --fulldeletedstore/the same way.Change
Cleaner: once it has found something to remove, it takes the workflow's run lock in every mode (default,--target,--all), before it asks or deletes anything, and holds it until it ends. It takes it with or without a store folder: the lock creates the folder. It exits withRunInProgressErrorwhile a run or retry holds the lock, and that error is no longer wrapped as "Cleaning failed".AgentManager.clean_directorytakeskeep=: the store is emptied around the held lock file, soclean --allleavesstore/holding only the empty<workflow>.db.lock. The next run locks that same file. Deleting it would let a run lock a new file beside the one still held, and Windows refuses to delete a file held open.cleandoes not open it for the batch registry (which would create a database).reset_workflow.py --full: the same rules: lock taken in every case and held through the wipe (agac'shold_run_lockwhere the framework imports, else the sameflockfrom plainpython3), store cleared around the lock file (clear_store), and only a store that was there before the lock is cleared. Withoutfcntland without the framework (Windows, plainpython3) it warns and goes on, as before. The soft reset is unchanged.cli/run.md,cli/utilities.md,data-io/output-format.md, the script docstring,SKILL.md,skills/ARCHITECTURE.mdand the realtime manifest say thatcleanrefuses during a run once it has something to remove, and that the lock file stays.retry --filter):test_retry_filter_holds_the_run_lock.pyasserts the lock is held for all of_filterand not taken by--dry-run. A lock taken and released at once passed every earlier test; it fails this one.Review history
The version of #1711 that merged took the lock only when
store/already existed, decided once before the prompt. A blind review reproduced the failure: a run that started whilecleanwaited at its prompt created the store, took the lock, and then lost its staging and target. Three wrong versions also passed its tests: lock taken and released before the delete, lock taken after the prompt, and the script releasing the lock before the wipe. This PR fixes the first and adds tests that fail on each of the others.Verification
gate.sh redrecordedtests/unit/cli/test_clean_and_reset_hold_the_run_lock.pyfailing on the merged Refuse agac clean and reset_workflow --full during a run #1711 code (13 failed, 3 controls passed): the lock is not held whilecleanasks or deletes with no store,clean --allandreset --fulldelete the lock file, andreset --fulldeletes its last path with no lock file left.gate.sh green: RED target passes,ruff format --check,ruff check,mypy agent_actions, full pytest 17633 passed, 37 skipped.test_a_refused_clean_or_reset_reclaims_nothing.py: aclean --alland areset --fullrefused for a run in progress release no batch record that the store names; the same commands with no run release it (live control).mutate.sh), each fails at least one test:cleanlock released before the delete;cleanwith no lock;keepignored inclean_directory; the script'swith held_run_lockreleased at once; the script with no lock;clear_storeignoring the lock file;retry --filterlock released before_filter. A reviewer also ran: lock after the prompt; no lock whenstore/is missing; script fallback with noflock; dry run taking the lock: all fail.store/is gone afterclean --allorreset --fullnow assert it holds exactly the lock file (== ["wf.db.lock"]), which is stricter.test_clean_all_leaves_no_store_behind_when_there_was_nonekeeps its intent: no database is made.risk.sh, 12 files): three lenses YES. Their items (a lock-only store counted as a store; the script clearing a store that appeared after its check; docs claiming every mode refuses; four docs still saying the store is removed; no test that the batch reclaim runs under the lock) are fixed in this diff, and a fourth reviewer checked that delta: YES. The edits to existing tests replace "store/is gone" with "store/holds exactly the lock file", which is stricter.gate.sh smoke(risk.sh:smoke_required=yes):scan-project.shclean;ql_flow_diagramonline ran 33 of 33 actions, store verified with 70 records and no failed or exhausted disposition.