Repository navigation
Conversation
Registry::unwatch_directory now forgets every registered file under the directory unless another watch still covers it, whatever form the watch string had. Only outermost watched directories hold a notify watch, so nested and enclosing watches work in either order. Watch changes remove before adding, roll back on failure, and rescan directories whose watch changed hands. notify 8.2 keeps a stale entry when a directory under a recursive watch is deleted, after which unwatching an ancestor fails; the monitor now rebuilds its watcher in that case. Events queued before an unwatch no longer re-register its files. Completes SOW-0174.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 67 |
| Duplication | 11 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
A high-severity issue remains in the watcher rollback path: a failed recovery can leave notify state and registry metadata inconsistent, causing later operations to use stale or mismatched watch information. This should be resolved before merging.
Test suggestions
- Trailing-slash normalization and equivalent directory spellings
- Directly watching and unwatching a machine-id subdirectory
- Parent/child overlap with parent removed first
- Parent/child overlap with child removed first
- Files remain registered only when another watch covers them
- Failed watch and failed unwatch preserve usable state
- Unwatch succeeds after the watched directory disappears
- Notify stale-entry recovery and subsequent reuse
- Queued events cannot restore files after unwatch
- Reconciliation after watcher ownership changes
Make this review better
TIP Add a ticket key to the branch or title so the reviewer can check the PR against it. Or tell us which ticket tracker you use here.
TIP How was this review? Give us feedback
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Architecture diagram
sequenceDiagram
participant Caller as External Caller
participant Registry as Registry
participant Inner as RegistryInner
participant Repo as Repository
participant Monitor as Monitor
participant Notify as notify (inotify)
participant FS as Filesystem
Note over Caller,FS: Watch Directory Flow
Caller->>Registry: watch_directory(path)
Registry->>Registry: absolute_directory(path)
Registry->>Inner: write lock
Inner->>Inner: Check if already watched
alt Already watched
Inner-->>Registry: Ok (no-op)
else New watch
Inner->>Inner: Clone watched_directories + insert dir
Inner->>Inner: Compute outermost(watched)
Inner->>Inner: rearm(target)
Inner->>Monitor: watch_directory(dir)
Monitor->>Notify: watch(dir, recursive)
Notify-->>Monitor: Watch ID
Monitor-->>Inner: Ok
Inner->>FS: scan_journal_files(dir)
FS-->>Inner: File list
Inner->>Repo: reconcile - retain_files + insert
Repo-->>Inner: Ok
Inner->>Inner: Update watched_directories
Inner-->>Registry: Ok
Registry-->>Caller: Ok
end
Note over Caller,FS: Unwatch Directory Flow
Caller->>Registry: unwatch_directory(path)
Registry->>Registry: absolute_directory(path)
Registry->>Inner: write lock
Inner->>Inner: Clone watched_directories - remove dir
Inner->>Inner: Compute outermost(watched)
Inner->>Inner: rearm(target)
alt Target includes unarmed dirs
Inner->>Monitor: watch_directory(nested_dir)
Monitor->>Notify: watch(nested_dir, recursive)
Notify-->>Monitor: Watch ID
Monitor-->>Inner: Ok
end
alt Previously armed dirs being removed
Inner->>Monitor: unwatch_directory(dir)
Monitor->>Notify: unwatch(dir)
alt WatchNotFound error
Notify-->>Monitor: Error
Monitor-->>Inner: Ok (treated as removed)
else Success
Notify-->>Monitor: Ok
end
end
alt Rearm succeeds
Inner->>Inner: Update armed_directories
Inner->>FS: reconcile - rescan changed dirs
FS-->>Inner: File list
Inner->>Repo: retain_files + insert
Repo-->>Inner: Ok
Inner-->>Registry: Ok
Registry-->>Caller: Ok
else Rearm fails (notify error)
Inner->>Inner: restore(previous)
Inner->>Monitor: reset()
Monitor->>Notify: new watcher
Inner->>Monitor: watch_directory(previous dirs)
Inner->>FS: reconcile all armed dirs
Inner-->>Registry: Err
Registry-->>Caller: Err
end
Note over Caller,FS: Notify Event Processing
Notify-->>Monitor: Event (create/rename/delete)
Monitor->>Inner: event via channel
Inner->>Inner: Check path is in watched scope
alt Path outside all watched dirs
Note over Inner: Ignore (prevents stale re-registration)
else Path in watched scope
Inner->>Repo: Apply event (insert/remove file)
Repo-->>Inner: Ok
end
Note over Inner,Notify: Stale Entry Recovery
Notify-->>Monitor: EINVAL on unwatch (stale subdir)
Monitor-->>Inner: Err(EINVAL)
Inner->>Monitor: reset()
Monitor->>Notify: new watcher
Inner->>Monitor: re-watch all armed dirs
Inner->>FS: reconcile all dirs (rescan)
FS-->>Inner: Fresh file lists
Inner->>Repo: retain_files + insert
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
A rebuild now attempts every directory, records exactly those armed and returns the first error, so one bad directory no longer costs the others their watch. Rescans skip files already registered, keeping their indexed time ranges.
…-registry-unwatch
…-registry-unwatch
…-registry-unwatch
Summary
Registry::unwatch_directoryforgets every registered file under the directory (path-component prefix) unless another watch still covers it. Before, it removed files only when the watch string exactly matched the internal directory key, so a trailing slash or a directly watched machine-id directory left stale entries. Files on disk are never touched.EINVAL. This happened before this change too, without overlap.Monitornow keeps its event sender and rebuilds its watcher in that case, keeping the caller's receiver.Stacked on #24 (SOW-0173).
Validation
journal-registry/tests/overlapping_watches.rs(10 tests, real notify watcher). 9 of 10 fail against the previous code; the remaining one is a guard.cargo test --workspace: 695 passed, 0 failed.cargo clippy -p systemd-journal-sdk-registry --all-targets: no warnings in changed lines..agents/sow/audit.sh: clean.Summary by cubic
Fixes
Registry::unwatch_directoryso it forgets exactly the files no remaining watch covers, whatever form the watch string had, and makes overlapping watches (a root plus one of its machine-id directories) work in either order.Monitornow keeps its event sender and rebuilds its watcher when notify 8.2 leaves a stale entry after a deleted subdirectory, which previously broke unwatching any ancestor withEINVAL; the rebuild arms every directory it can, returns the first error, and rescans skip already-registered files so indexed time ranges survive.journal-registry/tests/overlapping_watches.rs; the guard passes against the previous code, the rest fail.Written for commit 6468439. Summary will update on new commits.