fix(state): throttle repeated VACUUM rewrites - #67351
Conversation
6c1716a to
df6bb05
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the expensive full-database rewrite. The repeated-VACUUM premise is real on current main: hermes_state.py:7609-7612 invokes VACUUM after every eligible pruning sweep, and the PR's success-only last_vacuum write is the right retry shape.
Problems
- The new 30-day threshold is only a hard-coded
SessionDBdefault. Current entrypoints pass no corresponding setting (cli.py:1860-1865;gateway/run.py:3309-3314), whileAGENTS.md:102-105requires behavioral thresholds inconfig.yaml. It also makes the current documentation inaccurate:website/docs/user-guide/sessions.md:692says every successful prune is followed byVACUUM. - The added tests cover successful recording, recent-marker suppression, and expiry, but not the stated failed-VACUUM retry guarantee. Add a raising
vacuum()case that verifies nolast_vacuummarker is written and a later due call retries.
Suggested changes
- Add and document
sessions.min_vacuum_interval_days: 30, then pass it from both startup callers. - Add the failed-VACUUM regression test.
Automated hermes-sweeper review.
| @@ -7564,12 +7564,16 @@ def maybe_auto_prune_and_vacuum( | |||
| min_interval_hours: int = 24, | |||
| vacuum: bool = True, | |||
| sessions_dir: Optional[Path] = None, | |||
| min_vacuum_interval_days: int = 30, | |||
There was a problem hiding this comment.
Please surface this behavioral threshold through sessions configuration and wire it from the CLI and gateway callers. Current callers only pass min_interval_hours, and the existing docs promise VACUUM after every pruning sweep; a hard-coded 30-day product behavior leaves both configuration and documentation out of sync.
There was a problem hiding this comment.
Addressed in 393fd1cf8: added sessions.min_vacuum_interval_days: 30 to DEFAULT_CONFIG, wired it through both CLI and gateway startup callers, updated the English and Chinese session docs/config examples, and added a failed-VACUUM regression proving the success marker remains unset and the next due call retries. Focused validation now passes 379 tests plus Ruff, py_compile, and git diff --check.
There was a problem hiding this comment.
Rebase note: upstream main advanced after the follow-up push, so the branch was rebased onto 5854aad8b and revalidated (379 tests plus Ruff, py_compile, and diff check). The review-response commit is now 4bbf0ce66; behavior is unchanged from the earlier referenced SHA.
393fd1c to
4bbf0ce
Compare
|
Merged via #76839 — thank you @nkreadly07. Both your commits were cherry-picked, so you remain the author in git history. The fix was correct and complete as written: the The only salvage work was mechanical retargeting: your branch predated the |
Summary
state.dbVACUUM rewrites independently from the normal auto-prune cadencesessions.min_vacuum_interval_dayswith a 30-day defaultstate_metaVACUUMrewrites the complete SQLite database and holds an exclusive lock. Auto-maintenance may run daily across long-lived entrypoints, so it should not repeat that expensive rewrite merely because rows were pruned during another routine sweep. This keeps pruning cadence unchanged while limiting VACUUM independently through normalconfig.yamlbehavior.Related: #54189
Verification
uv run --extra dev pytest tests/test_hermes_state.py tests/test_session_vacuum_config.py -q --tb=short— 379 passeduv run --extra dev ruff check hermes_state.py cli.py gateway/run.py hermes_cli/config.py tests/test_hermes_state.py tests/test_session_vacuum_config.py— passeduv run python -m py_compile hermes_state.py cli.py gateway/run.py hermes_cli/config.py tests/test_hermes_state.py tests/test_session_vacuum_config.py— passedgit diff --check origin/main...HEAD— passedReview follow-up
Addressed the sweeper feedback in
4bbf0ce66:sessions.min_vacuum_interval_days: 30toDEFAULT_CONFIGNotes
main(5854aad8b) after the review follow-upconfig.yamllast_vacuummarker is written only afterVACUUMsucceeds