[bugfix] parent status replied to status not dereferenced sometimes (#2587)

* much simplified DereferenceStatusAncestors(), also handles edge cases now

* perform status acceptibility check before handling even as forward

* don't further dereference ancestors if they're up to date

* call enrichStatusSafely() directly to ensure we get error messages

* change getStatusByURI() semantics to return error + old model on failed update, fix deref ancestor to check for staleness before refetch

* perform a nil-check on the status.Local variable, in case it hasn't been set on new status attempting refresh

* more consistently set returned parent status, don't check if updated

* only home-timeline statuses if explicitly visible AND not explicitly invisible!

* fix broken test now that status acceptibility checks happen on forwarded statuses

1 files changed, 15 insertions, 14 deletions

diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go
index 067203780..d51d3078e 100644
--- a/internal/federation/dereferencing/account.go
+++ b/internal/federation/dereferencing/account.go
@@ -372,18 +372,18 @@ func (d *Dereferencer) enrichAccountSafely(
 
 	// By default use account.URI
 	// as the per-URI deref lock.
-	var lockKey string
+	var uriStr string
 	if account.URI != "" {
-		lockKey = account.URI
+		uriStr = account.URI
 	} else {
 		// No URI is set yet, instead generate a faux-one from user+domain.
-		lockKey = "https://" + account.Domain + "/users/" + account.Username
+		uriStr = "https://" + account.Domain + "/users/" + account.Username
 	}
 
 	// 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(lockKey)
+	unlock := d.state.FedLocks.Lock(uriStr)
 	unlock = doOnce(unlock)
 	defer unlock()
 
@@ -395,12 +395,7 @@ func (d *Dereferencer) enrichAccountSafely(
 		accountable,
 	)
 
-	if code := gtserror.StatusCode(err); code >= 400 {
-		// No matter what, log the error
-		// so instance admins have an idea
-		// why something isn't working.
-		log.Info(ctx, err)
-
+	if gtserror.StatusCode(err) >= 400 {
 		if account.IsNew() {
 			// This was a new account enrich
 			// attempt which failed before we
@@ -417,7 +412,7 @@ func (d *Dereferencer) enrichAccountSafely(
 		// return the model we had stored already.
 		account.FetchedAt = time.Now()
 		if err := d.state.DB.UpdateAccount(ctx, account, "fetched_at"); err != nil {
-			log.Errorf(ctx, "error updating account fetched_at: %v", err)
+			log.Error(ctx, "error updating %s fetched_at: %v", uriStr, err)
 		}
 	}
 
@@ -435,7 +430,7 @@ func (d *Dereferencer) enrichAccountSafely(
 		// in a call to db.Put(Account). Look again in DB by URI.
 		latest, err = d.state.DB.GetAccountByURI(ctx, account.URI)
 		if err != nil {
-			err = gtserror.Newf("error getting account %s from database after race: %w", lockKey, err)
+			err = gtserror.Newf("error getting account %s from database after race: %w", uriStr, err)
 		}
 	}
 
@@ -1070,11 +1065,17 @@ func (d *Dereferencer) dereferenceAccountFeatured(ctx context.Context, requestUs
 		// we still know it was *meant* to be pinned.
 		statusURIs = append(statusURIs, statusURI)
 
+		// Search for status by URI. Note this may return an existing model
+		// we have stored with an error from attempted update, so check both.
 		status, _, _, err := d.getStatusByURI(ctx, requestUser, statusURI)
 		if err != nil {
-			// We couldn't get the status, bummer. Just log + move on, we can try later.
 			log.Errorf(ctx, "error getting status from featured collection %s: %v", statusURI, err)
-			continue
+
+			if status == nil {
+				// This is only unactionable
+				// if no status was returned.
+				continue
+			}
 		}
 
 		// If the status was already pinned, we don't need to do anything.