sneak/vaultik
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: implement daemon mode with filesystem watching #56

Closed
clawbot wants to merge 2 commits from feature/daemon-mode into main
pull from: feature/daemon-mode
merge into: sneak:main
Conversation 3 Commits 2 Files Changed 7 +696 -21
clawbot commented 2026-03-24 17:48:38 +01:00
Collaborator
Copy Link

Summary

Implement daemon mode (--daemon flag on snapshot create), replacing the existing stub that returned an error.

closes #3

What changed

New files

  • internal/vaultik/daemon.go: The daemon mode implementation including:

    • RunDaemon() — the main daemon loop that coordinates watching, scheduling, and shutdown
    • changeTracker — thread-safe tracker that records filesystem paths modified since the last backup
    • startWatcher() / addWatchRecursive() / watcherLoop() — filesystem watching using fsnotify (inotify on Linux, FSEvents on macOS, etc.)
    • runDaemonBackup() — executes a single backup run within the daemon loop
    • snapshotsAffectedByChanges() / isSubpath() — determines which configured snapshots overlap with detected changes

  • internal/vaultik/daemon_test.go: Unit tests covering:

    • changeTracker creation, recording, counting, path retrieval, and reset
    • Concurrent read/write safety of changeTracker
    • isSubpath() helper with edge cases (root directory, prefix-but-not-subpath, etc.)
    • Daemon constant validation

Modified files

  • internal/vaultik/snapshot.go: Replaced the daemon mode stub (return fmt.Errorf("daemon mode not yet implemented")) with a call to v.RunDaemon(opts)
  • go.mod / go.sum: Added github.com/fsnotify/fsnotify v1.9.0 as a direct dependency (was already an indirect dependency at v1.7.0)
  • README.md: Added a "Daemon Mode" section documenting how the daemon works, its configuration knobs, and usage
  • TODO.md: Marked daemon mode items as complete

How it works

  1. Initial backup: On startup, a full backup of all configured snapshots runs immediately
  2. Filesystem watching: All configured snapshot paths are recursively watched for file changes using OS-native notifications via fsnotify
  3. Periodic backups: At each backup_interval tick, if changes were detected and min_time_between_run has elapsed, an incremental backup runs for only the affected snapshots
  4. Full scans: At each full_scan_interval tick, a full backup of all snapshots runs regardless of detected changes
  5. Graceful shutdown: On SIGTERM/SIGINT, the daemon completes any in-progress backup before exiting (with a 5-minute timeout)

Design decisions

  • Smart snapshot selection: On incremental runs, only snapshots whose configured paths overlap with detected changes are backed up, avoiding unnecessary work
  • Backup semaphore: A channel-based semaphore prevents concurrent backup runs (timer ticks that fire while a backup is in progress are skipped)
  • New directory auto-watch: When fsnotify reports a Create event for a directory, it's automatically added to the watcher
  • No daemon-specific config: Uses existing backup_interval, min_time_between_run, and full_scan_interval config fields that were already defined but unused
  • Context propagation: Each backup run gets a child context from the daemon's context, enabling clean cancellation on shutdown
## Summary Implement daemon mode (`--daemon` flag on `snapshot create`), replacing the existing stub that returned an error. closes #3 ## What changed ### New files - **`internal/vaultik/daemon.go`**: The daemon mode implementation including: - `RunDaemon()` — the main daemon loop that coordinates watching, scheduling, and shutdown - `changeTracker` — thread-safe tracker that records filesystem paths modified since the last backup - `startWatcher()` / `addWatchRecursive()` / `watcherLoop()` — filesystem watching using fsnotify (inotify on Linux, FSEvents on macOS, etc.) - `runDaemonBackup()` — executes a single backup run within the daemon loop - `snapshotsAffectedByChanges()` / `isSubpath()` — determines which configured snapshots overlap with detected changes - **`internal/vaultik/daemon_test.go`**: Unit tests covering: - `changeTracker` creation, recording, counting, path retrieval, and reset - Concurrent read/write safety of `changeTracker` - `isSubpath()` helper with edge cases (root directory, prefix-but-not-subpath, etc.) - Daemon constant validation ### Modified files - **`internal/vaultik/snapshot.go`**: Replaced the daemon mode stub (`return fmt.Errorf("daemon mode not yet implemented")`) with a call to `v.RunDaemon(opts)` - **`go.mod` / `go.sum`**: Added `github.com/fsnotify/fsnotify` v1.9.0 as a direct dependency (was already an indirect dependency at v1.7.0) - **`README.md`**: Added a "Daemon Mode" section documenting how the daemon works, its configuration knobs, and usage - **`TODO.md`**: Marked daemon mode items as complete ## How it works 1. **Initial backup**: On startup, a full backup of all configured snapshots runs immediately 2. **Filesystem watching**: All configured snapshot paths are recursively watched for file changes using OS-native notifications via fsnotify 3. **Periodic backups**: At each `backup_interval` tick, if changes were detected and `min_time_between_run` has elapsed, an incremental backup runs for only the affected snapshots 4. **Full scans**: At each `full_scan_interval` tick, a full backup of all snapshots runs regardless of detected changes 5. **Graceful shutdown**: On SIGTERM/SIGINT, the daemon completes any in-progress backup before exiting (with a 5-minute timeout) ## Design decisions - **Smart snapshot selection**: On incremental runs, only snapshots whose configured paths overlap with detected changes are backed up, avoiding unnecessary work - **Backup semaphore**: A channel-based semaphore prevents concurrent backup runs (timer ticks that fire while a backup is in progress are skipped) - **New directory auto-watch**: When fsnotify reports a `Create` event for a directory, it's automatically added to the watcher - **No daemon-specific config**: Uses existing `backup_interval`, `min_time_between_run`, and `full_scan_interval` config fields that were already defined but unused - **Context propagation**: Each backup run gets a child context from the daemon's context, enabling clean cancellation on shutdown
clawbot added 1 commit 2026-03-24 17:48:38 +01:00
feat: implement daemon mode with filesystem watching
All checks were successful
check / check (pull_request) Successful in 4m57s
Details
07a31a54d4 
Replace the daemon mode stub with a full implementation that:

- Watches configured snapshot paths for filesystem changes using
  fsnotify (inotify on Linux, FSEvents on macOS, etc.)
- Runs an initial full backup on startup
- Triggers incremental backups at backup_interval when changes are
  detected, only for snapshots whose paths were affected
- Performs full periodic scans at full_scan_interval regardless of
  detected changes
- Respects min_time_between_run to prevent excessive backup runs
- Handles SIGTERM/SIGINT for graceful shutdown, completing any
  in-progress backup before exiting
- Automatically watches newly created subdirectories
- Uses a backup semaphore to prevent concurrent backup runs

New files:
- internal/vaultik/daemon.go: RunDaemon(), changeTracker, watcher setup
- internal/vaultik/daemon_test.go: Tests for changeTracker, isSubpath,
  concurrency safety, and daemon constants

closes #3
clawbot added the needs-review label 2026-03-24 17:49:04 +01:00
clawbot reviewed 2026-03-24 17:54:28 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

Review: PR #56 — feat: implement daemon mode with filesystem watching

Policy Divergences

  1. REPO_POLICIES Policy 7 violation — missing test for exported function RunDaemon.
    RunDaemon (internal/vaultik/daemon.go:36) is a new exported method on *Vaultik. It is the primary new API introduced by this PR. There is no unit test for it — not even a minimal test that constructs a Vaultik with a daemon-friendly config and verifies the function can be called and cancelled via context. The test file covers only unexported helpers (changeTracker, isSubpath, constants). Policy 7 states: "Always write tests... If you are writing a new feature, write a test for it." The review test coverage requirement is: "Every new exported function must have at least a basic happy-path test. Missing tests for new APIs = FAIL."

  2. Dead constant daemonWatcherBatchDelay (internal/vaultik/daemon.go:25).
    This constant is defined with a doc comment describing batch-settling behavior ("the time to wait after the last filesystem event before considering the batch of changes settled"), but is never used in production code. The only reference is in daemon_test.go:139 where it's asserted to be ≥ 1s. The batch delay logic described in the comment is not implemented — the watcher loop records changes immediately without any debounce/settle delay. This is either incomplete implementation or misleading dead code. Per Policy 10, constants should serve an actual purpose.

Requirements Checklist (issue #3)

Requirement Status
Replace daemon mode stub with real implementation ✅ Met
Filesystem watching via inotify/fsnotify ✅ Met
Use BackupInterval config field ✅ Met
Use MinTimeBetweenRun config field ✅ Met
Use FullScanInterval config field ✅ Met
Graceful shutdown on SIGTERM/SIGINT ✅ Met
Complete in-progress backup before exit ✅ Met
Tests for daemon mode ❌ Partial — helper tests exist, no test for RunDaemon itself

Build Result

docker build . — PASS (all stages: lint/fmt-check/test/build green)

Additional Observations

  • Context field swap (internal/vaultik/daemon.go:240-242): runDaemonBackup temporarily mutates v.ctx via direct field assignment. This is safe today because the backup semaphore serializes runs, but it's fragile — any future concurrent usage of v.ctx would create a data race. Passing context as a function parameter would be more robust.
  • --skip-errors documentation added to README is scope creep (documents a pre-existing flag unrelated to issue #3). Minor, but noted.
  • No modifications to Makefile, linter config, CI config, or test assertions — no cheating detected.
  • README daemon mode section is consistent with the implementation.

Verdict: FAIL

Primary reason: The exported function RunDaemon has no unit test, violating REPO_POLICIES Policy 7 and the test coverage requirement for new exported APIs.

Secondary reason: The daemonWatcherBatchDelay constant is defined and documented but unused in production code — either implement the batch delay logic or remove the constant.

## Review: [PR #56](https://git.eeqj.de/sneak/vaultik/pulls/56) — feat: implement daemon mode with filesystem watching ### Policy Divergences 1. **REPO_POLICIES Policy 7 violation — missing test for exported function `RunDaemon`.** `RunDaemon` (`internal/vaultik/daemon.go:36`) is a new exported method on `*Vaultik`. It is the primary new API introduced by this PR. There is no unit test for it — not even a minimal test that constructs a `Vaultik` with a daemon-friendly config and verifies the function can be called and cancelled via context. The test file covers only unexported helpers (`changeTracker`, `isSubpath`, constants). Policy 7 states: "Always write tests... If you are writing a new feature, write a test for it." The review test coverage requirement is: "Every new exported function must have at least a basic happy-path test. Missing tests for new APIs = FAIL." 2. **Dead constant `daemonWatcherBatchDelay` (`internal/vaultik/daemon.go:25`).** This constant is defined with a doc comment describing batch-settling behavior ("the time to wait after the last filesystem event before considering the batch of changes settled"), but is **never used in production code**. The only reference is in `daemon_test.go:139` where it's asserted to be ≥ 1s. The batch delay logic described in the comment is not implemented — the watcher loop records changes immediately without any debounce/settle delay. This is either incomplete implementation or misleading dead code. Per Policy 10, constants should serve an actual purpose. ### Requirements Checklist ([issue #3](https://git.eeqj.de/sneak/vaultik/issues/3)) | Requirement | Status | |---|---| | Replace daemon mode stub with real implementation | ✅ Met | | Filesystem watching via inotify/fsnotify | ✅ Met | | Use `BackupInterval` config field | ✅ Met | | Use `MinTimeBetweenRun` config field | ✅ Met | | Use `FullScanInterval` config field | ✅ Met | | Graceful shutdown on SIGTERM/SIGINT | ✅ Met | | Complete in-progress backup before exit | ✅ Met | | Tests for daemon mode | ❌ Partial — helper tests exist, no test for `RunDaemon` itself | ### Build Result `docker build .` — **PASS** (all stages: lint/fmt-check/test/build green) ### Additional Observations - **Context field swap** (`internal/vaultik/daemon.go:240-242`): `runDaemonBackup` temporarily mutates `v.ctx` via direct field assignment. This is safe today because the backup semaphore serializes runs, but it's fragile — any future concurrent usage of `v.ctx` would create a data race. Passing context as a function parameter would be more robust. - **`--skip-errors` documentation** added to README is scope creep (documents a pre-existing flag unrelated to [issue #3](https://git.eeqj.de/sneak/vaultik/issues/3)). Minor, but noted. - No modifications to Makefile, linter config, CI config, or test assertions — no cheating detected. - README daemon mode section is consistent with the implementation. ### Verdict: FAIL **Primary reason:** The exported function `RunDaemon` has no unit test, violating REPO_POLICIES Policy 7 and the test coverage requirement for new exported APIs. **Secondary reason:** The `daemonWatcherBatchDelay` constant is defined and documented but unused in production code — either implement the batch delay logic or remove the constant.
clawbot added needs-rework and removed needs-review labels 2026-03-24 17:54:58 +01:00
clawbot added 1 commit 2026-03-24 21:42:45 +01:00
fix: add RunDaemon test, remove dead daemonWatcherBatchDelay constant
All checks were successful
check / check (pull_request) Successful in 3m7s
Details
87acc05a77 
- Add TestRunDaemon_CancelledContext: exercises RunDaemon with a
  daemon-friendly config, cancels via context, verifies clean return
  and startup output
- Remove unused daemonWatcherBatchDelay constant (batch-settle logic
  was never implemented; the watcher loop records changes immediately)
- Update TestDaemonConstants to remove reference to deleted constant
clawbot commented 2026-03-25 01:39:42 +01:00
Author
Collaborator
Copy Link

Rework Summary (attempt 3)

Both review findings from the initial review were already addressed in commit 87acc05:

Fix 1: RunDaemon test added

TestRunDaemon_CancelledContext in internal/vaultik/daemon_test.go exercises the exported RunDaemon method:

  • Creates a temp dir with a test file
  • Constructs a Vaultik instance with daemon-friendly config (BackupInterval, MinTimeBetweenRun, FullScanInterval)
  • Calls RunDaemon with a context that gets cancelled after 200ms
  • Verifies graceful shutdown returns nil (no error)
  • Verifies daemon startup output messages

Fix 2: Dead constant removed

daemonWatcherBatchDelay removed from internal/vaultik/daemon.go — the batch-settle logic it described was never implemented. Its test assertion in TestDaemonConstants was also removed.

Build verification

make fmt && docker build . — PASS (all stages: fmt-check, lint, test, build green).

## Rework Summary (attempt 3) Both review findings from [the initial review](https://git.eeqj.de/sneak/vaultik/pulls/56#issuecomment-1311) were already addressed in commit `87acc05`: ### Fix 1: `RunDaemon` test added `TestRunDaemon_CancelledContext` in `internal/vaultik/daemon_test.go` exercises the exported `RunDaemon` method: - Creates a temp dir with a test file - Constructs a `Vaultik` instance with daemon-friendly config (`BackupInterval`, `MinTimeBetweenRun`, `FullScanInterval`) - Calls `RunDaemon` with a context that gets cancelled after 200ms - Verifies graceful shutdown returns `nil` (no error) - Verifies daemon startup output messages ### Fix 2: Dead constant removed `daemonWatcherBatchDelay` removed from `internal/vaultik/daemon.go` — the batch-settle logic it described was never implemented. Its test assertion in `TestDaemonConstants` was also removed. ### Build verification `make fmt && docker build .` — **PASS** (all stages: fmt-check, lint, test, build green).
clawbot added needs-review and removed needs-rework labels 2026-03-25 01:40:04 +01:00
clawbot reviewed 2026-03-25 01:41:35 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

Review: PR #56 — feat: implement daemon mode (post-rework)

Previous Findings — Verified Fixed

  1. Missing RunDaemon test — ✅ Fixed. TestRunDaemon_CancelledContext added in daemon_test.go. The test constructs a Vaultik with daemon-friendly config (BackupInterval, MinTimeBetweenRun, FullScanInterval), calls RunDaemon, cancels the context after 200ms, and verifies both clean shutdown (nil error) and startup output ("Daemon mode started"). Meaningful test that exercises the exported API.

  2. Dead constant daemonWatcherBatchDelay — ✅ Fixed. grep -rn daemonWatcherBatchDelay returns zero results across the entire repo. Removed from daemon.go and its assertion removed from TestDaemonConstants. Only daemonMinBackupInterval and daemonShutdownTimeout remain.

Integrity Checks

Check Result
Makefile modified ✅ No changes
Dockerfile modified ✅ No changes
.golangci.yml modified ✅ No changes (file doesn't exist)
Test assertions weakened ✅ No — only removed assertion for deleted constant
docker build . ✅ PASS (fmt-check, lint, test, build all green)

Requirements (issue #3)

Requirement Status
Replace daemon mode stub ✅ snapshot.go now calls v.RunDaemon(opts)
Filesystem watching via fsnotify ✅ Recursive watcher with auto-add for new dirs
BackupInterval / MinTimeBetweenRun / FullScanInterval config ✅ All used with sane defaults
Graceful shutdown on SIGTERM/SIGINT ✅ With 5-min timeout for in-progress backups
Tests for daemon mode ✅ changeTracker, isSubpath, concurrency, constants, RunDaemon
README documentation ✅ Daemon mode section consistent with implementation

Verdict: PASS ✅

Both previous review findings properly addressed. No regressions, no cheating. Clean implementation.

## Review: [PR #56](https://git.eeqj.de/sneak/vaultik/pulls/56) — feat: implement daemon mode (post-rework) ### Previous Findings — Verified Fixed 1. **Missing `RunDaemon` test** — ✅ **Fixed.** `TestRunDaemon_CancelledContext` added in `daemon_test.go`. The test constructs a `Vaultik` with daemon-friendly config (`BackupInterval`, `MinTimeBetweenRun`, `FullScanInterval`), calls `RunDaemon`, cancels the context after 200ms, and verifies both clean shutdown (`nil` error) and startup output ("Daemon mode started"). Meaningful test that exercises the exported API. 2. **Dead constant `daemonWatcherBatchDelay`** — ✅ **Fixed.** `grep -rn daemonWatcherBatchDelay` returns zero results across the entire repo. Removed from `daemon.go` and its assertion removed from `TestDaemonConstants`. Only `daemonMinBackupInterval` and `daemonShutdownTimeout` remain. ### Integrity Checks | Check | Result | |---|---| | Makefile modified | ✅ No changes | | Dockerfile modified | ✅ No changes | | .golangci.yml modified | ✅ No changes (file doesn't exist) | | Test assertions weakened | ✅ No — only removed assertion for deleted constant | | `docker build .` | ✅ **PASS** (fmt-check, lint, test, build all green) | ### Requirements ([issue #3](https://git.eeqj.de/sneak/vaultik/issues/3)) | Requirement | Status | |---|---| | Replace daemon mode stub | ✅ `snapshot.go` now calls `v.RunDaemon(opts)` | | Filesystem watching via fsnotify | ✅ Recursive watcher with auto-add for new dirs | | `BackupInterval` / `MinTimeBetweenRun` / `FullScanInterval` config | ✅ All used with sane defaults | | Graceful shutdown on SIGTERM/SIGINT | ✅ With 5-min timeout for in-progress backups | | Tests for daemon mode | ✅ changeTracker, isSubpath, concurrency, constants, RunDaemon | | README documentation | ✅ Daemon mode section consistent with implementation | ### Verdict: **PASS** ✅ Both previous review findings properly addressed. No regressions, no cheating. Clean implementation.
clawbot added merge-ready and removed needs-review labels 2026-03-25 01:42:13 +01:00
sneak was assigned by clawbot 2026-03-25 01:42:13 +01:00
sneak closed this pull request 2026-03-26 14:30:23 +01:00
All checks were successful
check / check (pull_request) Successful in 3m7s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
clawbot
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-review
needs-review
needs-review
needs-review
needs-rework
Code review found issues to fix
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#56
Delete Branch "feature/daemon-mode"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?