feat(agent/agentcontainers): add file watcher and dirty status #17573
Conversation
| server := &http.Server{ | ||
| Handler: a.apiHandler(), | ||
| BaseContext: func(net.Listener) context.Context { return ctx }, |
There was a problem hiding this comment.
Review: This change allows all r.Contexts to inherit the graceful context from the agent, ensuring cancellation on agent shutdown.
| return r | ||
| return r, func() error { | ||
| return containerAPI.Close() | ||
| } |
There was a problem hiding this comment.
Review: We don't want to just rely on agent context, we will also explicitly clean up running services in the containerAPI.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces file watcher functionality and a new “dirty” flag to track modifications in devcontainer configuration files. Key changes include:
- Adding a “dirty” field to both the TypeScript and Go devcontainer types.
- Implementing file watcher functionality using fsnotify and integrating it into the API to mark devcontainers as dirty on configuration changes.
- Updating tests and API logic to clear the dirty flag appropriately after container recreation.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/api/typesGenerated.ts | Added a new readonly “dirty” field to the WorkspaceAgentDevcontainer interface. |
| codersdk/workspaceagents.go | Added the Dirty boolean field for tracking runtime dirtiness. |
| agent/api.go | Modified apiHandler return signature and integrated cleanup; updated API logic for file watching. |
| agent/agentcontainers/watcher/* | Added implementations and tests for file watchers (FSNotify and Noop). |
| agent/agentcontainers/api_test.go & api_internal_test.go | Updated tests to exercise file watcher behavior and dirty flag handling. |
| agent/agentcontainers/api.go | Integrated watcher logic within API methods to mark and clear the dirty flag. |
| agent/agent.go | Updated tailnet creation to use the new apiHandler cleanup functionality. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (2)
agent/agentcontainers/api.go:476
- [nitpick] Consider renaming 'configModifiedTimes' to 'configFileModifiedTimes' to improve clarity on its purpose.
api.configModifiedTimes[configPath] = modifiedAt
agent/agent.go:1484
- Ensure that the cleanup function 'closeAPIHAndler' is invoked in all execution paths to avoid resource leaks, even if an error occurs later on.
apiHandler, closeAPIHAndler := a.apiHandler()
cfbd699 to
51fb44e
Compare
51fb44e to
bfeb1da
Compare
There was a problem hiding this comment.
I feel like this package could be moved outside of agentcontainers for later re-use, but that doesn't have to be done in this PR.
There was a problem hiding this comment.
It's a valid suggestion, but I'd like to defer until the need arises to keep devcontainer related stuff contained (heh).
9f483ed to
9cf5415
Compare
0b16448 to
eaf5922
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a file watcher and introduces a dirty flag to track when devcontainer configuration files change. Key changes include adding a "dirty" property to devcontainer types, integrating a file system watcher into the API logic, and supplementing the feature with comprehensive tests.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| site/src/api/typesGenerated.ts | Added a "dirty" flag to WorkspaceAgentDevcontainer interface. |
| codersdk/workspaceagents.go | Added a "Dirty" field to the Go struct representing the devcontainer. |
| agent/api.go | Modified apiHandler signature and integrated file watcher functionality. |
| agent/agentcontainers/watcher/* | Added and tested file watcher implementations (fsnotify and noop). |
| agent/agentcontainers/api*.go | Updated API logic to track dirty state and integrate file watcher events. |
| agent/agent.go | Updated tailnet creation logic to obtain and close the API handler. |
Files not reviewed (1)
- go.mod: Language not supported
| lockCh: make(chan struct{}, 1), | ||
| devcontainerNames: make(map[string]struct{}), | ||
| knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{}, | ||
| configFileModifiedTimes: make(map[string]time.Time), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍🏻
Fixes coder/internal#479
Fixes coder/internal#480