Discharge7443/fleet
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

Fixed error when updating a script to exactly match the contents of another script. (#32438)

Browse Source 
Fixes #31580 

Fixes issues
- When updating a script to exactly match the content of another script,
we fail
- When updating one script which happens to match content of another
script, both get updated and not just the one being edited

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files)
for more information.

## Testing

- [x] Added/updated automated tests
- [x] QA'd all new/changed functionality manually


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Resolved error when updating a script to exactly match another
script’s contents.
* Improved handling of script content updates: identical contents are
deduplicated and unused versions are cleaned up.
* Scheduled/pending runs are canceled on content updates with clearer
cancellation messaging.

* **Documentation**
  * Added changelog entry describing the fix.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Victor Lyuboslavsky
2025-08-29 12:38:37 -05:00
committed by GitHub
parent 8783bba7e0
commit cbea2bf12e
3 changed files with 212 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
changes/31580-duplicate-scripts
+1
View File
@@ -0,0 +1 @@
* Fixed error when updating a script to exactly match the contents of another script.
server/datastore/mysql/scripts.go
+82 -23
View File
@@ -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 {
server/datastore/mysql/scripts_test.go
+129 -2
View File
@@ -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)
}