diff --git a/control/contract_test.go b/control/contract_test.go index 5277990d..bd756d9e 100644 --- a/control/contract_test.go +++ b/control/contract_test.go @@ -479,6 +479,9 @@ func (fakeEnvironmentRepository) UpdateEnvironment(context.Context, control.Work func (fakeEnvironmentRepository) DeleteEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID) error { return nil } +func (fakeEnvironmentRepository) DeleteEnvironmentUnlessReferenced(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) error { + return nil +} func (fakeEnvironmentRepository) CountSessionsByEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) (int, error) { return 0, nil } diff --git a/control/ports.go b/control/ports.go index 863d2b22..09900f55 100644 --- a/control/ports.go +++ b/control/ports.go @@ -114,6 +114,7 @@ type SessionRepository interface { // ListEnvironments → Store.ListEnvironments // UpdateEnvironment → Store.UpdateEnvironment // DeleteEnvironment → Store.DeleteEnvironment +// DeleteEnvironmentUnlessReferenced -> Store.DeleteEnvironmentUnlessReferenced // CountSessionsByEnvironment → Store.CountSessionsByEnvironment // SetEnvironmentSnapshot → Store.SetEnvironmentSnapshot type EnvironmentRepository interface { @@ -122,6 +123,9 @@ type EnvironmentRepository interface { ListEnvironments(ctx context.Context, ws WorkspaceID, q EnvironmentQuery) ([]Environment, string, error) UpdateEnvironment(ctx context.Context, ws WorkspaceID, e Environment) (Environment, error) DeleteEnvironment(ctx context.Context, ws WorkspaceID, id EnvironmentID) error + // DeleteEnvironmentUnlessReferenced atomically deletes envID unless a + // session in states still references it: ErrConflict then, ErrNotFound if envID is already gone. + DeleteEnvironmentUnlessReferenced(ctx context.Context, ws WorkspaceID, id EnvironmentID, states []SessionState) error // CountSessionsByEnvironment counts sessions on envID whose state is in // states; an empty states counts every session on the environment. CountSessionsByEnvironment(ctx context.Context, ws WorkspaceID, envID EnvironmentID, states []SessionState) (int, error) diff --git a/controlapp/environments.go b/controlapp/environments.go index 59cbefe9..e7c0ed11 100644 --- a/controlapp/environments.go +++ b/controlapp/environments.go @@ -235,9 +235,8 @@ func (s *EnvironmentService) UpdateEnvironment(ctx context.Context, scope contro } // DeleteEnvironment reads and authorizes the environment, then refuses the -// delete while any non-terminal session still references it. A repository -// ErrNotFound remains ErrNotFound even if another workspace holds the same -// opaque ID. +// delete while any non-terminal session still references it, in one +// repository call. ErrNotFound persists even under another workspace's ID. func (s *EnvironmentService) DeleteEnvironment(ctx context.Context, scope control.Scope, cmd control.DeleteEnvironment) error { if err := scope.Validate(); err != nil { return control.ErrInvalid @@ -253,18 +252,14 @@ func (s *EnvironmentService) DeleteEnvironment(ctx context.Context, scope contro return control.ErrDenied } - n, err := s.environments.CountSessionsByEnvironment(ctx, scope.WorkspaceID, cmd.ID, control.NonTerminal) - if err != nil { - return control.ErrUnavailable - } - if n != 0 { - return control.ErrConflict - } return s.uow.Run(ctx, func(ctx context.Context) error { - if err := s.environments.DeleteEnvironment(ctx, scope.WorkspaceID, cmd.ID); err != nil { + if err := s.environments.DeleteEnvironmentUnlessReferenced(ctx, scope.WorkspaceID, cmd.ID, control.NonTerminal); err != nil { if errors.Is(err, control.ErrNotFound) { return control.ErrNotFound } + if errors.Is(err, control.ErrConflict) { + return control.ErrConflict + } return control.ErrUnavailable } return recordEvent(ctx, s.ids, s.events, s.clock, scope, control.ActionDelete, diff --git a/controlapp/environments_test.go b/controlapp/environments_test.go index 629b113d..1b309a9d 100644 --- a/controlapp/environments_test.go +++ b/controlapp/environments_test.go @@ -286,8 +286,8 @@ func TestDeleteEnvironmentGuard(t *testing.T) { if err := f.svc.DeleteEnvironment(context.Background(), sessionTestScope(), control.DeleteEnvironment{ID: "env_example"}); !errors.Is(err, control.ErrConflict) { t.Fatalf("delete with live session: got %v, want ErrConflict", err) } - if f.log.has("environments:delete") { - t.Fatalf("delete reached the repository despite a live session: %v", f.log.snapshot()) + if _, ok := f.repo.rows["env_example"]; !ok { + t.Fatalf("environment removed despite a live session") } // No live session deletes and records an event. diff --git a/controlapp/fleet_external_test.go b/controlapp/fleet_external_test.go index 080515eb..f8a4497f 100644 --- a/controlapp/fleet_external_test.go +++ b/controlapp/fleet_external_test.go @@ -160,6 +160,9 @@ func (fleetExtEnvironments) UpdateEnvironment(context.Context, control.Workspace func (fleetExtEnvironments) DeleteEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID) error { return control.ErrUnsupported } +func (fleetExtEnvironments) DeleteEnvironmentUnlessReferenced(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) error { + return control.ErrUnsupported +} func (fleetExtEnvironments) CountSessionsByEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) (int, error) { return 0, nil } diff --git a/controlapp/fleet_test.go b/controlapp/fleet_test.go index ba72972f..bfa390f8 100644 --- a/controlapp/fleet_test.go +++ b/controlapp/fleet_test.go @@ -374,6 +374,9 @@ func (f *fleetFakeEnvironments) UpdateEnvironment(context.Context, control.Works func (f *fleetFakeEnvironments) DeleteEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID) error { return control.ErrUnsupported } +func (f *fleetFakeEnvironments) DeleteEnvironmentUnlessReferenced(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) error { + return control.ErrUnsupported +} func (f *fleetFakeEnvironments) CountSessionsByEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) (int, error) { return 0, nil } diff --git a/controlapp/repotest/repotest.go b/controlapp/repotest/repotest.go index 993c8c2e..eb4f684c 100644 --- a/controlapp/repotest/repotest.go +++ b/controlapp/repotest/repotest.go @@ -77,6 +77,7 @@ func cases() []suiteCase { {"E6 guarded snapshot and its capability", caseEnvironmentSnapshot}, {"E7 count sessions by environment", caseCountSessionsByEnvironment}, {"E8 an empty workspace is invalid on every environment method", caseEnvironmentEmptyWorkspace}, + {"E9 delete-unless-referenced guards, then deletes, then reports gone", caseDeleteEnvironmentUnlessReferenced}, {"F1 runner round trip and order", caseRunnerRoundTrip}, {"F2 runners are isolated by pool", caseRunnerPoolIsolation}, @@ -1097,6 +1098,32 @@ func caseCountSessionsByEnvironment(t *testing.T, s Stores) { } } +// caseDeleteEnvironmentUnlessReferenced (E9) pins the guard's contract: +// refuse while a session in the given states still references the +// environment, delete once none does, report ErrNotFound once it is gone. +func caseDeleteEnvironmentUnlessReferenced(t *testing.T, s Stores) { + ctx := context.Background() + + env := mustCreateEnv(t, s, Alpha, fixtureEnvironment("env_seq", "dev")) + mustCreate(t, s, Alpha, control.Session{ + ID: "sess_seq", CreatorID: "act_a", State: control.StateQueued, PoolID: PoolA, EnvironmentID: env.ID}) + if err := s.Environments.DeleteEnvironmentUnlessReferenced(ctx, Alpha, env.ID, control.NonTerminal); !errors.Is(err, control.ErrConflict) { + t.Fatalf("delete with a live session: err = %v, want ErrConflict", err) + } + if _, err := s.Environments.GetEnvironment(ctx, Alpha, env.ID); err != nil { + t.Fatalf("environment removed despite a live session: %v", err) + } + if err := s.Sessions.Transition(ctx, Alpha, "sess_seq", control.NonTerminal, control.StateDestroyed, control.TransitionOpts{}); err != nil { + t.Fatal(err) + } + if err := s.Environments.DeleteEnvironmentUnlessReferenced(ctx, Alpha, env.ID, control.NonTerminal); err != nil { + t.Fatalf("delete once the only session is terminal: %v", err) + } + if err := s.Environments.DeleteEnvironmentUnlessReferenced(ctx, Alpha, env.ID, control.NonTerminal); !errors.Is(err, control.ErrNotFound) { + t.Fatalf("delete an already-gone environment: err = %v, want ErrNotFound", err) + } +} + // caseEnvironmentEmptyWorkspace (E8) pins that no environment method accepts // an unscoped call. func caseEnvironmentEmptyWorkspace(t *testing.T, s Stores) { diff --git a/controlapp/sessions_external_test.go b/controlapp/sessions_external_test.go index 6b2e0455..02836413 100644 --- a/controlapp/sessions_external_test.go +++ b/controlapp/sessions_external_test.go @@ -118,6 +118,9 @@ func (sessionExtEnvironmentRepo) UpdateEnvironment(context.Context, control.Work func (sessionExtEnvironmentRepo) DeleteEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID) error { return nil } +func (sessionExtEnvironmentRepo) DeleteEnvironmentUnlessReferenced(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) error { + return nil +} func (sessionExtEnvironmentRepo) CountSessionsByEnvironment(context.Context, control.WorkspaceID, control.EnvironmentID, []control.SessionState) (int, error) { return 0, nil } diff --git a/controlapp/sessions_test.go b/controlapp/sessions_test.go index cdefb9be..72ef1d4a 100644 --- a/controlapp/sessions_test.go +++ b/controlapp/sessions_test.go @@ -373,6 +373,26 @@ func (r *sessionStubEnvironmentRepo) CountSessionsByEnvironment(ctx context.Cont return r.liveSessionCount, nil } +func (r *sessionStubEnvironmentRepo) DeleteEnvironmentUnlessReferenced(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID, states []control.SessionState) error { + r.log.add("environments:delete-unless-referenced") + r.lastCountStates = states + if r.countErr != nil { + return r.countErr + } + if r.liveSessionCount != 0 { + return control.ErrConflict + } + if r.deleteErr != nil { + return r.deleteErr + } + e, ok := r.rows[id] + if !ok || e.WorkspaceID != ws { + return control.ErrNotFound + } + delete(r.rows, id) + return nil +} + func (r *sessionStubEnvironmentRepo) SetEnvironmentSnapshot(ctx context.Context, ws control.WorkspaceID, envID control.EnvironmentID, expectHash, ref string, runnerID control.RunnerID) error { r.log.add("environments:set-snapshot") return nil diff --git a/controlapp/uow_test.go b/controlapp/uow_test.go index beb702da..cfdedfbe 100644 --- a/controlapp/uow_test.go +++ b/controlapp/uow_test.go @@ -102,6 +102,11 @@ func (r *uowEnvRepo) DeleteEnvironment(ctx context.Context, ws control.Workspace return r.sessionStubEnvironmentRepo.DeleteEnvironment(ctx, ws, id) } +func (r *uowEnvRepo) DeleteEnvironmentUnlessReferenced(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID, states []control.SessionState) error { + r.deleteDepth = unitDepth(ctx) + return r.sessionStubEnvironmentRepo.DeleteEnvironmentUnlessReferenced(ctx, ws, id, states) +} + // uowRecorder is the event recorder reporting the depth each record ran at. type uowRecorder struct { *sessionStubEventRecorder diff --git a/internal/controld/environment_delete_race_test.go b/internal/controld/environment_delete_race_test.go new file mode 100644 index 00000000..96a94d65 --- /dev/null +++ b/internal/controld/environment_delete_race_test.go @@ -0,0 +1,74 @@ +// internal/controld/environment_delete_race_test.go +package controld + +import ( + "context" + "net/http" + "testing" + + "github.com/tokencanopy/rainier/control" +) + +// raceCreateSessionStore wraps MemStore to seed a session against an +// environment the moment its own delete call runs, the latest point a +// concurrent CreateSession could still land. +type raceCreateSessionStore struct { + MemStore + envID control.EnvironmentID + triggered bool +} + +func (r *raceCreateSessionStore) Environments() control.EnvironmentRepository { + return raceCreateSessionEnvironments{EnvironmentRepository: r.MemStore.Environments(), owner: r} +} + +type raceCreateSessionEnvironments struct { + control.EnvironmentRepository + owner *raceCreateSessionStore +} + +func (r raceCreateSessionEnvironments) seedRacer(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID) { + if o := r.owner; !o.triggered && id == o.envID { + o.triggered = true + if _, err := o.MemStore.Sessions().CreateSession(ctx, ws, control.Session{ + ID: "sess_racer", CreatorID: "usr_test", State: control.StateQueued, + PoolID: installPool, EnvironmentID: id, + }); err != nil { + panic("raceCreateSessionStore: seeding the racer session: " + err.Error()) + } + } +} + +func (r raceCreateSessionEnvironments) DeleteEnvironment(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID) error { + r.seedRacer(ctx, ws, id) + return r.EnvironmentRepository.DeleteEnvironment(ctx, ws, id) +} + +func (r raceCreateSessionEnvironments) DeleteEnvironmentUnlessReferenced(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID, states []control.SessionState) error { + r.seedRacer(ctx, ws, id) + return r.EnvironmentRepository.DeleteEnvironmentUnlessReferenced(ctx, ws, id, states) +} + +// TestDeleteEnvironmentSeesASessionCreatedRightBeforeItsOwnDelete pins a +// session created immediately before the delete's own repository call: +// the guard must still see it, whichever delete path is live. +func TestDeleteEnvironmentSeesASessionCreatedRightBeforeItsOwnDelete(t *testing.T) { + race := &raceCreateSessionStore{MemStore: NewMemStore()} + _, ts := newTestControldOver(t, race) + _, adminTok := loginUser(t, race, "root", "admin") + created := createEnv(t, ts, adminTok, envCreateBody("dev", nil)) + race.envID = control.EnvironmentID(created.ID) + + resp := doRequest(t, ts, http.MethodDelete, "/v0/environments/"+created.ID, adminTok, nil, nil) + raw := readBody(t, resp) + if !race.triggered { + t.Fatalf("the racer session was never seeded; this test proves nothing") + } + if resp.StatusCode != http.StatusConflict { + t.Fatalf("status = %d, want 409 (sess_racer was created immediately before the delete); body=%s", resp.StatusCode, raw) + } + if _, err := race.Environments().GetEnvironment(context.Background(), installWorkspace, + control.EnvironmentID(created.ID)); err != nil { + t.Errorf("environment removed despite the racer session: %v", err) + } +} diff --git a/internal/controld/memstore.go b/internal/controld/memstore.go index be03f935..a6f2c253 100644 --- a/internal/controld/memstore.go +++ b/internal/controld/memstore.go @@ -600,6 +600,33 @@ func (r memEnvironments) DeleteEnvironment(ctx context.Context, ws control.Works return nil } +// DeleteEnvironmentUnlessReferenced holds m.mu across the whole check and +// delete, one critical section instead of the two lock acquisitions +// CountSessionsByEnvironment and DeleteEnvironment used separately. +func (r memEnvironments) DeleteEnvironmentUnlessReferenced(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID, states []control.SessionState) error { + if ws == "" { + return control.ErrInvalid + } + m := r.m + m.mu.Lock() + defer m.mu.Unlock() + key := environmentKey{ws, id} + if _, ok := m.environments[key]; !ok { + return control.ErrNotFound + } + for k, s := range m.sessions { + if k.ws != ws || s.EnvironmentID != id { + continue + } + if len(states) == 0 || slices.Contains(states, s.State) { + return control.ErrConflict + } + } + delete(m.environments, key) + delete(m.snapshots, key) + return nil +} + func (r memEnvironments) CountSessionsByEnvironment(ctx context.Context, ws control.WorkspaceID, envID control.EnvironmentID, states []control.SessionState) (int, error) { if ws == "" { return 0, control.ErrInvalid diff --git a/internal/controld/pgstore/environments.go b/internal/controld/pgstore/environments.go index 5fd612a5..fa8e506e 100644 --- a/internal/controld/pgstore/environments.go +++ b/internal/controld/pgstore/environments.go @@ -340,6 +340,44 @@ func (r pgEnvironments) DeleteEnvironment(ctx context.Context, ws control.Worksp return nil } +// DeleteEnvironmentUnlessReferenced folds the check into the DELETE's own +// WHERE clause, so the decision and the write are one statement instead of +// the separate CountSessionsByEnvironment-then-DeleteEnvironment pair. +func (r pgEnvironments) DeleteEnvironmentUnlessReferenced(ctx context.Context, ws control.WorkspaceID, id control.EnvironmentID, states []control.SessionState) error { + if ws == "" { + return control.ErrInvalid + } + sql := `DELETE FROM environments WHERE workspace_id = $1 AND id = $2 AND NOT EXISTS ( + SELECT 1 FROM sessions WHERE workspace_id = $1 AND environment_id = $2` + args := []any{string(ws), string(id)} + if len(states) > 0 { + strs := make([]string, len(states)) + for i, st := range states { + strs[i] = string(st) + } + args = append(args, strs) + sql += ` AND state = ANY($3)` + } + sql += `)` + ct, err := r.s.q(ctx).Exec(ctx, sql, args...) + if err != nil { + return unavailable("delete environment unless referenced", err) + } + if ct.RowsAffected() > 0 { + return nil + } + // Zero rows means either the environment is already gone or it is still + // referenced; disambiguate with a read, the same stale-vs-not-found shape + // SetEnvironmentSnapshot already uses. + if _, err := r.GetEnvironment(ctx, ws, id); err != nil { + if errors.Is(err, control.ErrNotFound) { + return control.ErrNotFound + } + return unavailable("delete environment unless referenced", err) + } + return control.ErrConflict +} + func (r pgEnvironments) CountSessionsByEnvironment(ctx context.Context, ws control.WorkspaceID, envID control.EnvironmentID, states []control.SessionState) (int, error) { if ws == "" { return 0, control.ErrInvalid