From 57cb4fe7482962aa8e5a05874a343474d5a453e7 Mon Sep 17 00:00:00 2001 From: kim Date: Fri, 3 Oct 2025 15:50:57 +0200 Subject: [PATCH] [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 Co-committed-by: kim --- internal/federation/dereferencing/status.go | 39 ++++--- .../federation/dereferencing/status_test.go | 103 +++++++++++++++++- 2 files changed, 120 insertions(+), 22 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,18 +277,6 @@ func (d *Dereferencer) enrichStatusSafely( ) (*gtsmodel.Status, ap.Statusable, bool, error) { uriStr := status.URI - 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) - } - } - // Acquire per-URI deref lock, wraping unlock // to safely defer in case of panic, while still // performing more granular unlocks when needed. @@ -296,6 +284,23 @@ func (d *Dereferencer) enrichStatusSafely( 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 { + + // 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) + } + } + // 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. + >smodel.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. + >smodel.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(