fix: retain terminal bundles with active leases - #366
codeforester wants to merge 2 commits into
Conversation
| lease_state = _run_lease_state(child) | ||
| lease_path = child / _RUN_LEASE_NAME | ||
| lease_present = lease_path.exists() or lease_path.is_symlink() | ||
| if lease_state == "active" or (lease_present and lease_state == "unknown"): |
There was a problem hiding this comment.
Confirmed regression: the just-finished run's own bundle vanishes from the diagnostic index on every invocation.
This check now applies the lease-active exclusion to every status, not just running. But RunRecorder.finish() (lib/python/base_cli/_lifecycle.py:64) calls refresh_run_bundle_index() right after writing terminal metadata — while context._run_lease is still held (it's only released later, at the end of context.cleanup()). At that moment _run_lease_state() reports active for the process's own just-terminated bundle, so _discover_run_bundles filters it out entirely, and since this call site never passes current_run_root to _write_run_index, the bundle isn't re-added either.
Reproduced directly: a single normal invocation (no concurrency, no slow cleanup hook needed) leaves .base-cli-run-index.json as {"bundles": [], ...} even though the bundle directory exists on disk with status: ok. The index only self-heals on the next prune/refresh call, so a one-off invocation can permanently ship an index missing its own run.
Failure scenario: run any base-cli app once; read <owner_root>/runs/.base-cli-run-index.json right after — it reports zero bundles despite the completed run's directory being present.
| retention never removes a bundle whose lease is active. A stale `running` | ||
| bundle holds an advisory `.base-cli-run-lease` through final cleanup, including | ||
| the brief period after metadata becomes terminal; retention never removes a | ||
| bundle whose lease is active or whose liveness cannot be established. A stale `running` |
There was a problem hiding this comment.
This new sentence overstates the guarantee: "retention never removes a bundle whose lease is active or whose liveness cannot be established." But the second commit in this PR ("keep lease-less terminal bundles eligible") deliberately does the opposite for a terminal bundle with no lease file at all — liveness there is 'unknown' (cannot be established), yet the bundle IS removed (that's the whole point of that commit, and is exercised by tests/test_adversarial_regressions.py::MultiprocessingRegressionTests::test_run_bundle_retention_remains_bounded_across_processes, which creates lease-less "ok" bundles and asserts they get pruned).
So "liveness cannot be established" only fails closed when a lease file is present but unreadable/unsupported — not when it's simply absent on a terminal bundle. Worth tightening the wording so it doesn't contradict the lease-less-terminal-bundle behavior this PR just added.
| lease_state = _run_lease_state(path) | ||
| lease_path = path / _RUN_LEASE_NAME | ||
| lease_present = lease_path.exists() or lease_path.is_symlink() | ||
| if lease_state == "active" or (lease_present and lease_state == "unknown"): |
There was a problem hiding this comment.
This 4-line lease-active/present-unknown check is duplicated verbatim from _discover_run_bundles (lines 515-518). Worth factoring into one helper, e.g. _lease_blocks_removal(path) -> bool, used by both _discover_run_bundles and _bundle_is_still_removable. As written, any future tweak to this policy (e.g. handling a new lease state, or fixing the redundant exists()/is_symlink() recompute noted in the sibling comment) has to be made twice, and it would be easy for the two copies to drift out of sync.
| # running records or a present but unreadable lease. | ||
| lease_state = _run_lease_state(child) | ||
| lease_path = child / _RUN_LEASE_NAME | ||
| lease_present = lease_path.exists() or lease_path.is_symlink() |
There was a problem hiding this comment.
Minor efficiency nit: _run_lease_state() already calls lease_path.is_symlink() and lease_path.is_file() internally to decide 'unknown'. This line then redoes an exists()/is_symlink() stat on the same path right after, for every bundle scanned (previously this lease check — and its stats — only ran for running bundles; now it runs for every bundle in the runs directory on every retention pass). Returning a bit from _run_lease_state (or inlining the presence check there) would avoid the duplicate syscalls per bundle.
1 participant
Summary
Closes #354
Validation
uv run --extra dev --extra typer --extra quality python -m pytest tests/test_run_bundle_retention.py tests/test_app_run_metadata.py::AppRunMetadataTests::test_terminal_run_lease_survives_a_concurrent_invocation_during_cleanup -q_runtime.pygit diff --check