Interval ("check") trigger for hooks#89
Merged
Merged
Conversation
Build on the hook foundation with a time-driven trigger: fire a volume's hook command on a cadence regardless of change. The motivating use is periodic backup verification — bitrot hits static data, so re-checks must run on a clock, not on an event. - config: optional `interval` on [volumes.X.hook], parsed with the same cadence rules as sync_every/index_every. - store: LatestHookRun(volume, trigger) for the due check. - agent: the scheduler evaluates the interval cadence (after any index/sync this tick — verify the source, then the backup) and fires with SQUIRREL_TRIGGER=interval, reusing the foundation's exec/env/best-effort/don't-stack/timeout path. volumeHasCadence now counts an interval hook, so a volume can want periodic verification with no index/sync schedule of its own. - README: document both triggers and the "don't double-schedule verification" trade-off (squirrel-as-scheduler vs tool-as-scheduler). Closes #86 Part of #84 https://claude.ai/code/session_016huzsmuzXRLEWJSCTMctEa
…el-hooks-interval-UkpNz
There was a problem hiding this comment.
Pull request overview
Adds an interval-driven (“check”) trigger to the existing per-volume hook mechanism so a volume’s configured hook command can run on a fixed cadence even when no content changes occurred, with outcomes recorded alongside existing hook runs.
Changes:
- Extend volume hook config with optional
intervalparsing/validation (reusing cadence rules). - Add
Store.LatestHookRun(volume, trigger)and scheduler logic to compute interval due-ness and fire interval hooks. - Update docs and tests to cover interval parsing, latest-run lookup, and scheduler firing behavior independent of indexing.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
store/hookruns.go |
Adds LatestHookRun to fetch the most recent hook run per (volume, trigger) for cadence checks. |
store/hookruns_test.go |
Adds unit test coverage for LatestHookRun. |
config/config.go |
Adds hook.interval to the resolved config and parses it using existing cadence parsing rules. |
config/hook_test.go |
Adds tests for interval parsing, defaulting, and invalid interval errors. |
agent/scheduler.go |
Adds interval-hook scheduling, due calculation, and counts interval hooks as a cadence that starts the scheduler. |
agent/hooks_test.go |
Adds scheduler integration test for interval hook firing cadence and recorded fields. |
README.md |
Documents the two hook triggers (change + interval) and the double-scheduling trade-off. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+171
to
+175
| SELECT `+hookRunColumns+` | ||
| FROM hook_runs | ||
| WHERE volume_id = ? AND trigger = ? | ||
| ORDER BY id DESC LIMIT 1 | ||
| `, volumeID, trigger) |
Comment on lines
+163
to
+165
| // LatestHookRun returns the most recent hook run (by id) for the given | ||
| // (volume, trigger), or sql.ErrNoRows when none exists. The interval-hook | ||
| // scheduler computes `now - last_run` from this row to decide whether a |
Comment on lines
+161
to
+163
| _, _ = s.BeginHookRun(ctx, HookRunSpec{VolumeID: vol.ID, Trigger: HookTriggerInterval}) | ||
| _, _ = s.BeginHookRun(ctx, HookRunSpec{VolumeID: vol.ID, Trigger: HookTriggerChange}) | ||
| latestInterval, _ := s.BeginHookRun(ctx, HookRunSpec{VolumeID: vol.ID, Trigger: HookTriggerInterval}) |
Comment on lines
+233
to
+236
| runs, _ := f.store.ListRuns(ctx, store.ListRunsOpts{}) | ||
| if len(runs) != 0 { | ||
| t.Fatalf("interval hook created %d runs rows; want 0 (it never indexes)", len(runs)) | ||
| } |
Comment on lines
+238
to
+239
| hooks, _ := f.store.ListHookRuns(ctx, store.HookRunListOpts{Descending: true}) | ||
| hr := hooks[0] |
The interval trigger added here gives hookRunner.fire a second caller passing store.HookTriggerInterval, so unparam no longer flags the trigger parameter and the suppression added on the foundation branch is no longer needed. https://claude.ai/code/session_016huzsmuzXRLEWJSCTMctEa
- LatestHookRun: order by started_at_ns (then id) so the idx_hook_runs_volume_trigger index satisfies both the filter and the sort — no separate sort pass as hook history grows; update the doc to match. - tests: stop ignoring errors from BeginHookRun / ListRuns / ListHookRuns so a DB/query failure fails fast at the real cause instead of surfacing as a confusing downstream assertion. https://claude.ai/code/session_016huzsmuzXRLEWJSCTMctEa
…el-hooks-interval-UkpNz
…ling Brings #88's review fixes into the interval branch (stderr tail capture, positive-timeout guard, the trigger↔triggering-run coupling) and updates TestLatestHookRun so its change-hook fixture carries a real triggering run as the new coupling now requires. https://claude.ai/code/session_016huzsmuzXRLEWJSCTMctEa
…el-hooks-interval-UkpNz
…el-hooks-interval-UkpNz
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the interval ("check") trigger to the generic hook mechanism — the second sub-issue of the tracker.
What this does
Fires a volume's hook command on a cadence, regardless of whether content changed. Verification is orthogonal to change — bitrot happens to static data — so re-checks have to run on a clock; the on-change trigger would never re-check data that's sitting still, which is exactly the data most at risk. This mirrors squirrel's own internal split (index/change events vs. the periodic deep scan), applied to the external copy.
It reuses the entire shared contract from the foundation — exec without a shell, the
SQUIRREL_*env vars, best-effort, don't-stack, timeout-bounded, generic outcome recorded — with the trigger discriminator set tointerval.Config
intervalis optional and parsed with the same cadence rules assync_every/index_every. Omitting it keeps the hook on-change only.Design points settled
SQUIRREL_TRIGGER).evaluateVolume, after any index/sync this tick — verify the source (HDD1), then nudge the external verify of the backup (HDD2) — but the two cadences stay decoupled: the interval hook fires on its own clock whether or not an index ran.volumeHasCadencenow counts an interval hook on its own, so a volume can want periodic verification with no squirrel index/sync schedule of its own.Changes
config/— optionalintervalon[volumes.X.hook]store/—LatestHookRun(volume, trigger)for the due checkagent/—maybeFireIntervalHook+intervalHookDuein the scheduler;volumeHasCadencehelperREADME.md— both triggers + the double-scheduling trade-offTests cover interval config parsing,
LatestHookRun, and the scheduler firing the interval hook on its cadence (due / not-yet-due / due-again) independent of any index run.go vet ./...clean; no new dependencies.Closes #86
Part of #84