Skip to content

SOW-0174: Overlapping registry watches and exact unwatch - #25

Open
ktsaou wants to merge 5 commits into
sow-0173/rust-registry-relative-dirfrom
sow-0174/rust-registry-unwatch
Open

ktsaou wants to merge 5 commits into
sow-0173/rust-registry-relative-dirfrom
sow-0174/rust-registry-unwatch

Conversation

@ktsaou

@ktsaou ktsaou commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Registry::unwatch_directory forgets 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.
  • Overlapping watches (a root plus one of its machine-id directories) now work in either order. Only outermost watched directories hold a notify watch; nested ones are covered by the recursive watch above them, so notify never sees overlapping watches.
  • Watch changes remove before adding, roll back on failure, and rescan directories whose watch changed hands (events in the gap are lost). A failed watch or unwatch leaves the registry unchanged; no directory can get stuck as watched.
  • notify 8.2 (inotify) keeps a stale entry when a directory under a recursive watch is deleted, after which unwatching any ancestor fails with EINVAL. This happened before this change too, without overlap. Monitor now keeps its event sender and rebuilds its watcher in that case, keeping the caller's receiver.
  • Create and rename events for paths outside every watched directory are ignored, so events queued before an unwatch do not re-register its files.

Stacked on #24 (SOW-0173).

Validation

  • New 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_directory so 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.

  • Unwatch previously removed files only when the watch string exactly matched the internal directory key, leaving stale entries for trailing slashes and directly watched machine-id directories; files on disk are never touched.
  • Only outermost watched directories hold a notify watch, so nested directories are covered by the recursive watch above them.
  • Watch changes remove before adding and roll back on failure, so a failed watch or unwatch leaves the registry unchanged and no directory gets stuck as watched.
  • Monitor now 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 with EINVAL; the rebuild arms every directory it can, returns the first error, and rescans skip already-registered files so indexed time ranges survive.
  • Create and rename events for paths outside every watched directory are ignored, so events queued before an unwatch do not re-register its files.
  • Covered by 11 new tests in 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.

View guided diff Turn on auto-fix

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.
@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f16b04a6-845d-4d5f-91a4-dc0ed8b8f2ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@codacy-production

codacy-production Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 67 complexity · 11 duplication

Metric Results
Complexity 67
Duplication 11

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

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.

Pull Request Overview

⚠️ Resolve issues before merging

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

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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
Loading

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.

This branch has not been deployed

No deployments
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