diff --git a/changes/31580-duplicate-scripts b/changes/31580-duplicate-scripts new file mode 100644 index 0000000000..675bec6511 --- /dev/null +++ b/changes/31580-duplicate-scripts @@ -0,0 +1 @@ +* Fixed error when updating a script to exactly match the contents of another script. diff --git a/server/datastore/mysql/scripts.go b/server/datastore/mysql/scripts.go index 52ab0df4c1..33bcd18e69 100644 --- a/server/datastore/mysql/scripts.go +++ b/server/datastore/mysql/scripts.go @@ -485,37 +485,60 @@ func (ds *Datastore) NewScript(ctx context.Context, script *fleet.Script) (*flee } func (ds *Datastore) UpdateScriptContents(ctx context.Context, scriptID uint, scriptContents string) (*fleet.Script, error) { - const stmt = ` -UPDATE script_contents -INNER JOIN - scripts ON scripts.script_content_id = script_contents.id -SET - contents = ?, - md5_checksum = UNHEX(?) -WHERE - scripts.id = ? -` - md5Checksum := md5ChecksumScriptContent(scriptContents) - - if err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { - _, err := tx.ExecContext(ctx, stmt, scriptContents, md5Checksum, scriptID) + err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { + // Get the current script_content_id + var oldContentID int64 + getCurrentStmt := `SELECT script_content_id FROM scripts WHERE id = ?` + err := sqlx.GetContext(ctx, tx, &oldContentID, getCurrentStmt, scriptID) if err != nil { - return ctxerr.Wrap(ctx, err, "updating script_contents") + return ctxerr.Wrap(ctx, err, "getting current script content id") } - if _, err := tx.ExecContext(ctx, "UPDATE scripts SET updated_at = NOW() WHERE id = ?", scriptID); err != nil { - return ctxerr.Wrap(ctx, err, "updating script updated_at time") + // Insert or get existing content (insertScriptContents handles deduplication) + scRes, err := insertScriptContents(ctx, tx, scriptContents) + if err != nil { + return ctxerr.Wrap(ctx, err, "inserting/getting script contents") + } + newContentID, _ := scRes.LastInsertId() + + // Update the script to point to the new content + if newContentID != oldContentID { + updateStmt := ` + UPDATE scripts + SET script_content_id = ? + WHERE id = ? + ` + _, err = tx.ExecContext(ctx, updateStmt, newContentID, scriptID) + if err != nil { + return ctxerr.Wrap(ctx, err, "updating script content reference") + } + + // Try to clean up the old content if no longer used + // Don't fail the transaction if cleanup fails; just log it + if err := ds.cleanupScriptContent(ctx, tx, uint(oldContentID)); err != nil { //nolint:gosec + level.Error(ds.logger).Log("msg", "failed to cleanup orphaned script content", + "script_id", scriptID, "old_content_id", oldContentID, "err", err) + ctxerr.Handle(ctx, err) + } + } else { + // Just update the timestamp + _, err = tx.ExecContext(ctx, "UPDATE scripts SET updated_at = NOW() WHERE id = ?", scriptID) + if err != nil { + return ctxerr.Wrap(ctx, err, "updating script updated_at time") + } } + // Cancel pending executions if err := ds.cancelUpcomingScriptActivities(ctx, tx, scriptID); err != nil { - return ctxerr.Wrap(ctx, err, "deleting upcoming script executions") + return ctxerr.Wrap(ctx, err, "canceling upcoming script executions") } return nil - }); err != nil { + }) + + if err != nil { return nil, ctxerr.Wrap(ctx, err, "updating script contents") } - return ds.Script(ctx, scriptID) } @@ -607,6 +630,43 @@ func md5ChecksumBytes(b []byte) string { return strings.ToUpper(hex.EncodeToString(rawChecksum[:])) } +func (ds *Datastore) cleanupScriptContent(ctx context.Context, tx sqlx.ExtContext, contentID uint) error { + // Check if this content is still being used anywhere + var usageCount int + stmt := ` + SELECT COUNT(*) FROM ( + SELECT 1 FROM scripts WHERE script_content_id = ? + UNION ALL + SELECT 1 FROM setup_experience_scripts WHERE script_content_id = ? + UNION ALL + SELECT 1 FROM software_installers WHERE + install_script_content_id = ? + OR uninstall_script_content_id = ? + OR post_install_script_content_id = ? + UNION ALL + SELECT 1 FROM script_upcoming_activities WHERE script_content_id = ? + UNION ALL + SELECT 1 FROM host_script_results WHERE script_content_id = ? + ) t + ` + err := sqlx.GetContext(ctx, tx, &usageCount, stmt, + contentID, contentID, contentID, contentID, contentID, contentID, contentID) + if err != nil { + return ctxerr.Wrap(ctx, err, "checking script content usage for cleanup") + } + + if usageCount == 0 { + // Not being used, safe to delete + deleteStmt := `DELETE FROM script_contents WHERE id = ?` + _, err = tx.ExecContext(ctx, deleteStmt, contentID) + if err != nil { + return ctxerr.Wrap(ctx, err, "deleting unused script content") + } + } + + return nil +} + func (ds *Datastore) Script(ctx context.Context, id uint) (*fleet.Script, error) { return ds.getScriptDB(ctx, ds.reader(ctx), id) } @@ -641,10 +701,9 @@ SELECT sc.contents FROM script_contents sc - JOIN scripts s + JOIN scripts s ON s.script_content_id = sc.id WHERE - s.script_content_id = sc.id - AND s.id = ?; + s.id = ? ` var contents []byte if err := sqlx.GetContext(ctx, ds.reader(ctx), &contents, getStmt, id); err != nil { diff --git a/server/datastore/mysql/scripts_test.go b/server/datastore/mysql/scripts_test.go index e5c0319f71..3a7d11022c 100644 --- a/server/datastore/mysql/scripts_test.go +++ b/server/datastore/mysql/scripts_test.go @@ -43,6 +43,9 @@ func TestScripts(t *testing.T) { {"TestDeleteScriptsAssignedToPolicy", testDeleteScriptsAssignedToPolicy}, {"TestDeletePendingHostScriptExecutionsForPolicy", testDeletePendingHostScriptExecutionsForPolicy}, {"UpdateScriptContents", testUpdateScriptContents}, + {"UpdateScriptToDuplicateContent", testUpdateScriptToDuplicateContent}, + {"UpdateSharedScriptContent", testUpdateSharedScriptContent}, + {"UpdateScriptToSameContent", testUpdateScriptToSameContent}, {"UpdateDeletingUpcomingScriptExecutions", testUpdateDeletingUpcomingScriptExecutions}, {"BatchExecute", testBatchExecute}, {"BatchExecuteWithStatus", testBatchExecuteWithStatus}, @@ -1709,9 +1712,10 @@ func testUpdateScriptContents(t *testing.T, ds *Datastore) { updatedScript, err := ds.UpdateScriptContents(ctx, originalScript.ID, "updated script") require.NoError(t, err) require.Equal(t, originalScript.ID, updatedScript.ID) - require.Equal(t, originalScript.ScriptContentID, updatedScript.ScriptContentID) + // With the fix, the script should get a new content ID since content changed + require.NotEqual(t, originalScript.ScriptContentID, updatedScript.ScriptContentID) - updatedContents, err := ds.GetScriptContents(ctx, originalScript.ScriptContentID) + updatedContents, err := ds.GetScriptContents(ctx, updatedScript.ID) require.NoError(t, err) require.Equal(t, "updated script", string(updatedContents)) require.NotEqual(t, oldScript.UpdatedAt, updatedScript.UpdatedAt) @@ -2785,3 +2789,126 @@ func testBatchSetScriptActivatesNextActivity(t *testing.T, ds *Datastore) { checkUpcomingActivities(t, ds, hosts[2]) checkUpcomingActivities(t, ds, hosts[3]) } + +// Test updating a script to match another script's contents +func testUpdateScriptToDuplicateContent(t *testing.T, ds *Datastore) { + ctx := t.Context() + + // Create two scripts with different content + script1, err := ds.NewScript(ctx, &fleet.Script{ + Name: "script1.sh", + ScriptContents: "echo hello", + }) + require.NoError(t, err) + + script2, err := ds.NewScript(ctx, &fleet.Script{ + Name: "script2.sh", + ScriptContents: "echo world", + }) + require.NoError(t, err) + + // Get initial content IDs + s1, err := ds.Script(ctx, script1.ID) + require.NoError(t, err) + s2, err := ds.Script(ctx, script2.ID) + require.NoError(t, err) + initialContentID1 := s1.ScriptContentID + initialContentID2 := s2.ScriptContentID + require.NotEqual(t, initialContentID1, initialContentID2) + + // Update script2 to have the same content as script1 + // This should NOT cause a duplicate key error + _, err = ds.UpdateScriptContents(ctx, script2.ID, "echo hello") + require.NoError(t, err) + // ScriptContents is not populated from the DB, check via GetScriptContents + // GetScriptContents takes a script ID, not script_content_id + updatedContents, err := ds.GetScriptContents(ctx, script2.ID) + require.NoError(t, err) + require.Equal(t, "echo hello", string(updatedContents)) + + // Verify both scripts now share the same content ID + s1After, err := ds.Script(ctx, script1.ID) + require.NoError(t, err) + s2After, err := ds.Script(ctx, script2.ID) + require.NoError(t, err) + require.Equal(t, s1After.ScriptContentID, s2After.ScriptContentID) + require.Equal(t, initialContentID1, s2After.ScriptContentID) + + // Verify the old content ID was cleaned up + var count int + err = sqlx.GetContext(ctx, ds.reader(ctx), &count, + `SELECT COUNT(*) FROM script_contents WHERE id = ?`, initialContentID2) + require.NoError(t, err) + require.Equal(t, 0, count, "old script content should be deleted") +} + +// Test modifying a script whose content currently matches another script's content +func testUpdateSharedScriptContent(t *testing.T, ds *Datastore) { + ctx := t.Context() + + // Create two scripts with the SAME content + sharedContent := "echo shared" + script1, err := ds.NewScript(ctx, &fleet.Script{ + Name: "script1.sh", + ScriptContents: sharedContent, + }) + require.NoError(t, err) + + script2, err := ds.NewScript(ctx, &fleet.Script{ + Name: "script2.sh", + ScriptContents: sharedContent, + }) + require.NoError(t, err) + + // Verify they share the same content ID + s1, err := ds.Script(ctx, script1.ID) + require.NoError(t, err) + s2, err := ds.Script(ctx, script2.ID) + require.NoError(t, err) + require.Equal(t, s1.ScriptContentID, s2.ScriptContentID) + + // Update script1 to different content + updated, err := ds.UpdateScriptContents(ctx, script1.ID, "echo modified") + require.NoError(t, err) + // ScriptContents is not populated from the DB, check via GetScriptContents + // GetScriptContents takes a script ID, not script_content_id + updatedContents, err := ds.GetScriptContents(ctx, script1.ID) + require.NoError(t, err) + require.Equal(t, "echo modified", string(updatedContents)) + + // CRITICAL: Verify script2 still has the original content + s2After, err := ds.Script(ctx, script2.ID) + require.NoError(t, err) + s2Contents, err := ds.GetScriptContents(ctx, script2.ID) + require.NoError(t, err) + require.Equal(t, sharedContent, string(s2Contents)) + require.NotEqual(t, updated.ScriptContentID, s2After.ScriptContentID) +} + +// Test updating script to same content -- a no-op case +func testUpdateScriptToSameContent(t *testing.T, ds *Datastore) { + ctx := t.Context() + + // Create a script + script, err := ds.NewScript(ctx, &fleet.Script{ + Name: "script.sh", + ScriptContents: "echo hello", + }) + require.NoError(t, err) + + s, err := ds.Script(ctx, script.ID) + require.NoError(t, err) + originalContentID := s.ScriptContentID + + // Update with the same content + _, err = ds.UpdateScriptContents(ctx, script.ID, "echo hello") + require.NoError(t, err) + updatedContents, err := ds.GetScriptContents(ctx, script.ID) + require.NoError(t, err) + require.Equal(t, "echo hello", string(updatedContents)) + + // Verify content ID hasn't changed + sAfter, err := ds.Script(ctx, script.ID) + require.NoError(t, err) + require.Equal(t, originalContentID, sAfter.ScriptContentID) +}