Skip to content

Harden daemon: socket permissions, env safety, telemetry, rewrite locking#1029

Closed
jwiegley wants to merge 1 commit into
mainfrom
johnw/review-harden-daemon
Closed

Harden daemon: socket permissions, env safety, telemetry, rewrite locking#1029
jwiegley wants to merge 1 commit into
mainfrom
johnw/review-harden-daemon

Conversation

@jwiegley
Copy link
Copy Markdown
Contributor

@jwiegley jwiegley commented Apr 9, 2026

  • Set umask(077) before creating control/trace sockets to prevent
    TOCTOU race with subsequent chmod
  • Set daemon directory permissions to 0700
  • Move env var sanitization before tokio runtime build to avoid
    unsafe env modification from worker threads
  • Track dropped telemetry envelopes and CAS records via atomic
    counters, expose in FamilyStatus
  • Make watermark update a confirmed operation via oneshot channel
  • Scope watermark pruning to the correct worktree prefix
  • Add file locking for rewrite log read-modify-write cycles

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

@jwiegley jwiegley force-pushed the johnw/review-security-fixes branch from bbff7f0 to eb75b9b Compare April 9, 2026 17:28
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch 2 times, most recently from 79c693b to a0f2607 Compare April 9, 2026 20:13
@jwiegley jwiegley force-pushed the johnw/review-security-fixes branch from eb75b9b to 897edee Compare April 9, 2026 20:13
@svarlamov svarlamov force-pushed the johnw/review-security-fixes branch from 897edee to b43e12d Compare April 11, 2026 14:50
@svarlamov svarlamov force-pushed the johnw/review-harden-daemon branch from a0f2607 to 9b1e189 Compare April 11, 2026 14:51
@jwiegley jwiegley force-pushed the johnw/review-security-fixes branch from b43e12d to 86f70d0 Compare April 15, 2026 19:52
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch from 9b1e189 to a301b61 Compare April 15, 2026 19:52
@jwiegley jwiegley force-pushed the johnw/review-security-fixes branch from 86f70d0 to 7eaef28 Compare April 15, 2026 20:15
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch from a301b61 to 7a0c335 Compare April 15, 2026 20:15
@jwiegley jwiegley marked this pull request as ready for review April 15, 2026 21:46
@jwiegley jwiegley requested a review from svarlamov April 15, 2026 21:46
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@jwiegley jwiegley changed the base branch from johnw/review-security-fixes to graphite-base/1029 April 16, 2026 06:40
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch from 104cb2d to 499b585 Compare April 16, 2026 06:40
@graphite-app graphite-app Bot changed the base branch from graphite-base/1029 to main April 16, 2026 06:41
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch 4 times, most recently from 6ba559c to f19c873 Compare April 23, 2026 05:39
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch 2 times, most recently from 5b60e7c to c053d28 Compare May 1, 2026 17:06
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch from c053d28 to fad265b Compare May 5, 2026 18:16
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 13 additional findings in Devin Review.

Open in Devin Review

Comment thread src/git/rewrite_log.rs
Comment on lines +579 to +590
fn acquire_rewrite_lock(lock_path: &std::path::Path) -> Option<LockFile> {
for attempt in 0..3 {
if let Some(lock) = LockFile::try_acquire(lock_path) {
return Some(lock);
}
if attempt < 2 {
std::thread::sleep(std::time::Duration::from_millis(100));
}
}
tracing::warn!("Failed to acquire rewrite log lock after 3 attempts, proceeding without lock");
None
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Rewrite log lock falls through on contention, defeating its purpose

The new acquire_rewrite_lock function at src/git/rewrite_log.rs:579-590 retries lock acquisition 3 times (~200ms total), then proceeds without the lock if all attempts fail. This defeats the purpose of the lock in exactly the scenario it's designed to protect against: when another process is performing the read-modify-write cycle. Process A holds the lock and is writing; Process B fails to acquire it 3 times, then proceeds lockless — both processes read the same file content, and one process's event is silently lost when the other overwrites it. The lock should either return an Err to the caller or block with a longer timeout rather than silently proceeding unprotected.

Prompt for agents
The function acquire_rewrite_lock in src/git/rewrite_log.rs:579-590 returns None when the lock cannot be acquired after 3 attempts. The caller at line 537 assigns it to _lock and proceeds with the read-modify-write regardless. This silently downgrades to no-lock mode precisely when another writer holds the lock, creating the race condition the lock was meant to prevent.

Two approaches to fix:
1. Return an error instead of None when the lock cannot be acquired, and propagate it to the caller. This is the safest approach but may cause failures in edge cases.
2. Increase the retry count and/or sleep duration significantly (e.g., 10 retries with 200ms sleep = 2 seconds total) so the fallback is only triggered in truly pathological cases, not during normal concurrent access.

The current 3 attempts with 100ms sleep (200ms total budget) is too tight for the lock to be meaningful — a concurrent writer doing file I/O on a slow filesystem could easily hold the lock for longer than 200ms.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch from fad265b to 4fec027 Compare May 6, 2026 19:37
…king

- Set umask(077) before creating control/trace sockets to prevent
  TOCTOU race with subsequent chmod
- Set daemon directory permissions to 0700
- Move env var sanitization before tokio runtime build to avoid
  unsafe env modification from worker threads
- Track dropped telemetry envelopes and CAS records via atomic
  counters, expose in FamilyStatus
- Make watermark update a confirmed operation via oneshot channel
- Scope watermark pruning to the correct worktree prefix
- Add file locking for rewrite log read-modify-write cycles

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jwiegley jwiegley force-pushed the johnw/review-harden-daemon branch from 4fec027 to c78bc02 Compare May 11, 2026 17:40
@jwiegley jwiegley closed this May 11, 2026
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.

1 participant