Skip to content

perf: avoid state file frequently write to disk#446

Merged
jdx merged 2 commits into
jdx:mainfrom
gaojunran:perf-state-file
May 26, 2026
Merged

perf: avoid state file frequently write to disk#446
jdx merged 2 commits into
jdx:mainfrom
gaojunran:perf-state-file

Conversation

@gaojunran

Copy link
Copy Markdown
Contributor

No description provided.

@greptile-apps

greptile-apps Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces a dirty-flag + TOML content-deduplication strategy to batch and skip redundant state-file disk writes. State mutations now go through typed helper methods that set an AtomicBool dirty flag, a 1-second background flush task drains dirty state, and write() compares the serialized TOML string against a cached last_content to skip I/O when nothing has actually changed.

  • StateFile gains dirty: AtomicBool and last_content: Mutex<Option<String>>, with a family of mutation methods (insert_daemon, set_active_port, retain_daemons, etc.) that set the flag only when the state actually changes.
  • A start_state_flush_task spawns a 1-second ticker with a CancellationToken; shutdown cancels the task and force-flushes any remaining dirty state before IPC teardown.
  • handle_ipc now calls flush_state().await after every request so CLI clients can immediately read consistent state from disk; the cron last_cron_triggered write is kept synchronous to close the crash-restart double-fire window flagged in a previous review.

Confidence Score: 5/5

Safe to merge — the flush logic is correct, all mutation paths mark dirty, the background task shuts down cleanly, and the cron crash-safety window is preserved.

The dirty-flag and content-deduplication paths are self-consistent: write() holds the file lock before serializing, compares against cached content, and only skips I/O when the TOML is byte-identical. The shutdown sequence cancels the background task before the force-flush, so the second flush is a no-op via content comparison. All previously raised concerns are addressed in this revision.

No files require special attention.

Important Files Changed

Filename Overview
src/state_file.rs Core refactor: adds AtomicBool dirty flag + Mutex-guarded last_content string cache; splits write logic into write()/write_unlocked()/write_raw(); all mutation methods now mark dirty conditionally.
src/supervisor/mod.rs Adds start_state_flush_task() (1s interval, CancellationToken shutdown) and flush_state(); shutdown sequence cancels the task then force-flushes remaining dirty state.
src/supervisor/ipc_handlers.rs Adds flush_state().await after every IPC response so CLI readers see consistent on-disk state; early-return path for Invalid requests correctly bypasses the flush.
src/supervisor/lifecycle.rs Migrates active_port mutation sites to set_active_port/clear_active_port; changes get_mut to get for PID check to satisfy the borrow checker when delegating to &mut self method.
src/supervisor/state.rs Mechanical migration of all direct state map mutations to the new typed helper methods; removes per-operation write() calls.
src/supervisor/watchers.rs Cron last_cron_triggered update now uses set_last_cron_triggered() but retains the immediate synchronous write() call, correctly preserving the crash-safety property noted in previous review.

Reviews (6): Last reviewed commit: "fix(state): fix write() lock ordering an..." | Re-trigger Greptile

Comment thread src/state_file.rs Outdated
Comment thread src/state_file.rs Outdated
Comment thread src/state_file.rs Outdated
Comment thread src/state_file.rs Outdated
Comment thread src/supervisor/mod.rs
@gaojunran

Copy link
Copy Markdown
Contributor Author

@greptileai

Comment thread src/supervisor/watchers.rs Outdated
@gaojunran

Copy link
Copy Markdown
Contributor Author

@greptileai

@gaojunran gaojunran marked this pull request as ready for review May 22, 2026 10:03
@jdx jdx merged commit 22d742a into jdx:main May 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants