Skip to content

feat(config): Implement config watcher and config reloads - #6353

Open
Dav1dde wants to merge 1 commit into
masterfrom
dav1d/reload-config-2
Open

feat(config): Implement config watcher and config reloads#6353
Dav1dde wants to merge 1 commit into
masterfrom
dav1d/reload-config-2

Conversation

@Dav1dde

@Dav1dde Dav1dde commented Sep 3, 2026

Copy link
Copy Markdown
Member

Implements auto reloading of the config, for now only auto reloads health. It's a self contained config section and is already well integrated with the config snapshots.

The health config is only an example this will get more interesting once we have real use-cases we want to support and start integrating arroyo as well as potentially options automator.

Closes: INGEST-1179

@Dav1dde
Dav1dde force-pushed the dav1d/reload-config-2 branch from 12afaa3 to c628aa1 Compare September 3, 2026 18:26
@Dav1dde Dav1dde self-assigned this Sep 3, 2026
@linear-code

linear-code Bot commented Sep 3, 2026

Copy link
Copy Markdown

INGEST-1179

@Dav1dde
Dav1dde force-pushed the dav1d/reload-config-2 branch from c628aa1 to 070c5d5 Compare September 3, 2026 18:35
@Dav1dde
Dav1dde force-pushed the dav1d/reload-config-2 branch from 070c5d5 to ffd5520 Compare September 3, 2026 18:39
@Dav1dde
Dav1dde marked this pull request as ready for review September 3, 2026 18:40
@Dav1dde
Dav1dde requested a review from a team as a code owner September 3, 2026 18:40
Comment on lines +51 to +61
Err(err) => {
relay_log::warn!(
error = &err as &dyn std::error::Error,
"failed to watch configuration file: {}",
to_add.display()
)
}
}
}

self.currently_watched = config.source_files().clone();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Ignoring the result of watcher.unwatch() can lead to state inconsistency between the OS watcher and currently_watched, causing spurious config reloads when the orphaned file is modified.
Severity: MEDIUM

Suggested Fix

Handle the Result from watcher.unwatch(). If the call fails, do not update the internal state self.currently_watched to remove the path. This ensures the internal state remains consistent with the OS-level watcher, allowing subsequent calls to update_watch to retry unwatching the file.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: relay-server/src/services/config_reload.rs#L43-L61

Potential issue: In the `update_watch` function, the result of
`watcher.unwatch(to_remove)` is ignored. The `unwatch` operation can fail due to
OS-level issues, such as the file being deleted or permissions changing. When this
happens, the OS-level file watcher may persist, but the internal state
`self.currently_watched` is updated unconditionally, removing the file from its
tracking. This inconsistency leads to a 'ghost' watcher. If the file is later modified,
the persistent OS watcher triggers an event, causing a spurious and potentially repeated
configuration reload, making the reload service unreliable.

Did we get this right? 👍 / 👎 to inform future reviews.

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ffd5520. Configure here.

}

self.currently_watched = config.source_files().clone();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Config file watches are not re-established

Medium Severity

update_watch only calls watch for paths not already in currently_watched, then copies source_files into that set even when watch fails. After an atomic replace, inotify drops the watch on the old inode, but the path set is unchanged so the new file is never watched. Later config updates are ignored, including Kubernetes ConfigMap swaps and editor/safe-save rewrites.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ffd5520. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant