NotificationsYou must be signed in to change notification settings
Fork905
Star10k

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mafredri merged 23 commits intomainfrommafredri/feat-agent-agentcontainers-watch

Apr 29, 2025

Merged

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

mafredri merged 23 commits intomainfrommafredri/feat-agent-agentcontainers-watch

Apr 29, 2025

Conversation

Copy link

Member

mafredri commentedApr 28, 2025

Fixescoder/internal#479
Fixescoder/internal#480

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

40c8de9

Fixescoder/internal#479Fixescoder/internal#480

github-actionsbot assignedmafredri

Apr 28, 2025

mafredri commented

Apr 28, 2025

View reviewed changes

agent/agent.go

		server := &http.Server{
		Handler: a.apiHandler(),
		BaseContext: func(net.Listener) context.Context { return ctx },

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

agent/api.go

		return r
		return r, func() error {
		return containerAPI.Close()
		}

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Review: We don't want to just rely on agent context, we will also explicitly clean up running services in the containerAPI.

make gen

a997f82

mafredri requested a review fromCopilot

April 28, 2025 11:21

CopilotAI reviewed

Apr 28, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

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()

fix copilot nits

0e568fb

mafredri requested review fromjohnstcn andDanielleMaywood

April 28, 2025 11:29

mafredri marked this pull request as ready for review

April 28, 2025 11:29

log event string in case not write

4f788d9

mafredri force-pushed themafredri/feat-agent-agentcontainers-watch branch fromcfbd699 to51fb44eCompare

April 28, 2025 11:38

fix chmod on macos

bfeb1da

mafredri force-pushed themafredri/feat-agent-agentcontainers-watch branch from51fb44e tobfeb1daCompare

April 28, 2025 11:39

johnstcn reviewed

Apr 28, 2025

View reviewed changes

agent/agentcontainers/watcher/watcher.go

Copy link

Member

Choose a reason for hiding this comment

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

I feel like this package could be moved outside ofagentcontainers for later re-use, but that doesn't have to be done in this PR.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

It's a valid suggestion, but I'd like to defer until the need arises to keep devcontainer related stuff contained (heh).

agent/agentcontainers/watcher/watcher.go OutdatedShow resolvedHide resolved

agent/agentcontainers/watcher/watcher_test.go OutdatedShow resolvedHide resolved

agent/agentcontainers/watcher/watcher.go OutdatedShow resolvedHide resolved

agent/agentcontainers/watcher/noop.go OutdatedShow resolvedHide resolved

agent/agentcontainers/api_test.goShow resolvedHide resolved

mafredri added7 commits

April 28, 2025 11:56

fix close of closed channel

45a978c

add close protection to fsnotify watcher as well

d2721a7

fix typo

c03bb7d

fix pr suggestions and improve start loop

aed7c30

update noop impl.

39526e6

s/file/path

7edee90

rewrite watcher impl. to watch dirs

9cf5415

mafredri force-pushed themafredri/feat-agent-agentcontainers-watch branch from9f483ed to9cf5415Compare

April 28, 2025 12:38

mafredri added6 commits

April 28, 2025 12:45

add rename and create to fsnotify test

de2a42e

add atomic file replacement

0cbee38

add paranoid reset

5ced7e1

fix oopsie

5a96a55

s/ErrWatcherClosed/ErrClosed/g

0b16448

inverse continue/return for clarity

eaf5922

mafredri force-pushed themafredri/feat-agent-agentcontainers-watch branch from0b16448 toeaf5922Compare

April 28, 2025 13:05

mafredri added4 commits

April 28, 2025 13:07

better close handling

617fed2

restore removed comment

8ee9914

add note about name

263c683

fix test setup/clock advancement

58a70e8

mafredri requested a review fromCopilot

April 28, 2025 13:40

CopilotAI reviewed

Apr 28, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

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

agent/agent.goShow resolvedHide resolved

mafredri requested a review fromjohnstcn

April 28, 2025 13:54

johnstcn approved these changes

Apr 28, 2025

View reviewed changes

agent/agentcontainers/api.go

		lockCh: make(chan struct{}, 1),
		devcontainerNames: make(map[string]struct{}),
		knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{},
		configFileModifiedTimes: make(map[string]time.Time),

Copy link

Member

johnstcnApr 28, 2025

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 amap[string]codersdk.NullTime

Copy link

MemberAuthor

mafredriApr 28, 2025

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 incodersdk types then we can totally use that type there 👍🏻

agent/agentcontainers/api.go OutdatedShow resolvedHide resolved

outdent

9f739ca

mafredri merged commit268a50c intomain

Apr 29, 2025

30 of 32 checks passed

mafredri deleted the mafredri/feat-agent-agentcontainers-watch branch

April 29, 2025 08:54

github-actionsbot locked and limited conversation to collaborators

Apr 29, 2025

Labels

None yet

2 participants

Movatterモバイル変換

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

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

Uh oh!

Conversation

mafredri commentedApr 28, 2025

Uh oh!

mafredriApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

mafredriApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

johnstcnApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

mafredriApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

johnstcnApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

mafredriApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!