Skip to content

feat(site): use websocket connection for devcontainer updates #18808

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 33 commits into from
Jul 14, 2025
Merged

feat(site): use websocket connection for devcontainer updates #18808

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
487ee95
feat(site): use websocket connection for devcontainer updates
DanielleMaywood Jul 3, 2025
cc42018
fix: some issues
DanielleMaywood Jul 7, 2025
fa46517
Merge branch 'main' into danielle/container-push
DanielleMaywood Jul 7, 2025
5aef560
Merge branch 'main' into danielle/container-push
DanielleMaywood Jul 8, 2025
975ef8b
chore: fix disconnect bug and add agentcontainers test
DanielleMaywood Jul 8, 2025
8fdeca3
Merge branch 'main' into danielle/container-push
DanielleMaywood Jul 9, 2025
6da941f
test: add coderd/ test
DanielleMaywood Jul 9, 2025
ff5725e
chore: appease formatter
DanielleMaywood Jul 9, 2025
178507c
chore: feedback
DanielleMaywood Jul 9, 2025
367b87d
chore: fix nil exception
DanielleMaywood Jul 9, 2025
34b17c4
chore: make gen
DanielleMaywood Jul 9, 2025
8f12460
fix: docs
DanielleMaywood Jul 9, 2025
81022fa
Merge branch 'main' into danielle/container-push
DanielleMaywood Jul 9, 2025
1768f7b
fix: only send when there are updates
DanielleMaywood Jul 9, 2025
8240663
chore: lint and format
DanielleMaywood Jul 9, 2025
6d97960
Merge branch 'main' into danielle/container-push
DanielleMaywood Jul 10, 2025
88a611d
chore: test `useAgentContainers`
DanielleMaywood Jul 10, 2025
001ccda
chore: check container ids match in `Equals` function
DanielleMaywood Jul 10, 2025
3e50965
chore: add logger to WatchContainers
DanielleMaywood Jul 10, 2025
6ce5c19
chore: reposition close of update channel
DanielleMaywood Jul 10, 2025
cd0c2d5
chore: rename `knownDevcontainers`
DanielleMaywood Jul 10, 2025
04a92a4
chore: use `WebsocketNetConn`
DanielleMaywood Jul 10, 2025
096a85e
chore: steal CloseRead
DanielleMaywood Jul 10, 2025
971f9d6
chore: check agents match
DanielleMaywood Jul 10, 2025
f24401f
test: parsing error and socket error
DanielleMaywood Jul 10, 2025
64d9252
chore: lint and format
DanielleMaywood Jul 10, 2025
40c3fd9
chore: give comment some love
DanielleMaywood Jul 14, 2025
1cda455
chore: re-use json encoder instead of recreating every time
DanielleMaywood Jul 14, 2025
2ded15f
fix: push initial dev container state in websocket
DanielleMaywood Jul 14, 2025
a87f388
fix: do not invalidateQuery + fix bad types
DanielleMaywood Jul 14, 2025
2de01f5
chore: appease linter
DanielleMaywood Jul 14, 2025
00fdae6
chore: broadcast updates in more places, add staleTime: Infinity
DanielleMaywood Jul 14, 2025
a4a4bb2
chore: appease linter
DanielleMaywood Jul 14, 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
chore: fix disconnect bug and add agentcontainers test
  • Loading branch information
@DanielleMaywood
DanielleMaywood committed Jul 8, 2025
commit 975ef8bfcaa763a4257f7c655f63be356202de5d
15 changes: 12 additions & 3 deletions agent/agentcontainers/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,14 +560,20 @@ func (api *API) watchContainers(rw http.ResponseWriter, r *http.Request) {
return
}

ctx = api.ctx
Copy link
Member

Choose a reason for hiding this comment

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

We could do this here instead:

	ctx, wsNetConn := codersdk.WebsocketNetConn(ctx, conn, websocket.MessageBinary)
	defer wsNetConn.Close()

Note that we're not passing api.ctx to WebsocketNetConn but that's OK because of the final select (see below).

Then a few important changes following:

  • Heartbeat doesn't change (needs Ping from conn)
  • We can omit conn.Close (handled via wsNetConn.Close)
  • Encoder uses wsNetConn
  • Final select has one entry for api.ctx and another for ctx

This follows practice we use elsewhere in the code-base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this possible? wsjson.NewEncoder requires a *websocket.Conn but wsNetConn is a net.Conn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wsjson.NewEncoder is a tiny wrapper, i'll just write the underlying logic using wsNetConn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 04a92a4

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, I hadn't actually seen/used wsjson.NewEncoder before. The CloseRead it calls would be ideal to retain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have re-added this in 😄 096a85e


go httpapi.Heartbeat(ctx, conn)
defer conn.Close(websocket.StatusNormalClosure, "connection closed")

encoder := wsjson.NewEncoder[codersdk.WorkspaceAgentListContainersResponse](conn, websocket.MessageText)
defer encoder.Close(websocket.StatusNormalClosure)

updateCh := make(chan struct{})
defer close(updateCh)
updateCh := make(chan struct{}, 1)
defer func() {
api.mu.Lock()
close(updateCh)
Copy link
Member

Choose a reason for hiding this comment

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

It'd make sense to move this to the defer below to keep the related code close to each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6ce5c19

api.mu.Unlock()
}()

api.mu.Lock()
api.updateChans = append(api.updateChans, updateCh)
Expand Down Expand Up @@ -644,7 +650,10 @@ func (api *API) updateContainers(ctx context.Context) error {

// Broadcast our updates
for _, ch := range api.updateChans {
ch <- struct{}{}
select {
case ch <- struct{}{}:
default:
}
}

api.logger.Debug(ctx, "containers updated successfully", slog.F("container_count", len(api.containers.Containers)), slog.F("warning_count", len(api.containers.Warnings)), slog.F("devcontainer_count", len(api.knownDevcontainers)))
Expand Down
68 changes: 68 additions & 0 deletions agent/agentcontainers/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/coder/coder/v2/pty"
"github.com/coder/coder/v2/testutil"
"github.com/coder/quartz"
"github.com/coder/websocket"
)

// fakeContainerCLI implements the agentcontainers.ContainerCLI interface for
Expand Down Expand Up @@ -441,6 +442,73 @@ func TestAPI(t *testing.T) {
logbuf.Reset()
})

t.Run("Watch", func(t *testing.T) {
t.Parallel()

fakeContainer1 := fakeContainer(t)
fakeContainer2 := fakeContainer(t)
fakeContainer3 := fakeContainer(t)

makeResponse := func(cts ...codersdk.WorkspaceAgentContainer) codersdk.WorkspaceAgentListContainersResponse {
return codersdk.WorkspaceAgentListContainersResponse{Containers: cts}
}

var (
ctx = testutil.Context(t, testutil.WaitShort)
mClock = quartz.NewMock(t)
updaterTickerTrap = mClock.Trap().TickerFunc("updaterLoop")
mCtrl = gomock.NewController(t)
mLister = acmock.NewMockContainerCLI(mCtrl)
logger = slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
)

mLister.EXPECT().List(gomock.Any()).Return(makeResponse(), nil)

api := agentcontainers.NewAPI(logger,
agentcontainers.WithClock(mClock),
agentcontainers.WithContainerCLI(mLister),
agentcontainers.WithContainerLabelIncludeFilter("this.label.does.not.exist.ignore.devcontainers", "true"),
)
api.Start()
defer api.Close()

srv := httptest.NewServer(api.Routes())
defer srv.Close()

updaterTickerTrap.MustWait(ctx).MustRelease(ctx)
defer updaterTickerTrap.Close()

client, _, err := websocket.Dial(ctx, srv.URL+"/watch", nil)
require.NoError(t, err)

for _, mockResponse := range []codersdk.WorkspaceAgentListContainersResponse{
makeResponse(),
makeResponse(fakeContainer1),
makeResponse(fakeContainer1, fakeContainer2),
makeResponse(fakeContainer1, fakeContainer2, fakeContainer3),
makeResponse(fakeContainer1, fakeContainer2),
makeResponse(fakeContainer1),
makeResponse(),
} {
mLister.EXPECT().List(gomock.Any()).Return(mockResponse, nil)

// Given: We allow the update loop to progress
_, aw := mClock.AdvanceNext()
aw.MustWait(ctx)

// When: We attempt to read a message from the socket.
mt, msg, err := client.Read(ctx)
require.NoError(t, err)
require.Equal(t, websocket.MessageText, mt)

// Then: We expect the receieved message matches the mocked response.
var got codersdk.WorkspaceAgentListContainersResponse
err = json.Unmarshal(msg, &got)
require.NoError(t, err)
require.Equal(t, mockResponse, got)
}
})

// List tests the API.getContainers method using a mock
// implementation. It specifically tests caching behavior.
t.Run("List", func(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions coderd/workspaceagents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,8 @@ func (api *API) watchWorkspaceAgentContainers(rw http.ResponseWriter, r *http.Re
return
}

ctx = api.ctx
Copy link
Member

Choose a reason for hiding this comment

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

See related comments on api.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 04a92a4


go httpapi.Heartbeat(ctx, conn)
defer conn.Close(websocket.StatusNormalClosure, "connection closed")

Expand Down