[bugfix] status refresh race condition causing double edit notifications (#4470)

# Description

fixes possible race condition of existing status being out-of-date in enrichStatus()

## Checklist

- [x] I/we have read the [GoToSocial contribution guidelines](https://codeberg.org/superseriousbusiness/gotosocial/src/branch/main/CONTRIBUTING.md).
- [x] I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
- [x] I/we have not leveraged AI to create the proposed changes.
- [x] I/we have performed a self-review of added code.
- [x] I/we have written code that is legible and maintainable by others.
- [x] I/we have commented the added code, particularly in hard-to-understand areas.
- [ ] I/we have made any necessary changes to documentation.
- [x] I/we have added tests that cover new code.
- [x] I/we have run tests and they pass locally with the changes.
- [x] I/we have run `go fmt ./...` and `golangci-lint run`.

Reviewed-on: https://codeberg.org/superseriousbusiness/gotosocial/pulls/4470
Co-authored-by: kim <grufwub@gmail.com>
Co-committed-by: kim <grufwub@gmail.com>

2 files changed, 119 insertions, 21 deletions

diff --git a/internal/federation/dereferencing/status.go b/internal/federation/dereferencing/status.go
index 0e86dca7c..fffaa88a6 100644
--- a/internal/federation/dereferencing/status.go
+++ b/internal/federation/dereferencing/status.go
@@ -277,25 +277,30 @@ func (d *Dereferencer) enrichStatusSafely(
 ) (*gtsmodel.Status, ap.Statusable, bool, error) {
 	uriStr := status.URI
 
+	// Acquire per-URI deref lock, wraping unlock
+	// to safely defer in case of panic, while still
+	// performing more granular unlocks when needed.
+	unlock := d.state.FedLocks.Lock(uriStr)
+	unlock = util.DoOnce(unlock)
+	defer unlock()
+
+	var err error
 	var isNew bool
 
 	// Check if this is a new status (to us).
 	if isNew = (status.ID == ""); !isNew {
 
-		// This is an existing status, first try to populate it. This
-		// is required by the checks below for existing tags, media etc.
-		if err := d.state.DB.PopulateStatus(ctx, status); err != nil {
-			log.Errorf(ctx, "error populating existing status %s: %v", uriStr, err)
+		// We reload the existing status, just to ensure we have the
+		// latest version of it. e.g. another racing thread might have
+		// just input a change but we still have an old status copy.
+		//
+		// Note: returned status will be fully populated, required below.
+		status, err = d.state.DB.GetStatusByID(ctx, status.ID)
+		if err != nil {
+			return nil, nil, false, gtserror.Newf("error getting up-to-date existing status: %w", err)
 		}
 	}
 
-	// Acquire per-URI deref lock, wraping unlock
-	// to safely defer in case of panic, while still
-	// performing more granular unlocks when needed.
-	unlock := d.state.FedLocks.Lock(uriStr)
-	unlock = util.DoOnce(unlock)
-	defer unlock()
-
 	// Perform status enrichment with passed vars.
 	latest, statusable, err := d.enrichStatus(ctx,
 		requestUser,
@@ -479,12 +484,10 @@ func (d *Dereferencer) enrichStatus(
 
 	// Ensure the final parsed status URI or URL matches
 	// the input URI we fetched (or received) it as.
-	matches, err := util.URIMatches(uri,
-		append(
-			ap.GetURL(statusable),      // status URL(s)
-			ap.GetJSONLDId(statusable), // status URI
-		)...,
-	)
+	matches, err := util.URIMatches(uri, append(
+		ap.GetURL(statusable),      // status URL(s)
+		ap.GetJSONLDId(statusable), // status URI
+	)...)
 	if err != nil {
 		return nil, nil, gtserror.Newf(
 			"error checking dereferenced status uri %s: %w",
diff --git a/internal/federation/dereferencing/status_test.go b/internal/federation/dereferencing/status_test.go
index 62b38f188..189fa2f23 100644
--- a/internal/federation/dereferencing/status_test.go
+++ b/internal/federation/dereferencing/status_test.go
@@ -18,7 +18,6 @@
 package dereferencing_test
 
 import (
-	"context"
 	"fmt"
 	"testing"
 	"time"
@@ -237,9 +236,7 @@ func (suite *StatusTestSuite) TestDereferenceStatusWithNonMatchingURI() {
 }
 
 func (suite *StatusTestSuite) TestDereferencerRefreshStatusUpdated() {
-	// Create a new context for this test.
-	ctx, cncl := context.WithCancel(suite.T().Context())
-	defer cncl()
+	ctx := suite.T().Context()
 
 	// The local account we will be fetching statuses as.
 	fetchingAccount := suite.testAccounts["local_account_1"]
@@ -343,6 +340,104 @@ func (suite *StatusTestSuite) TestDereferencerRefreshStatusUpdated() {
 	}
 }
 
+func (suite *StatusTestSuite) TestDereferencerRefreshStatusRace() {
+	ctx := suite.T().Context()
+
+	// The local account we will be fetching statuses as.
+	fetchingAccount := suite.testAccounts["local_account_1"]
+
+	// The test status in question that we will be dereferencing from "remote".
+	testURIStr := "https://unknown-instance.com/users/brand_new_person/statuses/01FE4NTHKWW7THT67EF10EB839"
+	testURI := testrig.URLMustParse(testURIStr)
+	testStatusable := suite.client.TestRemoteStatuses[testURIStr]
+
+	// Fetch the remote status first to load it into instance.
+	testStatus, statusable, err := suite.dereferencer.GetStatusByURI(ctx,
+		fetchingAccount.Username,
+		testURI,
+	)
+	suite.NotNil(statusable)
+	suite.NoError(err)
+
+	// Take a snapshot of current
+	// state of the test status.
+	beforeEdit := copyStatus(testStatus)
+
+	// Edit the "remote" statusable obj.
+	suite.editStatusable(testStatusable,
+		"updated status content!",
+		"CW: edited status content",
+		beforeEdit.Language,        // no change
+		*beforeEdit.Sensitive,      // no change
+		beforeEdit.AttachmentIDs,   // no change
+		getPollOptions(beforeEdit), // no change
+		getPollVotes(beforeEdit),   // no change
+		time.Now(),
+	)
+
+	// Refresh with a given statusable to updated to edited copy.
+	afterEdit, statusable, err := suite.dereferencer.RefreshStatus(ctx,
+		fetchingAccount.Username,
+		testStatus,
+		testStatusable,
+		instantFreshness,
+	)
+	suite.NotNil(statusable)
+	suite.NoError(err)
+
+	// verify updated status details.
+	suite.verifyEditedStatusUpdate(
+
+		// the original status
+		// before any changes.
+		beforeEdit,
+
+		// latest status
+		// being tested.
+		afterEdit,
+
+		// expected current state.
+		&gtsmodel.StatusEdit{
+			Content:        "updated status content!",
+			ContentWarning: "CW: edited status content",
+			Language:       beforeEdit.Language,
+			Sensitive:      beforeEdit.Sensitive,
+			AttachmentIDs:  beforeEdit.AttachmentIDs,
+			PollOptions:    getPollOptions(beforeEdit),
+			PollVotes:      getPollVotes(beforeEdit),
+			// createdAt never changes
+		},
+
+		// expected historic edit.
+		&gtsmodel.StatusEdit{
+			Content:        beforeEdit.Content,
+			ContentWarning: beforeEdit.ContentWarning,
+			Language:       beforeEdit.Language,
+			Sensitive:      beforeEdit.Sensitive,
+			AttachmentIDs:  beforeEdit.AttachmentIDs,
+			PollOptions:    getPollOptions(beforeEdit),
+			PollVotes:      getPollVotes(beforeEdit),
+			CreatedAt:      beforeEdit.UpdatedAt(),
+		},
+	)
+
+	// Now make another attempt to refresh, using the old copy of the
+	// status. This should still successfully update based on our passed
+	// freshness window, but it *should* refetch the provided status to
+	// check for race shenanigans and realize that no edit has occurred.
+	afterBodge, statusable, err := suite.dereferencer.RefreshStatus(ctx,
+		fetchingAccount.Username,
+		beforeEdit,
+		testStatusable,
+		instantFreshness,
+	)
+	suite.NotNil(statusable)
+	suite.NoError(err)
+
+	// Check that no further edit occurred on status.
+	suite.Equal(afterEdit.EditIDs, afterBodge.EditIDs)
+}
+
 // editStatusable updates the given statusable attributes.
 // note that this acts on the original object, no copying.
 func (suite *StatusTestSuite) editStatusable(