eliminate override flag, explicitly take and release ownership, add c…

…omments, update tests

internal/deployment/deployment.go

@@ -142,19 +142,20 @@ func (d *Deployment) Write(w io.Writer) error {
 	return enc.Encode(d)
 }
 
+// When being called from methods active during a deployment, they will be running within a
+// go function with a state which includes a localID. By supplying it when calling this method,
+// they protect the deployment file from being updated by any methods active but not current.
+//
+// NOTE: deployment threads currently run to completion, so when a user "dismisses" a deployment
+// the go functions continue to want to update the record (even though they might be "old" news)
 func (d *Deployment) WriteFile(
 	path util.AbsolutePath,
-	localId string,
-	setOwnership bool,
+	localIdIfDeploying string,
 	log logging.Logger,
 ) (*Deployment, error) {
-	log.Debug("Attempting to update deployment record", "path", path, "localId", localId, "setOwnership", setOwnership)
+	log.Debug("Attempting to update deployment record", "path", path, "localIdIfDeploying", localIdIfDeploying)
 
-	if setOwnership {
-		// Establish ownership of a deployment record, as is done at beginning
-		// of each publishing run.
-		ActiveDeploymentRegistry.Set(path.String(), localId)
-	} else {
+	if localIdIfDeploying != "" {
 		// we will only update the deployment record, if the local id passed in
 		// owns the record (as determined by the ActiveDeploymentRegistry)
 		// matches (which confirms the ownership of the record vs. another deployment thread)
@@ -168,7 +169,7 @@ func (d *Deployment) WriteFile(
 			}
 		}
 		if existingDeployment != nil {
-			if !ActiveDeploymentRegistry.Check(path.String(), localId) {
+			if !ActiveDeploymentRegistry.Check(path.String(), localIdIfDeploying) {
 				log.Debug("Skipping deployment record update since existing record is being updated by another thread.")
 				return existingDeployment, nil
 			}
@@ -177,7 +178,13 @@ func (d *Deployment) WriteFile(
 				return existingDeployment, nil
 			}
 		}
+	} else {
+		// Protect against overlaps of deployment thread updates and user initiated updates
+		// (which pass in no value for localIdIfDeploying
+		ActiveDeploymentRegistry.Lock()
+		defer ActiveDeploymentRegistry.Unlock()
 	}
+
 	log.Debug("Updating deployment record", "path", path)
 
 	err := path.Dir().MkdirAll(0777)

internal/deployment/deployment_owner_registry.go

@@ -16,7 +16,9 @@ func (o *DeploymentOwnerRegistry) Set(deploymentPath string, localId string) {
 	o.Lock()
 	defer o.Unlock()
 
-	o.m[deploymentPath] = localId
+	if localId != "" {
+		o.m[deploymentPath] = localId
+	}
 }
 
 func (o *DeploymentOwnerRegistry) Check(deploymentPath string, localId string) bool {
@@ -27,6 +29,16 @@ func (o *DeploymentOwnerRegistry) Check(deploymentPath string, localId string) b
 	return owner == localId
 }
 
+func (o *DeploymentOwnerRegistry) Clear(deploymentPath string, localId string) {
+	o.Lock()
+	defer o.Unlock()
+
+	owner := o.m[deploymentPath]
+	if owner == localId {
+		delete(o.m, deploymentPath)
+	}
+}
+
 func (o *DeploymentOwnerRegistry) Reset() {
 	o.Lock()
 	defer o.Unlock()

internal/deployment/deployment_owner_registry_test.go

@@ -18,13 +18,30 @@ func TestDeploymentOwnerMapSuite(t *testing.T) {
 }
 
 func (s *DeploymentOwnerMapSuite) SetupTest() {
+	ActiveDeploymentRegistry.Reset()
+}
+
+func (s *DeploymentOwnerMapSuite) TestReset() {
+	s.Equal(0, len(ActiveDeploymentRegistry.m))
+
+	ActiveDeploymentRegistry.Set("abc/def", "123")
+	ActiveDeploymentRegistry.Set("abc/def2", "124")
+	ActiveDeploymentRegistry.Set("abc/def3", "125")
+	s.Equal(3, len(ActiveDeploymentRegistry.m))
+
+	ActiveDeploymentRegistry.Reset()
+	s.Equal(0, len(ActiveDeploymentRegistry.m))
 }
 
 func (s *DeploymentOwnerMapSuite) TestSet() {
+	ActiveDeploymentRegistry.Set("abc/def", "")
+	s.Equal(0, len(ActiveDeploymentRegistry.m))
+
 	ActiveDeploymentRegistry.Set("abc/def", "123")
 	ActiveDeploymentRegistry.Set("abc/def2", "124")
 	ActiveDeploymentRegistry.Set("abc/def3", "125")
 	s.Equal(true, ActiveDeploymentRegistry.Check("abc/def", "123"))
+
 }
 
 func (s *DeploymentOwnerMapSuite) TestCheckAndSet() {
@@ -36,3 +53,19 @@ func (s *DeploymentOwnerMapSuite) TestCheckAndSet() {
 	// non-existing
 	s.Equal(false, ActiveDeploymentRegistry.Check("xyz", "789"))
 }
+
+func (s *DeploymentOwnerMapSuite) TestClear() {
+	ActiveDeploymentRegistry.Set("abc/def", "123")
+	ActiveDeploymentRegistry.Set("abc/def2", "124")
+	ActiveDeploymentRegistry.Set("abc/def3", "125")
+	s.Equal(3, len(ActiveDeploymentRegistry.m))
+
+	// Valid request w/ local ID match
+	ActiveDeploymentRegistry.Clear("abc/def", "123")
+	s.Equal(2, len(ActiveDeploymentRegistry.m))
+
+	// Ignored request w/ non match
+	ActiveDeploymentRegistry.Clear("abc/def3", "999")
+	// still should be same number as before the operation
+	s.Equal(2, len(ActiveDeploymentRegistry.m))
+}

internal/deployment/deployment_test.go

@@ -51,7 +51,7 @@ func (s *DeploymentSuite) createDeploymentFile(name string) *Deployment {
 		Version:        "3.4.5",
 		PackageManager: "pip",
 	}
-	_, err := d.WriteFile(path, "abc", false, s.log)
+	_, err := d.WriteFile(path, "abc", s.log)
 	s.NoError(err)
 	return d
 }
@@ -113,7 +113,7 @@ func (s *DeploymentSuite) TestFromFileErr() {
 func (s *DeploymentSuite) TestWriteFile() {
 	deploymentFile := GetDeploymentPath(s.cwd, "myTargetName")
 	d := New()
-	_, err := d.WriteFile(deploymentFile, "", false, s.log)
+	_, err := d.WriteFile(deploymentFile, "", s.log)
 	s.NoError(err)
 
 	content, err := deploymentFile.ReadFile()
@@ -145,70 +145,58 @@ func (s *DeploymentSuite) TestWriteFile() {
 func (s *DeploymentSuite) TestWriteFileOptions() {
 
 	type testOptions struct {
-		existingLocalID   string
+		owningLocalID     string
 		updateLocalID     string
 		existingDismissed string
-		override          bool
 		expectedSuccess   bool
 	}
 
+	// Need to rethink...
 	tests := [...]testOptions{
+		// Non deployment thread test cases
+		// ownership take-over w/o dismissal
 		{
-			existingLocalID:   "123",
-			updateLocalID:     "123",
+			owningLocalID:     "123",
+			updateLocalID:     "",
 			existingDismissed: "",
-			override:          false,
 			expectedSuccess:   true,
 		},
+		// ownership take-over w/ dismissal
 		{
-			existingLocalID:   "123",
-			updateLocalID:     "123",
+			owningLocalID:     "123",
+			updateLocalID:     "",
 			existingDismissed: "2025-01-08T17:10:22-08:00",
-			override:          false,
-			expectedSuccess:   false,
+			expectedSuccess:   true,
 		},
+		// deployment in progress take-over tests
+		// ownership match w/ no dismissal
 		{
-			existingLocalID:   "123",
+			owningLocalID:     "123",
 			updateLocalID:     "123",
 			existingDismissed: "",
-			override:          true,
 			expectedSuccess:   true,
 		},
+		// ownership match w/ dismissal
 		{
-			existingLocalID:   "123",
+			owningLocalID:     "123",
 			updateLocalID:     "123",
 			existingDismissed: "2025-01-08T17:10:22-08:00",
-			override:          true,
-			expectedSuccess:   true,
+			expectedSuccess:   false,
 		},
+		// ownership mismatch w/o dismissal
 		{
-			existingLocalID:   "123",
+			owningLocalID:     "123",
 			updateLocalID:     "456",
 			existingDismissed: "",
-			override:          false,
 			expectedSuccess:   false,
 		},
+		// ownership mismatch w/ dismissal
 		{
-			existingLocalID:   "123",
+			owningLocalID:     "123",
 			updateLocalID:     "456",
 			existingDismissed: "2025-01-08T17:10:22-08:00",
-			override:          false,
 			expectedSuccess:   false,
 		},
-		{
-			existingLocalID:   "123",
-			updateLocalID:     "456",
-			existingDismissed: "",
-			override:          true,
-			expectedSuccess:   true,
-		},
-		{
-			existingLocalID:   "123",
-			updateLocalID:     "456",
-			existingDismissed: "2025-01-08T17:10:22-08:00",
-			override:          true,
-			expectedSuccess:   true,
-		},
 	}
 
 	for ndx, test := range tests {
@@ -219,9 +207,9 @@ func (s *DeploymentSuite) TestWriteFileOptions() {
 		deploymentFile := GetDeploymentPath(s.cwd, "myTargetName")
 		d := New()
 		d.ConfigName = "original" // Tests use this field to detect changes in file on disk
-		ActiveDeploymentRegistry.Set(deploymentFile.String(), test.existingLocalID)
+		ActiveDeploymentRegistry.Set(deploymentFile.String(), test.owningLocalID)
 		d.DismissedAt = test.existingDismissed
-		returnedD, err := d.WriteFile(deploymentFile, test.existingLocalID, false, s.log)
+		returnedD, err := d.WriteFile(deploymentFile, test.owningLocalID, s.log)
 		s.NoError(err)
 		s.Equal("original", returnedD.ConfigName, "Failed iteration %d of test (location 1)", i)
 
@@ -232,7 +220,7 @@ func (s *DeploymentSuite) TestWriteFileOptions() {
 
 		// try and update it
 		origD.ConfigName = "updated"
-		returnedD, err = origD.WriteFile(deploymentFile, test.updateLocalID, test.override, s.log)
+		returnedD, err = origD.WriteFile(deploymentFile, test.updateLocalID, s.log)
 		s.NoError(err)
 		if test.expectedSuccess {
 			s.Equal("updated", returnedD.ConfigName, "Failed iteration %d of test (location 3)", i)
@@ -256,7 +244,7 @@ func (s *DeploymentSuite) TestWriteFileErr() {
 	readonlyFs := afero.NewReadOnlyFs(deploymentFile.Fs())
 	readonlyFile := deploymentFile.WithFs(readonlyFs)
 	deployment := New()
-	_, err := deployment.WriteFile(readonlyFile, "", false, s.log)
+	_, err := deployment.WriteFile(readonlyFile, "", s.log)
 	s.NotNil(err)
 }
 

internal/publish/bundle.go

@@ -106,7 +106,7 @@ func (p *defaultPublisher) createAndUploadBundle(
 		p.Target.Renv = lockfile
 	}
 
-	_, err = p.writeDeploymentRecord(false)
+	_, err = p.writeDeploymentRecord()
 	if err != nil {
 		return "", err
 	}

internal/publish/publish.go

@@ -134,7 +134,7 @@ func (p *defaultPublisher) emitErrorEvents(err error) {
 	// Record the error in the deployment record
 	if p.Target != nil {
 		p.Target.Error = agentErr
-		_, writeErr := p.writeDeploymentRecord(false)
+		_, writeErr := p.writeDeploymentRecord()
 		if writeErr != nil {
 			p.log.Warn("failed to write updated deployment record", "name", p.TargetName, "err", writeErr)
 		}
@@ -201,7 +201,7 @@ func (p *defaultPublisher) PublishDirectory() error {
 	return err
 }
 
-func (p *defaultPublisher) writeDeploymentRecord(setOwnership bool) (*deployment.Deployment, error) {
+func (p *defaultPublisher) writeDeploymentRecord() (*deployment.Deployment, error) {
 	if p.SaveName == "" {
 		// Redeployment
 		p.log.Debug("No SaveName found while redeploying.", "deployment", p.TargetName)
@@ -219,7 +219,7 @@ func (p *defaultPublisher) writeDeploymentRecord(setOwnership bool) (*deployment
 
 	recordPath := deployment.GetDeploymentPath(p.Dir, p.SaveName)
 
-	return p.Target.WriteFile(recordPath, string(p.State.LocalID), setOwnership, p.log)
+	return p.Target.WriteFile(recordPath, string(p.State.LocalID), p.log)
 }
 
 func CancelDeployment(
@@ -240,7 +240,7 @@ func CancelDeployment(
 	target.DismissedAt = time.Now().Format(time.RFC3339)
 
 	// Possibly update the deployment file
-	d, err := target.WriteFile(deploymentPath, localID, false, log)
+	d, err := target.WriteFile(deploymentPath, localID, log)
 	return d, err
 }
 
@@ -292,7 +292,7 @@ func (p *defaultPublisher) createDeploymentRecord(
 	// localID to be recorded into the deployment record file, which
 	// then keeps old threads from updating the file for previous
 	// publishing attempts
-	_, err := p.writeDeploymentRecord(true)
+	_, err := p.writeDeploymentRecord()
 	return err
 }
 

internal/services/api/delete_deployment_test.go

@@ -60,7 +60,7 @@ func createSampleDeployment(root util.AbsolutePath, name string) (*deployment.De
 	d.DashboardURL = "/connect/#/apps/12345678"
 	d.DirectURL = "/content/12345678/"
 	d.LogsURL = "/connect/#/apps/12345678/logs"
-	_, err := d.WriteFile(path, "", false, logging.New())
+	_, err := d.WriteFile(path, "", logging.New())
 	return d, err
 }
 

internal/services/api/get_deployment_env_test.go

@@ -54,7 +54,7 @@ func (s *GetDeploymentEnvSuite) TestGetDeploymentEnv() {
 	d := deployment.New()
 	d.ID = "123"
 	d.ServerURL = "https://connect.example.com"
-	d.WriteFile(path, "", false, s.log)
+	d.WriteFile(path, "", s.log)
 
 	lister := &accounts.MockAccountList{}
 	acct := &accounts.Account{
@@ -120,7 +120,7 @@ func (s *GetDeploymentEnvSuite) TestGetDeploymentEnvFileError() {
 func (s *GetDeploymentEnvSuite) TestGetDeploymentEnvDeploymentNotDeployed() {
 	path := deployment.GetDeploymentPath(s.cwd, "dep")
 	d := deployment.New()
-	d.WriteFile(path, "", false, s.log)
+	d.WriteFile(path, "", s.log)
 
 	h := GetDeploymentEnvironmentHandlerFunc(s.cwd, s.log, &accounts.MockAccountList{})
 
@@ -140,7 +140,7 @@ func (s *GetDeploymentEnvSuite) TestGetDeploymentEnvNoCredential() {
 	d := deployment.New()
 	d.ID = "123"
 	d.ServerURL = "https://connect.example.com"
-	d.WriteFile(path, "", false, s.log)
+	d.WriteFile(path, "", s.log)
 
 	lister := &accounts.MockAccountList{}
 	lister.On("GetAccountByServerURL", "https://connect.example.com").Return(nil, errors.New("no such account"))
@@ -163,7 +163,7 @@ func (s *GetDeploymentEnvSuite) TestGetDeploymentEnvPassesStatusFromServer() {
 	d := deployment.New()
 	d.ID = "123"
 	d.ServerURL = "https://connect.example.com"
-	d.WriteFile(path, "", false, s.log)
+	d.WriteFile(path, "", s.log)
 
 	lister := &accounts.MockAccountList{}
 	acct := &accounts.Account{

internal/services/api/get_deployment_test.go

@@ -119,7 +119,7 @@ func (s *GetDeploymentSuite) TestGetPreDeployment() {
 	d.ServerType = accounts.ServerTypeConnect
 	testError := errors.New("test error")
 	d.Error = types.AsAgentError(testError)
-	_, err := d.WriteFile(path, "", false, s.log)
+	_, err := d.WriteFile(path, "", s.log)
 	s.NoError(err)
 
 	h := GetDeploymentHandlerFunc(s.cwd, s.log)

internal/services/api/get_deployments_test.go

@@ -178,7 +178,7 @@ func createAlternateDeployment(root util.AbsolutePath, name string) (*deployment
 	cfg.Type = config.ContentTypeHTML
 	cfg.Entrypoint = "index.html"
 	d.Configuration = cfg
-	_, err := d.WriteFile(path, "", false, logging.New())
+	_, err := d.WriteFile(path, "", logging.New())
 	return d, err
 }
 
@@ -232,7 +232,7 @@ func (s *GetDeploymentsSuite) makeSubdirDeployment(name string, subdir string) (
 	d.DashboardURL = "/connect/#/apps/abc123"
 	d.DirectURL = "/content/abc123/"
 	d.LogsURL = "/connect/#/apps/abc123/logs"
-	_, err = d.WriteFile(path, "", false, s.log)
+	_, err = d.WriteFile(path, "", s.log)
 	return d, err
 }