Skip to content

Commit c281c77

Browse files
DanielleMaywoodDanielleMaywood
committed
chore: wrap in a transaction
Wrap the calls to InsertWorkspaceAgent and InsertWorkspaceApp in a transaction so that they either all succeed or none succeed.
1 parent 65495fe commit c281c77

File tree

2 files changed

+150
-94
lines changed

2 files changed

+150
-94
lines changed

‎coderd/agentapi/subagent.go

Lines changed: 104 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -54,108 +54,118 @@ func (a *SubAgentAPI) CreateSubAgent(ctx context.Context, req *agentproto.Create
5454

5555
createdAt := a.Clock.Now()
5656

57-
subAgent, err := a.Database.InsertWorkspaceAgent(ctx, database.InsertWorkspaceAgentParams{
58-
ID: uuid.New(),
59-
ParentID: uuid.NullUUID{Valid: true, UUID: parentAgent.ID},
60-
CreatedAt: createdAt,
61-
UpdatedAt: createdAt,
62-
Name: agentName,
63-
ResourceID: parentAgent.ResourceID,
64-
AuthToken: uuid.New(),
65-
AuthInstanceID: parentAgent.AuthInstanceID,
66-
Architecture: req.Architecture,
67-
EnvironmentVariables: pqtype.NullRawMessage{},
68-
OperatingSystem: req.OperatingSystem,
69-
Directory: req.Directory,
70-
InstanceMetadata: pqtype.NullRawMessage{},
71-
ResourceMetadata: pqtype.NullRawMessage{},
72-
ConnectionTimeoutSeconds: parentAgent.ConnectionTimeoutSeconds,
73-
TroubleshootingURL: parentAgent.TroubleshootingURL,
74-
MOTDFile: "",
75-
DisplayApps: []database.DisplayApp{},
76-
DisplayOrder: 0,
77-
APIKeyScope: parentAgent.APIKeyScope,
78-
})
79-
if err != nil {
80-
return nil, xerrors.Errorf("insert sub agent: %w", err)
81-
}
57+
var subAgent database.WorkspaceAgent
58+
59+
if err := a.Database.InTx(func(tx database.Store) error {
60+
var err error
61+
62+
subAgent, err = tx.InsertWorkspaceAgent(ctx, database.InsertWorkspaceAgentParams{
63+
ID: uuid.New(),
64+
ParentID: uuid.NullUUID{Valid: true, UUID: parentAgent.ID},
65+
CreatedAt: createdAt,
66+
UpdatedAt: createdAt,
67+
Name: agentName,
68+
ResourceID: parentAgent.ResourceID,
69+
AuthToken: uuid.New(),
70+
AuthInstanceID: parentAgent.AuthInstanceID,
71+
Architecture: req.Architecture,
72+
EnvironmentVariables: pqtype.NullRawMessage{},
73+
OperatingSystem: req.OperatingSystem,
74+
Directory: req.Directory,
75+
InstanceMetadata: pqtype.NullRawMessage{},
76+
ResourceMetadata: pqtype.NullRawMessage{},
77+
ConnectionTimeoutSeconds: parentAgent.ConnectionTimeoutSeconds,
78+
TroubleshootingURL: parentAgent.TroubleshootingURL,
79+
MOTDFile: "",
80+
DisplayApps: []database.DisplayApp{},
81+
DisplayOrder: 0,
82+
APIKeyScope: parentAgent.APIKeyScope,
83+
})
84+
if err != nil {
85+
return xerrors.Errorf("insert sub agent: %w", err)
86+
}
8287

83-
appSlugs := make(map[string]struct{})
84-
for i, app := range req.Apps {
85-
slug := app.Slug
86-
if slug == "" {
87-
return nil, codersdk.ValidationError{
88-
Field: fmt.Sprintf("apps[%d].slug", i),
89-
Detail: "app must have a slug or name set",
88+
appSlugs := make(map[string]struct{})
89+
for i, app := range req.Apps {
90+
slug := app.Slug
91+
if slug == "" {
92+
return codersdk.ValidationError{
93+
Field: fmt.Sprintf("apps[%d].slug", i),
94+
Detail: "app must have a slug or name set",
95+
}
9096
}
91-
}
92-
if !provisioner.AppSlugRegex.MatchString(slug) {
93-
return nil, codersdk.ValidationError{
94-
Field: fmt.Sprintf("apps[%d].slug", i),
95-
Detail: fmt.Sprintf("app slug %q does not match regex %q", slug, provisioner.AppSlugRegex),
97+
if !provisioner.AppSlugRegex.MatchString(slug) {
98+
return codersdk.ValidationError{
99+
Field: fmt.Sprintf("apps[%d].slug", i),
100+
Detail: fmt.Sprintf("app slug %q does not match regex %q", slug, provisioner.AppSlugRegex),
101+
}
96102
}
97-
}
98-
if _, exists := appSlugs[slug]; exists {
99-
return nil, codersdk.ValidationError{
100-
Field: fmt.Sprintf("apps[%d].slug", i),
101-
Detail: fmt.Sprintf("app slug %q is already in use", slug),
103+
if _, exists := appSlugs[slug]; exists {
104+
return codersdk.ValidationError{
105+
Field: fmt.Sprintf("apps[%d].slug", i),
106+
Detail: fmt.Sprintf("app slug %q is already in use", slug),
107+
}
108+
}
109+
appSlugs[slug] = struct{}{}
110+
health := database.WorkspaceAppHealthDisabled
111+
if app.Healthcheck == nil {
112+
app.Healthcheck = &agentproto.CreateSubAgentRequest_App_Healthcheck{}
113+
}
114+
if app.Healthcheck.Url != "" {
115+
health = database.WorkspaceAppHealthInitializing
102116
}
103-
}
104-
appSlugs[slug] = struct{}{}
105-
health := database.WorkspaceAppHealthDisabled
106-
if app.Healthcheck == nil {
107-
app.Healthcheck = &agentproto.CreateSubAgentRequest_App_Healthcheck{}
108-
}
109-
if app.Healthcheck.Url != "" {
110-
health = database.WorkspaceAppHealthInitializing
111-
}
112117

113-
sharingLevel := database.AppSharingLevelOwner
114-
switch app.GetShare() {
115-
case agentproto.CreateSubAgentRequest_App_AUTHENTICATED:
116-
sharingLevel = database.AppSharingLevelAuthenticated
117-
case agentproto.CreateSubAgentRequest_App_PUBLIC:
118-
sharingLevel = database.AppSharingLevelPublic
119-
}
118+
sharingLevel := database.AppSharingLevelOwner
119+
switch app.GetShare() {
120+
case agentproto.CreateSubAgentRequest_App_AUTHENTICATED:
121+
sharingLevel = database.AppSharingLevelAuthenticated
122+
case agentproto.CreateSubAgentRequest_App_PUBLIC:
123+
sharingLevel = database.AppSharingLevelPublic
124+
}
120125

121-
openIn := database.WorkspaceAppOpenInSlimWindow
122-
if app.GetOpenIn() == agentproto.CreateSubAgentRequest_App_TAB {
123-
openIn = database.WorkspaceAppOpenInTab
124-
}
126+
openIn := database.WorkspaceAppOpenInSlimWindow
127+
if app.GetOpenIn() == agentproto.CreateSubAgentRequest_App_TAB {
128+
openIn = database.WorkspaceAppOpenInTab
129+
}
125130

126-
_, err := a.Database.InsertWorkspaceApp(ctx, database.InsertWorkspaceAppParams{
127-
ID: uuid.New(),
128-
CreatedAt: createdAt,
129-
AgentID: subAgent.ID,
130-
Slug: app.Slug,
131-
DisplayName: app.GetDisplayName(),
132-
Icon: app.GetIcon(),
133-
Command: sql.NullString{
134-
Valid: app.GetCommand() != "",
135-
String: app.GetCommand(),
136-
},
137-
Url: sql.NullString{
138-
Valid: app.GetUrl() != "",
139-
String: app.GetUrl(),
140-
},
141-
External: app.GetExternal(),
142-
Subdomain: app.GetSubdomain(),
143-
SharingLevel: sharingLevel,
144-
HealthcheckUrl: app.Healthcheck.Url,
145-
HealthcheckInterval: app.Healthcheck.Interval,
146-
HealthcheckThreshold: app.Healthcheck.Threshold,
147-
Health: health,
148-
DisplayOrder: app.GetOrder(),
149-
Hidden: app.GetHidden(),
150-
OpenIn: openIn,
151-
DisplayGroup: sql.NullString{
152-
Valid: app.GetGroup() != "",
153-
String: app.GetGroup(),
154-
},
155-
})
156-
if err != nil {
157-
return nil, xerrors.Errorf("insert workspace app: %w", err)
131+
_, err := tx.InsertWorkspaceApp(ctx, database.InsertWorkspaceAppParams{
132+
ID: uuid.New(),
133+
CreatedAt: createdAt,
134+
AgentID: subAgent.ID,
135+
Slug: app.Slug,
136+
DisplayName: app.GetDisplayName(),
137+
Icon: app.GetIcon(),
138+
Command: sql.NullString{
139+
Valid: app.GetCommand() != "",
140+
String: app.GetCommand(),
141+
},
142+
Url: sql.NullString{
143+
Valid: app.GetUrl() != "",
144+
String: app.GetUrl(),
145+
},
146+
External: app.GetExternal(),
147+
Subdomain: app.GetSubdomain(),
148+
SharingLevel: sharingLevel,
149+
HealthcheckUrl: app.Healthcheck.Url,
150+
HealthcheckInterval: app.Healthcheck.Interval,
151+
HealthcheckThreshold: app.Healthcheck.Threshold,
152+
Health: health,
153+
DisplayOrder: app.GetOrder(),
154+
Hidden: app.GetHidden(),
155+
OpenIn: openIn,
156+
DisplayGroup: sql.NullString{
157+
Valid: app.GetGroup() != "",
158+
String: app.GetGroup(),
159+
},
160+
})
161+
if err != nil {
162+
return xerrors.Errorf("insert workspace app: %w", err)
163+
}
158164
}
165+
166+
return nil
167+
}, nil); err != nil {
168+
return nil, err
159169
}
160170

161171
return &agentproto.CreateSubAgentResponse{

‎coderd/agentapi/subagent_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,52 @@ func TestSubAgentAPI(t *testing.T) {
173173
}
174174
})
175175
}
176+
177+
t.Run("TransactionRollbackOnAppError", func(t *testing.T) {
178+
t.Parallel()
179+
180+
// Skip test on in-memory database since transactions are not fully supported
181+
if !dbtestutil.WillUsePostgres() {
182+
t.Skip("Transaction behavior requires PostgreSQL")
183+
}
184+
185+
log := testutil.Logger(t)
186+
ctx := testutil.Context(t, testutil.WaitShort)
187+
clock := quartz.NewMock(t)
188+
189+
db, org := newDatabaseWithOrg(t)
190+
user, agent := newUserWithWorkspaceAgent(t, db, org)
191+
api := newAgentAPI(t, log, db, clock, user, org, agent)
192+
193+
// When: We create a sub agent with valid name but invalid app that will cause transaction to fail
194+
_, err := api.CreateSubAgent(ctx, &proto.CreateSubAgentRequest{
195+
Name: "test-agent",
196+
Directory: "/workspaces/test",
197+
Architecture: "amd64",
198+
OperatingSystem: "linux",
199+
Apps: []*proto.CreateSubAgentRequest_App{
200+
{
201+
Slug: "valid-app",
202+
DisplayName: ptr.Ref("Valid App"),
203+
},
204+
{
205+
Slug: "Invalid_App_Slug", // This will cause validation error inside transaction
206+
DisplayName: ptr.Ref("Invalid App"),
207+
},
208+
},
209+
})
210+
211+
// Then: The request should fail with validation error
212+
require.Error(t, err)
213+
var validationErr codersdk.ValidationError
214+
require.ErrorAs(t, err, &validationErr)
215+
require.Equal(t, "apps[1].slug", validationErr.Field)
216+
217+
// And: No sub agents should be created (transaction rolled back)
218+
subAgents, err := db.GetWorkspaceAgentsByParentID(dbauthz.AsSystemRestricted(ctx), agent.ID) //nolint:gocritic // this is a test.
219+
require.NoError(t, err)
220+
require.Empty(t, subAgents, "Expected no sub agents to be created after transaction rollback")
221+
})
176222
})
177223

178224
t.Run("CreateSubAgentWithApps", func(t *testing.T) {

0 commit comments

Comments
 (0)