Skip to content

feat(agent/agentcontainers): add file watcher and dirty status #17573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Apr 29, 2025
+909 −47
Merged

feat(agent/agentcontainers): add file watcher and dirty status #17573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
40c8de9
feat(agent/agentcontainers): add file watcher and dirty status
mafredri Apr 28, 2025
a997f82
make gen
mafredri Apr 28, 2025
0e568fb
fix copilot nits
mafredri Apr 28, 2025
4f788d9
log event string in case not write
mafredri Apr 28, 2025
bfeb1da
fix chmod on macos
mafredri Apr 28, 2025
45a978c
fix close of closed channel
mafredri Apr 28, 2025
d2721a7
add close protection to fsnotify watcher as well
mafredri Apr 28, 2025
c03bb7d
fix typo
mafredri Apr 28, 2025
aed7c30
fix pr suggestions and improve start loop
mafredri Apr 28, 2025
39526e6
update noop impl.
mafredri Apr 28, 2025
7edee90
s/file/path
mafredri Apr 28, 2025
9cf5415
rewrite watcher impl. to watch dirs
mafredri Apr 28, 2025
de2a42e
add rename and create to fsnotify test
mafredri Apr 28, 2025
0cbee38
add atomic file replacement
mafredri Apr 28, 2025
5ced7e1
add paranoid reset
mafredri Apr 28, 2025
5a96a55
fix oopsie
mafredri Apr 28, 2025
0b16448
s/ErrWatcherClosed/ErrClosed/g
mafredri Apr 28, 2025
eaf5922
inverse continue/return for clarity
mafredri Apr 28, 2025
617fed2
better close handling
mafredri Apr 28, 2025
8ee9914
restore removed comment
mafredri Apr 28, 2025
263c683
add note about name
mafredri Apr 28, 2025
58a70e8
fix test setup/clock advancement
mafredri Apr 28, 2025
9f739ca
outdent
mafredri Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix copilot nits
  • Loading branch information
@mafredri
mafredri committed Apr 28, 2025
commit 0e568fb0779596bf1e23c032f0141257e945f6c3
3 changes: 3 additions & 0 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1482,6 +1482,9 @@ func (a *agent) createTailnet(
if err = a.trackGoroutine(func() {
defer apiListener.Close()
apiHandler, closeAPIHAndler := a.apiHandler()
defer func() {
_ = closeAPIHAndler()
}()
server := &http.Server{
BaseContext: func(net.Listener) context.Context { return ctx },
Copy link
Member Author

Choose a reason for hiding this comment

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

Review: This change allows all r.Contexts to inherit the graceful context from the agent, ensuring cancellation on agent shutdown.

Handler: apiHandler,
Expand Down
38 changes: 19 additions & 19 deletions agent/agentcontainers/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ type API struct {

// lockCh protects the below fields. We use a channel instead of a
// mutex so we can handle cancellation properly.
lockCh chan struct{}
containers codersdk.WorkspaceAgentListContainersResponse
mtime time.Time
devcontainerNames map[string]struct{} // Track devcontainer names to avoid duplicates.
knownDevcontainers []codersdk.WorkspaceAgentDevcontainer // Track predefined and runtime-detected devcontainers.
configModifiedTimes map[string]time.Time // Track when config files were last modified.
lockCh chan struct{}
containers codersdk.WorkspaceAgentListContainersResponse
mtime time.Time
devcontainerNames map[string]struct{} // Track devcontainer names to avoid duplicates.
knownDevcontainers []codersdk.WorkspaceAgentDevcontainer // Track predefined and runtime-detected devcontainers.
configFileModifiedTimes map[string]time.Time // Track when config files were last modified.
}

// Option is a functional option for API.
Expand Down Expand Up @@ -108,16 +108,16 @@ func WithWatcher(w watcher.Watcher) Option {
func NewAPI(logger slog.Logger, options ...Option) *API {
ctx, cancel := context.WithCancel(context.Background())
api := &API{
ctx: ctx,
cancel: cancel,
done: make(chan struct{}),
logger: logger,
clock: quartz.NewReal(),
cacheDuration: defaultGetContainersCacheDuration,
lockCh: make(chan struct{}, 1),
devcontainerNames: make(map[string]struct{}),
knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{},
configModifiedTimes: make(map[string]time.Time),
ctx: ctx,
cancel: cancel,
done: make(chan struct{}),
logger: logger,
clock: quartz.NewReal(),
cacheDuration: defaultGetContainersCacheDuration,
lockCh: make(chan struct{}, 1),
devcontainerNames: make(map[string]struct{}),
knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{},
configFileModifiedTimes: make(map[string]time.Time),
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, but if we ever decide to expose this in the API we may want to refactor this to a map[string]codersdk.NullTime

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure we need to store a null time in the map, that can already be represented by missing map entry or zero time. But if we expose it in codersdk types then we can totally use that type there 👍🏻

}
for _, opt := range options {
opt(api)
Expand Down Expand Up @@ -281,7 +281,7 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC
// Check if this container was created after the config
// file was modified.
if configFile != "" && api.knownDevcontainers[knownIndex].Dirty {
lastModified, hasModTime := api.configModifiedTimes[configFile]
lastModified, hasModTime := api.configFileModifiedTimes[configFile]
if hasModTime && container.CreatedAt.After(lastModified) {
api.logger.Info(ctx, "clearing dirty flag for container created after config modification",
slog.F("container", container.ID),
Expand Down Expand Up @@ -316,7 +316,7 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC

dirty := dirtyStates[workspaceFolder]
if dirty {
lastModified, hasModTime := api.configModifiedTimes[configFile]
lastModified, hasModTime := api.configFileModifiedTimes[configFile]
if hasModTime && container.CreatedAt.After(lastModified) {
api.logger.Info(ctx, "new container created after config modification, not marking as dirty",
slog.F("container", container.ID),
Expand Down Expand Up @@ -473,7 +473,7 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) {
}

// Record the timestamp of when this configuration file was modified.
api.configModifiedTimes[configPath] = modifiedAt
api.configFileModifiedTimes[configPath] = modifiedAt

for i := range api.knownDevcontainers {
if api.knownDevcontainers[i].ConfigPath == configPath {
Expand Down
Loading